Xen devel - May 2006 - [patch] [3/3] dom0_ops explicitly sized types

Replace ''long'' in the dom0_ops and privcmd structures with
uint64_t.
As discussed previously, the these operations are not
performance-sensitive, so the additional cache footprint shouldn''t be
an
issue.

Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>

diff -r c1434b779f36 linux-2.6-xen-sparse/include/xen/public/privcmd.h
--- a/linux-2.6-xen-sparse/include/xen/public/privcmd.h	Wed May 24 17:45:02 2006
-0500
+++ b/linux-2.6-xen-sparse/include/xen/public/privcmd.h	Wed May 24 18:03:19 2006
-0500
@@ -39,14 +39,14 @@
 
 typedef struct privcmd_hypercall
 {
-	unsigned long op;
-	unsigned long arg[5];
+	uint64_t op;
+	uint64_t arg[5];
 } privcmd_hypercall_t;
 
 typedef struct privcmd_mmap_entry {
-	unsigned long va;
-	unsigned long mfn;
-	unsigned long npages;
+	uint64_t va;
+	uint64_t mfn;
+	uint64_t npages;
 } privcmd_mmap_entry_t; 
 
 typedef struct privcmd_mmap {
@@ -58,7 +58,7 @@ typedef struct privcmd_mmapbatch {
 typedef struct privcmd_mmapbatch {
 	int num;     /* number of pages to populate */
 	domid_t dom; /* target domain */
-	unsigned long addr;  /* virtual address */
+	uint64_t addr;  /* virtual address */
 	xen_pfn_t __user *arr; /* array of mfns - top nibble set on err */
 } privcmd_mmapbatch_t; 
 
diff -r c1434b779f36 xen/include/public/dom0_ops.h
--- a/xen/include/public/dom0_ops.h	Wed May 24 17:45:02 2006 -0500
+++ b/xen/include/public/dom0_ops.h	Wed May 24 18:03:19 2006 -0500
@@ -19,18 +19,18 @@
  * This makes sure that old versions of dom0 tools will stop working in a
  * well-defined way (rather than crashing the machine, for instance).
  */
-#define DOM0_INTERFACE_VERSION   0x03000000
+#define DOM0_INTERFACE_VERSION   0x03000001
 
 /************************************************************************/
 
 #define DOM0_GETMEMLIST        2
 struct dom0_getmemlist {
     /* IN variables. */
-    domid_t       domain;
-    unsigned long max_pfns;
+    domid_t  domain;
+    uint64_t max_pfns;
     XEN_GUEST_HANDLE(xen_pfn_t) buffer;
     /* OUT variables. */
-    unsigned long num_pfns;
+    uint64_t num_pfns;
 };
 typedef struct dom0_getmemlist dom0_getmemlist_t;
 DEFINE_XEN_GUEST_HANDLE(dom0_getmemlist_t);
@@ -96,9 +96,9 @@ struct dom0_getdomaininfo {
 #define DOMFLAGS_SHUTDOWNMASK 255 /* DOMFLAGS_SHUTDOWN guest-supplied code.  */
 #define DOMFLAGS_SHUTDOWNSHIFT 16
     uint32_t flags;
-    unsigned long tot_pages;
-    unsigned long max_pages;
-    unsigned long shared_info_frame;       /* MFN of shared_info struct */
+    uint64_t tot_pages;
+    uint64_t max_pages;
+    uint64_t shared_info_frame;   /* MFN of shared_info struct */
     uint64_t cpu_time;
     uint32_t nr_online_vcpus;     /* Number of VCPUs currently online. */
     uint32_t max_vcpu_id;         /* Maximum VCPUID in use by this domain. */
@@ -162,7 +162,7 @@ DEFINE_XEN_GUEST_HANDLE(dom0_settime_t);
 
 struct dom0_getpageframeinfo {
     /* IN variables. */
-    unsigned long mfn;     /* Machine page frame number to query.       */
+    uint64_t      mfn;     /* Machine page frame number to query.       */
     domid_t domain;        /* To which domain does the frame belong?    */
     /* OUT variables. */
     /* Is the page PINNED to a type? */
@@ -213,7 +213,7 @@ struct dom0_tbufcontrol {
     cpumap_t      cpu_mask;
     uint32_t      evt_mask;
     /* OUT variables */
-    unsigned long buffer_mfn;
+    uint64_t      buffer_mfn;
     uint32_t size;
 };
 typedef struct dom0_tbufcontrol dom0_tbufcontrol_t;
@@ -229,8 +229,8 @@ struct dom0_physinfo {
     uint32_t sockets_per_node;
     uint32_t nr_nodes;
     uint32_t cpu_khz;
-    unsigned long total_pages;
-    unsigned long free_pages;
+    uint64_t total_pages;
+    uint64_t free_pages;
     uint32_t hw_cap[8];
 };
 typedef struct dom0_physinfo dom0_physinfo_t;
@@ -276,7 +276,7 @@ struct dom0_shadow_control {
     uint32_t       op;
     XEN_GUEST_HANDLE(ulong) dirty_bitmap;
     /* IN/OUT variables. */
-    unsigned long  pages;        /* size of buffer, updated with actual size */
+    uint64_t       pages;        /* size of buffer, updated with actual size */
     /* OUT variables. */
     struct dom0_shadow_control_stats stats;
 };
@@ -286,8 +286,8 @@ DEFINE_XEN_GUEST_HANDLE(dom0_shadow_cont
 #define DOM0_SETDOMAINMAXMEM   28
 struct dom0_setdomainmaxmem {
     /* IN variables. */
-    domid_t       domain;
-    unsigned long max_memkb;
+    domid_t  domain;
+    uint64_t max_memkb;
 };
 typedef struct dom0_setdomainmaxmem dom0_setdomainmaxmem_t;
 DEFINE_XEN_GUEST_HANDLE(dom0_setdomainmaxmem_t);
@@ -295,8 +295,8 @@ DEFINE_XEN_GUEST_HANDLE(dom0_setdomainma
 #define DOM0_GETPAGEFRAMEINFO2 29   /* batched interface */
 struct dom0_getpageframeinfo2 {
     /* IN variables. */
-    domid_t        domain;
-    unsigned long  num;
+    domid_t  domain;
+    uint64_t num;
     /* IN/OUT variables. */
     XEN_GUEST_HANDLE(ulong) array;
 };
@@ -313,12 +313,12 @@ DEFINE_XEN_GUEST_HANDLE(dom0_getpagefram
 #define DOM0_ADD_MEMTYPE         31
 struct dom0_add_memtype {
     /* IN variables. */
-    unsigned long mfn;
-    unsigned long nr_mfns;
-    uint32_t      type;
-    /* OUT variables. */
-    uint32_t      handle;
-    uint32_t      reg;
+    uint64_t mfn;
+    uint64_t nr_mfns;
+    uint32_t type;
+    /* OUT variables. */
+    uint32_t handle;
+    uint32_t reg;
 };
 typedef struct dom0_add_memtype dom0_add_memtype_t;
 DEFINE_XEN_GUEST_HANDLE(dom0_add_memtype_t);
@@ -345,8 +345,8 @@ struct dom0_read_memtype {
     /* IN variables. */
     uint32_t reg;
     /* OUT variables. */
-    unsigned long mfn;
-    unsigned long nr_mfns;
+    uint64_t mfn;
+    uint64_t nr_mfns;
     uint32_t type;
 };
 typedef struct dom0_read_memtype dom0_read_memtype_t;
@@ -499,8 +499,8 @@ DEFINE_XEN_GUEST_HANDLE(dom0_irq_permiss
 #define DOM0_IOMEM_PERMISSION 47
 struct dom0_iomem_permission {
     domid_t  domain;          /* domain to be affected */
-    unsigned long first_mfn;  /* first page (physical page number) in range */
-    unsigned long nr_mfns;    /* number of pages in range (>0) */
+    uint64_t first_mfn;       /* first page (physical page number) in range */
+    uint64_t nr_mfns;         /* number of pages in range (>0) */
     uint8_t allow_access;     /* allow (!0) or deny (0) access to range? */
 };
 typedef struct dom0_iomem_permission dom0_iomem_permission_t;
@@ -509,7 +509,7 @@ DEFINE_XEN_GUEST_HANDLE(dom0_iomem_permi
 #define DOM0_HYPERCALL_INIT   48
 struct dom0_hypercall_init {
     domid_t  domain;          /* domain to be affected */
-    unsigned long mfn;        /* machine frame to be initialised */
+    uint64_t mfn;             /* machine frame to be initialised */
 };
 typedef struct dom0_hypercall_init dom0_hypercall_init_t;
 DEFINE_XEN_GUEST_HANDLE(dom0_hypercall_init_t);




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

On Wed, May 24, 2006 at 06:04:34PM -0500, Hollis Blanchard wrote:
> Replace ''long'' in the dom0_ops and privcmd structures
with uint64_t.
I am all in favor of identifying interfaces using explicitly sized
types. The standard kernel style is to use u64 not uint64_t though.

Cheers,
Muli

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

On Thu, 2006-05-25 at 18:39 +0300, Muli Ben-Yehuda
wrote:
> On Wed, May 24, 2006 at 06:04:34PM -0500, Hollis Blanchard wrote:
> 
> > Replace ''long'' in the dom0_ops and privcmd
structures with uint64_t.
> 
> I am all in favor of identifying interfaces using explicitly sized
> types. The standard kernel style is to use u64 not uint64_t though.
(There is plenty of uint.._t in Linux.)

I assume you''re referring specifically to the privcmd structure change,
which is the only Linux-specific part of the patch. The privcmd
structure is shared between userspace and the kernel. Since "u64" is a
kernel type, we need to use "uint64_t", which has meaning in
userspace.

-- 
Hollis Blanchard
IBM Linux Technology Center


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

On Thu, May 25, 2006 at 02:02:10PM -0500, Hollis Blanchard
wrote:
> On Thu, 2006-05-25 at 18:39 +0300, Muli Ben-Yehuda wrote:
> > On Wed, May 24, 2006 at 06:04:34PM -0500, Hollis Blanchard wrote:
> > 
> > > Replace ''long'' in the dom0_ops and privcmd
structures with uint64_t.
> > 
> > I am all in favor of identifying interfaces using explicitly sized
> > types. The standard kernel style is to use u64 not uint64_t though.
> 
> (There is plenty of uint.._t in Linux.)
Yeah... but. That''s not the Officially Sanctioned Way, except in
headers that are shared with userspace, and in a perfect world no
headers would be shared with userspace.
> I assume you''re referring specifically to the privcmd structure
change,
> which is the only Linux-specific part of the patch. The privcmd
> structure is shared between userspace and the kernel. Since "u64"
is a
> kernel type, we need to use "uint64_t", which has meaning in
> userspace.
Oh well, I didn''t realize it was shared with userspace... sorry for
the false alarm.

Cheers,
Muli

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

On Fri, 2006-05-26 at 16:32 +0300, Muli Ben-Yehuda
wrote:
> > I assume you''re referring specifically to the privcmd
structure change,
> > which is the only Linux-specific part of the patch. The privcmd
> > structure is shared between userspace and the kernel. Since
"u64" is a
> > kernel type, we need to use "uint64_t", which has meaning in
> > userspace.
> 
> Oh well, I didn''t realize it was shared with userspace... sorry
for
> the false alarm.
I think Linus'' opinion[0] is that kernel headers which are shared with
userspace cannot assume that stdint.h has been included so you need to
use __u64 and friends.

Ian.

[0] http://www.ussg.iu.edu/hypermail/linux/kernel/0412.1/1456.html.
There''s been plenty of other traffic in lkml about it too.



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

On Fri, 2006-05-26 at 15:28 +0100, Ian Campbell wrote:
> On Fri, 2006-05-26 at 16:32 +0300, Muli Ben-Yehuda wrote:
> > > I assume you''re referring specifically to the privcmd
structure change,
> > > which is the only Linux-specific part of the patch. The privcmd
> > > structure is shared between userspace and the kernel. Since
"u64" is a
> > > kernel type, we need to use "uint64_t", which has
meaning in
> > > userspace.
> > 
> > Oh well, I didn''t realize it was shared with userspace...
sorry for
> > the false alarm.
> 
> I think Linus'' opinion[0] is that kernel headers which are shared
with
> userspace cannot assume that stdint.h has been included so you need to
> use __u64 and friends.
> 
> Ian.
> 
> [0] http://www.ussg.iu.edu/hypermail/linux/kernel/0412.1/1456.html.
> There''s been plenty of other traffic in lkml about it too.
There certainly has been plenty of traffic, but I have seen no clear
statements (other than "THIS IS BAD" with little explanation). In fact
there was just a thread about it this month
(http://www.ussg.iu.edu/hypermail/linux/kernel/0605.0/0146.html), and I
still don''t understand the objections.

If you use __u64, you''d need to include some header defining what __u64
is, so you''re requiring another header anyways. You might as well use
the standard stdint.h rather than inventing your own.

-- 
Hollis Blanchard
IBM Linux Technology Center


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

On Fri, 2006-05-26 at 10:30 -0500, Hollis Blanchard
wrote:
> On Fri, 2006-05-26 at 15:28 +0100, Ian Campbell wrote:
> > On Fri, 2006-05-26 at 16:32 +0300, Muli Ben-Yehuda wrote:
> > > > I assume you''re referring specifically to the
privcmd structure change,
> > > > which is the only Linux-specific part of the patch. The
privcmd
> > > > structure is shared between userspace and the kernel. Since
"u64" is a
> > > > kernel type, we need to use "uint64_t", which has
meaning in
> > > > userspace.
> > > 
> > > Oh well, I didn''t realize it was shared with
userspace... sorry for
> > > the false alarm.
> > 
> > I think Linus'' opinion[0] is that kernel headers which are
shared with
> > userspace cannot assume that stdint.h has been included so you need to
> > use __u64 and friends.
> > 
> > Ian.
> > 
> > [0] http://www.ussg.iu.edu/hypermail/linux/kernel/0412.1/1456.html.
> > There''s been plenty of other traffic in lkml about it too.
> 
> There certainly has been plenty of traffic, but I have seen no clear
> statements (other than "THIS IS BAD" with little explanation). In
fact
> there was just a thread about it this month
> (http://www.ussg.iu.edu/hypermail/linux/kernel/0605.0/0146.html), and I
> still don''t understand the objections.
> 
> If you use __u64, you''d need to include some header defining what
__u64
> is, so you''re requiring another header anyways. You might as well
use
> the standard stdint.h rather than inventing your own.
As I understand it the issue is that the C spec makes the __* namespace
available for the kernel to pollute and reserves the non-prefixed
namespace for application usage. (Single underscore is for compiler or
libc internals or something).

An application which has not included stdint.h itself can expect to use
uint64_t however it wishes, including making it a struct, a function or
even something as gross as a 32 bit signed int etc. Exporting some other
idea of what uint64_t should be via the kernel headers breaks this. I
guess this is more of a problem when a kernel header gets indirectly
included -- presumably an app which includes a kernel header directly
knows what it is doing.

Anyway, regardless of any opinion expressed here the Linux gatekeepers
will no doubt insist on __uN. We might as well do it now as change it
later.

Ian.



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

Ian Campbell wrote:
>> If you use __u64, you''d need to include some header defining
what __u64
>> is, so you''re requiring another header anyways. You might as
well use
>> the standard stdint.h rather than inventing your own.
>>     
>
> As I understand it the issue is that the C spec makes the __* namespace
> available for the kernel to pollute and reserves the non-prefixed
> namespace for application usage. (Single underscore is for compiler or
> libc internals or something).
>   
The relevant portions of the spec are:


      7.1.3 Reserved identifiers



       [#1] Each header declares or defines all identifiers  listed
       in  its  associated  subclause,  and  optionally declares or
       defines identifiers listed in its associated future  library
       directions   subclause  and  identifiers  which  are  always
       reserved either for  any  use  or  for  use  as  file  scope
       identifiers.

          - All identifiers  that  begin  with  an  underscore  and
            either  an  uppercase  letter or another underscore are
            always reserved for any use.

          - All identifiers  that  begin  with  an  underscore  are
            always  reserved  for  use as macros and as identifiers
            with file scope in  both  the  ordinary  and  tag  name
            spaces.

          - Each macro name in  any  of  the  following  subclauses
            (including  the  future library directions) is reserved
            for use as specified if any of its  associated  headers
            is  included;  unless  explicitly stated otherwise (see
            7.1.8 <http://www.vmunix.com/%7Egabor/c/draft.html#7.1.8>).

          - All identifiers with external linkage  in  any  of  the
            following  subclauses  (including  the  future  library
            directions) are always reserved for use as  identifiers
            with external linkage.133

          - Each identifier with file scope listed in  any  of  the
            following  subclauses  (including  the  future  library
            directions) is reserved for use  as  macro  and  as  an
            identifier  with  file  scope in the same name space if
            any of its associated headers is included.

       [#2] No other identifiers  are  reserved.   If  the  program
       declares  or  defines an identifier that is reserved in that
       context (other than as allowed by 7.1.8
<http://www.vmunix.com/%7Egabor/c/draft.html#7.1.8>),  the  behavior  is
       undefined.134


It''s pretty clear that __* names are reserved.  The kernel is wrong 
here.  The best explanation I''ve gotten is "egocentrism" :-)

Regards,

Anthony Liguori
> An application which has not included stdint.h itself can expect to use
> uint64_t however it wishes, including making it a struct, a function or
> even something as gross as a 32 bit signed int etc. Exporting some other
> idea of what uint64_t should be via the kernel headers breaks this. I
> guess this is more of a problem when a kernel header gets indirectly
> included -- presumably an app which includes a kernel header directly
> knows what it is doing.
>
> Anyway, regardless of any opinion expressed here the Linux gatekeepers
> will no doubt insist on __uN. We might as well do it now as change it
> later.
>
> Ian.
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>   

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

On Fri, 2006-05-26 at 17:27 +0100, Ian Campbell wrote:
> 
> Anyway, regardless of any opinion expressed here the Linux gatekeepers
> will no doubt insist on __uN. We might as well do it now as change it
> later.
And where will userspace, e.g. tools/libxc/xc_linux.c, find the
definition of __u64?

-- 
Hollis Blanchard
IBM Linux Technology Center


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

Anthony Liguori <aliguori@us.ibm.com> writes:
> Ian Campbell wrote:
>>> If you use __u64, you''d need to include some header
defining what __u64
>>> is, so you''re requiring another header anyways. You might
as well use
>>> the standard stdint.h rather than inventing your own.
>>>
>>
>> As I understand it the issue is that the C spec makes the __* namespace
>> available for the kernel to pollute and reserves the non-prefixed
>> namespace for application usage. (Single underscore is for compiler or
>> libc internals or something).
>>
>
> The relevant portions of the spec are:
[...]
> It''s pretty clear that __* names are reserved.  The kernel is
wrong
> here.  The best explanation I''ve gotten is "egocentrism"
:-)
Reserved for what?  The `implementation''.  Which is the combination of
compiler, libc and whatever kernel headers libc lets through.  How
these guys divvy up their reserved name space is up to them, and we
better play by their rules.

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

* Hollis Blanchard (hollisb@us.ibm.com) wrote:
> On Fri, 2006-05-26 at 17:27 +0100, Ian Campbell wrote:
> > 
> > Anyway, regardless of any opinion expressed here the Linux gatekeepers
> > will no doubt insist on __uN. We might as well do it now as change it
> > later.
> 
> And where will userspace, e.g. tools/libxc/xc_linux.c, find the
> definition of __u64?
Same place they do for the rest of shared headers (glib-kernheaders,
for example).  Please use the proper form (u64 internal to kernel, and
__u64 for header visisble to userspace).  I''m going to have to clean it
up anyway.

thanks,
-chris

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

On Fri, May 26, 2006 at 11:57:46AM -0700, Chris Wright wrote:
> > > Anyway, regardless of any opinion expressed here the Linux
gatekeepers
> > > will no doubt insist on __uN. We might as well do it now as
change it
> > > later.
> > 
> > And where will userspace, e.g. tools/libxc/xc_linux.c, find the
> > definition of __u64?
> 
> Same place they do for the rest of shared headers (glib-kernheaders,
> for example).  Please use the proper form (u64 internal to kernel, and
> __u64 for header visisble to userspace).  I''m going to have to
clean it
> up anyway.
This won''t work on platforms that don''t provide __u64. Xen
needs to
define them somewhere for the userspace.

regards
john

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

* John Levon (levon@movementarian.org) wrote:
> This won''t work on platforms that don''t provide __u64.
Xen needs to
> define them somewhere for the userspace.
+++ b/linux-2.6-xen-sparse/include/xen/public/privcmd.h

That''s specifically the part I''m talking about.

WRT Xen''s public headers, that''s a different issue, however
there''s
still some of the same namespace concerns.  Of course, I''d be thrilled
to make it all __u64 in public headers w/ appropriate types.h involved.
And realisitically, platforms will all need some santized version of the
headers anyway.

thanks,
-chris

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

On Fri, May 26, 2006 at 12:57:30PM -0700, Chris Wright wrote:
> > This won''t work on platforms that don''t provide
__u64. Xen needs to
> > define them somewhere for the userspace.
> 
> +++ b/linux-2.6-xen-sparse/include/xen/public/privcmd.h
> 
> That''s specifically the part I''m talking about.
> 
> WRT Xen''s public headers, that''s a different issue,
however there''s
> still some of the same namespace concerns.  Of course, I''d be
thrilled
> to make it all __u64 in public headers w/ appropriate types.h involved.
> And realisitically, platforms will all need some santized version of the
> headers anyway.
Yes, but hopefully it will be possible to provide the structure
definitions in some generic form, possibly by including another header
that has the platform-specific parts (such as ioctl numbers, __u64
definitions on Solaris, etc.)

It would be a shame if we had to duplicate the structure definitions 3
or more times.

regards
john

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