v6 changes:
- Check compiler before using [0] or [].
v5 changes:
- Fix prototypes in v4v.h for do_v4v_op
- Add padding to make sure all the structures
are 64 bits aligned
- Replace [0] with []
v4 changes:
- Stop using ssize_t, use long instead
- Return -MSGSIZE for message bigger than 2GB
- Fixup size handling
- Rename protocol with message_type to avoid the
confusion with the IP protocol flag
- Replaced V4V_DOMID_ANY value with DOMID_INVALID
- Replaced v4v_pfn_t with xen_pfn_t
- Add padding to struct v4v_info
- Fixup hypercall documentation
- Move V4V_ROUNDUP to v4v.c
- Remove v4v_utils.h (we could potentially add it later).
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.
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 (3):
xen: virq, remove VIRQ_XC_RESERVED
xen: events, exposes evtchn_alloc_unbound_domain
xen: Add V4V implementation
xen/arch/x86/hvm/hvm.c | 6 +-
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 | 39 +-
xen/common/v4v.c | 1905 ++++++++++++++++++++++++++++++++++++
xen/include/asm-x86/guest_access.h | 3 +
xen/include/public/v4v.h | 324 ++++++
xen/include/public/xen.h | 3 +-
xen/include/xen/event.h | 3 +
xen/include/xen/sched.h | 4 +
xen/include/xen/v4v.h | 133 +++
13 files changed, 2421 insertions(+), 17 deletions(-)
create mode 100644 xen/common/v4v.c
create mode 100644 xen/include/public/v4v.h
create mode 100644 xen/include/xen/v4v.h
--
1.7.9.5
_______________________________________________
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. Signed-off-by: Jan Beulich <JBeulich@suse.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-Sep-20 11:42 UTC
[PATCH 3/4] 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 | 39 +++++++++++++++++++++++++++------------ xen/include/xen/event.h | 3 +++ 2 files changed, 30 insertions(+), 12 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 | 6 +- 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 | 1905 ++++++++++++++++++++++++++++++++++++ xen/include/public/v4v.h | 324 ++++++ xen/include/public/xen.h | 2 +- xen/include/xen/sched.h | 4 + xen/include/xen/v4v.h | 133 +++ 10 files changed, 2388 insertions(+), 4 deletions(-) create mode 100644 xen/common/v4v.c create mode 100644 xen/include/public/v4v.h create mode 100644 xen/include/xen/v4v.h _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
>>> On 20.09.12 at 13:42, Jean Guyader <jean.guyader@citrix.com> wrote: >+ case V4VOP_register_ring: >+ { >+ XEN_GUEST_HANDLE(v4v_ring_t) ring_hnd >+ guest_handle_cast(arg1, v4v_ring_t); >+ XEN_GUEST_HANDLE(xen_pfn_t) pfn_hnd >+ guest_handle_cast(arg2, xen_pfn_t); >+ uint32_t npage = arg3; >+ if ( unlikely(!guest_handle_okay(ring_hnd, 1)) ) >+ goto out; >+ if ( unlikely(!guest_handle_okay(pfn_hnd, npage)) ) >+ goto out;Here and below - this isn''t sufficient for compat guests, or else you give them a way to point into the compat m2p table.>+ if ( unlikely(!guest_handle_okay(addr_hnd, 1)) ) >+ goto out; >+ if ( copy_from_guest(&addr, addr_hnd, 1) ) >+ goto out;The former is redundant with the latter. Also, if you already used guest_handle_okay() on an array, you then can (and should) use the cheaper __copy_{to,from}_guest() functions. While looking at this, I also spotted at least on guest access without error checking. Plus many guest accesses happen with some sort of lock held - that''ll cause problems when the guest memory you''re trying to access was paged out (quite likely you would be able to point out other such cases, but those likely predate paging, and hence are an oversight of whoever introduced the paging code).>+struct v4v_ring_message_header >+{ >+ uint32_t len; >+ uint32_t pad0;Why? v4v_addr_t is 4-byte aligned.>+ v4v_addr_t source; >+ uint32_t message_type; >+ uint32_t pad1;And again, why?>+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L >+ uint8_t data[]; >+#elif defined(__GNUC__) >+ uint8_t data[0]; >+#endif >+};...>+/* >+ * V4VOP_register_ring >+ * >+ * Registers a ring with Xen. If a ring with the same v4v_ring_id exists, >+ * the hypercall will return -EEXIST. >+ * >+ * do_v4v_op(V4VOP_unregister_ring, >+ * XEN_GUEST_HANDLE(v4v_ring_t), XEN_GUEST_HANDLE(xen_pfn_t), >+ * npage, 0) >+ */ >+#define V4VOP_register_ring 1"register" or "unregister"? Also, note the use of v4v_ring_t here, but ...>+/* >+ * V4VOP_unregister_ring >+ * >+ * Unregister a ring. >+ * >+ * do_v4v_op(V4VOP_unregister_ring, >+ * XEN_GUEST_HANDLE(v4v_ring), >+ * NULL, 0, 0) >+ */ >+#define V4VOP_unregister_ring 2... not here. Please be consistent, even if this is just comments. (Again at least for V4VOP_sendv.)>+/* >+ * V4VOP_viptables_list >+ * >+ * Delete a filtering rules at a position or the ruleDelete? Also, here and above, make clear whether you talk about a single or multiple rules.>+ * that matches "rule". >+ * >+ * do_v4v_op(V4VOP_viptables_list, >+ * XEN_GUEST_HANDLE(v4v_vitpables_list_t) list, >+ * NULL, 0, 0) >+ */ >+#define V4VOP_viptables_list 8Also, with all of the padding fields and unused arguments (for certain sub-ops) - if you see the slightest chance of them ever getting used for some extension, you will want to make sure they got zero-filled by the guest. Jan
On 09/20/12 12:42, Jean Guyader 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. >Could we rename "viptables" in "v4vtables", or something that doesn''t contain "IP" in the name ? Since IP is a totally unrelated protocol, it doesn''t make sense here.
On 22 September 2012 14:47, Julian Pidancet <julian.pidancet@gmail.com> wrote:> On 09/20/12 12:42, Jean Guyader 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. >> > > Could we rename "viptables" in "v4vtables", or something that doesn''t > contain "IP" in the name ? Since IP is a totally unrelated protocol, it > doesn''t make sense here. >Ok sure, I don''t have any objections to that. Jean
On 20 September 2012 13:20, Jan Beulich <JBeulich@suse.com> wrote:>>>> On 20.09.12 at 13:42, Jean Guyader <jean.guyader@citrix.com> wrote: >>+ case V4VOP_register_ring: >>+ { >>+ XEN_GUEST_HANDLE(v4v_ring_t) ring_hnd >>+ guest_handle_cast(arg1, v4v_ring_t); >>+ XEN_GUEST_HANDLE(xen_pfn_t) pfn_hnd >>+ guest_handle_cast(arg2, xen_pfn_t); >>+ uint32_t npage = arg3; >>+ if ( unlikely(!guest_handle_okay(ring_hnd, 1)) ) >>+ goto out; >>+ if ( unlikely(!guest_handle_okay(pfn_hnd, npage)) ) >>+ goto out; > > Here and below - this isn''t sufficient for compat guests, or else > you give them a way to point into the compat m2p table. >I''ll probably switch to uint64_t for the v4v mfn list instead of using xen_pfn_t which are unsigned long. That way I can save the need for a compat wrapper.>>+ if ( unlikely(!guest_handle_okay(addr_hnd, 1)) ) >>+ goto out; >>+ if ( copy_from_guest(&addr, addr_hnd, 1) ) >>+ goto out; > > The former is redundant with the latter. Also, if you already > used guest_handle_okay() on an array, you then can (and > should) use the cheaper __copy_{to,from}_guest() functions. > While looking at this, I also spotted at least on guest access > without error checking. Plus many guest accesses happen > with some sort of lock held - that''ll cause problems when the > guest memory you''re trying to access was paged out (quite > likely you would be able to point out other such cases, but > those likely predate paging, and hence are an oversight of > whoever introduced the paging code). >There are plenty of other place in Xen where we copy data from a guest with some sort of lock held. I understand that the new code should do it''s best to make sure it works correctly with the Xen paging code. The best way to solve this might not be try to to avoid copying from guest memory under a lock. I''ve discussed this with Tim and maybe we could introduce a new copy_from_guest which is aware of the paging and returns a specific error, and then we could cleanly unlock everything and issue a continuation that will fixup the memory.>>+struct v4v_ring_message_header >>+{ >>+ uint32_t len; >>+ uint32_t pad0; > > Why? v4v_addr_t is 4-byte aligned. > >>+ v4v_addr_t source; >>+ uint32_t message_type; >>+ uint32_t pad1; > > And again, why? > >>+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L >>+ uint8_t data[]; >>+#elif defined(__GNUC__) >>+ uint8_t data[0]; >>+#endif >>+}; > ... >>+/* >>+ * V4VOP_register_ring >>+ * >>+ * Registers a ring with Xen. If a ring with the same v4v_ring_id exists, >>+ * the hypercall will return -EEXIST. >>+ * >>+ * do_v4v_op(V4VOP_unregister_ring, >>+ * XEN_GUEST_HANDLE(v4v_ring_t), XEN_GUEST_HANDLE(xen_pfn_t), >>+ * npage, 0) >>+ */ >>+#define V4VOP_register_ring 1 > > "register" or "unregister"? > > Also, note the use of v4v_ring_t here, but ... > >>+/* >>+ * V4VOP_unregister_ring >>+ * >>+ * Unregister a ring. >>+ * >>+ * do_v4v_op(V4VOP_unregister_ring, >>+ * XEN_GUEST_HANDLE(v4v_ring), >>+ * NULL, 0, 0) >>+ */ >>+#define V4VOP_unregister_ring 2 > > ... not here. Please be consistent, even if this is just comments. > (Again at least for V4VOP_sendv.) > >>+/* >>+ * V4VOP_viptables_list >>+ * >>+ * Delete a filtering rules at a position or the rule > > Delete? Also, here and above, make clear whether you talk > about a single or multiple rules. > >>+ * that matches "rule". >>+ * >>+ * do_v4v_op(V4VOP_viptables_list, >>+ * XEN_GUEST_HANDLE(v4v_vitpables_list_t) list, >>+ * NULL, 0, 0) >>+ */ >>+#define V4VOP_viptables_list 8 > > Also, with all of the padding fields and unused arguments (for > certain sub-ops) - if you see the slightest chance of them ever > getting used for some extension, you will want to make sure > they got zero-filled by the guest. >Thanks for the review, Jean
>>> On 04.10.12 at 14:03, Jean Guyader <jean.guyader@gmail.com> wrote: > On 20 September 2012 13:20, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 20.09.12 at 13:42, Jean Guyader <jean.guyader@citrix.com> wrote: >>>+ case V4VOP_register_ring: >>>+ { >>>+ XEN_GUEST_HANDLE(v4v_ring_t) ring_hnd >>>+ guest_handle_cast(arg1, v4v_ring_t); >>>+ XEN_GUEST_HANDLE(xen_pfn_t) pfn_hnd >>>+ guest_handle_cast(arg2, xen_pfn_t); >>>+ uint32_t npage = arg3; >>>+ if ( unlikely(!guest_handle_okay(ring_hnd, 1)) ) >>>+ goto out; >>>+ if ( unlikely(!guest_handle_okay(pfn_hnd, npage)) ) >>>+ goto out; >> >> Here and below - this isn''t sufficient for compat guests, or else >> you give them a way to point into the compat m2p table. >> > > I''ll probably switch to uint64_t for the v4v mfn list instead of using > xen_pfn_t which > are unsigned long. That way I can save the need for a compat wrapper.But that comment of yours doesn''t address the problem I pointed out.>>>+ if ( unlikely(!guest_handle_okay(addr_hnd, 1)) ) >>>+ goto out; >>>+ if ( copy_from_guest(&addr, addr_hnd, 1) ) >>>+ goto out; >> >> The former is redundant with the latter. Also, if you already >> used guest_handle_okay() on an array, you then can (and >> should) use the cheaper __copy_{to,from}_guest() functions. >> While looking at this, I also spotted at least on guest access >> without error checking. Plus many guest accesses happen >> with some sort of lock held - that''ll cause problems when the >> guest memory you''re trying to access was paged out (quite >> likely you would be able to point out other such cases, but >> those likely predate paging, and hence are an oversight of >> whoever introduced the paging code). >> > > There are plenty of other place in Xen where we copy data from > a guest with some sort of lock held. > > I understand that the new code should do it''s best to make sure > it works correctly with the Xen paging code. > > The best way to solve this might not be try to to avoid copying from > guest memory under a lock. I''ve discussed this with Tim and maybe > we could introduce a new copy_from_guest which is aware of the paging > and returns a specific error, and then we could cleanly unlock everything > and issue a continuation that will fixup the memory.That''s certainly an alternative. Jan
On 4 October 2012 13:11, Jan Beulich <JBeulich@suse.com> wrote:>>>> On 04.10.12 at 14:03, Jean Guyader <jean.guyader@gmail.com> wrote: >> On 20 September 2012 13:20, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 20.09.12 at 13:42, Jean Guyader <jean.guyader@citrix.com> wrote: >>>>+ case V4VOP_register_ring: >>>>+ { >>>>+ XEN_GUEST_HANDLE(v4v_ring_t) ring_hnd >>>>+ guest_handle_cast(arg1, v4v_ring_t); >>>>+ XEN_GUEST_HANDLE(xen_pfn_t) pfn_hnd >>>>+ guest_handle_cast(arg2, xen_pfn_t); >>>>+ uint32_t npage = arg3; >>>>+ if ( unlikely(!guest_handle_okay(ring_hnd, 1)) ) >>>>+ goto out; >>>>+ if ( unlikely(!guest_handle_okay(pfn_hnd, npage)) ) >>>>+ goto out; >>> >>> Here and below - this isn''t sufficient for compat guests, or else >>> you give them a way to point into the compat m2p table. >>> >> >> I''ll probably switch to uint64_t for the v4v mfn list instead of using >> xen_pfn_t which >> are unsigned long. That way I can save the need for a compat wrapper. > > But that comment of yours doesn''t address the problem I pointed > out. > >>>>+ if ( unlikely(!guest_handle_okay(addr_hnd, 1)) ) >>>>+ goto out; >>>>+ if ( copy_from_guest(&addr, addr_hnd, 1) ) >>>>+ goto out; >>> >>> The former is redundant with the latter. Also, if you already >>> used guest_handle_okay() on an array, you then can (and >>> should) use the cheaper __copy_{to,from}_guest() functions. >>> While looking at this, I also spotted at least on guest access >>> without error checking. Plus many guest accesses happen >>> with some sort of lock held - that''ll cause problems when the >>> guest memory you''re trying to access was paged out (quite >>> likely you would be able to point out other such cases, but >>> those likely predate paging, and hence are an oversight of >>> whoever introduced the paging code). >>> >> >> There are plenty of other place in Xen where we copy data from >> a guest with some sort of lock held. >> >> I understand that the new code should do it''s best to make sure >> it works correctly with the Xen paging code. >> >> The best way to solve this might not be try to to avoid copying from >> guest memory under a lock. I''ve discussed this with Tim and maybe >> we could introduce a new copy_from_guest which is aware of the paging >> and returns a specific error, and then we could cleanly unlock everything >> and issue a continuation that will fixup the memory. > > That''s certainly an alternative. >Another way (probably a easier way) would be to add a "mlock" mechanism. Some of the pages could be lock which mean that we know before hand that a copy_from_guest won''t raise a paging error. In my case it would be ideal. I can comment it out in the register/unregister ring code, so when we have such mechanism in place the v4v code could be updated. Jean
On 4 October 2012 13:11, Jan Beulich <JBeulich@suse.com> wrote:>>>> On 04.10.12 at 14:03, Jean Guyader <jean.guyader@gmail.com> wrote: >> On 20 September 2012 13:20, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 20.09.12 at 13:42, Jean Guyader <jean.guyader@citrix.com> wrote: >>>>+ case V4VOP_register_ring: >>>>+ { >>>>+ XEN_GUEST_HANDLE(v4v_ring_t) ring_hnd >>>>+ guest_handle_cast(arg1, v4v_ring_t); >>>>+ XEN_GUEST_HANDLE(xen_pfn_t) pfn_hnd >>>>+ guest_handle_cast(arg2, xen_pfn_t); >>>>+ uint32_t npage = arg3; >>>>+ if ( unlikely(!guest_handle_okay(ring_hnd, 1)) ) >>>>+ goto out; >>>>+ if ( unlikely(!guest_handle_okay(pfn_hnd, npage)) ) >>>>+ goto out; >>> >>> Here and below - this isn''t sufficient for compat guests, or else >>> you give them a way to point into the compat m2p table. >>> >> >> I''ll probably switch to uint64_t for the v4v mfn list instead of using >> xen_pfn_t which >> are unsigned long. That way I can save the need for a compat wrapper. > > But that comment of yours doesn''t address the problem I pointed > out. >[Resent, CCing everyone this time] I''m sorry, I don''t really get what you mean them. I''ve tried to get all my struct layout such as all the offset for the field are the same for 64b and 32b, this way I thought I could get away with doing a compat wrapper. Thanks, Jean
>>> On 04.10.12 at 17:02, Jean Guyader <jean.guyader@gmail.com> wrote: > On 4 October 2012 13:11, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 04.10.12 at 14:03, Jean Guyader <jean.guyader@gmail.com> wrote: >>> On 20 September 2012 13:20, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 20.09.12 at 13:42, Jean Guyader <jean.guyader@citrix.com> wrote: >>>>>+ case V4VOP_register_ring: >>>>>+ { >>>>>+ XEN_GUEST_HANDLE(v4v_ring_t) ring_hnd >>>>>+ guest_handle_cast(arg1, v4v_ring_t); >>>>>+ XEN_GUEST_HANDLE(xen_pfn_t) pfn_hnd >>>>>+ guest_handle_cast(arg2, xen_pfn_t); >>>>>+ uint32_t npage = arg3; >>>>>+ if ( unlikely(!guest_handle_okay(ring_hnd, 1)) ) >>>>>+ goto out; >>>>>+ if ( unlikely(!guest_handle_okay(pfn_hnd, npage)) ) >>>>>+ goto out; >>>> >>>> Here and below - this isn''t sufficient for compat guests, or else >>>> you give them a way to point into the compat m2p table. >>>> >>> >>> I''ll probably switch to uint64_t for the v4v mfn list instead of using >>> xen_pfn_t which >>> are unsigned long. That way I can save the need for a compat wrapper. >> >> But that comment of yours doesn''t address the problem I pointed >> out. >> > > I''m sorry, I don''t really get what you mean them. I''ve tried to get > all my struct > layout such as all the offset for the field are the same for 64b and 32b, > this > way I thought I could get away with doing a compat wrapper.guest_handle_okay() simply isn''t suitable for checking compat mode pointers - compat_handle_okay() needs to be used for them instead. Jan
At 16:03 +0100 on 04 Oct (1349366589), Jean Guyader wrote:> On 4 October 2012 13:11, Jan Beulich <JBeulich@suse.com> wrote: > >>>> On 04.10.12 at 14:03, Jean Guyader <jean.guyader@gmail.com> wrote: > >> On 20 September 2012 13:20, Jan Beulich <JBeulich@suse.com> wrote: > >>>>>> On 20.09.12 at 13:42, Jean Guyader <jean.guyader@citrix.com> wrote: > >>>>+ case V4VOP_register_ring: > >>>>+ { > >>>>+ XEN_GUEST_HANDLE(v4v_ring_t) ring_hnd > >>>>+ guest_handle_cast(arg1, v4v_ring_t); > >>>>+ XEN_GUEST_HANDLE(xen_pfn_t) pfn_hnd > >>>>+ guest_handle_cast(arg2, xen_pfn_t); > >>>>+ uint32_t npage = arg3; > >>>>+ if ( unlikely(!guest_handle_okay(ring_hnd, 1)) ) > >>>>+ goto out; > >>>>+ if ( unlikely(!guest_handle_okay(pfn_hnd, npage)) ) > >>>>+ goto out; > >>> > >>> Here and below - this isn''t sufficient for compat guests, or else > >>> you give them a way to point into the compat m2p table. > >>> > >> > >> I''ll probably switch to uint64_t for the v4v mfn list instead of using > >> xen_pfn_t which > >> are unsigned long. That way I can save the need for a compat wrapper. > > > > But that comment of yours doesn''t address the problem I pointed > > out. > > > > [Resent, CCing everyone this time] > > I''m sorry, I don''t really get what you mean them. I''ve tried to get > all my struct > layout such as all the offset for the field are the same for 64b and 32b, this > way I thought I could get away with doing a compat wrapper.Even if the args don''t need translation, compat-mode guests have different VA layouts and need different range checks (though I''m not sure why these aren''t automatically adjusted based on current). AIUI you need to use compat_handle_okay() instead of guest_handle_okay() to check the handles if is_pv_32on64_domain(current). Tim.
On 4 October 2012 16:12, Tim Deegan <tim@xen.org> wrote:> At 16:03 +0100 on 04 Oct (1349366589), Jean Guyader wrote: >> On 4 October 2012 13:11, Jan Beulich <JBeulich@suse.com> wrote: >> >>>> On 04.10.12 at 14:03, Jean Guyader <jean.guyader@gmail.com> wrote: >> >> On 20 September 2012 13:20, Jan Beulich <JBeulich@suse.com> wrote: >> >>>>>> On 20.09.12 at 13:42, Jean Guyader <jean.guyader@citrix.com> wrote: >> >>>>+ case V4VOP_register_ring: >> >>>>+ { >> >>>>+ XEN_GUEST_HANDLE(v4v_ring_t) ring_hnd >> >>>>+ guest_handle_cast(arg1, v4v_ring_t); >> >>>>+ XEN_GUEST_HANDLE(xen_pfn_t) pfn_hnd >> >>>>+ guest_handle_cast(arg2, xen_pfn_t); >> >>>>+ uint32_t npage = arg3; >> >>>>+ if ( unlikely(!guest_handle_okay(ring_hnd, 1)) ) >> >>>>+ goto out; >> >>>>+ if ( unlikely(!guest_handle_okay(pfn_hnd, npage)) ) >> >>>>+ goto out; >> >>> >> >>> Here and below - this isn''t sufficient for compat guests, or else >> >>> you give them a way to point into the compat m2p table. >> >>> >> >> >> >> I''ll probably switch to uint64_t for the v4v mfn list instead of using >> >> xen_pfn_t which >> >> are unsigned long. That way I can save the need for a compat wrapper. >> > >> > But that comment of yours doesn''t address the problem I pointed >> > out. >> > >> >> [Resent, CCing everyone this time] >> >> I''m sorry, I don''t really get what you mean them. I''ve tried to get >> all my struct >> layout such as all the offset for the field are the same for 64b and 32b, this >> way I thought I could get away with doing a compat wrapper. > > Even if the args don''t need translation, compat-mode guests have > different VA layouts and need different range checks (though I''m not > sure why these aren''t automatically adjusted based on current). > > AIUI you need to use compat_handle_okay() instead of guest_handle_okay() > to check the handles if is_pv_32on64_domain(current). >How about something like that? Jean _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
>>> On 04.10.12 at 19:00, Jean Guyader <jean.guyader@gmail.com> wrote: > On 4 October 2012 16:12, Tim Deegan <tim@xen.org> wrote: >> AIUI you need to use compat_handle_okay() instead of guest_handle_okay() >> to check the handles if is_pv_32on64_domain(current). >> > > How about something like that?Could be done, but not by modifying guest_handle_okay() (which penalizes all its users), nor by (incorrectly) open-coding compat_handle_okay(). And neither should any such implementation use is_pv_32on64_domain() twice - use the conditional operator instead (that way you also avoid evaluating arguments twice). So you could either introduce e.g. any_guest_handle_okay(), or switch all current users of guest_handle_okay() to, say, native_handle_okay() (perhaps with the exception of those where the compat mode wrapper source files #define guest_handle_okay() to compat_handle_okay(), which could then be dropped). Jan
>>> On 05.10.12 at 10:52, "Jan Beulich" <JBeulich@suse.com> wrote: >>>> On 04.10.12 at 19:00, Jean Guyader <jean.guyader@gmail.com> wrote: >> On 4 October 2012 16:12, Tim Deegan <tim@xen.org> wrote: >>> AIUI you need to use compat_handle_okay() instead of guest_handle_okay() >>> to check the handles if is_pv_32on64_domain(current). >>> >> >> How about something like that? > > Could be done, but not by modifying guest_handle_okay() (which > penalizes all its users), nor by (incorrectly) open-coding > compat_handle_okay(). And neither should any such implementation > use is_pv_32on64_domain() twice - use the conditional operator > instead (that way you also avoid evaluating arguments twice). > > So you could either introduce e.g. any_guest_handle_okay(), or > switch all current users of guest_handle_okay() to, say, > native_handle_okay() (perhaps with the exception of those where > the compat mode wrapper source files #define guest_handle_okay() > to compat_handle_okay(), which could then be dropped).Actually, after some more recalling of how all this was put together, you can use the existing guest_handle_okay() on anything that legitimately is a XEN_GUEST_HANDLE(). In particular, direct hypercall arguments can be expressed that way since they get properly zero-extended from 32 to 64 bits on the hypercall entry path. The things needing extra consideration are only handles embedded in structures - you need to either translate these handles, or validate that their upper halves are zero. That''s also one of the reasons why I didn''t make the guest memory accessor/validation macros transparently handle both cases. Jan
On 5 October 2012 10:07, Jan Beulich <JBeulich@suse.com> wrote:>>>> On 05.10.12 at 10:52, "Jan Beulich" <JBeulich@suse.com> wrote: >>>>> On 04.10.12 at 19:00, Jean Guyader <jean.guyader@gmail.com> wrote: >>> On 4 October 2012 16:12, Tim Deegan <tim@xen.org> wrote: >>>> AIUI you need to use compat_handle_okay() instead of guest_handle_okay() >>>> to check the handles if is_pv_32on64_domain(current). >>>> >>> >>> How about something like that? >> >> Could be done, but not by modifying guest_handle_okay() (which >> penalizes all its users), nor by (incorrectly) open-coding >> compat_handle_okay(). And neither should any such implementation >> use is_pv_32on64_domain() twice - use the conditional operator >> instead (that way you also avoid evaluating arguments twice). >> >> So you could either introduce e.g. any_guest_handle_okay(), or >> switch all current users of guest_handle_okay() to, say, >> native_handle_okay() (perhaps with the exception of those where >> the compat mode wrapper source files #define guest_handle_okay() >> to compat_handle_okay(), which could then be dropped). > > Actually, after some more recalling of how all this was put together, > you can use the existing guest_handle_okay() on anything that > legitimately is a XEN_GUEST_HANDLE(). In particular, direct > hypercall arguments can be expressed that way since they get > properly zero-extended from 32 to 64 bits on the hypercall entry > path. The things needing extra consideration are only handles > embedded in structures - you need to either translate these > handles, or validate that their upper halves are zero. > > That''s also one of the reasons why I didn''t make the guest > memory accessor/validation macros transparently handle both > cases. >Ok, thanks for the background info. In my hypercall I only use direct hypercall argument handle. The other handles that I use have been created with guest_handle_add_offset from the original argument handle. Considering this, I think I might be ok. Thanks, Jean