Re: [rfc] [patch] "frame number" size in hypercall ABI

Re: [rfc] [patch] "frame number" size in hypercall ABI

[Joe Bonasera]

> Message: 3
> Date: Wed, 19 Apr 2006 08:24:44 +0100
> From: Keir Fraser <Keir.Fraser@cl.cam.ac.uk>
> Subject: [Xen-devel] Re: [rfc] [patch] "frame number" size in
> 	hypercall ABI
> To: Hollis Blanchard <hollisb@us.ibm.com>
> Cc: xen-devel <xen-devel@lists.xensource.com>
> Message-ID: <e3a01a5452fa05ddd61fde154e01d557@cl.cam.ac.uk>
> Content-Type: text/plain; charset=US-ASCII; format=flowed
> 
> 
> On 18 Apr 2006, at 20:17, Hollis Blanchard wrote:
> 
...
>>
>>I also haven't converted Xen itself to use frameno_t except where
>>absolutely necessary. It isn't required to solve my immediate 
>>problem...
> 
> 
> I think the patch looks okay in principle, except I notice a couple of 
> 'framno_t' misspellings. I think 'pfn_t' would be more in keeping with 
> the Xen naming scheme for arbitrary page frame numbers, but perhaps 
> that name is a bit unnecessarily cryptic. What is the immediate problem 
> you're needing to solve?
> 
>   -- Keir

Please don't use pfn_t, that's what Solaris already uses internally for
physical frame numbers. In the Xen code for Solaris, I introduced
mfn_t as the corresponding type for Xen's machine frame numbers.
frameno_t is fine, as I can just define mfn_t and frameno_t to be
the same, but conflicting pfn_t's would be problematic.

Joe

Re: Re: [rfc] [patch] "frame number" size in hypercall ABI

[Keir Fraser]

On 19 Apr 2006, at 16:32, Joe Bonasera wrote:

>> I think the patch looks okay in principle, except I notice a couple 
>> of 'framno_t' misspellings. I think 'pfn_t' would be more in keeping 
>> with the Xen naming scheme for arbitrary page frame numbers, but 
>> perhaps that name is a bit unnecessarily cryptic. What is the 
>> immediate problem you're needing to solve?
>>   -- Keir
>
> Please don't use pfn_t, that's what Solaris already uses internally for
> physical frame numbers. In the Xen code for Solaris, I introduced
> mfn_t as the corresponding type for Xen's machine frame numbers.
> frameno_t is fine, as I can just define mfn_t and frameno_t to be
> the same, but conflicting pfn_t's would be problematic.

Fair enough. We should really avoid polluting the typedef namespace 
with such short names anyway.

  -- Keir

Re: [patch] "frame number" size in hypercall ABI

[Hollis Blanchard]

On Wed, 2006-04-19 at 16:43 +0100, Keir Fraser wrote:
> On 19 Apr 2006, at 16:32, Joe Bonasera wrote:
> 
> >> I think the patch looks okay in principle, except I notice a couple 
> >> of 'framno_t' misspellings. I think 'pfn_t' would be more in keeping 
> >> with the Xen naming scheme for arbitrary page frame numbers, but 
> >> perhaps that name is a bit unnecessarily cryptic. What is the 
> >> immediate problem you're needing to solve?
> >>   -- Keir
> >
> > Please don't use pfn_t, that's what Solaris already uses internally for
> > physical frame numbers. In the Xen code for Solaris, I introduced
> > mfn_t as the corresponding type for Xen's machine frame numbers.
> > frameno_t is fine, as I can just define mfn_t and frameno_t to be
> > the same, but conflicting pfn_t's would be problematic.
> 
> Fair enough. We should really avoid polluting the typedef namespace 
> with such short names anyway.

"xen_frameno_t" then?

Attached is the updated patch, with typos fixed and a couple other
corrections. I've also added the type to arch-x86_64.h and arch-ia64.h,
so I think the patch is ready to be applied.

-- 
Hollis Blanchard
IBM Linux Technology Center

Represent frame numbers with a typedef instead of "unsigned long".

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

diff -r c4eead8a925b linux-2.6-xen-sparse/include/xen/public/privcmd.h
--- a/linux-2.6-xen-sparse/include/xen/public/privcmd.h	Sun Apr 16 15:41:31 2006 +0100
+++ b/linux-2.6-xen-sparse/include/xen/public/privcmd.h	Wed Apr 19 11:39:31 2006 -0500
@@ -45,7 +45,7 @@ typedef struct privcmd_hypercall
 
 typedef struct privcmd_mmap_entry {
 	unsigned long va;
-	unsigned long mfn;
+	frameno_t mfn;
 	unsigned long npages;
 } privcmd_mmap_entry_t; 
 
diff -r c4eead8a925b tools/debugger/libxendebug/xendebug.c
--- a/tools/debugger/libxendebug/xendebug.c	Sun Apr 16 15:41:31 2006 +0100
+++ b/tools/debugger/libxendebug/xendebug.c	Wed Apr 19 11:39:31 2006 -0500
@@ -57,7 +57,7 @@ typedef struct domain_context           
     vcpu_guest_context_t context[MAX_VIRT_CPUS];
 
     long            total_pages;
-    unsigned long  *page_array;
+    frameno_t      *page_array;
 
     unsigned long   cr3_phys[MAX_VIRT_CPUS];
     unsigned long  *cr3_virt[MAX_VIRT_CPUS];
diff -r c4eead8a925b tools/ioemu/vl.c
--- a/tools/ioemu/vl.c	Sun Apr 16 15:41:31 2006 +0100
+++ b/tools/ioemu/vl.c	Wed Apr 19 11:39:31 2006 -0500
@@ -149,11 +149,11 @@ TextConsole *vga_console;
 TextConsole *vga_console;
 CharDriverState *serial_hds[MAX_SERIAL_PORTS];
 int xc_handle;
-unsigned long *vgapage_array;
-unsigned long *freepage_array;
+frameno_t *vgapage_array;
+frameno_t *freepage_array;
 unsigned long free_pages;
 void *vtop_table;
-unsigned long toptab;
+frameno_t toptab;
 unsigned long vgaram_pages;
 
 /***********************************************************/
@@ -2673,7 +2673,8 @@ int main(int argc, char **argv)
     int serial_device_index;
     char qemu_dm_logfilename[64];
     const char *loadvm = NULL;
-    unsigned long nr_pages, extra_pages, ram_pages, *page_array;
+    unsigned long nr_pages, extra_pages, ram_pages;
+    frameno_t *page_array;
     xc_dominfo_t info;
     extern void *shared_page;
     extern void *shared_vram;
@@ -3156,8 +3157,8 @@ int main(int argc, char **argv)
         exit(-1);
     }
 
-    if ( (page_array = (unsigned long *)
-                        malloc(nr_pages * sizeof(unsigned long))) == NULL)
+    if ( (page_array = (frameno_t *)
+                        malloc(nr_pages * sizeof(frameno_t))) == NULL)
     {
         fprintf(logfile, "malloc returned error %d\n", errno);
         exit(-1);
@@ -3239,7 +3240,7 @@ int main(int argc, char **argv)
                                        page_array[0]);
 #endif
 
-    fprintf(logfile, "shared page at pfn:%lx, mfn: %lx\n", (nr_pages-1),
+    fprintf(logfile, "shared page at pfn:%lx, mfn: %"PRIx64"\n", (nr_pages-1),
            (page_array[nr_pages - 1]));
 
     /* we always create the cdrom drive, even if no disk is there */
diff -r c4eead8a925b tools/libxc/xc_core.c
--- a/tools/libxc/xc_core.c	Sun Apr 16 15:41:31 2006 +0100
+++ b/tools/libxc/xc_core.c	Wed Apr 19 11:39:31 2006 -0500
@@ -11,7 +11,7 @@ static int
 static int
 copy_from_domain_page(int xc_handle,
                       uint32_t domid,
-                      unsigned long mfn,
+                      frameno_t mfn,
                       void *dst_page)
 {
     void *vaddr = xc_map_foreign_range(
@@ -93,7 +93,7 @@ xc_domain_dumpcore_via_callback(int xc_h
         printf("Could not get the page frame list\n");
         goto error_out;
     }
-    sts = dump_rtn(args, (char *)page_array, nr_pages * sizeof(unsigned long));
+    sts = dump_rtn(args, (char *)page_array, nr_pages * sizeof(frameno_t));
     if ( sts != 0 )
         goto error_out;
 
diff -r c4eead8a925b tools/libxc/xc_domain.c
--- a/tools/libxc/xc_domain.c	Sun Apr 16 15:41:31 2006 +0100
+++ b/tools/libxc/xc_domain.c	Wed Apr 19 11:39:31 2006 -0500
@@ -291,7 +291,7 @@ int xc_domain_memory_increase_reservatio
                                           unsigned long nr_extents,
                                           unsigned int extent_order,
                                           unsigned int address_bits,
-                                          unsigned long *extent_start)
+                                          frameno_t *extent_start)
 {
     int err;
     struct xen_memory_reservation reservation = {
@@ -322,7 +322,7 @@ int xc_domain_memory_decrease_reservatio
                                           uint32_t domid,
                                           unsigned long nr_extents,
                                           unsigned int extent_order,
-                                          unsigned long *extent_start)
+                                          frameno_t *extent_start)
 {
     int err;
     struct xen_memory_reservation reservation = {
diff -r c4eead8a925b tools/libxc/xc_hvm_build.c
--- a/tools/libxc/xc_hvm_build.c	Sun Apr 16 15:41:31 2006 +0100
+++ b/tools/libxc/xc_hvm_build.c	Wed Apr 19 11:39:31 2006 -0500
@@ -135,7 +135,7 @@ static void set_hvm_info_checksum(struct
  * hvmloader will use this info to set BIOS accordingly
  */
 static int set_hvm_info(int xc_handle, uint32_t dom,
-                        unsigned long *pfn_list, unsigned int vcpus,
+                        frameno_t *pfn_list, unsigned int vcpus,
                         unsigned int pae, unsigned int acpi, unsigned int apic)
 {
     char *va_map;
@@ -176,9 +176,9 @@ static int setup_guest(int xc_handle,
                        unsigned int acpi,
                        unsigned int apic,
                        unsigned int store_evtchn,
-                       unsigned long *store_mfn)
-{
-    unsigned long *page_array = NULL;
+                       frameno_t *store_mfn)
+{
+    frameno_t *page_array = NULL;
     unsigned long count, i;
     unsigned long long ptr;
     xc_mmu_t *mmu = NULL;
@@ -190,7 +190,7 @@ static int setup_guest(int xc_handle,
     struct domain_setup_info dsi;
     unsigned long long v_end;
 
-    unsigned long shared_page_frame = 0;
+    frameno_t shared_page_frame = 0;
     shared_iopage_t *sp;
 
     memset(&dsi, 0, sizeof(struct domain_setup_info));
@@ -223,7 +223,7 @@ static int setup_guest(int xc_handle,
         goto error_out;
     }
 
-    if ( (page_array = malloc(nr_pages * sizeof(unsigned long))) == NULL )
+    if ( (page_array = malloc(nr_pages * sizeof(frameno_t))) == NULL )
     {
         PERROR("Could not allocate memory.\n");
         goto error_out;
@@ -346,7 +346,7 @@ static int xc_hvm_build_internal(int xc_
                                  unsigned int acpi,
                                  unsigned int apic,
                                  unsigned int store_evtchn,
-                                 unsigned long *store_mfn)
+                                 frameno_t *store_mfn)
 {
     dom0_op_t launch_op, op;
     int rc, i;
@@ -592,7 +592,7 @@ int xc_hvm_build(int xc_handle,
                  unsigned int acpi,
                  unsigned int apic,
                  unsigned int store_evtchn,
-                 unsigned long *store_mfn)
+                 frameno_t *store_mfn)
 {
     char *image;
     int  sts;
@@ -628,7 +628,7 @@ int xc_hvm_build_mem(int xc_handle,
                      unsigned int acpi,
                      unsigned int apic,
                      unsigned int store_evtchn,
-                     unsigned long *store_mfn)
+                     frameno_t *store_mfn)
 {
     int           sts;
     unsigned long img_len;
diff -r c4eead8a925b tools/libxc/xc_ia64_stubs.c
--- a/tools/libxc/xc_ia64_stubs.c	Sun Apr 16 15:41:31 2006 +0100
+++ b/tools/libxc/xc_ia64_stubs.c	Wed Apr 19 11:39:31 2006 -0500
@@ -31,8 +31,8 @@ int xc_linux_save(int xc_handle, int io_
 }
 
 int xc_linux_restore(int xc_handle, int io_fd, uint32_t dom, unsigned long nr_pfns,
-                     unsigned int store_evtchn, unsigned long *store_mfn,
-                     unsigned int console_evtchn, unsigned long *console_mfn)
+                     unsigned int store_evtchn, frameno_t *store_mfn,
+                     unsigned int console_evtchn, frameno_t *console_mfn)
 {
     PERROR("xc_linux_restore not implemented\n");
     return -1;
@@ -51,7 +51,7 @@ xc_plan9_build(int xc_handle,
 
 int xc_ia64_get_pfn_list(int xc_handle,
                          uint32_t domid,
-                         unsigned long *pfn_buf,
+                         frameno_t *pfn_buf, 
                          unsigned int start_page,
                          unsigned int nr_pages)
 {
@@ -65,7 +65,7 @@ int xc_ia64_get_pfn_list(int xc_handle,
     op.u.getmemlist.buffer   = pfn_buf;
 
     if ( (max_pfns != -1UL)
-        && mlock(pfn_buf, nr_pages * sizeof(unsigned long)) != 0 )
+        && mlock(pfn_buf, nr_pages * sizeof(frameno_t)) != 0 )
     {
         PERROR("Could not lock pfn list buffer");
         return -1;
@@ -74,7 +74,7 @@ int xc_ia64_get_pfn_list(int xc_handle,
     ret = do_dom0_op(xc_handle, &op);
 
     if (max_pfns != -1UL)
-        (void)munlock(pfn_buf, nr_pages * sizeof(unsigned long));
+        (void)munlock(pfn_buf, nr_pages * sizeof(frameno_t));
 
     return (ret < 0) ? -1 : op.u.getmemlist.num_pfns;
 }
@@ -93,10 +93,10 @@ int xc_ia64_copy_to_domain_pages(int xc_
 {
     // N.B. gva should be page aligned
 
-    unsigned long *page_array = NULL;
+    frameno_t *page_array = NULL;
     int i;
 
-    if ( (page_array = malloc(nr_pages * sizeof(unsigned long))) == NULL ){
+    if ( (page_array = malloc(nr_pages * sizeof(frameno_t))) == NULL ){
         PERROR("Could not allocate memory");
         goto error_out;
     }
@@ -643,7 +643,7 @@ int xc_hvm_build(int xc_handle,
                  unsigned int acpi,
                  unsigned int apic,
                  unsigned int store_evtchn,
-                 unsigned long *store_mfn)
+                 frameno_t *store_mfn)
 {
     dom0_op_t launch_op, op;
     int rc ;
diff -r c4eead8a925b tools/libxc/xc_linux_build.c
--- a/tools/libxc/xc_linux_build.c	Sun Apr 16 15:41:31 2006 +0100
+++ b/tools/libxc/xc_linux_build.c	Wed Apr 19 11:39:31 2006 -0500
@@ -18,6 +18,7 @@
 #include "xc_aout9.h"
 #include <stdlib.h>
 #include <unistd.h>
+#include <inttypes.h>
 #include <zlib.h>
 
 #if defined(__i386__)
@@ -144,7 +145,7 @@ int load_initrd(int xc_handle, domid_t d
 int load_initrd(int xc_handle, domid_t dom,
                 struct initrd_info *initrd,
                 unsigned long physbase,
-                unsigned long *phys_to_mach)
+                frameno_t *phys_to_mach)
 {
     char page[PAGE_SIZE];
     unsigned long pfn_start, pfn, nr_pages;
@@ -197,7 +198,7 @@ static int setup_pg_tables(int xc_handle
                            vcpu_guest_context_t *ctxt,
                            unsigned long dsi_v_start,
                            unsigned long v_end,
-                           unsigned long *page_array,
+                           frameno_t *page_array,
                            unsigned long vpt_start,
                            unsigned long vpt_end,
                            unsigned shadow_mode_enabled)
@@ -259,7 +260,7 @@ static int setup_pg_tables_pae(int xc_ha
                                vcpu_guest_context_t *ctxt,
                                unsigned long dsi_v_start,
                                unsigned long v_end,
-                               unsigned long *page_array,
+                               frameno_t *page_array,
                                unsigned long vpt_start,
                                unsigned long vpt_end,
                                unsigned shadow_mode_enabled)
@@ -352,7 +353,7 @@ static int setup_pg_tables_64(int xc_han
                               vcpu_guest_context_t *ctxt,
                               unsigned long dsi_v_start,
                               unsigned long v_end,
-                              unsigned long *page_array,
+                              frameno_t *page_array,
                               unsigned long vpt_start,
                               unsigned long vpt_end,
                               int shadow_mode_enabled)
@@ -459,11 +460,11 @@ static int setup_guest(int xc_handle,
                        const char *cmdline,
                        unsigned long shared_info_frame,
                        unsigned long flags,
-                       unsigned int store_evtchn, unsigned long *store_mfn,
-                       unsigned int console_evtchn, unsigned long *console_mfn,
+                       unsigned int store_evtchn, frameno_t *store_mfn,
+                       unsigned int console_evtchn, frameno_t *console_mfn,
                        uint32_t required_features[XENFEAT_NR_SUBMAPS])
 {
-    unsigned long *page_array = NULL;
+    frameno_t *page_array = NULL;
     struct load_funcs load_funcs;
     struct domain_setup_info dsi;
     unsigned long vinitrd_start;
@@ -490,7 +491,7 @@ static int setup_guest(int xc_handle,
 
     start_page = dsi.v_start >> PAGE_SHIFT;
     pgnr = (v_end - dsi.v_start) >> PAGE_SHIFT;
-    if ( (page_array = malloc(pgnr * sizeof(unsigned long))) == NULL )
+    if ( (page_array = malloc(pgnr * sizeof(frameno_t))) == NULL )
     {
         PERROR("Could not allocate memory");
         goto error_out;
@@ -614,11 +615,11 @@ static int setup_guest(int xc_handle,
                        const char *cmdline,
                        unsigned long shared_info_frame,
                        unsigned long flags,
-                       unsigned int store_evtchn, unsigned long *store_mfn,
-                       unsigned int console_evtchn, unsigned long *console_mfn,
+                       unsigned int store_evtchn, frameno_t *store_mfn,
+                       unsigned int console_evtchn, frameno_t *console_mfn,
                        uint32_t required_features[XENFEAT_NR_SUBMAPS])
 {
-    unsigned long *page_array = NULL;
+    frameno_t *page_array = NULL;
     unsigned long count, i, hypercall_pfn;
     start_info_t *start_info;
     shared_info_t *shared_info;
@@ -629,7 +630,7 @@ static int setup_guest(int xc_handle,
 
     unsigned long nr_pt_pages;
     unsigned long physmap_pfn;
-    unsigned long *physmap, *physmap_e;
+    frameno_t *physmap, *physmap_e;
 
     struct load_funcs load_funcs;
     struct domain_setup_info dsi;
@@ -782,7 +783,7 @@ static int setup_guest(int xc_handle,
         goto error_out;
     }
 
-    if ( (page_array = malloc(nr_pages * sizeof(unsigned long))) == NULL )
+    if ( (page_array = malloc(nr_pages * sizeof(frameno_t))) == NULL )
     {
         PERROR("Could not allocate memory");
         goto error_out;
@@ -872,8 +873,8 @@ static int setup_guest(int xc_handle,
             ((uint64_t)page_array[count] << PAGE_SHIFT) | MMU_MACHPHYS_UPDATE,
             count) )
         {
-            fprintf(stderr,"m2p update failure p=%lx m=%lx\n",
-                    count, page_array[count]);
+            fprintf(stderr,"m2p update failure p=%lx m=%"PRIx64"\n",
+                    count, (unsigned long long)page_array[count]);
             munmap(physmap, PAGE_SIZE);
             goto error_out;
         }
@@ -1041,9 +1042,9 @@ static int xc_linux_build_internal(int x
                                    const char *features,
                                    unsigned long flags,
                                    unsigned int store_evtchn,
-                                   unsigned long *store_mfn,
+                                   frameno_t *store_mfn,
                                    unsigned int console_evtchn,
-                                   unsigned long *console_mfn)
+                                   frameno_t *console_mfn)
 {
     dom0_op_t launch_op;
     DECLARE_DOM0_OP;
@@ -1201,9 +1202,9 @@ int xc_linux_build_mem(int xc_handle,
                        const char *features,
                        unsigned long flags,
                        unsigned int store_evtchn,
-                       unsigned long *store_mfn,
+                       frameno_t *store_mfn,
                        unsigned int console_evtchn,
-                       unsigned long *console_mfn)
+                       frameno_t *console_mfn)
 {
     int            sts;
     char          *img_buf;
@@ -1267,9 +1268,9 @@ int xc_linux_build(int xc_handle,
                    const char *features,
                    unsigned long flags,
                    unsigned int store_evtchn,
-                   unsigned long *store_mfn,
+                   frameno_t *store_mfn,
                    unsigned int console_evtchn,
-                   unsigned long *console_mfn)
+                   frameno_t *console_mfn)
 {
     char *image = NULL;
     unsigned long image_size;
diff -r c4eead8a925b tools/libxc/xc_linux_restore.c
--- a/tools/libxc/xc_linux_restore.c	Sun Apr 16 15:41:31 2006 +0100
+++ b/tools/libxc/xc_linux_restore.c	Wed Apr 19 11:39:31 2006 -0500
@@ -104,18 +104,18 @@ int uncanonicalize_pagetable(unsigned lo
 
 int xc_linux_restore(int xc_handle, int io_fd,
                      uint32_t dom, unsigned long nr_pfns,
-                     unsigned int store_evtchn, unsigned long *store_mfn,
-                     unsigned int console_evtchn, unsigned long *console_mfn)
+                     unsigned int store_evtchn, frameno_t *store_mfn,
+                     unsigned int console_evtchn, frameno_t *console_mfn)
 {
     DECLARE_DOM0_OP;
     int rc = 1, i, n;
-    unsigned long mfn, pfn;
+    frameno_t mfn, pfn;
     unsigned int prev_pc, this_pc;
     int verify = 0;
     int nraces = 0;
 
     /* The new domain's shared-info frame number. */
-    unsigned long shared_info_frame;
+    frameno_t shared_info_frame;
     unsigned char shared_info_page[PAGE_SIZE]; /* saved contents from file */
     shared_info_t *shared_info = (shared_info_t *)shared_info_page;
 
@@ -126,7 +126,7 @@ int xc_linux_restore(int xc_handle, int 
     unsigned long *pfn_type = NULL;
 
     /* A table of MFNs to map in the current region */
-    unsigned long *region_mfn = NULL;
+    frameno_t *region_mfn = NULL;
 
     /* Types of the pfns in the current region */
     unsigned long region_pfn_type[MAX_BATCH_SIZE];
@@ -135,7 +135,7 @@ int xc_linux_restore(int xc_handle, int 
     unsigned long *page = NULL;
 
     /* A copy of the pfn-to-mfn table frame list. */
-    unsigned long *p2m_frame_list = NULL;
+    frameno_t *p2m_frame_list = NULL;
 
     /* A temporary mapping of the guest's start_info page. */
     start_info_t *start_info;
@@ -183,9 +183,9 @@ int xc_linux_restore(int xc_handle, int 
 
 
     /* We want zeroed memory so use calloc rather than malloc. */
-    p2m        = calloc(sizeof(unsigned long), max_pfn);
+    p2m        = calloc(sizeof(frameno_t), max_pfn);
     pfn_type   = calloc(sizeof(unsigned long), max_pfn);
-    region_mfn = calloc(sizeof(unsigned long), MAX_BATCH_SIZE);
+    region_mfn = calloc(sizeof(frameno_t), MAX_BATCH_SIZE);
 
     if ((p2m == NULL) || (pfn_type == NULL) || (region_mfn == NULL)) {
         ERR("memory alloc failed");
@@ -193,7 +193,7 @@ int xc_linux_restore(int xc_handle, int 
         goto out;
     }
 
-    if (mlock(region_mfn, sizeof(unsigned long) * MAX_BATCH_SIZE)) {
+    if (mlock(region_mfn, sizeof(frameno_t) * MAX_BATCH_SIZE)) {
         ERR("Could not mlock region_mfn");
         goto out;
     }
diff -r c4eead8a925b tools/libxc/xc_linux_save.c
--- a/tools/libxc/xc_linux_save.c	Sun Apr 16 15:41:31 2006 +0100
+++ b/tools/libxc/xc_linux_save.c	Wed Apr 19 11:39:31 2006 -0500
@@ -27,7 +27,7 @@
 
 
 /* max mfn of the whole machine */
-static unsigned long max_mfn;
+static frameno_t max_mfn;
 
 /* virtual starting address of the hypervisor */
 static unsigned long hvirt_start;
@@ -36,13 +36,13 @@ static unsigned int pt_levels;
 static unsigned int pt_levels;
 
 /* total number of pages used by the current guest */
-static unsigned long max_pfn;
+static frameno_t max_pfn;
 
 /* Live mapping of the table mapping each PFN to its current MFN. */
-static unsigned long *live_p2m = NULL;
+static frameno_t *live_p2m = NULL;
 
 /* Live mapping of system MFN to PFN table. */
-static unsigned long *live_m2p = NULL;
+static frameno_t *live_m2p = NULL;
 
 /* grep fodder: machine_to_phys */
 
@@ -324,7 +324,7 @@ static int print_stats(int xc_handle, ui
 }
 
 
-static int analysis_phase(int xc_handle, uint32_t domid, int max_pfn,
+static int analysis_phase(int xc_handle, uint32_t domid, frameno_t max_pfn,
                           unsigned long *arr, int runs)
 {
     long long start, now;
@@ -508,14 +508,14 @@ static unsigned long *xc_map_m2p(int xc_
     privcmd_mmap_t ioctlx;
     privcmd_mmap_entry_t *entries;
     unsigned long m2p_chunks, m2p_size;
-    unsigned long *m2p;
+    frameno_t *m2p; 
     int i, rc;
 
     m2p_size   = M2P_SIZE(max_mfn);
     m2p_chunks = M2P_CHUNKS(max_mfn);
 
     xmml.max_extents = m2p_chunks;
-    if (!(xmml.extent_start = malloc(m2p_chunks * sizeof(unsigned long)))) {
+    if (!(xmml.extent_start = malloc(m2p_chunks * sizeof(frameno_t)))) { 
         ERR("failed to allocate space for m2p mfns");
         return NULL;
     }
@@ -571,7 +571,7 @@ int xc_linux_save(int xc_handle, int io_
     int sent_last_iter, skip_this_iter;
 
     /* The new domain's shared-info frame number. */
-    unsigned long shared_info_frame;
+    frameno_t shared_info_frame;
 
     /* A copy of the CPU context of the guest. */
     vcpu_guest_context_t ctxt;
@@ -584,11 +584,11 @@ int xc_linux_save(int xc_handle, int io_
     char page[PAGE_SIZE];
 
     /* Double and single indirect references to the live P2M table */
-    unsigned long *live_p2m_frame_list_list = NULL;
-    unsigned long *live_p2m_frame_list = NULL;
+    frameno_t *live_p2m_frame_list_list = NULL;
+    frameno_t *live_p2m_frame_list = NULL;
 
     /* A copy of the pfn-to-mfn table frame list. */
-    unsigned long *p2m_frame_list = NULL;
+    frameno_t *p2m_frame_list = NULL;
 
     /* Live mapping of shared info structure */
     shared_info_t *live_shinfo = NULL;
@@ -713,11 +713,11 @@ int xc_linux_save(int xc_handle, int io_
     memcpy(p2m_frame_list, live_p2m_frame_list, P2M_FL_SIZE);
 
     /* Canonicalise the pfn-to-mfn table frame-number list. */
-    for (i = 0; i < max_pfn; i += ulpp) {
-        if (!translate_mfn_to_pfn(&p2m_frame_list[i/ulpp])) {
+    for (i = 0; i < max_pfn; i += fpp) {
+        if (!translate_mfn_to_pfn(&p2m_frame_list[i/fpp])) {
             ERR("Frame# in pfn-to-mfn frame list is not in pseudophys");
-            ERR("entry %d: p2m_frame_list[%ld] is 0x%lx", i, i/ulpp,
-                p2m_frame_list[i/ulpp]);
+            ERR("entry %d: p2m_frame_list[%ld] is 0x"PRIx64, i, i/fpp,
+                p2m_frame_list[i/fpp]);
             goto out;
         }
     }
@@ -819,7 +819,7 @@ int xc_linux_save(int xc_handle, int io_
 
     /* Start writing out the saved-domain record. */
 
-    if(!write_exact(io_fd, &max_pfn, sizeof(unsigned long))) {
+    if(!write_exact(io_fd, &max_pfn, sizeof(frameno_t))) {
         ERR("write: max_pfn");
         goto out;
     }
diff -r c4eead8a925b tools/libxc/xc_load_elf.c
--- a/tools/libxc/xc_load_elf.c	Sun Apr 16 15:41:31 2006 +0100
+++ b/tools/libxc/xc_load_elf.c	Wed Apr 19 11:39:31 2006 -0500
@@ -24,10 +24,10 @@ static int
 static int
 loadelfimage(
     const char *image, unsigned long image_size, int xch, uint32_t dom,
-    unsigned long *parray, struct domain_setup_info *dsi);
+    frameno_t *parray, struct domain_setup_info *dsi);
 static int
 loadelfsymtab(
-    const char *image, int xch, uint32_t dom, unsigned long *parray,
+    const char *image, int xch, uint32_t dom, frameno_t *parray,
     struct domain_setup_info *dsi);
 
 int probe_elf(const char *image,
@@ -187,7 +187,7 @@ static int
 static int
 loadelfimage(
     const char *image, unsigned long elfsize, int xch, uint32_t dom,
-    unsigned long *parray, struct domain_setup_info *dsi)
+    frameno_t *parray, struct domain_setup_info *dsi)
 {
     Elf_Ehdr *ehdr = (Elf_Ehdr *)image;
     Elf_Phdr *phdr;
@@ -237,7 +237,7 @@ loadelfimage(
 
 static int
 loadelfsymtab(
-    const char *image, int xch, uint32_t dom, unsigned long *parray,
+    const char *image, int xch, uint32_t dom, frameno_t *parray,
     struct domain_setup_info *dsi)
 {
     Elf_Ehdr *ehdr = (Elf_Ehdr *)image, *sym_ehdr;
diff -r c4eead8a925b tools/libxc/xc_private.c
--- a/tools/libxc/xc_private.c	Sun Apr 16 15:41:31 2006 +0100
+++ b/tools/libxc/xc_private.c	Wed Apr 19 11:39:31 2006 -0500
@@ -8,7 +8,7 @@
 #include <xen/memory.h>
 
 void *xc_map_foreign_batch(int xc_handle, uint32_t dom, int prot,
-                           unsigned long *arr, int num )
+                           frameno_t *arr, int num )
 {
     privcmd_mmapbatch_t ioctlx;
     void *addr;
@@ -36,7 +36,7 @@ void *xc_map_foreign_batch(int xc_handle
 
 void *xc_map_foreign_range(int xc_handle, uint32_t dom,
                            int size, int prot,
-                           unsigned long mfn )
+                           frameno_t mfn )
 {
     privcmd_mmap_t ioctlx;
     privcmd_mmap_entry_t entry;
@@ -309,7 +309,7 @@ long long xc_domain_get_cpu_usage( int x
 
 int xc_get_pfn_list(int xc_handle,
                     uint32_t domid,
-                    unsigned long *pfn_buf,
+                    frameno_t *pfn_buf, 
                     unsigned long max_pfns)
 {
     DECLARE_DOM0_OP;
@@ -320,10 +320,10 @@ int xc_get_pfn_list(int xc_handle,
     op.u.getmemlist.buffer   = pfn_buf;
 
 #ifdef VALGRIND
-    memset(pfn_buf, 0, max_pfns * sizeof(unsigned long));
+    memset(pfn_buf, 0, max_pfns * sizeof(frameno_t));
 #endif
 
-    if ( mlock(pfn_buf, max_pfns * sizeof(unsigned long)) != 0 )
+    if ( mlock(pfn_buf, max_pfns * sizeof(frameno_t)) != 0 )
     {
         PERROR("xc_get_pfn_list: pfn_buf mlock failed");
         return -1;
@@ -331,7 +331,7 @@ int xc_get_pfn_list(int xc_handle,
 
     ret = do_dom0_op(xc_handle, &op);
 
-    safe_munlock(pfn_buf, max_pfns * sizeof(unsigned long));
+    safe_munlock(pfn_buf, max_pfns * sizeof(frameno_t));
 
 #if 0
 #ifdef DEBUG
@@ -362,7 +362,7 @@ long xc_get_tot_pages(int xc_handle, uin
 
 int xc_copy_to_domain_page(int xc_handle,
                            uint32_t domid,
-                           unsigned long dst_pfn,
+                           frameno_t dst_pfn, 
                            const char *src_page)
 {
     void *vaddr = xc_map_foreign_range(
@@ -410,7 +410,7 @@ unsigned long xc_get_filesz(int fd)
 }
 
 void xc_map_memcpy(unsigned long dst, const char *src, unsigned long size,
-                   int xch, uint32_t dom, unsigned long *parray,
+                   int xch, uint32_t dom, frameno_t *parray,
                    unsigned long vstart)
 {
     char *va;
@@ -477,9 +477,9 @@ int xc_version(int xc_handle, int cmd, v
 }
 
 unsigned long xc_make_page_below_4G(
-    int xc_handle, uint32_t domid, unsigned long mfn)
-{
-    unsigned long new_mfn;
+    int xc_handle, uint32_t domid, frameno_t mfn)
+{
+    frameno_t new_mfn;
 
     if ( xc_domain_memory_decrease_reservation(
         xc_handle, domid, 1, 0, &mfn) != 0 )
diff -r c4eead8a925b tools/libxc/xenguest.h
--- a/tools/libxc/xenguest.h	Sun Apr 16 15:41:31 2006 +0100
+++ b/tools/libxc/xenguest.h	Wed Apr 19 11:39:31 2006 -0500
@@ -39,8 +39,8 @@ int xc_linux_save(int xc_handle, int io_
  */
 int xc_linux_restore(int xc_handle, int io_fd, uint32_t dom,
                      unsigned long nr_pfns, unsigned int store_evtchn,
-                     unsigned long *store_mfn, unsigned int console_evtchn,
-                     unsigned long *console_mfn);
+                     frameno_t *store_mfn, unsigned int console_evtchn,
+                     frameno_t *console_mfn);
 
 /**
  * This function will create a domain for a paravirtualized Linux
@@ -66,9 +66,9 @@ int xc_linux_build(int xc_handle,
                    const char *features,
                    unsigned long flags,
                    unsigned int store_evtchn,
-                   unsigned long *store_mfn,
+                   frameno_t *store_mfn,
                    unsigned int console_evtchn,
-                   unsigned long *console_mfn);
+                   frameno_t *console_mfn);
 
 /**
  * This function will create a domain for a paravirtualized Linux
@@ -98,9 +98,9 @@ int xc_linux_build_mem(int xc_handle,
                        const char *features,
                        unsigned long flags,
                        unsigned int store_evtchn,
-                       unsigned long *store_mfn,
+                       frameno_t *store_mfn,
                        unsigned int console_evtchn,
-                       unsigned long *console_mfn);
+                       frameno_t *console_mfn);
 
 int xc_hvm_build(int xc_handle,
                  uint32_t domid,
@@ -111,7 +111,7 @@ int xc_hvm_build(int xc_handle,
                  unsigned int acpi,
                  unsigned int apic,
                  unsigned int store_evtchn,
-                 unsigned long *store_mfn);
+                 frameno_t *store_mfn);
 
 int xc_hvm_build_mem(int xc_handle,
                      uint32_t domid,
@@ -123,6 +123,6 @@ int xc_hvm_build_mem(int xc_handle,
                      unsigned int acpi,
                      unsigned int apic,
                      unsigned int store_evtchn,
-                     unsigned long *store_mfn);
+                     frameno_t *store_mfn);
 
 #endif /* XENGUEST_H */
diff -r c4eead8a925b tools/libxc/xg_save_restore.h
--- a/tools/libxc/xg_save_restore.h	Sun Apr 16 15:41:31 2006 +0100
+++ b/tools/libxc/xg_save_restore.h	Wed Apr 19 11:39:31 2006 -0500
@@ -105,23 +105,23 @@ static int get_platform_info(int xc_hand
 */
 #define M2P_SHIFT       L2_PAGETABLE_SHIFT_PAE
 #define M2P_CHUNK_SIZE  (1 << M2P_SHIFT)
-#define M2P_SIZE(_m)    ROUNDUP(((_m) * sizeof(unsigned long)), M2P_SHIFT)
+#define M2P_SIZE(_m)    ROUNDUP(((_m) * sizeof(frameno_t)), M2P_SHIFT)
 #define M2P_CHUNKS(_m)  (M2P_SIZE((_m)) >> M2P_SHIFT)
 
 /* Size in bytes of the P2M (rounded up to the nearest PAGE_SIZE bytes) */
-#define P2M_SIZE        ROUNDUP((max_pfn * sizeof(unsigned long)), PAGE_SHIFT)
+#define P2M_SIZE        ROUNDUP((max_pfn * sizeof(frameno_t)), PAGE_SHIFT)
 
 /* Number of unsigned longs in a page */
-#define ulpp            (PAGE_SIZE/sizeof(unsigned long))
+#define fpp            (PAGE_SIZE/sizeof(frameno_t))
 
 /* Number of entries in the pfn_to_mfn_frame_list */
-#define P2M_FL_ENTRIES  (((max_pfn)+ulpp-1)/ulpp)
+#define P2M_FL_ENTRIES  (((max_pfn)+fpp-1)/fpp)
 
 /* Size in bytes of the pfn_to_mfn_frame_list     */
-#define P2M_FL_SIZE     ((P2M_FL_ENTRIES)*sizeof(unsigned long))
+#define P2M_FL_SIZE     ((P2M_FL_ENTRIES)*sizeof(frameno_t))
 
 /* Number of entries in the pfn_to_mfn_frame_list_list */
-#define P2M_FLL_ENTRIES (((max_pfn)+(ulpp*ulpp)-1)/(ulpp*ulpp))
+#define P2M_FLL_ENTRIES (((max_pfn)+(fpp*fpp)-1)/(fpp*fpp))
 
 /* Current guests allow 8MB 'slack' in their P2M */
 #define NR_SLACK_ENTRIES   ((8 * 1024 * 1024) / PAGE_SIZE)
diff -r c4eead8a925b tools/python/xen/lowlevel/xc/xc.c
--- a/tools/python/xen/lowlevel/xc/xc.c	Sun Apr 16 15:41:31 2006 +0100
+++ b/tools/python/xen/lowlevel/xc/xc.c	Wed Apr 19 11:39:31 2006 -0500
@@ -371,7 +371,7 @@ static PyObject *pyxc_hvm_build(XcObject
     int pae  = 0;
     int acpi = 0;
     int apic = 0;
-    unsigned long store_mfn = 0;
+    frameno_t store_mfn = 0;
 
     static char *kwd_list[] = { "dom", "store_evtchn",
 				"memsize", "image", "vcpus", "pae", "acpi", "apic",
diff -r c4eead8a925b xen/common/memory.c
--- a/xen/common/memory.c	Sun Apr 16 15:41:31 2006 +0100
+++ b/xen/common/memory.c	Wed Apr 19 11:39:31 2006 -0500
@@ -31,7 +31,7 @@ static long
 static long
 increase_reservation(
     struct domain *d, 
-    GUEST_HANDLE(ulong) extent_list,
+    GUEST_HANDLE(frameno_t) extent_list,
     unsigned int   nr_extents,
     unsigned int   extent_order,
     unsigned int   flags,
@@ -80,7 +80,7 @@ static long
 static long
 populate_physmap(
     struct domain *d, 
-    GUEST_HANDLE(ulong) extent_list,
+    GUEST_HANDLE(frameno_t) extent_list,
     unsigned int  nr_extents,
     unsigned int  extent_order,
     unsigned int  flags,
@@ -177,7 +177,7 @@ static long
 static long
 decrease_reservation(
     struct domain *d,
-    GUEST_HANDLE(ulong) extent_list,
+    GUEST_HANDLE(frameno_t) extent_list,
     unsigned int   nr_extents,
     unsigned int   extent_order,
     unsigned int   flags,
diff -r c4eead8a925b xen/include/asm-x86/shadow.h
--- a/xen/include/asm-x86/shadow.h	Sun Apr 16 15:41:31 2006 +0100
+++ b/xen/include/asm-x86/shadow.h	Wed Apr 19 11:39:31 2006 -0500
@@ -1231,8 +1231,8 @@ static inline unsigned long __shadow_sta
  * Either returns PGT_none, or PGT_l{1,2,3,4}_page_table.
  */
 static inline u32
-shadow_max_pgtable_type(struct domain *d, unsigned long gpfn,
-                        unsigned long *smfn)
+shadow_max_pgtable_type(struct domain *d, frameno_t gpfn,
+                        frameno_t *smfn)
 {
     struct shadow_status *x;
     u32 pttype = PGT_none, type;
@@ -1503,7 +1503,7 @@ static inline void guest_physmap_add_pag
 }
 
 static inline void guest_physmap_remove_page(
-    struct domain *d, unsigned long gpfn, unsigned long mfn)
+    struct domain *d, frameno_t gpfn, frameno_t mfn)
 {
     struct domain_mmap_cache c1, c2;
     unsigned long type;
diff -r c4eead8a925b xen/include/public/arch-ia64.h
--- a/xen/include/public/arch-ia64.h	Sun Apr 16 15:41:31 2006 +0100
+++ b/xen/include/public/arch-ia64.h	Wed Apr 19 11:39:31 2006 -0500
@@ -27,6 +27,9 @@ DEFINE_GUEST_HANDLE(int);
 DEFINE_GUEST_HANDLE(int);
 DEFINE_GUEST_HANDLE(long);
 DEFINE_GUEST_HANDLE(void);
+
+typedef unsigned long frameno_t;
+DEFINE_GUEST_HANDLE(frameno_t);
 #endif
 
 /* Maximum number of virtual CPUs in multi-processor guests. */
@@ -295,7 +298,7 @@ typedef mapped_regs_t vpd_t;
 
 typedef struct {
     unsigned int flags;
-    unsigned long start_info_pfn;
+    frameno_t start_info_pfn;
 } arch_shared_info_t;
 
 typedef struct {
diff -r c4eead8a925b xen/include/public/arch-x86_32.h
--- a/xen/include/public/arch-x86_32.h	Sun Apr 16 15:41:31 2006 +0100
+++ b/xen/include/public/arch-x86_32.h	Wed Apr 19 11:39:31 2006 -0500
@@ -29,6 +29,9 @@ DEFINE_GUEST_HANDLE(int);
 DEFINE_GUEST_HANDLE(int);
 DEFINE_GUEST_HANDLE(long);
 DEFINE_GUEST_HANDLE(void);
+
+typedef unsigned long frameno_t;
+DEFINE_GUEST_HANDLE(frameno_t);
 #endif
 
 /*
@@ -157,9 +160,9 @@ DEFINE_GUEST_HANDLE(vcpu_guest_context_t
 DEFINE_GUEST_HANDLE(vcpu_guest_context_t);
 
 typedef struct arch_shared_info {
-    unsigned long max_pfn;                  /* max pfn that appears in table */
+    frameno_t max_pfn;                  /* max pfn that appears in table */
     /* Frame containing list of mfns containing list of mfns containing p2m. */
-    unsigned long pfn_to_mfn_frame_list_list;
+    frameno_t pfn_to_mfn_frame_list_list;
     unsigned long nmi_reason;
 } arch_shared_info_t;
 
diff -r c4eead8a925b xen/include/public/arch-x86_64.h
--- a/xen/include/public/arch-x86_64.h	Sun Apr 16 15:41:31 2006 +0100
+++ b/xen/include/public/arch-x86_64.h	Wed Apr 19 11:39:31 2006 -0500
@@ -29,6 +29,9 @@ DEFINE_GUEST_HANDLE(int);
 DEFINE_GUEST_HANDLE(int);
 DEFINE_GUEST_HANDLE(long);
 DEFINE_GUEST_HANDLE(void);
+
+typedef unsigned long frameno_t;
+DEFINE_GUEST_HANDLE(frameno_t);
 #endif
 
 /*
@@ -233,9 +236,9 @@ DEFINE_GUEST_HANDLE(vcpu_guest_context_t
 DEFINE_GUEST_HANDLE(vcpu_guest_context_t);
 
 typedef struct arch_shared_info {
-    unsigned long max_pfn;                  /* max pfn that appears in table */
+    frameno_t max_pfn;                  /* max pfn that appears in table */
     /* Frame containing list of mfns containing list of mfns containing p2m. */
-    unsigned long pfn_to_mfn_frame_list_list;
+    frameno_t pfn_to_mfn_frame_list_list;
     unsigned long nmi_reason;
 } arch_shared_info_t;
 
diff -r c4eead8a925b xen/include/public/dom0_ops.h
--- a/xen/include/public/dom0_ops.h	Sun Apr 16 15:41:31 2006 +0100
+++ b/xen/include/public/dom0_ops.h	Wed Apr 19 11:39:31 2006 -0500
@@ -28,7 +28,7 @@ typedef struct dom0_getmemlist {
     /* IN variables. */
     domid_t       domain;
     unsigned long max_pfns;
-    GUEST_HANDLE(ulong) buffer;
+    GUEST_HANDLE(frameno_t) buffer;
     /* OUT variables. */
     unsigned long num_pfns;
 } dom0_getmemlist_t;
@@ -468,7 +468,7 @@ DEFINE_GUEST_HANDLE(dom0_iomem_permissio
 #define DOM0_HYPERCALL_INIT   48
 typedef struct dom0_hypercall_init {
     domid_t  domain;          /* domain to be affected */
-    unsigned long mfn;        /* machine frame to be initialised */
+    frameno_t mfn;            /* machine frame to be initialised */
 } dom0_hypercall_init_t;
 DEFINE_GUEST_HANDLE(dom0_hypercall_init_t);
 
diff -r c4eead8a925b xen/include/public/memory.h
--- a/xen/include/public/memory.h	Sun Apr 16 15:41:31 2006 +0100
+++ b/xen/include/public/memory.h	Wed Apr 19 11:39:31 2006 -0500
@@ -29,7 +29,7 @@ typedef struct xen_memory_reservation {
      *   OUT: GMFN bases of extents that were allocated
      *   (NB. This command also updates the mach_to_phys translation table)
      */
-    GUEST_HANDLE(ulong) extent_start;
+    GUEST_HANDLE(frameno_t) extent_start;
 
     /* Number of extents, and size/alignment of each (2^extent_order pages). */
     unsigned long  nr_extents;

Re: [patch] "frame number" size in hypercall ABI

[Keir Fraser]

On 19 Apr 2006, at 17:44, Hollis Blanchard wrote:

> "xen_frameno_t" then?

xen_pfn_t? Definitely won't conflict with anyone, and I prefer 'pfn' to 
'frameno' as it's more consistent with other names we have in the 
interface.

> Attached is the updated patch, with typos fixed and a couple other
> corrections. I've also added the type to arch-x86_64.h and arch-ia64.h,
> so I think the patch is ready to be applied.

What about the Linux kernel -- shouldn't that be changed too? At least 
where it handles arrays of longs passed to memory_op()?

Inside Xen, does shadow.h really need changing at all? Once entries are 
unpacked from an array by a hypercall they could just be passed round 
as longs, right?

Sorry about the to'ing-and-fro'ing but we need to make sure interface 
changes are complete and correct and this one is inevitably pretty 
far-reaching.

  -- Keir

Re: [patch] "frame number" size in hypercall ABI

[Joe Bonasera]

Keir Fraser wrote:
> 
> On 19 Apr 2006, at 17:44, Hollis Blanchard wrote:
> 
>> "xen_frameno_t" then?
> 
> 
> xen_pfn_t? Definitely won't conflict with anyone, and I prefer 'pfn' to 
> 'frameno' as it's more consistent with other names we have in the 
> interface.

why not xen_mfn_t? Is "p" here for "page" or "physical"? using
"m" makes it a bit more explicit.

> 
>> Attached is the updated patch, with typos fixed and a couple other
>> corrections. I've also added the type to arch-x86_64.h and arch-ia64.h,
>> so I think the patch is ready to be applied.
> 
> 
> What about the Linux kernel -- shouldn't that be changed too? At least 
> where it handles arrays of longs passed to memory_op()?
> 
> Inside Xen, does shadow.h really need changing at all? Once entries are 
> unpacked from an array by a hypercall they could just be passed round as 
> longs, right?
> 
> Sorry about the to'ing-and-fro'ing but we need to make sure interface 
> changes are complete and correct and this one is inevitably pretty 
> far-reaching.
> 
>   -- Keir
> 

Re: [patch] "frame number" size in hypercall ABI

[Hollis Blanchard]

On Wed, 2006-04-19 at 19:25 +0100, Keir Fraser wrote:
> On 19 Apr 2006, at 17:44, Hollis Blanchard wrote:
> 
> > "xen_frameno_t" then?
> 
> xen_pfn_t? Definitely won't conflict with anyone, and I prefer 'pfn' to 
> 'frameno' as it's more consistent with other names we have in the 
> interface.

Well technically the PFN list is actually a list of MFNs, right? I think
both PFNs and MFNs are passed across this interface... would you want
separate types for those?

> > Attached is the updated patch, with typos fixed and a couple other
> > corrections. I've also added the type to arch-x86_64.h and arch-ia64.h,
> > so I think the patch is ready to be applied.
> 
> What about the Linux kernel -- shouldn't that be changed too? At least 
> where it handles arrays of longs passed to memory_op()?

In theory yes. I've been trying to limit myself to changes that I
absolutely need. A typical ppc64 system has 32-bit userland, 64-bit
kernel, 64-bit hypervisor, so practically speaking the kernel doesn't
need to change.

> Inside Xen, does shadow.h really need changing at all? Once entries are 
> unpacked from an array by a hypercall they could just be passed round 
> as longs, right?

Right, it looks like that part of the patch is bogus. I removed the
shadow.h changes; I can resubmit, or I can wait to see if other issues
are found...

> Sorry about the to'ing-and-fro'ing but we need to make sure interface 
> changes are complete and correct and this one is inevitably pretty 
> far-reaching.

I agree.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [patch] "frame number" size in hypercall ABI

[Keir Fraser]

On 19 Apr 2006, at 20:30, Hollis Blanchard wrote:

>> xen_pfn_t? Definitely won't conflict with anyone, and I prefer 'pfn' 
>> to
>> 'frameno' as it's more consistent with other names we have in the
>> interface.
>
> Well technically the PFN list is actually a list of MFNs, right? I 
> think
> both PFNs and MFNs are passed across this interface... would you want
> separate types for those?

Ah, well I came up with a reasonably consistent naming scheme some time 
ago. It is commented in one of the interface header files I believe, 
and here it is again:
  MFN: machine frame number (real host machine address)
  GPFN: guest pseudo-physical frame number (the illusory contiguous phys 
addr space)
  GMFN: guest machine frame number (this is the special one -- it's 
==GPFN for an auto-translated guest, and ==MFN for normal 
paravirtualised guests). It represents what the guest *thinks* are 
MFNs.
  PFN: a catch-all for any kind of frame number. 'Physical' here can 
mean guest-physical, machine-physical or guest-machine-physical.

So, for us, 'pfn' is kind of a polymorphic name. What you are thinking 
of as 'pfn' we usually call 'gpfn'. This is just the most convenient 
way the naming turned out when I was looking to apply a consistent 
naming scheme across the hypervisor and its interfaces with least 
changes. :-)

>>> Attached is the updated patch, with typos fixed and a couple other
>>> corrections. I've also added the type to arch-x86_64.h and 
>>> arch-ia64.h,
>>> so I think the patch is ready to be applied.
>>
>> What about the Linux kernel -- shouldn't that be changed too? At least
>> where it handles arrays of longs passed to memory_op()?
>
> In theory yes. I've been trying to limit myself to changes that I
> absolutely need. A typical ppc64 system has 32-bit userland, 64-bit
> kernel, 64-bit hypervisor, so practically speaking the kernel doesn't
> need to change.

An interface change must be consistently applied. I'd rather have a 
bigger self-consistent patch than a small one that nibbles at the issue 
it is trying to solve.

  -- Keir

Re: [patch] "frame number" size in hypercall ABI

[Hollis Blanchard]

On Wed, 2006-04-19 at 19:25 +0100, Keir Fraser wrote:
> On 19 Apr 2006, at 17:44, Hollis Blanchard wrote:
> 
> > Attached is the updated patch, with typos fixed and a couple other
> > corrections. I've also added the type to arch-x86_64.h and arch-ia64.h,
> > so I think the patch is ready to be applied.
> 
> What about the Linux kernel -- shouldn't that be changed too? At least 
> where it handles arrays of longs passed to memory_op()?
> 
> Inside Xen, does shadow.h really need changing at all? Once entries are 
> unpacked from an array by a hypercall they could just be passed round 
> as longs, right?
> 
> Sorry about the to'ing-and-fro'ing but we need to make sure interface 
> changes are complete and correct and this one is inevitably pretty 
> far-reaching.

So are you saying you want the patch to include Linux kernel changes?
But not Xen changes (e.g. shadow.h) other than right at the interface?

I needed to change all users in libxc because they all index into
page_array[] as unsigned long. Theoretically speaking you could have the
same problem within Linux and Xen as well. The word "unpacking" suggests
a conversion step, which is something I'm not introducing here.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [patch] "frame number" size in hypercall ABI

[Keir Fraser]

On 20 Apr 2006, at 17:09, Hollis Blanchard wrote:

> So are you saying you want the patch to include Linux kernel changes?
> But not Xen changes (e.g. shadow.h) other than right at the interface?

Yes, at least where Linux is handling arrays that will get passed to a 
hypercall or the array could end up the wrong size! Of course it really 
doesn't matter for singleton values so they can safely be left as-is.

The same goes for Xen too, but I believe the arrays are only handled in 
the very hypercall functions that receive them. For example, many 
functions in common/memory.c will need to read/write values to from a 
xen_pfn_t, but these singleton values can then be passed around inside 
Xen as longs.

> I needed to change all users in libxc because they all index into
> page_array[] as unsigned long. Theoretically speaking you could have 
> the
> same problem within Linux and Xen as well. The word "unpacking" 
> suggests
> a conversion step, which is something I'm not introducing here.

The arrays aren't passed around inside Xen (I'm pretty sure). As I say 
above, we read/write one-by-one.

  -- Keir

Xen terminology wiki page

[Hollis Blanchard]

On Wed, 2006-04-19 at 21:11 +0100, Keir Fraser wrote:
> 
> Ah, well I came up with a reasonably consistent naming scheme some time 
> ago. It is commented in one of the interface header files I believe, 
> and here it is again:
>   MFN: machine frame number (real host machine address)
>   GPFN: guest pseudo-physical frame number (the illusory contiguous phys 
> addr space)
>   GMFN: guest machine frame number (this is the special one -- it's 
> ==GPFN for an auto-translated guest, and ==MFN for normal 
> paravirtualised guests). It represents what the guest *thinks* are 
> MFNs.
>   PFN: a catch-all for any kind of frame number. 'Physical' here can 
> mean guest-physical, machine-physical or guest-machine-physical.

FYI, I've put this into a wiki page:
http://wiki.xensource.com/xenwiki/XenTerminology . The list should be
expanded to include HVM, SVM, VT(-x, -i), xenbus, xen store, ...

Probably the best approach would be a 1- or 2-line definition, followed
by a link to more details when available (other wiki pages, external web
pages, etc).

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: Xen terminology wiki page

[Keir Fraser]

On 20 Apr 2006, at 19:13, Hollis Blanchard wrote:

> FYI, I've put this into a wiki page:
> http://wiki.xensource.com/xenwiki/XenTerminology . The list should be
> expanded to include HVM, SVM, VT(-x, -i), xenbus, xen store, ...
>
> Probably the best approach would be a 1- or 2-line definition, followed
> by a link to more details when available (other wiki pages, external 
> web
> pages, etc).

This looks like a neat idea.

  Thanks,
  Keir

Re: [patch] "frame number" size in hypercall ABI

[Hollis Blanchard]

On Thu, 2006-04-20 at 17:18 +0100, Keir Fraser wrote:
> On 20 Apr 2006, at 17:09, Hollis Blanchard wrote:
> 
> > So are you saying you want the patch to include Linux kernel changes?
> > But not Xen changes (e.g. shadow.h) other than right at the interface?
> 
> Yes, at least where Linux is handling arrays that will get passed to a 
> hypercall or the array could end up the wrong size! Of course it really 
> doesn't matter for singleton values so they can safely be left as-is.
> 
> The same goes for Xen too, but I believe the arrays are only handled in 
> the very hypercall functions that receive them. For example, many 
> functions in common/memory.c will need to read/write values to from a 
> xen_pfn_t, but these singleton values can then be passed around inside 
> Xen as longs.

But those single values can't be passed around as longs in other areas,
such as libxc. In libxc there is usually a path of xc_get_pfn_list()
-> ... -> xc_map_foreign_page() (or whatever), and I need to make sure
that the PFNs don't get truncated to 32 bits anywhere along that path.

So I don't want to limit this patch to just *arrays* of PFNs. That means
it would have to encompass all "frame numbers" in the Xen interface,
including dom0_ops, memory_ops, start_info, shared_info... Is that a
logical split for you?

Are you waiting for this patch to be finalized before applying the
GET/SET_HANDLE patch?

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [patch] "frame number" size in hypercall ABI

[Keir Fraser]

On 20 Apr 2006, at 22:15, Hollis Blanchard wrote:

> But those single values can't be passed around as longs in other areas,
> such as libxc. In libxc there is usually a path of xc_get_pfn_list()
> -> ... -> xc_map_foreign_page() (or whatever), and I need to make sure
> that the PFNs don't get truncated to 32 bits anywhere along that path.

Is 44 bits of machine address not enough on ppc? That's a pretty big 
address space, even if RAM is moderately sparse.

> So I don't want to limit this patch to just *arrays* of PFNs. That 
> means
> it would have to encompass all "frame numbers" in the Xen interface,
> including dom0_ops, memory_ops, start_info, shared_info... Is that a
> logical split for you?

It makes sense in the interface as you really care about the size there.

> Are you waiting for this patch to be finalized before applying the
> GET/SET_HANDLE patch?

You wanted to do the xen_pfn_t patch first. For the other, the names 
should be GET/SET_GUEST_HANDLE.

  -- Keir

Re: [patch] "frame number" size in hypercall ABI

[Hollis Blanchard]

On Thu, 2006-04-20 at 23:35 +0100, Keir Fraser wrote:
> 
> > Are you waiting for this patch to be finalized before applying the
> > GET/SET_HANDLE patch?
> 
> You wanted to do the xen_pfn_t patch first. For the other, the names 
> should be GET/SET_GUEST_HANDLE. 

I didn't actually; I'm submitting in parallel. :) I will resend that
patch with the rename.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: Re: [rfc] [patch] "frame number" size in hypercall ABI

[Hollis Blanchard]

On Apr 19, 2006, at 10:05 AM, Ewan Mellor wrote:

> On Wed, Apr 19, 2006 at 09:08:49AM -0500, Hollis Blanchard wrote:
>
>> (If we require a 64-bit libxc in this case, that will also require a
>> 64-bit python, and 64-bit versions of all the python modules. No
>> distribution, other than maybe Gentoo, currently does this, and it
>> would be a big PITA for them to do so.)
>
> Silence your heretic anti-Debian libels!  ;-)  Comparing them to 
> Gentoo,
> indeed...
>
> There is a Debian AMD64 / EM64T port, which will become official with 
> the next
> release (insert compulsory "some time in 2010" joke here).

Sorry for being unclear; I'm not talking about x86-64. AFAIK it is 
always a performance win to use 64-bit binaries on x86-64 because 
32-bit x86 is so bad.

This is not at all the case with 64-bit PowerPC: it is usually a 
performance loss to simply recompile 32-bit binaries as 64-bit. 
Accordingly, only some initially misguided PowerPC distributions are 
100% 64-bit.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: Re: [rfc] [patch] "frame number" size in hypercall ABI

[Ewan Mellor]

On Wed, Apr 19, 2006 at 09:08:49AM -0500, Hollis Blanchard wrote:

> (If we require a 64-bit libxc in this case, that will also require a 
> 64-bit python, and 64-bit versions of all the python modules. No 
> distribution, other than maybe Gentoo, currently does this, and it 
> would be a big PITA for them to do so.)

Silence your heretic anti-Debian libels!  ;-)  Comparing them to Gentoo,
indeed...

There is a Debian AMD64 / EM64T port, which will become official with the next
release (insert compulsory "some time in 2010" joke here).

Ewan.