v3 changes:
- Switch to event channel
- Allocated a unbound event channel
per domain.
- Add a new v4v call to share the
event channel port.
- Public headers with actual type definition
- Align all the v4v type to 64 bits
- Modify v4v MAGIC numbers because we won''t
but backward compatible anymore
- Merge insert and insertv
- Merge send and sendv
- Turn all the lock prerequisite from comment
to ASSERT()
- Make use or write_atomic instead of volatile pointers
- Merge v4v_memcpy_to_guest_ring and
v4v_memcpy_to_guest_ring_from_guest
- Introduce copy_from_guest_maybe that can take
a void * and a handle as src address.
- TODO:
- Add libv4v userspace code
v2 changes:
- Cleanup plugin header
- Include basic access control
- Use guest_handle_for_field
Jan Beulich (1):
xen: Introduce guest_handle_for_field
Jean Guyader (4):
xen: add ssize_t
xen: virq, remove VIRQ_XC_RESERVED
xen: events, exposes evtchn_alloc_unbound_domain
xen: Add V4V implementation
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 | 13 +-
xen/common/event_channel.c | 33 +-
xen/common/v4v.c | 1895 ++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/types.h | 1 +
xen/include/asm-x86/guest_access.h | 3 +
xen/include/asm-x86/types.h | 6 +
xen/include/public/v4v.h | 291 ++++++
xen/include/public/xen.h | 3 +-
xen/include/xen/event.h | 2 +
xen/include/xen/sched.h | 4 +
xen/include/xen/v4v.h | 134 +++
xen/include/xen/v4v_utils.h | 276 ++++++
17 files changed, 2665 insertions(+), 12 deletions(-)
create mode 100644 xen/common/v4v.c
create mode 100644 xen/include/public/v4v.h
create mode 100644 xen/include/xen/v4v.h
create mode 100644 xen/include/xen/v4v_utils.h
--
1.7.9.5
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Signed-off-by: Jean Guyader <jean.guyader@citrix.com> --- xen/include/asm-arm/types.h | 1 + xen/include/asm-x86/types.h | 6 ++++++ 2 files changed, 7 insertions(+) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
This helper turns a field of a GUEST_HANDLE in a GUEST_HANDLE. From: Jan Beulich <JBeulich@suse.com> Signed-off-by: Jean Guyader <jean.guyader@citrix.com> --- xen/include/asm-x86/guest_access.h | 3 +++ 1 file changed, 3 insertions(+) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
VIRQ_XC_RESERVED was reserved for V4V but we have switched to event channels so this place holder is no longer required. Signed-off-by: Jean Guyader <jean.guyader@citrix.com> --- xen/include/public/xen.h | 1 - 1 file changed, 1 deletion(-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jean Guyader
2012-Aug-03 19:50 UTC
[PATCH 4/5] xen: events, exposes evtchn_alloc_unbound_domain
Exposes evtchn_alloc_unbound_domain to the rest of Xen so we can create allocated unbound evtchn within Xen. Signed-off-by: Jean Guyader <jean.guyader@citrix.com> --- xen/common/event_channel.c | 33 +++++++++++++++++++++++++++------ xen/include/xen/event.h | 2 ++ 2 files changed, 29 insertions(+), 6 deletions(-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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@citrix.com> --- 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 | 13 +- xen/common/v4v.c | 1895 ++++++++++++++++++++++++++++++++++++ xen/include/public/v4v.h | 291 ++++++ xen/include/public/xen.h | 2 +- xen/include/xen/sched.h | 4 + xen/include/xen/v4v.h | 134 +++ xen/include/xen/v4v_utils.h | 276 ++++++ 12 files changed, 2626 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 create mode 100644 xen/include/xen/v4v_utils.h _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On 3 August 2012 20:50, Jean Guyader <jean.guyader@citrix.com> wrote:> v3 changes: > - Switch to event channel > - Allocated a unbound event channel > per domain. > - Add a new v4v call to share the > event channel port. > - Public headers with actual type definition > - Align all the v4v type to 64 bits > - Modify v4v MAGIC numbers because we won''t > but backward compatible anymore > - Merge insert and insertv > - Merge send and sendv > - Turn all the lock prerequisite from comment > to ASSERT() > - Make use or write_atomic instead of volatile pointers > - Merge v4v_memcpy_to_guest_ring and > v4v_memcpy_to_guest_ring_from_guest > - Introduce copy_from_guest_maybe that can take > a void * and a handle as src address.- Replace 6 arguments hypercalls with 5 arguments hypercalls. Jean
>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> wrote:Without finally explaining why you need this type in the first place, I''ll continue to NAK this patch. (This is made even worse by the fact taht the two inline functions in patch 5 that make use of the type appear to be unused.) Jan
>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> wrote: > VIRQ_XC_RESERVED was reserved for V4V but we have switched > to event channels so this place holder is no longer required.I''m fine with this change, but is a future re-use of the value indeed not going to cause problems on XenServer (or wherever else this is patch set coming from)? Jan> Signed-off-by: Jean Guyader <jean.guyader@citrix.com> > --- > xen/include/public/xen.h | 1 - > 1 file changed, 1 deletion(-)
Jan Beulich
2012-Aug-06 08:19 UTC
Re: [PATCH 4/5] xen: events, exposes evtchn_alloc_unbound_domain
>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> wrote: >--- a/xen/common/event_channel.c >+++ b/xen/common/event_channel.c >@@ -51,6 +51,8 @@ > > #define consumer_is_xen(e) (!!(e)->xen_consumer) > >+static long __evtchn_close(struct domain *d, int port);What is this needed for?>+ > /* > * The function alloc_unbound_xen_event_channel() allows an arbitrary > * notifier function to be specified. However, very few unique functions >@@ -161,18 +163,18 @@ static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) > { > struct evtchn *chn; > struct domain *d; >- int port; >+ evtchn_port_t port; > domid_t dom = alloc->dom; >- long rc; >+ int rc; > > rc = rcu_lock_target_domain_by_id(dom, &d); > if ( rc ) > return rc; > >- spin_lock(&d->event_lock); >+ rc = evtchn_alloc_unbound_domain(d, &port); >+ if ( rc ) >+ ERROR_EXIT_DOM(rc, d); > >- if ( (port = get_free_port(d)) < 0 ) >- ERROR_EXIT_DOM(port, d); > chn = evtchn_from_port(d, port); > > rc = xsm_evtchn_unbound(d, chn, alloc->remote_dom); >@@ -186,12 +188,31 @@ static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) > alloc->port = port; > > out: >- spin_unlock(&d->event_lock); > rcu_unlock_domain(d); > > return rc; > } > >+int evtchn_alloc_unbound_domain(struct domain *d, evtchn_port_t *port) >+{ >+ struct evtchn *chn; >+ int free_port = 0; >+ >+ spin_lock(&d->event_lock); >+ >+ if ( (free_port = get_free_port(d)) < 0 ) >+ goto out; >+ chn = evtchn_from_port(d, free_port);The code below this is not really a plain breakout from the function above:>+ chn->state = ECS_UNBOUND;The equivalent to this ought to be removed from the original function as being redundant.>+ chn->u.unbound.remote_domid = DOMID_INVALID;The single caller here will immediately overwrite this value. It would seem more clean to simply pass in the intended value, and eliminate the corresponding code from the caller too. Jan>+ *port = free_port; >+ /* Everything is fine, returns 0 */ >+ free_port = 0; >+ >+ out: >+ spin_unlock(&d->event_lock); >+ return free_port; >+} > > static long evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind) > {
>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> wrote: >--- /dev/null >+++ b/xen/include/public/v4v.h >... >+#define V4V_DOMID_ANY 0x7fffUI 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 >+#endifThis 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_tV4V_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
On 6 August 2012 09:10, Jan Beulich <JBeulich@suse.com> wrote:>>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> wrote: >> VIRQ_XC_RESERVED was reserved for V4V but we have switched >> to event channels so this place holder is no longer required. > > I''m fine with this change, but is a future re-use of the value indeed > not going to cause problems on XenServer (or wherever else this > is patch set coming from)? >That may need to be confirmed but I don''t think XenServer is using v4v yet, their plan is to pick it up from upstream. So removing this VIRQ should be fine. Jean
On 6 August 2012 09:08, Jan Beulich <JBeulich@suse.com> wrote:>>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> wrote: > > Without finally explaining why you need this type in the first place, > I''ll continue to NAK this patch. (This is made even worse by the fact > taht the two inline functions in patch 5 that make use of the type > appear to be unused.) >Understood. I''ll switch to use long instead of ssize_t in my forthcoming patch serie. Jean
On 06/08/12 15:46, Jean Guyader wrote:> On 6 August 2012 09:10, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> wrote: >>> VIRQ_XC_RESERVED was reserved for V4V but we have switched >>> to event channels so this place holder is no longer required. >> I''m fine with this change, but is a future re-use of the value indeed >> not going to cause problems on XenServer (or wherever else this >> is patch set coming from)? >> > That may need to be confirmed but I don''t think XenServer is using v4v > yet, their plan > is to pick it up from upstream. So removing this VIRQ should be fine. > > JeanYes - we don''t use it, but are interested when XenClient can successfully upstream it. ~Andrew> > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel-- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com
On Mon, 2012-08-06 at 15:46 +0100, Jean Guyader wrote:> On 6 August 2012 09:10, Jan Beulich <JBeulich@suse.com> wrote: > >>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> wrote: > >> VIRQ_XC_RESERVED was reserved for V4V but we have switched > >> to event channels so this place holder is no longer required. > > > > I''m fine with this change, but is a future re-use of the value indeed > > not going to cause problems on XenServer (or wherever else this > > is patch set coming from)? > > > > That may need to be confirmed but I don''t think XenServer is using v4v > yetI think Jan probably meant XenClient (i.e. that being the place where v4v is already deployed). There''s no harm in keeping this # reserved indefinitely, with a suitable comment, I think? The only reason not to would be if this address space was limited, but I don''t think that is the case with VIRQs
On 6 August 2012 15:56, Ian Campbell <Ian.Campbell@citrix.com> wrote:> On Mon, 2012-08-06 at 15:46 +0100, Jean Guyader wrote: >> On 6 August 2012 09:10, Jan Beulich <JBeulich@suse.com> wrote: >> >>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> wrote: >> >> VIRQ_XC_RESERVED was reserved for V4V but we have switched >> >> to event channels so this place holder is no longer required. >> > >> > I''m fine with this change, but is a future re-use of the value indeed >> > not going to cause problems on XenServer (or wherever else this >> > is patch set coming from)? >> > >> >> That may need to be confirmed but I don''t think XenServer is using v4v >> yet > > I think Jan probably meant XenClient (i.e. that being the place where > v4v is already deployed). > > There''s no harm in keeping this # reserved indefinitely, with a suitable > comment, I think? The only reason not to would be if this address space > was limited, but I don''t think that is the case with VIRQs > >I think if XenClient rebase to a new version of Xen we will probably use the version of v4v that comes with it and we will not try to rebase the old code on the newer Xen. But if you think we should keep it I don''t mind. Jean
On Mon, 2012-08-06 at 16:01 +0100, Jean Guyader wrote:> On 6 August 2012 15:56, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Mon, 2012-08-06 at 15:46 +0100, Jean Guyader wrote: > >> On 6 August 2012 09:10, Jan Beulich <JBeulich@suse.com> wrote: > >> >>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> wrote: > >> >> VIRQ_XC_RESERVED was reserved for V4V but we have switched > >> >> to event channels so this place holder is no longer required. > >> > > >> > I''m fine with this change, but is a future re-use of the value indeed > >> > not going to cause problems on XenServer (or wherever else this > >> > is patch set coming from)? > >> > > >> > >> That may need to be confirmed but I don''t think XenServer is using v4v > >> yet > > > > I think Jan probably meant XenClient (i.e. that being the place where > > v4v is already deployed). > > > > There''s no harm in keeping this # reserved indefinitely, with a suitable > > comment, I think? The only reason not to would be if this address space > > was limited, but I don''t think that is the case with VIRQs > > > > > > I think if XenClient rebase to a new version of Xen we will probably > use the version of > v4v that comes with it and we will not try to rebase the old code on > the newer Xen.I think Jan''s concern was if a current client runs on some future version of Xen which has reused that VIRQ for something else, some sort of weirdness would probably ensue? Probably not as bad for a VIRQ as reusing a hypercall number...> > But if you think we should keep it I don''t mind. > > Jean
>>> On 06.08.12 at 17:13, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Mon, 2012-08-06 at 16:01 +0100, Jean Guyader wrote: >> On 6 August 2012 15:56, Ian Campbell <Ian.Campbell@citrix.com> wrote: >> > On Mon, 2012-08-06 at 15:46 +0100, Jean Guyader wrote: >> >> On 6 August 2012 09:10, Jan Beulich <JBeulich@suse.com> wrote: >> >> >>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> wrote: >> >> >> VIRQ_XC_RESERVED was reserved for V4V but we have switched >> >> >> to event channels so this place holder is no longer required. >> >> > >> >> > I''m fine with this change, but is a future re-use of the value indeed >> >> > not going to cause problems on XenServer (or wherever else this >> >> > is patch set coming from)? >> >> > >> >> >> >> That may need to be confirmed but I don''t think XenServer is using v4v >> >> yet >> > >> > I think Jan probably meant XenClient (i.e. that being the place where >> > v4v is already deployed). >> > >> > There''s no harm in keeping this # reserved indefinitely, with a suitable >> > comment, I think? The only reason not to would be if this address space >> > was limited, but I don''t think that is the case with VIRQs >> > >> > >> >> I think if XenClient rebase to a new version of Xen we will probably >> use the version of >> v4v that comes with it and we will not try to rebase the old code on >> the newer Xen. > > I think Jan''s concern was if a current client runs on some future > version of Xen which has reused that VIRQ for something else, some sort > of weirdness would probably ensue?That was exactly the point of my inquiry. Jan
At 15:47 +0100 on 06 Aug (1344268059), Jean Guyader wrote:> On 6 August 2012 09:08, Jan Beulich <JBeulich@suse.com> wrote: > >>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> wrote: > > > > Without finally explaining why you need this type in the first place, > > I''ll continue to NAK this patch. (This is made even worse by the fact > > taht the two inline functions in patch 5 that make use of the type > > appear to be unused.) > > > > Understood. I''ll switch to use long instead of ssize_t in my > forthcoming patch serie.Please use an explicitly 64-bit type - AFAICS you''re holding the sum of some 64-bit length fields. Cheers, Tim.
Tim Deegan
2012-Aug-09 10:06 UTC
Re: [PATCH 4/5] xen: events, exposes evtchn_alloc_unbound_domain
At 20:50 +0100 on 03 Aug (1344027053), Jean Guyader wrote:> > Exposes evtchn_alloc_unbound_domain to the rest of > Xen so we can create allocated unbound evtchn within Xen. > > Signed-off-by: Jean Guyader <jean.guyader@citrix.com>> @@ -161,18 +163,18 @@ static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) > { > struct evtchn *chn; > struct domain *d; > - int port; > + evtchn_port_t port; > domid_t dom = alloc->dom; > - long rc; > + int rc;The function returns long; if you''re tidying this up to be an int, might as well change the return type too.> > rc = rcu_lock_target_domain_by_id(dom, &d); > if ( rc ) > return rc; > > - spin_lock(&d->event_lock); > + rc = evtchn_alloc_unbound_domain(d, &port);This moves some of the setting of channel state before the xsm hook. Also, the state changes lower down in this function are no longer under the event_lock. :( Cheers, Tim.
On 9 August 2012 10:51, Tim Deegan <tim@xen.org> wrote:> At 15:47 +0100 on 06 Aug (1344268059), Jean Guyader wrote: >> On 6 August 2012 09:08, Jan Beulich <JBeulich@suse.com> wrote: >> >>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> wrote: >> > >> > Without finally explaining why you need this type in the first place, >> > I''ll continue to NAK this patch. (This is made even worse by the fact >> > taht the two inline functions in patch 5 that make use of the type >> > appear to be unused.) >> > >> >> Understood. I''ll switch to use long instead of ssize_t in my >> forthcoming patch serie. > > Please use an explicitly 64-bit type - AFAICS you''re holding the sum of > some 64-bit length fields. >Ok but ssize_t is kind of a funny one. It should accept everything that size_t can accept + negative values. The linux kernel is defining ssize_t as int for 32b and long for 64b. I could do the same then check for the size of the copy and if it''s bigger than MAX_INT|MAX_LONG return -EMSGSIZE. http://lxr.free-electrons.com/source/include/asm-generic/posix_types.h#L68 (look for __kernel_ssize_t) Jean
Ian Campbell
2012-Aug-09 10:23 UTC
Re: [PATCH 4/5] xen: events, exposes evtchn_alloc_unbound_domain
On Thu, 2012-08-09 at 11:06 +0100, Tim Deegan wrote:> At 20:50 +0100 on 03 Aug (1344027053), Jean Guyader wrote: > > > > Exposes evtchn_alloc_unbound_domain to the rest of > > Xen so we can create allocated unbound evtchn within Xen. > > > > Signed-off-by: Jean Guyader <jean.guyader@citrix.com> > > > @@ -161,18 +163,18 @@ static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) > > { > > struct evtchn *chn; > > struct domain *d; > > - int port; > > + evtchn_port_t port; > > domid_t dom = alloc->dom; > > - long rc; > > + int rc; > > The function returns long; if you''re tidying this up to be an int, might > as well change the return type too.I''m not sure if this is relevant but Jan just sent a patch to "make all (native) hypercalls consistently have "long" return type". I think/suspect this rc here turns into the result of the hypercall? Jan''s patch was motivated by something to do with sign extension when a hypercall''s int return is written to the long in the multicall arg struct which causes strangeness. Perhaps not totally relevant to evtchn_alloc which is unlikely to be in a MC. Ian.
Tim Deegan
2012-Aug-09 10:35 UTC
Re: [PATCH 4/5] xen: events, exposes evtchn_alloc_unbound_domain
At 11:23 +0100 on 09 Aug (1344511426), Ian Campbell wrote:> On Thu, 2012-08-09 at 11:06 +0100, Tim Deegan wrote: > > At 20:50 +0100 on 03 Aug (1344027053), Jean Guyader wrote: > > > > > > Exposes evtchn_alloc_unbound_domain to the rest of > > > Xen so we can create allocated unbound evtchn within Xen. > > > > > > Signed-off-by: Jean Guyader <jean.guyader@citrix.com> > > > > > @@ -161,18 +163,18 @@ static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) > > > { > > > struct evtchn *chn; > > > struct domain *d; > > > - int port; > > > + evtchn_port_t port; > > > domid_t dom = alloc->dom; > > > - long rc; > > > + int rc; > > > > The function returns long; if you''re tidying this up to be an int, might > > as well change the return type too. > > I''m not sure if this is relevant but Jan just sent a patch to "make all > (native) hypercalls consistently have "long" return type". I > think/suspect this rc here turns into the result of the hypercall? > > Jan''s patch was motivated by something to do with sign extension when a > hypercall''s int return is written to the long in the multicall arg > struct which causes strangeness. Perhaps not totally relevant to > evtchn_alloc which is unlikely to be in a MC.Yes, this eventually ends up in a hypercall handler, but s/long/int/ here doesn''t cause problems because - rc is only ever set to an ''int'' value here so we can''t lose data from the type being too narrow; and - Those int values get cast up to long (either in here or in the caller) directly, which will sign-extend the. It really doesn''t matter whether this function returns an int or a long, but it''s a bit untidy to change it half-way. Tim.
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 0x45fe852220b801d4Thanks 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.
>>> On 09.08.12 at 12:19, Jean Guyader <jean.guyader@gmail.com> wrote: > On 9 August 2012 10:51, Tim Deegan <tim@xen.org> wrote: >> At 15:47 +0100 on 06 Aug (1344268059), Jean Guyader wrote: >>> On 6 August 2012 09:08, Jan Beulich <JBeulich@suse.com> wrote: >>> >>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> wrote: >>> > >>> > Without finally explaining why you need this type in the first place, >>> > I''ll continue to NAK this patch. (This is made even worse by the fact >>> > taht the two inline functions in patch 5 that make use of the type >>> > appear to be unused.) >>> > >>> >>> Understood. I''ll switch to use long instead of ssize_t in my >>> forthcoming patch serie. >> >> Please use an explicitly 64-bit type - AFAICS you''re holding the sum of >> some 64-bit length fields. >> > > Ok but ssize_t is kind of a funny one. It should accept everything > that size_t can accept + negative values.No. It''s the same relation as between e.g. "signed int" and "unsigned int". Value preserving conversions are only guaranteed for non-negative values fitting both types. Jan
Jean Guyader
2012-Aug-09 10:40 UTC
Re: [PATCH 4/5] xen: events, exposes evtchn_alloc_unbound_domain
On 9 August 2012 11:35, Tim Deegan <tim@xen.org> wrote:> At 11:23 +0100 on 09 Aug (1344511426), Ian Campbell wrote: >> On Thu, 2012-08-09 at 11:06 +0100, Tim Deegan wrote: >> > At 20:50 +0100 on 03 Aug (1344027053), Jean Guyader wrote: >> > > >> > > Exposes evtchn_alloc_unbound_domain to the rest of >> > > Xen so we can create allocated unbound evtchn within Xen. >> > > >> > > Signed-off-by: Jean Guyader <jean.guyader@citrix.com> >> > >> > > @@ -161,18 +163,18 @@ static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) >> > > { >> > > struct evtchn *chn; >> > > struct domain *d; >> > > - int port; >> > > + evtchn_port_t port; >> > > domid_t dom = alloc->dom; >> > > - long rc; >> > > + int rc; >> > >> > The function returns long; if you''re tidying this up to be an int, might >> > as well change the return type too. >> >> I''m not sure if this is relevant but Jan just sent a patch to "make all >> (native) hypercalls consistently have "long" return type". I >> think/suspect this rc here turns into the result of the hypercall? >> >> Jan''s patch was motivated by something to do with sign extension when a >> hypercall''s int return is written to the long in the multicall arg >> struct which causes strangeness. Perhaps not totally relevant to >> evtchn_alloc which is unlikely to be in a MC. > > Yes, this eventually ends up in a hypercall handler, but s/long/int/ > here doesn''t cause problems because > - rc is only ever set to an ''int'' value here so we can''t lose data > from the type being too narrow; and > - Those int values get cast up to long (either in here or in the > caller) directly, which will sign-extend the. > > It really doesn''t matter whether this function returns an int or a long, > but it''s a bit untidy to change it half-way. >The main reason why I changed it only base ERROR_EXIT_DOM expects an int based on the format string. I guess I could cast the long in int for the call to ERROR_EXIT_DOM but that doesn''t really look nice either. Jean
On 9 August 2012 11:39, Jan Beulich <JBeulich@suse.com> wrote:>>>> On 09.08.12 at 12:19, Jean Guyader <jean.guyader@gmail.com> wrote: >> On 9 August 2012 10:51, Tim Deegan <tim@xen.org> wrote: >>> At 15:47 +0100 on 06 Aug (1344268059), Jean Guyader wrote: >>>> On 6 August 2012 09:08, Jan Beulich <JBeulich@suse.com> wrote: >>>> >>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> wrote: >>>> > >>>> > Without finally explaining why you need this type in the first place, >>>> > I''ll continue to NAK this patch. (This is made even worse by the fact >>>> > taht the two inline functions in patch 5 that make use of the type >>>> > appear to be unused.) >>>> > >>>> >>>> Understood. I''ll switch to use long instead of ssize_t in my >>>> forthcoming patch serie. >>> >>> Please use an explicitly 64-bit type - AFAICS you''re holding the sum of >>> some 64-bit length fields. >>> >> >> Ok but ssize_t is kind of a funny one. It should accept everything >> that size_t can accept + negative values. > > No. It''s the same relation as between e.g. "signed int" and > "unsigned int". Value preserving conversions are only guaranteed > for non-negative values fitting both types. >ssize_t is a *signed* type, I was wrong by saying that it should accept all the range of a size_t, it allows only a subset of it. read/write used ssize_t as a return type. From man 2 read: If count is zero, read() returns zero and has no other results. If count is greater than SSIZE_MAX, the result is unspecified. Would it be ok to claim the same thing here? i.e. if count > INT_MAX the result is unspecified. Jean
At 11:19 +0100 on 09 Aug (1344511142), Jean Guyader wrote:> On 9 August 2012 10:51, Tim Deegan <tim@xen.org> wrote: > > At 15:47 +0100 on 06 Aug (1344268059), Jean Guyader wrote: > >> On 6 August 2012 09:08, Jan Beulich <JBeulich@suse.com> wrote: > >> >>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> wrote: > >> > > >> > Without finally explaining why you need this type in the first place, > >> > I''ll continue to NAK this patch. (This is made even worse by the fact > >> > taht the two inline functions in patch 5 that make use of the type > >> > appear to be unused.) > >> > > >> > >> Understood. I''ll switch to use long instead of ssize_t in my > >> forthcoming patch serie. > > > > Please use an explicitly 64-bit type - AFAICS you''re holding the sum of > > some 64-bit length fields. > > > > Ok but ssize_t is kind of a funny one. It should accept everything > that size_t can accept + negative values.Given that you start with user-supplied 64-bit iov lengths, there is no such type. :) And now that I look at it, your handling of sizes is pretty confused. v4v_ringbuf_insertv() returns a ssize_t, but calculates it internally as an int32_t, which is _smaller_ than the size_t it starts with (which is the sum of some guest-supplied uint64_ts). And then v4v_sendv() puts that ssize_t into an int again before returning it as a size_t, which d_v4v_op() casts as a long to give to the guest. Whee! Can you please make that lot consistent, and then audit the whole path from user-supplied iovec lists to actual copies and make sure that there are no overflows? I think that explicitly limiting your sum-of-iovec-lengths to 31 bits would be perfectly reasonable (1 hypercall per 2GB copy), and would avoid a lot of pain here. As long as it was documented in the interface, of course! Cheers, Tim.
On 9 August 2012 11:59, Tim Deegan <tim@xen.org> wrote:> At 11:19 +0100 on 09 Aug (1344511142), Jean Guyader wrote: >> On 9 August 2012 10:51, Tim Deegan <tim@xen.org> wrote: >> > At 15:47 +0100 on 06 Aug (1344268059), Jean Guyader wrote: >> >> On 6 August 2012 09:08, Jan Beulich <JBeulich@suse.com> wrote: >> >> >>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> wrote: >> >> > >> >> > Without finally explaining why you need this type in the first place, >> >> > I''ll continue to NAK this patch. (This is made even worse by the fact >> >> > taht the two inline functions in patch 5 that make use of the type >> >> > appear to be unused.) >> >> > >> >> >> >> Understood. I''ll switch to use long instead of ssize_t in my >> >> forthcoming patch serie. >> > >> > Please use an explicitly 64-bit type - AFAICS you''re holding the sum of >> > some 64-bit length fields. >> > >> >> Ok but ssize_t is kind of a funny one. It should accept everything >> that size_t can accept + negative values. > > Given that you start with user-supplied 64-bit iov lengths, there is no > such type. :) > > And now that I look at it, your handling of sizes is pretty confused. > v4v_ringbuf_insertv() returns a ssize_t, but calculates it internally as > an int32_t, which is _smaller_ than the size_t it starts with (which is > the sum of some guest-supplied uint64_ts). And then v4v_sendv() puts > that ssize_t into an int again before returning it as a size_t, which > d_v4v_op() casts as a long to give to the guest. Whee! > > Can you please make that lot consistent, and then audit the whole path > from user-supplied iovec lists to actual copies and make sure that there > are no overflows? > > I think that explicitly limiting your sum-of-iovec-lengths to 31 bits > would be perfectly reasonable (1 hypercall per 2GB copy), and would > avoid a lot of pain here. As long as it was documented in the > interface, of course! >Ok fine that works for me. I''ll do that for the next version. Jean
>>> On 09.08.12 at 12:48, Jean Guyader <jean.guyader@gmail.com> wrote: > On 9 August 2012 11:39, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 09.08.12 at 12:19, Jean Guyader <jean.guyader@gmail.com> wrote: >>> On 9 August 2012 10:51, Tim Deegan <tim@xen.org> wrote: >>>> At 15:47 +0100 on 06 Aug (1344268059), Jean Guyader wrote: >>>>> On 6 August 2012 09:08, Jan Beulich <JBeulich@suse.com> wrote: >>>>> >>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> wrote: >>>>> > >>>>> > Without finally explaining why you need this type in the first place, >>>>> > I''ll continue to NAK this patch. (This is made even worse by the fact >>>>> > taht the two inline functions in patch 5 that make use of the type >>>>> > appear to be unused.) >>>>> > >>>>> >>>>> Understood. I''ll switch to use long instead of ssize_t in my >>>>> forthcoming patch serie. >>>> >>>> Please use an explicitly 64-bit type - AFAICS you''re holding the sum of >>>> some 64-bit length fields. >>>> >>> >>> Ok but ssize_t is kind of a funny one. It should accept everything >>> that size_t can accept + negative values. >> >> No. It''s the same relation as between e.g. "signed int" and >> "unsigned int". Value preserving conversions are only guaranteed >> for non-negative values fitting both types. >> > > ssize_t is a *signed* type, I was wrong by saying that it should > accept all the range of a size_t, it allows only > a subset of it. read/write used ssize_t as a return type. > > From man 2 read: > If count is zero, read() returns zero and has no other results. If > count is greater than SSIZE_MAX, the result is unspecified. > > Would it be ok to claim the same thing here? i.e. if count > INT_MAX > the result is unspecified.Sure, except that I think you wanted to use long and hence LONG_MAX. Jan
Jean Guyader
2012-Aug-09 23:25 UTC
Re: [PATCH 4/5] xen: events, exposes evtchn_alloc_unbound_domain
On 09/08 11:40, Jean Guyader wrote:> On 9 August 2012 11:35, Tim Deegan <tim@xen.org> wrote: > > At 11:23 +0100 on 09 Aug (1344511426), Ian Campbell wrote: > >> On Thu, 2012-08-09 at 11:06 +0100, Tim Deegan wrote: > >> > At 20:50 +0100 on 03 Aug (1344027053), Jean Guyader wrote: > >> > > > >> > > Exposes evtchn_alloc_unbound_domain to the rest of > >> > > Xen so we can create allocated unbound evtchn within Xen. > >> > > > >> > > Signed-off-by: Jean Guyader <jean.guyader@citrix.com> > >> > > >> > > @@ -161,18 +163,18 @@ static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) > >> > > { > >> > > struct evtchn *chn; > >> > > struct domain *d; > >> > > - int port; > >> > > + evtchn_port_t port; > >> > > domid_t dom = alloc->dom; > >> > > - long rc; > >> > > + int rc; > >> > > >> > The function returns long; if you''re tidying this up to be an int, might > >> > as well change the return type too. > >> > >> I''m not sure if this is relevant but Jan just sent a patch to "make all > >> (native) hypercalls consistently have "long" return type". I > >> think/suspect this rc here turns into the result of the hypercall? > >> > >> Jan''s patch was motivated by something to do with sign extension when a > >> hypercall''s int return is written to the long in the multicall arg > >> struct which causes strangeness. Perhaps not totally relevant to > >> evtchn_alloc which is unlikely to be in a MC. > > > > Yes, this eventually ends up in a hypercall handler, but s/long/int/ > > here doesn''t cause problems because > > - rc is only ever set to an ''int'' value here so we can''t lose data > > from the type being too narrow; and > > - Those int values get cast up to long (either in here or in the > > caller) directly, which will sign-extend the. > > > > It really doesn''t matter whether this function returns an int or a long, > > but it''s a bit untidy to change it half-way. > > > > The main reason why I changed it only base ERROR_EXIT_DOM expects an int based > on the format string. I guess I could cast the long in int for the > call to ERROR_EXIT_DOM > but that doesn''t really look nice either. >Hi, Here is a new version that should address the comments from Tim and Jan. Signed-off-by: Jean Guyader <jean.guyader@citrix.com> Jean _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jan Beulich
2012-Aug-10 07:35 UTC
Re: [PATCH 4/5] xen: events, exposes evtchn_alloc_unbound_domain
>>> On 10.08.12 at 01:25, Jean Guyader <jean.guyader@citrix.com> wrote: >--- a/xen/common/event_channel.c >+++ b/xen/common/event_channel.c >@@ -159,9 +159,8 @@ static int get_free_port(struct domain *d) > > static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) > { >- struct evtchn *chn; > struct domain *d; >- int port; >+ evtchn_port_t port; > domid_t dom = alloc->dom; > long rc; > >@@ -169,26 +168,47 @@ static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) > if ( rc ) > return rc; > >+ rc = evtchn_alloc_unbound_domain(d, &port,Any reason you can''t pass &alloc->port here directly?>+ alloc->remote_dom == DOMID_SELF ? current->domain->domain_id >+ : alloc->remote_dom);Any reason this can''t/shouldn''t be done in the called function? Jan
Jean Guyader
2012-Aug-10 07:51 UTC
Re: [PATCH 4/5] xen: events, exposes evtchn_alloc_unbound_domain
On 10/08 08:35, Jan Beulich wrote:> >>> On 10.08.12 at 01:25, Jean Guyader <jean.guyader@citrix.com> wrote: > >--- a/xen/common/event_channel.c > >+++ b/xen/common/event_channel.c > >@@ -159,9 +159,8 @@ static int get_free_port(struct domain *d) > > > > static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) > > { > >- struct evtchn *chn; > > struct domain *d; > >- int port; > >+ evtchn_port_t port; > > domid_t dom = alloc->dom; > > long rc; > > > >@@ -169,26 +168,47 @@ static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) > > if ( rc ) > > return rc; > > > >+ rc = evtchn_alloc_unbound_domain(d, &port, > > Any reason you can''t pass &alloc->port here directly? > > >+ alloc->remote_dom == DOMID_SELF ? current->domain->domain_id > >+ : alloc->remote_dom); > > Any reason this can''t/shouldn''t be done in the called function? >No specific reason for both of those thing. Here is a new version based on your comments. Thanks for reviewing, Jean _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jan Beulich
2012-Aug-10 07:57 UTC
Re: [PATCH 4/5] xen: events, exposes evtchn_alloc_unbound_domain
>>> On 10.08.12 at 09:51, Jean Guyader <jean.guyader@citrix.com> wrote: > No specific reason for both of those thing. Here is a new version based > on your comments.Reviewed-by: Jan Beulich <jbeulich@suse.com>
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
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.
On 13 August 2012 10:38, Tim Deegan <tim@xen.org> 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
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.
On 6 August 2012 09:45, Jan Beulich <JBeulich@suse.com> wrote:>>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> 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
Jean Guyader
2012-Aug-23 12:03 UTC
Re: [PATCH 4/5] xen: events, exposes evtchn_alloc_unbound_domain
On 10 August 2012 08:57, Jan Beulich <JBeulich@suse.com> wrote:>>>> On 10.08.12 at 09:51, Jean Guyader <jean.guyader@citrix.com> wrote: >> No specific reason for both of those thing. Here is a new version based >> on your comments. > > Reviewed-by: Jan Beulich <jbeulich@suse.com> >I have found a copy/paste bug in this patch. This statement was executed unconditionally, not just inside the if statement. chn->u.unbound.remote_domid = remote_domid; Attached the fixed version. Jean _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
>>> On 23.08.12 at 13:57, Jean Guyader <jean.guyader@gmail.com> wrote: > On 6 August 2012 09:45, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> 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
On 6 August 2012 01:45, Jan Beulich <JBeulich@suse.com> wrote:>>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> 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. >
On 24 August 2012 13:06, Jan Beulich <JBeulich@suse.com> wrote:>>>> On 23.08.12 at 13:57, Jean Guyader <jean.guyader@gmail.com> wrote: >> On 6 August 2012 09:45, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@citrix.com> 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
Ross Philipson
2013-Jun-11 17:10 UTC
Re: [PATCH 5/5] xen: Add V4V implementation - padding question
> >>> ... >>> +struct v4v_info >>> +{ >>> + uint64_t ring_magic; >>> + uint64_t data_magic; >>> + evtchn_port_t evtchn; >> >> Missing padding at the end? >> > > ack. >At one point during the review of an earlier version of the V4V patch set, Jan requested that this pad be added to the v4v_info struct. I understand the padding in all the other structs but I don''t understand this one. This struct is not included in any other structs and is not followed by any data. Thanks Ross
Tim Deegan
2013-Jun-11 17:25 UTC
Re: [PATCH 5/5] xen: Add V4V implementation - padding question
At 13:10 -0400 on 11 Jun (1370956242), Ross Philipson wrote:> > > >>>... > >>>+struct v4v_info > >>>+{ > >>>+ uint64_t ring_magic; > >>>+ uint64_t data_magic; > >>>+ evtchn_port_t evtchn; > >> > >>Missing padding at the end? > >> > > > >ack. > > > > At one point during the review of an earlier version of the V4V patch > set, Jan requested that this pad be added to the v4v_info struct. I > understand the padding in all the other structs but I don''t understand > this one. This struct is not included in any other structs and is not > followed by any data.64-bit Xen would see this as a 24-byte struct, even without explicit padding. That would surprise a 32-bit guest that allocated what it saw as a 20-byte struct for Xen to copy into. Tim.
Ross Philipson
2013-Jun-11 17:40 UTC
Re: [PATCH 5/5] xen: Add V4V implementation - padding question
On 06/11/2013 01:25 PM, Tim Deegan wrote:> At 13:10 -0400 on 11 Jun (1370956242), Ross Philipson wrote: >>> >>>>> ... >>>>> +struct v4v_info >>>>> +{ >>>>> + uint64_t ring_magic; >>>>> + uint64_t data_magic; >>>>> + evtchn_port_t evtchn; >>>> >>>> Missing padding at the end? >>>> >>> >>> ack. >>> >> >> At one point during the review of an earlier version of the V4V patch >> set, Jan requested that this pad be added to the v4v_info struct. I >> understand the padding in all the other structs but I don''t understand >> this one. This struct is not included in any other structs and is not >> followed by any data. > > 64-bit Xen would see this as a 24-byte struct, even without explicit > padding. That would surprise a 32-bit guest that allocated what it saw > as a 20-byte struct for Xen to copy into.Ah yes, of course. Thanks for the quick response. Ross> > Tim.
Ross Philipson
2013-Jun-11 17:54 UTC
Re: [PATCH 5/5] xen: Add V4V implementation - padding question
On 06/11/2013 01:40 PM, Ross Philipson wrote:> On 06/11/2013 01:25 PM, Tim Deegan wrote: >> At 13:10 -0400 on 11 Jun (1370956242), Ross Philipson wrote: >>>> >>>>>> ... >>>>>> +struct v4v_info >>>>>> +{ >>>>>> + uint64_t ring_magic; >>>>>> + uint64_t data_magic; >>>>>> + evtchn_port_t evtchn; >>>>> >>>>> Missing padding at the end? >>>>> >>>> >>>> ack. >>>> >>> >>> At one point during the review of an earlier version of the V4V patch >>> set, Jan requested that this pad be added to the v4v_info struct. I >>> understand the padding in all the other structs but I don''t understand >>> this one. This struct is not included in any other structs and is not >>> followed by any data. >> >> 64-bit Xen would see this as a 24-byte struct, even without explicit >> padding. That would surprise a 32-bit guest that allocated what it saw >> as a 20-byte struct for Xen to copy into. > > Ah yes, of course. Thanks for the quick response. > > RossI guess that means that this struct is unhappy then... typedef struct v4vtables_rule { v4v_addr_t src; -- 8b v4v_addr_t dst; -- 8b uint32_t accept; -- 4b } v4vtables_rule_t;> >> >> Tim. > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Tim Deegan
2013-Jun-11 18:04 UTC
Re: [PATCH 5/5] xen: Add V4V implementation - padding question
At 13:54 -0400 on 11 Jun (1370958882), Ross Philipson wrote:> On 06/11/2013 01:40 PM, Ross Philipson wrote: > >On 06/11/2013 01:25 PM, Tim Deegan wrote: > >>At 13:10 -0400 on 11 Jun (1370956242), Ross Philipson wrote: > >>>> > >>>>>>... > >>>>>>+struct v4v_info > >>>>>>+{ > >>>>>>+ uint64_t ring_magic; > >>>>>>+ uint64_t data_magic; > >>>>>>+ evtchn_port_t evtchn; > >>>>> > >>>>>Missing padding at the end? > >>>>> > >>>> > >>>>ack. > >>>> > >>> > >>>At one point during the review of an earlier version of the V4V patch > >>>set, Jan requested that this pad be added to the v4v_info struct. I > >>>understand the padding in all the other structs but I don''t understand > >>>this one. This struct is not included in any other structs and is not > >>>followed by any data. > >> > >>64-bit Xen would see this as a 24-byte struct, even without explicit > >>padding. That would surprise a 32-bit guest that allocated what it saw > >>as a 20-byte struct for Xen to copy into. > > > >Ah yes, of course. Thanks for the quick response. > > > >Ross > > I guess that means that this struct is unhappy then... > > typedef struct v4vtables_rule > { > v4v_addr_t src; -- 8b > v4v_addr_t dst; -- 8b > uint32_t accept; -- 4b > } v4vtables_rule_t;Surprisingly, no: since it contains no 8-byte fields, both x86 architectures will see it as a 20-byte (4-byte-aligned) struct. Not sure about ARM, though. Tim.
Jan Beulich
2013-Jun-12 07:45 UTC
Re: [PATCH 5/5] xen: Add V4V implementation - padding question
>>> On 11.06.13 at 20:04, Tim Deegan <tim@xen.org> wrote: > At 13:54 -0400 on 11 Jun (1370958882), Ross Philipson wrote: >> I guess that means that this struct is unhappy then... >> >> typedef struct v4vtables_rule >> { >> v4v_addr_t src; -- 8b >> v4v_addr_t dst; -- 8b >> uint32_t accept; -- 4b >> } v4vtables_rule_t; > > Surprisingly, no: since it contains no 8-byte fields, both x86 > architectures will see it as a 20-byte (4-byte-aligned) struct. > Not sure about ARM, though.That''s why all such structures should go through the public header compat mode checking, so that eventual inconsistencies would be noticed right away rather than at the end of some extended debugging session. Jan
Stefano Stabellini
2013-Jun-13 17:21 UTC
Re: [PATCH 5/5] xen: Add V4V implementation - padding question
On Tue, 11 Jun 2013, Tim Deegan wrote:> At 13:54 -0400 on 11 Jun (1370958882), Ross Philipson wrote: > > On 06/11/2013 01:40 PM, Ross Philipson wrote: > > >On 06/11/2013 01:25 PM, Tim Deegan wrote: > > >>At 13:10 -0400 on 11 Jun (1370956242), Ross Philipson wrote: > > >>>> > > >>>>>>... > > >>>>>>+struct v4v_info > > >>>>>>+{ > > >>>>>>+ uint64_t ring_magic; > > >>>>>>+ uint64_t data_magic; > > >>>>>>+ evtchn_port_t evtchn; > > >>>>> > > >>>>>Missing padding at the end? > > >>>>> > > >>>> > > >>>>ack. > > >>>> > > >>> > > >>>At one point during the review of an earlier version of the V4V patch > > >>>set, Jan requested that this pad be added to the v4v_info struct. I > > >>>understand the padding in all the other structs but I don''t understand > > >>>this one. This struct is not included in any other structs and is not > > >>>followed by any data. > > >> > > >>64-bit Xen would see this as a 24-byte struct, even without explicit > > >>padding. That would surprise a 32-bit guest that allocated what it saw > > >>as a 20-byte struct for Xen to copy into. > > > > > >Ah yes, of course. Thanks for the quick response. > > > > > >Ross > > > > I guess that means that this struct is unhappy then... > > > > typedef struct v4vtables_rule > > { > > v4v_addr_t src; -- 8b > > v4v_addr_t dst; -- 8b > > uint32_t accept; -- 4b > > } v4vtables_rule_t; > > Surprisingly, no: since it contains no 8-byte fields, both x86 > architectures will see it as a 20-byte (4-byte-aligned) struct. > Not sure about ARM, though.same on ARM and ARM64