Skip to contentSkip to conversation

do_callback_op()'s second argument can be const

PATCH In review Jan Beulich 2 participants 7 messages Jan 18, 2008

lore
Share & tools
lore

Jan Beulich

author
#0 lore raw

PATCH 1/1 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 <[email protected]>
xen/arch/ia64/xen/hypercall.c +1 -4
Index: 2008-01-07/xen/arch/ia64/xen/hypercall.c
===================================================================
@@ -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;
xen/arch/x86/x86_32/traps.c +1 -1
Index: 2008-01-07/xen/arch/x86/x86_32/traps.c
===================================================================
@@ -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;
xen/arch/x86/x86_64/traps.c +1 -1
Index: 2008-01-07/xen/arch/x86/x86_64/traps.c
===================================================================
@@ -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;
xen/include/asm-x86/guest_access.h +4 -2
Index: 2008-01-07/xen/include/asm-x86/guest_access.h
===================================================================
@@ -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/include/public/arch-ia64.h +0 -11
Index: 2008-01-07/xen/include/public/arch-ia64.h
===================================================================
@@ -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
xen/include/public/arch-powerpc.h +0 -10
Index: 2008-01-07/xen/include/public/arch-powerpc.h
===================================================================
@@ -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
xen/include/public/arch-x86/xen.h +0 -10
Index: 2008-01-07/xen/include/public/arch-x86/xen.h
===================================================================
@@ -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
xen/include/public/xen.h +14 -0
Index: 2008-01-07/xen/include/public/xen.h
===================================================================
@@ -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
  */
#1 lore raw

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

Quoted 24 lines · Jan Beulich

--- 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

xen/include/asm-x86/guest_access.h +4 -2
@@ -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)) :  \
author
#2 lore raw
Quoted 3 lines
Quoted 1 line
Quoted 1 line

Keir Fraser <[email protected]> 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.

Quoted 2 lines

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

#3 lore raw
Quoted 4 lines · Jan Beulich
Quoted 2 lines

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

#4 lore raw
Quoted 10 lines · Keir Fraser
Quoted 4 lines
Quoted 2 lines

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

author
#5 lore raw
Quoted 8 lines
Quoted 1 line
Quoted 1 line

Keir Fraser <[email protected]> 18.01.08 16:44 >>>

Quoted 5 lines · Keir Fraser
Quoted 4 lines
Quoted 2 lines

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.

Quoted 8 lines
Quoted 5 lines

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

author
#6 lore raw

Re: [PATCH] do_callback_op()'s second argument can beconst

In reply to Keir Fraser
Quoted 6 lines
Quoted 1 line
Quoted 1 line

Keir Fraser <[email protected]> 18.01.08 16:11 >>>

Quoted 3 lines · Jan Beulich
Quoted 2 lines

I certainly don't see how the chunk below fixes
anything -- doesn't it take two lines to state what took one line before?

Quoted 7 lines
Quoted 1 line

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