Mailing List Archive: Xen: Devel

[PATCH 5/5] xen: Add V4V implementation

Index | Next | Previous | View Threaded

jean.guyader at citrix

May 31, 2012, 8:07 AM

Post #1 of 13 (207 views)
Permalink

[PATCH 5/5] xen: Add V4V implementation

Setup of v4v domains a domain gets created and cleanup
when a domain die. Wire up the v4v hypercall.

Include v4v internal and public headers.

Signed-off-by: Jean Guyader <jean.guyader [at] citrix>
---
xen/arch/x86/hvm/hvm.c | 9 +-
xen/arch/x86/x86_32/entry.S | 2 +
xen/arch/x86/x86_64/compat/entry.S | 2 +
xen/arch/x86/x86_64/entry.S | 2 +
xen/common/Makefile | 1 +
xen/common/domain.c | 11 +-
xen/common/v4v.c | 1779 ++++++++++++++++++++++++++++++++++++
xen/include/public/v4v.h | 544 +++++++++++
xen/include/public/xen.h | 2 +-
xen/include/xen/sched.h | 5 +
xen/include/xen/v4v.h | 109 +++
11 files changed, 2461 insertions(+), 5 deletions(-)
create mode 100644 xen/common/v4v.c
create mode 100644 xen/include/public/v4v.h
create mode 100644 xen/include/xen/v4v.h

Attachments:

0005-xen-Add-V4V-implementation.patch (69.0 KB)

jean.guyader at citrix

May 31, 2012, 7:52 AM

Post #2 of 13 (197 views)
Permalink

[PATCH 5/5] xen: Add V4V implementation [In reply to]

Attachments:

0005-xen-Add-V4V-implementation.patch (69.0 KB)

JBeulich at suse

May 31, 2012, 8:59 AM

Post #3 of 13 (201 views)
Permalink

Re: [PATCH 5/5] xen: Add V4V implementation [In reply to]

>>> On 31.05.12 at 17:07, Jean Guyader <jean.guyader [at] citrix> wrote:
> Setup of v4v domains a domain gets created and cleanup
> when a domain die. Wire up the v4v hypercall.
>
> Include v4v internal and public headers.

Iirc there was discussion about 6-argument hypercalls already,
and it was suggested to convert the argument set to a structure
instead. Am I mis-remembering, or is there any new reason why
introducing these is necessary?

Further, the public header doesn't seem to meet basic
requirements. For example, who told you that each and every
compiler other than gcc understands #pragma warning(...)?
Nor is adding mb() again an option (see io/ring.h), not to speak
of adding an MSVC specific implementation (this should be a
requirement on the consumers of the header). Nor any other
inline functions - they just don't belong here imo (if you
absolutely need those, they should be in a conditional that by
default prevents them from being seen by the compiler - with
that, some of the other hackery you did in the earlier patches
also becomes unnecessary). Structure packing should be
achieved using explicit padding fields rather than using compiler-
dependent constructs.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

JBeulich at suse

Aug 6, 2012, 1:45 AM

Post #4 of 13 (183 views)
Permalink

Re: [PATCH 5/5] xen: Add V4V implementation [In reply to]

>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader [at] citrix> wrote:
>--- /dev/null
>+++ b/xen/include/public/v4v.h
>...
>+#define V4V_DOMID_ANY 0x7fffU

I think I asked this before - why not use one of the pre-existing
DOMID values? And if there is a good reason, it should be said
here in a comment, to avoid the same question being asked
later again.

>...
>+typedef uint64_t v4v_pfn_t;

We already have xen_pfn_t, so why do you need yet another
flavor?

>...
>+struct v4v_info
>+{
>+ uint64_t ring_magic;
>+ uint64_t data_magic;
>+ evtchn_port_t evtchn;

Missing padding at the end?

>+};
>+typedef struct v4v_info v4v_info_t;
>+
>+#define V4V_ROUNDUP(a) (((a) +0xf ) & ~0xf)

Doesn't seem to belong here. Or is the subsequent comment
actually related to this (in which case it should be moved ahead
of the definition and made match it).

>+/*
>+ * Messages on the ring are padded to 128 bits
>+ * Len here refers to the exact length of the data not including the
>+ * 128 bit header. The message uses
>+ * ((len +0xf) & ~0xf) + sizeof(v4v_ring_message_header) bytes
>+ */
>...
>+/*
>+ * HYPERCALLS
>+ */
>...

In the block below here, please get the naming (do_v4v_op()
vs v4v_hypercall()) and the use of newlines (either always one
or always two between individual hypercall descriptions)
consistent. Hmm, even the descriptions don't seem to always
match the definitions (not really obvious because apparently
again the descriptions follow the definitions, whereas the
opposite is the usual way to arrange things).

>--- /dev/null
>+++ b/xen/include/xen/v4v_utils.h
>...
>+/* Compiler specific hacks */
>+#if defined(__GNUC__)
>+# define V4V_UNUSED __attribute__ ((unused))
>+# ifndef __STRICT_ANSI__
>+# define V4V_INLINE inline
>+# else
>+# define V4V_INLINE
>+# endif
>+#else /* !__GNUC__ */
>+# define V4V_UNUSED
>+# define V4V_INLINE
>+#endif

This suggests the header is really intended to be public?

>...
>+static V4V_INLINE uint32_t
>+v4v_ring_bytes_to_read (volatile struct v4v_ring *r)

No space between function name and opening parenthesis
(throughout this file).

>...
>+V4V_UNUSED static V4V_INLINE ssize_t

V4V_UNUSED? Doesn't make sense in conjunction with
V4V_INLINE, at least as long as you're using GNU extensions
anyway (see above as to the disposition of the header).

>+v4v_copy_out (struct v4v_ring *r, struct v4v_addr *from, uint32_t * protocol,
>+ void *_buf, size_t t, int consume)

Dead functions shouldn't be placed here.

>...
>+static ssize_t
>+v4v_copy_out_offset (struct v4v_ring *r, struct v4v_addr *from,
>+ uint32_t * protocol, void *_buf, size_t t, int consume,
>+ size_t skip) V4V_UNUSED;
>+
>+V4V_INLINE static ssize_t
>+v4v_copy_out_offset (struct v4v_ring *r, struct v4v_addr *from,
>+ uint32_t * protocol, void *_buf, size_t t, int consume,
>+ size_t skip)
>+{

What's the point of having a declaration followed immediately by
a definition? Also, the function is dead too.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

tim at xen

Aug 9, 2012, 3:38 AM

Post #5 of 13 (181 views)
Permalink

Re: [PATCH 5/5] xen: Add V4V implementation [In reply to]

Hi,

This looks pretty good; I think you've addressed almost all my comments
except for one, which is really a design decision raether than an
implementation one. As I said last time:

] And what about protocol? Protocol seems to have ended up as a bit of a
] second-class citizen in v4v; it's defined, and indeed required, but not
] used for routing or for acccess control, so all traffic to a given port
] _on every protocol_ ends up on the same ring.
]
] This is the inverse of the TCP/IP namespace that you're copying, where
] protocol demux happens before port demux. And I think it will bite
] someone if you ever, for example, want to send ICMP or GRE over a v4v
] channel.

I see Jan has some comments on the detail; all I have to add is this:

At 20:50 +0100 on 03 Aug (1344027054), Jean Guyader wrote:
> +
> +
> +struct list_head viprules = LIST_HEAD_INIT(viprules);
> +static DEFINE_RWLOCK(viprules_lock);

Please add comments describing this lock and where it comes in the
locking order relative to the locks below -- it looks like it's always
taken after any other locks but please make it clear.

> +/*
> + * Structure definitions
> + */
> +
> +#define V4V_RING_MAGIC 0xA822f72bb0b9d8cc
> +#define V4V_RING_DATA_MAGIC 0x45fe852220b801d4

Thanks for getting rid of the #ifdefs here, but you need to keep the
'ULL' suffix so this will compile properly on 32-bit.

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

jean.guyader at citrix

Aug 10, 2012, 9:51 AM

Post #6 of 13 (183 views)
Permalink

Re: [PATCH 5/5] xen: Add V4V implementation [In reply to]

On 09/08 11:38, Tim Deegan wrote:
> Hi,
>
> This looks pretty good; I think you've addressed almost all my comments
> except for one, which is really a design decision raether than an
> implementation one. As I said last time:
>
> ] And what about protocol? Protocol seems to have ended up as a bit of a
> ] second-class citizen in v4v; it's defined, and indeed required, but not
> ] used for routing or for acccess control, so all traffic to a given port
> ] _on every protocol_ ends up on the same ring.
> ]
> ] This is the inverse of the TCP/IP namespace that you're copying, where
> ] protocol demux happens before port demux. And I think it will bite
> ] someone if you ever, for example, want to send ICMP or GRE over a v4v
> ] channel.
>

The protocol field is used to inform about the type a message on the ring.

Right now we use two protocols in our linux driver: V4V_PROTO_DGRAM and
V4V_PROTO_STREAM. In the future that could probably be extended to new protocol
like V4V_PROTO_ICMP for instance.

The demultiplexing will happens at the other end, the driver can look at the
message and decide what to do with it based on the protocol field.

Jean

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

tim at xen

Aug 13, 2012, 2:38 AM

Post #7 of 13 (178 views)
Permalink

Re: [PATCH 5/5] xen: Add V4V implementation [In reply to]

At 17:51 +0100 on 10 Aug (1344621069), Jean Guyader wrote:
> On 09/08 11:38, Tim Deegan wrote:
> > Hi,
> >
> > This looks pretty good; I think you've addressed almost all my comments
> > except for one, which is really a design decision raether than an
> > implementation one. As I said last time:
> >
> > ] And what about protocol? Protocol seems to have ended up as a bit of a
> > ] second-class citizen in v4v; it's defined, and indeed required, but not
> > ] used for routing or for acccess control, so all traffic to a given port
> > ] _on every protocol_ ends up on the same ring.
> > ]
> > ] This is the inverse of the TCP/IP namespace that you're copying, where
> > ] protocol demux happens before port demux. And I think it will bite
> > ] someone if you ever, for example, want to send ICMP or GRE over a v4v
> > ] channel.
> >
>
> The protocol field is used to inform about the type a message on the ring.
>
> Right now we use two protocols in our linux driver: V4V_PROTO_DGRAM and
> V4V_PROTO_STREAM. In the future that could probably be extended to new protocol
> like V4V_PROTO_ICMP for instance.
>
> The demultiplexing will happens at the other end, the driver can look at the
> message and decide what to do with it based on the protocol field.

Yes, I understand all that - what I'm saying is that it seems like a
design flaw to me. The namespace in V4V, as proposed, looks like this:

Protocol
Port
Domain

and it would be more sensible to do (like the IP stack):

Port
Protocol
Domain.

Or at the very least the protocol should be made part of the endpoint
address, and not just part of the packet header. As it stands:

- The handlers for port X in _all_ protocols _have_ to share a
ring. That seems kind of plausible because the IANA port assignments
never give the same port number to different services on TCP and UDP,
but will it make sense for every new protocol? Is it sensible to
require, say, an L2TP service to make its connection IDs not clash
with V4V_PROTO_DGRAM and V4V_PROTO_STREAM users?

It may not even make sense in existing protocols. It's common enough
for DNS servers to use different ACLs (and indeed different servers)
for TCP and UDP.

- Relatedly, every protocol _has_ to have port numbers. How would you
register an ICMP listener, for example? You'd have to do something
gross like declare a particular port to be the ICMP port so that you
could demux it, or indeed send it in the first place.

You say:

> The demultiplexing will happens at the other end, the driver can look at the
> message and decide what to do with it based on the protocol field.

I'm willing to accept that argument, but only if we extend it to ports
too, get rid of all the namespace and ACL code in Xen and leave each
domain with a single RX ring that the (single) guest driver must demux. :P

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

jean.guyader at gmail

Aug 13, 2012, 5:43 AM

Post #8 of 13 (176 views)
Permalink

Re: [PATCH 5/5] xen: Add V4V implementation [In reply to]

On 13 August 2012 10:38, Tim Deegan <tim [at] xen> wrote:
> At 17:51 +0100 on 10 Aug (1344621069), Jean Guyader wrote:
>> On 09/08 11:38, Tim Deegan wrote:
>> > Hi,
>> >
>> > This looks pretty good; I think you've addressed almost all my comments
>> > except for one, which is really a design decision raether than an
>> > implementation one. As I said last time:
>> >
>> > ] And what about protocol? Protocol seems to have ended up as a bit of a
>> > ] second-class citizen in v4v; it's defined, and indeed required, but not
>> > ] used for routing or for acccess control, so all traffic to a given port
>> > ] _on every protocol_ ends up on the same ring.
>> > ]
>> > ] This is the inverse of the TCP/IP namespace that you're copying, where
>> > ] protocol demux happens before port demux. And I think it will bite
>> > ] someone if you ever, for example, want to send ICMP or GRE over a v4v
>> > ] channel.
>> >
>>
>> The protocol field is used to inform about the type a message on the ring.
>>
>> Right now we use two protocols in our linux driver: V4V_PROTO_DGRAM and
>> V4V_PROTO_STREAM. In the future that could probably be extended to new protocol
>> like V4V_PROTO_ICMP for instance.
>>
>> The demultiplexing will happens at the other end, the driver can look at the
>> message and decide what to do with it based on the protocol field.
>
> Yes, I understand all that - what I'm saying is that it seems like a
> design flaw to me. The namespace in V4V, as proposed, looks like this:
>
> Protocol
> Port
> Domain
>

Protocol isn't part of the namespace - I think that's
where the confusion arises. The namespace is exclusively
Port/Domain. Protocol is there to describe the content of
_a particular message_ in the ring. It is included in the
hypercalls (rather than, say, being the first n bytes of
all messages) to force all senders to use it.

The other key point here is confusion as to what V4V is.
V4V is _not_ a tcp or udp clone. V4V exists to provide
a mechanism to transfer messages (which are arbitary
strings of bytes, labeled with a "protocol") between
endpoitns (labeled by a domain and port).

An example use of this facility uses two v4v rings to
implement something which looks quite like TCP. In this
case to distinguish TCP-a-like messages from other
such messages that may or may not be present on the ring,
the messages are labeled with a protocol field that
indicates what upper layer should handle these messages.

One can easily immagine circumstances where messages of
many different protocols are present on the ring. An
obvious example might be a message type which implemented
some sort of flow control, that quenced or started
transmission on a partner ring, regardless of what other
messages were being sent on the rings.

> and it would be more sensible to do (like the IP stack):
>
> Port
> Protocol
> Domain.
>
> Or at the very least the protocol should be made part of the endpoint
> address, and not just part of the packet header. As it stands:
>
> - The handlers for port X in _all_ protocols _have_ to share a
> ring. That seems kind of plausible because the IANA port assignments
> never give the same port number to different services on TCP and UDP,
> but will it make sense for every new protocol? Is it sensible to
> require, say, an L2TP service to make its connection IDs not clash
> with V4V_PROTO_DGRAM and V4V_PROTO_STREAM users?
>
> It may not even make sense in existing protocols. It's common enough
> for DNS servers to use different ACLs (and indeed different servers)
> for TCP and UDP.
>

V4V isn't IP, but you raise a valid point. We should
define two ranges of 16 bit V4V port numbers one which
is "well known" for the use of TCP encapsulation, and
one which is "well known" for the use of UDP
encapsulation. That then makes the concept that you
think that protocol should be, be part of the
endpoint address, and it leaves the thing I misleadingly
called "protocol" free to be the message type label.

> - Relatedly, every protocol _has_ to have port numbers. How would you
> register an ICMP listener, for example? You'd have to do something
> gross like declare a particular port to be the ICMP port so that you
> could demux it, or indeed send it in the first place.
>
> You say:
>
>> The demultiplexing will happens at the other end, the driver can look at the
>> message and decide what to do with it based on the protocol field.
>
> I'm willing to accept that argument, but only if we extend it to ports
> too, get rid of all the namespace and ACL code in Xen and leave each
> domain with a single RX ring that the (single) guest driver must demux. :P
>

Cheers,
Jean

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

tim at xen

Aug 16, 2012, 5:32 AM

Post #9 of 13 (174 views)
Permalink

Re: [PATCH 5/5] xen: Add V4V implementation [In reply to]

At 13:43 +0100 on 13 Aug (1344865403), Jean Guyader wrote:
> Protocol isn't part of the namespace - I think that's
> where the confusion arises. The namespace is exclusively
> Port/Domain. Protocol is there to describe the content of
> _a particular message_ in the ring.

OK.

In that case, I think the hypervisor shouldn't handle it at all. That
can be done in the client drivers, and the V4V_PROTO definitions and
maybe packet format stuff can go in documentation.

> It is included in the hypercalls (rather than, say, being the first n
> bytes of all messages) to force all senders to use it.

I don't think that's very useful. It just forces any V4V user who
doesn't need multiple protocols to make up a number for form's sake, and
since Xen doesn't do any checking on the field, it doesn't protect the
receiver from anything.

I guess what I'm saying is, either 'protocol' (or whatever name you give
it) is part of the v4v addressing, in which case it should be treated
properly and demuxed before port, or it's not, in which case it
needn't be in the interface at all.

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

jean.guyader at gmail

Aug 23, 2012, 4:57 AM

Post #10 of 13 (156 views)
Permalink

Re: [PATCH 5/5] xen: Add V4V implementation [In reply to]

On 6 August 2012 09:45, Jan Beulich <JBeulich [at] suse> wrote:
>>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader [at] citrix> wrote:
>>--- /dev/null
>>+++ b/xen/include/public/v4v.h
>>...
>>+#define V4V_DOMID_ANY 0x7fffU
>
> I think I asked this before - why not use one of the pre-existing
> DOMID values? And if there is a good reason, it should be said
> here in a comment, to avoid the same question being asked
> later again.
>
>>...
>>+typedef uint64_t v4v_pfn_t;
>
> We already have xen_pfn_t, so why do you need yet another
> flavor?
>
>>...
>>+struct v4v_info
>>+{
>>+ uint64_t ring_magic;
>>+ uint64_t data_magic;
>>+ evtchn_port_t evtchn;
>
> Missing padding at the end?
>
>>+};
>>+typedef struct v4v_info v4v_info_t;
>>+
>>+#define V4V_ROUNDUP(a) (((a) +0xf ) & ~0xf)
>
> Doesn't seem to belong here. Or is the subsequent comment
> actually related to this (in which case it should be moved ahead
> of the definition and made match it).
>
>>+/*
>>+ * Messages on the ring are padded to 128 bits
>>+ * Len here refers to the exact length of the data not including the
>>+ * 128 bit header. The message uses
>>+ * ((len +0xf) & ~0xf) + sizeof(v4v_ring_message_header) bytes
>>+ */
>>...
>>+/*
>>+ * HYPERCALLS
>>+ */
>>...
>
> In the block below here, please get the naming (do_v4v_op()
> vs v4v_hypercall()) and the use of newlines (either always one
> or always two between individual hypercall descriptions)
> consistent. Hmm, even the descriptions don't seem to always
> match the definitions (not really obvious because apparently
> again the descriptions follow the definitions, whereas the
> opposite is the usual way to arrange things).
>
>>--- /dev/null
>>+++ b/xen/include/xen/v4v_utils.h
>>...
>>+/* Compiler specific hacks */
>>+#if defined(__GNUC__)
>>+# define V4V_UNUSED __attribute__ ((unused))
>>+# ifndef __STRICT_ANSI__
>>+# define V4V_INLINE inline
>>+# else
>>+# define V4V_INLINE
>>+# endif
>>+#else /* !__GNUC__ */
>>+# define V4V_UNUSED
>>+# define V4V_INLINE
>>+#endif
>
> This suggests the header is really intended to be public?
>
>>...
>>+static V4V_INLINE uint32_t
>>+v4v_ring_bytes_to_read (volatile struct v4v_ring *r)
>
> No space between function name and opening parenthesis
> (throughout this file).
>
>>...
>>+V4V_UNUSED static V4V_INLINE ssize_t
>
> V4V_UNUSED? Doesn't make sense in conjunction with
> V4V_INLINE, at least as long as you're using GNU extensions
> anyway (see above as to the disposition of the header).
>
>>+v4v_copy_out (struct v4v_ring *r, struct v4v_addr *from, uint32_t * protocol,
>>+ void *_buf, size_t t, int consume)
>
> Dead functions shouldn't be placed here.
>
>>...
>>+static ssize_t
>>+v4v_copy_out_offset (struct v4v_ring *r, struct v4v_addr *from,
>>+ uint32_t * protocol, void *_buf, size_t t, int consume,
>>+ size_t skip) V4V_UNUSED;
>>+
>>+V4V_INLINE static ssize_t
>>+v4v_copy_out_offset (struct v4v_ring *r, struct v4v_addr *from,
>>+ uint32_t * protocol, void *_buf, size_t t, int consume,
>>+ size_t skip)
>>+{
>
> What's the point of having a declaration followed immediately by
> a definition? Also, the function is dead too.
>

This file (v4v_utils.h) has utility that could be used by drivers, we don't use
them in Xen but we through it will be convenient to have such function
accessible for one to write a v4v driver a v4v driver.

What would be the right place for those?

Thanks,
Jean

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

JBeulich at suse

Aug 24, 2012, 1:06 PM

Post #11 of 13 (155 views)
Permalink

Re: [PATCH 5/5] xen: Add V4V implementation [In reply to]

>>> On 23.08.12 at 13:57, Jean Guyader <jean.guyader [at] gmail> wrote:
> On 6 August 2012 09:45, Jan Beulich <JBeulich [at] suse> wrote:
>>>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader [at] citrix> wrote:
>>>--- /dev/null
>>>+++ b/xen/include/public/v4v.h
>>>...
>>>+#define V4V_DOMID_ANY 0x7fffU
>>
>> I think I asked this before - why not use one of the pre-existing
>> DOMID values? And if there is a good reason, it should be said
>> here in a comment, to avoid the same question being asked
>> later again.
>>
>>>...
>>>+typedef uint64_t v4v_pfn_t;
>>
>> We already have xen_pfn_t, so why do you need yet another
>> flavor?
>>
>>>...
>>>+struct v4v_info
>>>+{
>>>+ uint64_t ring_magic;
>>>+ uint64_t data_magic;
>>>+ evtchn_port_t evtchn;
>>
>> Missing padding at the end?
>>
>>>+};
>>>+typedef struct v4v_info v4v_info_t;
>>>+
>>>+#define V4V_ROUNDUP(a) (((a) +0xf ) & ~0xf)
>>
>> Doesn't seem to belong here. Or is the subsequent comment
>> actually related to this (in which case it should be moved ahead
>> of the definition and made match it).
>>
>>>+/*
>>>+ * Messages on the ring are padded to 128 bits
>>>+ * Len here refers to the exact length of the data not including the
>>>+ * 128 bit header. The message uses
>>>+ * ((len +0xf) & ~0xf) + sizeof(v4v_ring_message_header) bytes
>>>+ */
>>>...
>>>+/*
>>>+ * HYPERCALLS
>>>+ */
>>>...
>>
>> In the block below here, please get the naming (do_v4v_op()
>> vs v4v_hypercall()) and the use of newlines (either always one
>> or always two between individual hypercall descriptions)
>> consistent. Hmm, even the descriptions don't seem to always
>> match the definitions (not really obvious because apparently
>> again the descriptions follow the definitions, whereas the
>> opposite is the usual way to arrange things).
>>
>>>--- /dev/null
>>>+++ b/xen/include/xen/v4v_utils.h
>>>...
>>>+/* Compiler specific hacks */
>>>+#if defined(__GNUC__)
>>>+# define V4V_UNUSED __attribute__ ((unused))
>>>+# ifndef __STRICT_ANSI__
>>>+# define V4V_INLINE inline
>>>+# else
>>>+# define V4V_INLINE
>>>+# endif
>>>+#else /* !__GNUC__ */
>>>+# define V4V_UNUSED
>>>+# define V4V_INLINE
>>>+#endif
>>
>> This suggests the header is really intended to be public?
>>
>>>...
>>>+static V4V_INLINE uint32_t
>>>+v4v_ring_bytes_to_read (volatile struct v4v_ring *r)
>>
>> No space between function name and opening parenthesis
>> (throughout this file).
>>
>>>...
>>>+V4V_UNUSED static V4V_INLINE ssize_t
>>
>> V4V_UNUSED? Doesn't make sense in conjunction with
>> V4V_INLINE, at least as long as you're using GNU extensions
>> anyway (see above as to the disposition of the header).
>>
>>>+v4v_copy_out (struct v4v_ring *r, struct v4v_addr *from, uint32_t *
> protocol,
>>>+ void *_buf, size_t t, int consume)
>>
>> Dead functions shouldn't be placed here.
>>
>>>...
>>>+static ssize_t
>>>+v4v_copy_out_offset (struct v4v_ring *r, struct v4v_addr *from,
>>>+ uint32_t * protocol, void *_buf, size_t t, int consume,
>>>+ size_t skip) V4V_UNUSED;
>>>+
>>>+V4V_INLINE static ssize_t
>>>+v4v_copy_out_offset (struct v4v_ring *r, struct v4v_addr *from,
>>>+ uint32_t * protocol, void *_buf, size_t t, int consume,
>>>+ size_t skip)
>>>+{
>>
>> What's the point of having a declaration followed immediately by
>> a definition? Also, the function is dead too.
>>
>
> This file (v4v_utils.h) has utility that could be used by drivers, we don't
> use
> them in Xen but we through it will be convenient to have such function
> accessible for one to write a v4v driver a v4v driver.
>
> What would be the right place for those?

I think public/io/ would be the best matching place, but it's
certainly also not ideal. Maybe we ought to introduce public/lib/
or some such for it?

But then again I don't see how this comment of yours relates to
the earlier comments I made.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

jean.guyader at gmail

Sep 1, 2012, 1:56 PM

Post #12 of 13 (145 views)
Permalink

Re: [PATCH 5/5] xen: Add V4V implementation [In reply to]

On 6 August 2012 01:45, Jan Beulich <JBeulich [at] suse> wrote:
>>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader [at] citrix> wrote:
>>--- /dev/null
>>+++ b/xen/include/public/v4v.h
>>...
>>+#define V4V_DOMID_ANY 0x7fffU
>
> I think I asked this before - why not use one of the pre-existing
> DOMID values? And if there is a good reason, it should be said
> here in a comment, to avoid the same question being asked
> later again.
>

I replaced 0x7fffU with DOMID_INVALID.

>>...
>>+typedef uint64_t v4v_pfn_t;
>
> We already have xen_pfn_t, so why do you need yet another
> flavor?
>

No, replaced with xen_pfn_t.

>>...
>>+struct v4v_info
>>+{
>>+ uint64_t ring_magic;
>>+ uint64_t data_magic;
>>+ evtchn_port_t evtchn;
>
> Missing padding at the end?
>

ack.

>>+};
>>+typedef struct v4v_info v4v_info_t;
>>+
>>+#define V4V_ROUNDUP(a) (((a) +0xf ) & ~0xf)
>
> Doesn't seem to belong here. Or is the subsequent comment
> actually related to this (in which case it should be moved ahead
> of the definition and made match it).
>

V4V_ROUNDUP is now in v4v.c and I move the description above
the define.

Thanks for the review,
Jean

>>+/*
>>+ * Messages on the ring are padded to 128 bits
>>+ * Len here refers to the exact length of the data not including the
>>+ * 128 bit header. The message uses
>>+ * ((len +0xf) & ~0xf) + sizeof(v4v_ring_message_header) bytes
>>+ */
>>...
>>+/*
>>+ * HYPERCALLS
>>+ */
>>...
>
> In the block below here, please get the naming (do_v4v_op()
> vs v4v_hypercall()) and the use of newlines (either always one
> or always two between individual hypercall descriptions)
> consistent. Hmm, even the descriptions don't seem to always
> match the definitions (not really obvious because apparently
> again the descriptions follow the definitions, whereas the
> opposite is the usual way to arrange things).
>
>>--- /dev/null
>>+++ b/xen/include/xen/v4v_utils.h
>>...
>>+/* Compiler specific hacks */
>>+#if defined(__GNUC__)
>>+# define V4V_UNUSED __attribute__ ((unused))
>>+# ifndef __STRICT_ANSI__
>>+# define V4V_INLINE inline
>>+# else
>>+# define V4V_INLINE
>>+# endif
>>+#else /* !__GNUC__ */
>>+# define V4V_UNUSED
>>+# define V4V_INLINE
>>+#endif
>
> This suggests the header is really intended to be public?
>
>>...
>>+static V4V_INLINE uint32_t
>>+v4v_ring_bytes_to_read (volatile struct v4v_ring *r)
>
> No space between function name and opening parenthesis
> (throughout this file).
>
>>...
>>+V4V_UNUSED static V4V_INLINE ssize_t
>
> V4V_UNUSED? Doesn't make sense in conjunction with
> V4V_INLINE, at least as long as you're using GNU extensions
> anyway (see above as to the disposition of the header).
>
>>+v4v_copy_out (struct v4v_ring *r, struct v4v_addr *from, uint32_t * protocol,
>>+ void *_buf, size_t t, int consume)
>
> Dead functions shouldn't be placed here.
>
>>...
>>+static ssize_t
>>+v4v_copy_out_offset (struct v4v_ring *r, struct v4v_addr *from,
>>+ uint32_t * protocol, void *_buf, size_t t, int consume,
>>+ size_t skip) V4V_UNUSED;
>>+
>>+V4V_INLINE static ssize_t
>>+v4v_copy_out_offset (struct v4v_ring *r, struct v4v_addr *from,
>>+ uint32_t * protocol, void *_buf, size_t t, int consume,
>>+ size_t skip)
>>+{
>
> What's the point of having a declaration followed immediately by
> a definition? Also, the function is dead too.
>

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

jean.guyader at gmail

Sep 1, 2012, 1:58 PM

Post #13 of 13 (150 views)
Permalink

Re: [PATCH 5/5] xen: Add V4V implementation [In reply to]

On 24 August 2012 13:06, Jan Beulich <JBeulich [at] suse> wrote:
>>>> On 23.08.12 at 13:57, Jean Guyader <jean.guyader [at] gmail> wrote:
>> On 6 August 2012 09:45, Jan Beulich <JBeulich [at] suse> wrote:
>>>>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader [at] citrix> wrote:
>>>>--- /dev/null
>>>>+++ b/xen/include/public/v4v.h
>>>>...
>>>>+#define V4V_DOMID_ANY 0x7fffU
>>>
>>> I think I asked this before - why not use one of the pre-existing
>>> DOMID values? And if there is a good reason, it should be said
>>> here in a comment, to avoid the same question being asked
>>> later again.
>>>
>>>>...
>>>>+typedef uint64_t v4v_pfn_t;
>>>
>>> We already have xen_pfn_t, so why do you need yet another
>>> flavor?
>>>
>>>>...
>>>>+struct v4v_info
>>>>+{
>>>>+ uint64_t ring_magic;
>>>>+ uint64_t data_magic;
>>>>+ evtchn_port_t evtchn;
>>>
>>> Missing padding at the end?
>>>
>>>>+};
>>>>+typedef struct v4v_info v4v_info_t;
>>>>+
>>>>+#define V4V_ROUNDUP(a) (((a) +0xf ) & ~0xf)
>>>
>>> Doesn't seem to belong here. Or is the subsequent comment
>>> actually related to this (in which case it should be moved ahead
>>> of the definition and made match it).
>>>
>>>>+/*
>>>>+ * Messages on the ring are padded to 128 bits
>>>>+ * Len here refers to the exact length of the data not including the
>>>>+ * 128 bit header. The message uses
>>>>+ * ((len +0xf) & ~0xf) + sizeof(v4v_ring_message_header) bytes
>>>>+ */
>>>>...
>>>>+/*
>>>>+ * HYPERCALLS
>>>>+ */
>>>>...
>>>
>>> In the block below here, please get the naming (do_v4v_op()
>>> vs v4v_hypercall()) and the use of newlines (either always one
>>> or always two between individual hypercall descriptions)
>>> consistent. Hmm, even the descriptions don't seem to always
>>> match the definitions (not really obvious because apparently
>>> again the descriptions follow the definitions, whereas the
>>> opposite is the usual way to arrange things).
>>>
>>>>--- /dev/null
>>>>+++ b/xen/include/xen/v4v_utils.h
>>>>...
>>>>+/* Compiler specific hacks */
>>>>+#if defined(__GNUC__)
>>>>+# define V4V_UNUSED __attribute__ ((unused))
>>>>+# ifndef __STRICT_ANSI__
>>>>+# define V4V_INLINE inline
>>>>+# else
>>>>+# define V4V_INLINE
>>>>+# endif
>>>>+#else /* !__GNUC__ */
>>>>+# define V4V_UNUSED
>>>>+# define V4V_INLINE
>>>>+#endif
>>>
>>> This suggests the header is really intended to be public?
>>>
>>>>...
>>>>+static V4V_INLINE uint32_t
>>>>+v4v_ring_bytes_to_read (volatile struct v4v_ring *r)
>>>
>>> No space between function name and opening parenthesis
>>> (throughout this file).
>>>
>>>>...
>>>>+V4V_UNUSED static V4V_INLINE ssize_t
>>>
>>> V4V_UNUSED? Doesn't make sense in conjunction with
>>> V4V_INLINE, at least as long as you're using GNU extensions
>>> anyway (see above as to the disposition of the header).
>>>
>>>>+v4v_copy_out (struct v4v_ring *r, struct v4v_addr *from, uint32_t *
>> protocol,
>>>>+ void *_buf, size_t t, int consume)
>>>
>>> Dead functions shouldn't be placed here.
>>>
>>>>...
>>>>+static ssize_t
>>>>+v4v_copy_out_offset (struct v4v_ring *r, struct v4v_addr *from,
>>>>+ uint32_t * protocol, void *_buf, size_t t, int consume,
>>>>+ size_t skip) V4V_UNUSED;
>>>>+
>>>>+V4V_INLINE static ssize_t
>>>>+v4v_copy_out_offset (struct v4v_ring *r, struct v4v_addr *from,
>>>>+ uint32_t * protocol, void *_buf, size_t t, int consume,
>>>>+ size_t skip)
>>>>+{
>>>
>>> What's the point of having a declaration followed immediately by
>>> a definition? Also, the function is dead too.
>>>
>>
>> This file (v4v_utils.h) has utility that could be used by drivers, we don't
>> use
>> them in Xen but we through it will be convenient to have such function
>> accessible for one to write a v4v driver a v4v driver.
>>
>> What would be the right place for those?
>
> I think public/io/ would be the best matching place, but it's
> certainly also not ideal. Maybe we ought to introduce public/lib/
> or some such for it?
>
> But then again I don't see how this comment of yours relates to
> the earlier comments I made.
>

Ok I have removed this file from the patch series, we already have a
copy of this file in the Linux driver so those helper functions can be found
there.

Thanks,
Jean

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads