Re: [Xen-devel] [PATCH] linux/privcmd: fix for proper operation in compat mode

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

>>> Simon Horman <horms@xxxxxxxxxxxx> 06.01.10 03:27 >>>
>On Tue, Jan 05, 2010 at 12:52:04PM +0000, Jan Beulich wrote:
>...
>> +#ifdef xen_pfn32_t
>> +            for (i = 0; !ret && i < n32.num; ++i) {
>> +                    xen_pfn_t mfn;
>> +
>> +                    if (get_user(mfn, arr + i) || put_user(mfn, arr32 + i))
>> +                            ret = -EFAULT;
>> +                    else if (mfn != (xen_pfn32_t)mfn)
>> +                            ret = -ERANGE;
>> +            }
>> +#endif
>>      }
>>              break;
>>      default:
>
>Perhaps this could be refactored to reduce or remove the #ifdefs ?

I couldn't think of a clean way.

>> --- head-2010-01-04.orig/include/xen/compat_ioctl.h  2007-07-10 
>> 09:42:30.000000000 +0200
>> +++ head-2010-01-04/include/xen/compat_ioctl.h       2009-12-17 
>> 15:40:40.000000000 +0100
>> @@ -23,6 +23,11 @@
>>  #define __LINUX_XEN_COMPAT_H__ 
>>  
>>  #include <linux/compat.h>
>> +#include <linux/compiler.h>
>> +
>> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
>> +#define xen_pfn32_t __u32
>> +#endif
>
>Is it usual in xen for a type to be a #define in xen?
>
>Ito my mind it would make things a lot clearer to use a #define called
>SOME_FEATURE and then either use __u32 directly for the type or
>typedef __u32 xen_pfn32_t.

This would be an alternative, but I don't think the code would look
much better afterwards.

>> @@ -34,7 +39,14 @@ struct privcmd_mmap_32 {
>>  struct privcmd_mmapbatch_32 {
>>      int num;     /* number of pages to populate */
>>      domid_t dom; /* target domain */
>> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
>> +    union {      /* virtual address */
>> +            __u64 addr __packed;
>> +            __u32 va;
>> +    };
>> +#else
>>      __u64 addr;  /* virtual address */
>> +#endif
>>      compat_uptr_t arr; /* array of mfns - top nibble set on err */
>>  };
>>  #define IOCTL_PRIVCMD_MMAP_32                   \
>
>Its unclear to me why va is necessary.
>It doesn't seem to be used anywhere in the patch.

With the addr field being __packed, the va field ensures the union is
4-byte aligned (as it is in the native 32-bit type).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel