Jan Beulich
2008-Jan-18 14:36 UTC
[Xen-devel] [PATCH] do_callback_op()''s second argument can be const
.. allowing the guest to declare these (mostly static) argument structures ''const'' (a Linux patch to that effect will follow). Instead of introducing a ''const void'' guest handle onn each architecture, the patch moves the common guest handle declarations into a common place. (PowerPC part done just based on analogy, not tested.) Signed-off-by: Jan Beulich <jbeulich@novell.com> Index: 2008-01-07/xen/arch/ia64/xen/hypercall.c ==================================================================--- 2008-01-07.orig/xen/arch/ia64/xen/hypercall.c 2007-12-05 17:13:57.000000000 +0100 +++ 2008-01-07/xen/arch/ia64/xen/hypercall.c 2008-01-07 12:11:43.000000000 +0100 @@ -34,9 +34,6 @@ #include <xen/perfc.h> #include <public/arch-ia64/debug_op.h> -extern long do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg); -extern long do_callback_op(int cmd, XEN_GUEST_HANDLE(void) arg); - static IA64FAULT xen_hypercall (struct pt_regs *regs) { @@ -457,7 +454,7 @@ static long unregister_guest_callback(st /* First time to add callback to xen/ia64, so let''s just stick to * the newer callback interface. */ -long do_callback_op(int cmd, XEN_GUEST_HANDLE(void) arg) +long do_callback_op(int cmd, XEN_GUEST_HANDLE(cvoid) arg) { long ret; Index: 2008-01-07/xen/arch/x86/x86_32/traps.c ==================================================================--- 2008-01-07.orig/xen/arch/x86/x86_32/traps.c 2007-11-26 16:57:03.000000000 +0100 +++ 2008-01-07/xen/arch/x86/x86_32/traps.c 2008-01-07 12:11:43.000000000 +0100 @@ -419,7 +419,7 @@ static long unregister_guest_callback(st } -long do_callback_op(int cmd, XEN_GUEST_HANDLE(void) arg) +long do_callback_op(int cmd, XEN_GUEST_HANDLE(cvoid) arg) { long ret; Index: 2008-01-07/xen/arch/x86/x86_64/traps.c ==================================================================--- 2008-01-07.orig/xen/arch/x86/x86_64/traps.c 2007-11-02 17:25:58.000000000 +0100 +++ 2008-01-07/xen/arch/x86/x86_64/traps.c 2008-01-07 12:11:43.000000000 +0100 @@ -470,7 +470,7 @@ static long unregister_guest_callback(st } -long do_callback_op(int cmd, XEN_GUEST_HANDLE(void) arg) +long do_callback_op(int cmd, XEN_GUEST_HANDLE(cvoid) arg) { long ret; Index: 2008-01-07/xen/include/asm-x86/guest_access.h ==================================================================--- 2008-01-07.orig/xen/include/asm-x86/guest_access.h 2007-12-07 11:51:50.000000000 +0100 +++ 2008-01-07/xen/include/asm-x86/guest_access.h 2008-01-07 12:11:43.000000000 +0100 @@ -34,7 +34,8 @@ */ #define copy_to_guest_offset(hnd, off, ptr, nr) ({ \ const typeof(*(ptr)) *_s = (ptr); \ - char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \ + void *_p = (hnd).p; \ + char (*_d)[sizeof(*_s)] = _p; \ ((void)((hnd).p == (ptr))); \ is_hvm_vcpu(current) ? \ copy_to_user_hvm(_d+(off), _s, sizeof(*_s)*(nr)) : \ @@ -82,7 +83,8 @@ #define __copy_to_guest_offset(hnd, off, ptr, nr) ({ \ const typeof(*(ptr)) *_s = (ptr); \ - char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \ + void *_p = (hnd).p; \ + char (*_d)[sizeof(*_s)] = _p; \ ((void)((hnd).p == (ptr))); \ is_hvm_vcpu(current) ? \ copy_to_user_hvm(_d+(off), _s, sizeof(*_s)*(nr)) : \ Index: 2008-01-07/xen/include/public/arch-ia64.h ==================================================================--- 2008-01-07.orig/xen/include/public/arch-ia64.h 2008-01-07 12:02:52.000000000 +0100 +++ 2008-01-07/xen/include/public/arch-ia64.h 2008-01-07 12:11:43.000000000 +0100 @@ -47,18 +47,7 @@ #endif #ifndef __ASSEMBLY__ -/* Guest handles for primitive C types. */ -__DEFINE_XEN_GUEST_HANDLE(uchar, unsigned char); -__DEFINE_XEN_GUEST_HANDLE(uint, unsigned int); -__DEFINE_XEN_GUEST_HANDLE(ulong, unsigned long); -__DEFINE_XEN_GUEST_HANDLE(u64, unsigned long); -DEFINE_XEN_GUEST_HANDLE(char); -DEFINE_XEN_GUEST_HANDLE(int); -DEFINE_XEN_GUEST_HANDLE(long); -DEFINE_XEN_GUEST_HANDLE(void); - typedef unsigned long xen_pfn_t; -DEFINE_XEN_GUEST_HANDLE(xen_pfn_t); #define PRI_xen_pfn "lx" #endif Index: 2008-01-07/xen/include/public/arch-powerpc.h ==================================================================--- 2008-01-07.orig/xen/include/public/arch-powerpc.h 2008-01-07 12:02:52.000000000 +0100 +++ 2008-01-07/xen/include/public/arch-powerpc.h 2008-01-07 12:11:43.000000000 +0100 @@ -47,17 +47,7 @@ #endif #ifndef __ASSEMBLY__ -/* Guest handles for primitive C types. */ -__DEFINE_XEN_GUEST_HANDLE(uchar, unsigned char); -__DEFINE_XEN_GUEST_HANDLE(uint, unsigned int); -__DEFINE_XEN_GUEST_HANDLE(ulong, unsigned long); -DEFINE_XEN_GUEST_HANDLE(char); -DEFINE_XEN_GUEST_HANDLE(int); -DEFINE_XEN_GUEST_HANDLE(long); -DEFINE_XEN_GUEST_HANDLE(void); - typedef unsigned long long xen_pfn_t; -DEFINE_XEN_GUEST_HANDLE(xen_pfn_t); #define PRI_xen_pfn "llx" #endif Index: 2008-01-07/xen/include/public/arch-x86/xen.h ==================================================================--- 2008-01-07.orig/xen/include/public/arch-x86/xen.h 2008-01-07 12:02:52.000000000 +0100 +++ 2008-01-07/xen/include/public/arch-x86/xen.h 2008-01-07 12:11:43.000000000 +0100 @@ -53,17 +53,7 @@ #endif #ifndef __ASSEMBLY__ -/* Guest handles for primitive C types. */ -__DEFINE_XEN_GUEST_HANDLE(uchar, unsigned char); -__DEFINE_XEN_GUEST_HANDLE(uint, unsigned int); -__DEFINE_XEN_GUEST_HANDLE(ulong, unsigned long); -DEFINE_XEN_GUEST_HANDLE(char); -DEFINE_XEN_GUEST_HANDLE(int); -DEFINE_XEN_GUEST_HANDLE(long); -DEFINE_XEN_GUEST_HANDLE(void); - typedef unsigned long xen_pfn_t; -DEFINE_XEN_GUEST_HANDLE(xen_pfn_t); #define PRI_xen_pfn "lx" #endif Index: 2008-01-07/xen/include/public/xen.h ==================================================================--- 2008-01-07.orig/xen/include/public/xen.h 2008-01-07 12:02:52.000000000 +0100 +++ 2008-01-07/xen/include/public/xen.h 2008-01-07 12:11:43.000000000 +0100 @@ -39,6 +39,20 @@ #error "Unsupported architecture" #endif +#ifndef __ASSEMBLY__ +/* Guest handles for primitive C types. */ +DEFINE_XEN_GUEST_HANDLE(char); +__DEFINE_XEN_GUEST_HANDLE(uchar, unsigned char); +DEFINE_XEN_GUEST_HANDLE(int); +__DEFINE_XEN_GUEST_HANDLE(uint, unsigned int); +DEFINE_XEN_GUEST_HANDLE(long); +__DEFINE_XEN_GUEST_HANDLE(ulong, unsigned long); +DEFINE_XEN_GUEST_HANDLE(void); +__DEFINE_XEN_GUEST_HANDLE(cvoid, const void); + +DEFINE_XEN_GUEST_HANDLE(xen_pfn_t); +#endif + /* * HYPERCALLS */ _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Jan-18 15:00 UTC
Re: [Xen-devel] [PATCH] do_callback_op()''s second argument can be const
This patch doesn''t seem very useful since the guest can trivially cast its way out of the const-ness. I certainly don''t see how the chunk below fixes anything -- doesn''t it take two lines to state what took one line before? -- Keir On 18/1/08 14:36, "Jan Beulich" <jbeulich@novell.com> wrote:> --- 2008-01-07.orig/xen/include/asm-x86/guest_access.h 2007-12-07 > 11:51:50.000000000 +0100 > +++ 2008-01-07/xen/include/asm-x86/guest_access.h 2008-01-07 > 12:11:43.000000000 +0100 > @@ -34,7 +34,8 @@ > */ > #define copy_to_guest_offset(hnd, off, ptr, nr) ({ \ > const typeof(*(ptr)) *_s = (ptr); \ > - char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \ > + void *_p = (hnd).p; \ > + char (*_d)[sizeof(*_s)] = _p; \ > ((void)((hnd).p == (ptr))); \ > is_hvm_vcpu(current) ? \ > copy_to_user_hvm(_d+(off), _s, sizeof(*_s)*(nr)) : \ > @@ -82,7 +83,8 @@ > > #define __copy_to_guest_offset(hnd, off, ptr, nr) ({ \ > const typeof(*(ptr)) *_s = (ptr); \ > - char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \ > + void *_p = (hnd).p; \ > + char (*_d)[sizeof(*_s)] = _p; \ > ((void)((hnd).p == (ptr))); \ > is_hvm_vcpu(current) ? \ > copy_to_user_hvm(_d+(off), _s, sizeof(*_s)*(nr)) : \_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2008-Jan-18 15:05 UTC
Re: [Xen-devel] [PATCH] do_callback_op()''s second argument can be const
>>> Keir Fraser <Keir.Fraser@cl.cam.ac.uk> 18.01.08 16:00 >>> >This patch doesn''t seem very useful since the guest can trivially cast its >way out of the const-ness.Casts should be avoided if possible, especially in C (in C++ you could indeed just cast away the constness without affecting the underlying type). The hypervisor side of this change is to guarantee the constness to the guest side is (to be, in a few seconds) promised by changes to the hypercall wrappers.>I certainly don''t see how the chunk below fixes >anything -- doesn''t it take two lines to state what took one line before?It fixes the build in the context of the other changes. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Jan-18 15:11 UTC
Re: [Xen-devel] [PATCH] do_callback_op()''s second argument can be const
On 18/1/08 15:05, "Jan Beulich" <jbeulich@novell.com> wrote:>> I certainly don''t see how the chunk below fixes >> anything -- doesn''t it take two lines to state what took one line before? > > It fixes the build in the context of the other changes.What''s the difference between assigning from a variable that is void* and assigning from a variable that you have cast to void*? Is this just to get round some stupid compiler warning that shouldn''t happen in the first place? That''s why I hate type attributes: const, volatile, and the rest. Stupid waste of time. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Jan-18 15:44 UTC
Re: [Xen-devel] [PATCH] do_callback_op()''s second argument can be const
On 18/1/08 15:11, "Keir Fraser" <Keir.Fraser@cl.cam.ac.uk> wrote:>>> I certainly don''t see how the chunk below fixes >>> anything -- doesn''t it take two lines to state what took one line before? >> >> It fixes the build in the context of the other changes. > > What''s the difference between assigning from a variable that is void* and > assigning from a variable that you have cast to void*? Is this just to get > round some stupid compiler warning that shouldn''t happen in the first place? > That''s why I hate type attributes: const, volatile, and the rest. Stupid > waste of time.I removed the chunk and I can''t get the build to fail on i386 or x86_64 with gcc 3.4 or gcc 4.1. Perhaps you build with more anal gcc warning settings? -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2008-Jan-18 15:44 UTC
Re: [Xen-devel] [PATCH] do_callback_op()''s second argument can beconst
>>> Keir Fraser <Keir.Fraser@cl.cam.ac.uk> 18.01.08 16:11 >>> >On 18/1/08 15:05, "Jan Beulich" <jbeulich@novell.com> wrote: > >>> I certainly don''t see how the chunk below fixes >>> anything -- doesn''t it take two lines to state what took one line before? >> >> It fixes the build in the context of the other changes. > >What''s the difference between assigning from a variable that is void* and >assigning from a variable that you have cast to void*? Is this just to get >round some stupid compiler warning that shouldn''t happen in the first place? >That''s why I hate type attributes: const, volatile, and the rest. Stupid >waste of time.It''s been a while since I wrote this, and I can''t reproduce any issue anymore when I remove it. I must have forgotten to undo that change once it wasn''t needed anymore - sorry about that. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2008-Jan-18 15:50 UTC
Re: [Xen-devel] [PATCH] do_callback_op()''s second argument can be const
>>> Keir Fraser <Keir.Fraser@cl.cam.ac.uk> 18.01.08 16:44 >>> >On 18/1/08 15:11, "Keir Fraser" <Keir.Fraser@cl.cam.ac.uk> wrote: > >>>> I certainly don''t see how the chunk below fixes >>>> anything -- doesn''t it take two lines to state what took one line before? >>> >>> It fixes the build in the context of the other changes. >> >> What''s the difference between assigning from a variable that is void* and >> assigning from a variable that you have cast to void*? Is this just to get >> round some stupid compiler warning that shouldn''t happen in the first place? >> That''s why I hate type attributes: const, volatile, and the rest. Stupid >> waste of time. > >I removed the chunk and I can''t get the build to fail on i386 or x86_64 with >gcc 3.4 or gcc 4.1. Perhaps you build with more anal gcc warning settings?Neither can I, as said already. I''m not using more strict compiler settings, but one thing I could imagine is that I first tested the constructs involved outside of Xen (and then with -Wall -W) and got some warning. But as said in the other reply, it may also be that this was simply a leftover. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel