[PATCH] shared_info size and padding fixes

[PATCH] shared_info size and padding fixes

[John Levon]

# HG changeset patch
# User john.levon@sun.com
# Date 1155157246 -3600
# Node ID e24df0e22cdfe6daaa0d4d40c88e13c0427a1515
# Parent  b60ea69932b1a4d10c3aae945a1ce1aa160c689b
sizeof(shared_info_t) can change even in compatible hypervisor versions.
Add a clear warning about this, and fix the few places relying on it.
In addition, add some padding to arch_shared_info, to allow future
additions to the structure without breaking known offsets of members
following it in the shared info.

Signed-off-by: John Levon <john.levon@sun.com>

diff -r b60ea69932b1 -r e24df0e22cdf tools/libxc/xc_hvm_build.c
--- a/tools/libxc/xc_hvm_build.c	Wed Aug 09 18:04:20 2006 +0100
+++ b/tools/libxc/xc_hvm_build.c	Wed Aug 09 22:00:46 2006 +0100
@@ -304,7 +304,7 @@ static int setup_guest(int xc_handle,
               xc_handle, dom, PAGE_SIZE, PROT_READ | PROT_WRITE,
               shared_info_frame)) == 0 )
         goto error_out;
-    memset(shared_info, 0, sizeof(shared_info_t));
+    memset(shared_info, 0, PAGE_SIZE);
     /* Mask all upcalls... */
     for ( i = 0; i < MAX_VIRT_CPUS; i++ )
         shared_info->vcpu_info[i].evtchn_upcall_mask = 1;
diff -r b60ea69932b1 -r e24df0e22cdf tools/libxc/xc_linux_build.c
--- a/tools/libxc/xc_linux_build.c	Wed Aug 09 18:04:20 2006 +0100
+++ b/tools/libxc/xc_linux_build.c	Wed Aug 09 22:00:46 2006 +0100
@@ -593,7 +593,7 @@ static int setup_guest(int xc_handle,
         xc_handle, dom, PAGE_SIZE, PROT_READ|PROT_WRITE, shared_info_frame);
     printf("shared_info = %p, err=%s frame=%lx\n",
            shared_info, strerror (errno), shared_info_frame);
-    //memset(shared_info, 0, sizeof(shared_info_t));
+    //memset(shared_info, 0, PAGE_SIZE);
     /* Mask all upcalls... */
     for ( i = 0; i < MAX_VIRT_CPUS; i++ )
         shared_info->vcpu_info[i].evtchn_upcall_mask = 1;
@@ -1064,7 +1064,7 @@ static int setup_guest(int xc_handle,
     /* shared_info page starts its life empty. */
     shared_info = xc_map_foreign_range(
         xc_handle, dom, PAGE_SIZE, PROT_READ|PROT_WRITE, shared_info_frame);
-    memset(shared_info, 0, sizeof(shared_info_t));
+    memset(shared_info, 0, PAGE_SIZE);
     /* Mask all upcalls... */
     for ( i = 0; i < MAX_VIRT_CPUS; i++ )
         shared_info->vcpu_info[i].evtchn_upcall_mask = 1;
diff -r b60ea69932b1 -r e24df0e22cdf tools/libxc/xc_linux_restore.c
--- a/tools/libxc/xc_linux_restore.c	Wed Aug 09 18:04:20 2006 +0100
+++ b/tools/libxc/xc_linux_restore.c	Wed Aug 09 22:00:46 2006 +0100
@@ -737,7 +737,7 @@ int xc_linux_restore(int xc_handle, int 
     /* Copy saved contents of shared-info page. No checking needed. */
     page = xc_map_foreign_range(
         xc_handle, dom, PAGE_SIZE, PROT_WRITE, shared_info_frame);
-    memcpy(page, shared_info, sizeof(shared_info_t));
+    memcpy(page, shared_info, PAGE_SIZE);
     munmap(page, PAGE_SIZE);
 
     /* Uncanonicalise the pfn-to-mfn table frame-number list. */
diff -r b60ea69932b1 -r e24df0e22cdf xen/arch/ia64/asm-offsets.c
--- a/xen/arch/ia64/asm-offsets.c	Wed Aug 09 18:04:20 2006 +0100
+++ b/xen/arch/ia64/asm-offsets.c	Wed Aug 09 22:00:46 2006 +0100
@@ -31,7 +31,6 @@ void foo(void)
 	DEFINE(IA64_SWITCH_STACK_SIZE, sizeof (struct switch_stack));
 	DEFINE(IA64_CPU_SIZE, sizeof (struct cpuinfo_ia64));
 	DEFINE(UNW_FRAME_INFO_SIZE, sizeof (struct unw_frame_info));
-	DEFINE(SHARED_INFO_SIZE, sizeof (struct shared_info));
 
 	BLANK();
 	DEFINE(IA64_MCA_CPU_INIT_STACK_OFFSET, offsetof (struct ia64_mca_cpu, init_stack));
diff -r b60ea69932b1 -r e24df0e22cdf xen/include/public/arch-ia64.h
--- a/xen/include/public/arch-ia64.h	Wed Aug 09 18:04:20 2006 +0100
+++ b/xen/include/public/arch-ia64.h	Wed Aug 09 22:00:46 2006 +0100
@@ -297,6 +297,8 @@ struct arch_shared_info {
 
     /* Interrupt vector for event channel.  */
     int evtchn_vector;
+
+    uint64_t pad[32];
 };
 typedef struct arch_shared_info arch_shared_info_t;
 
diff -r b60ea69932b1 -r e24df0e22cdf xen/include/public/arch-powerpc.h
--- a/xen/include/public/arch-powerpc.h	Wed Aug 09 18:04:20 2006 +0100
+++ b/xen/include/public/arch-powerpc.h	Wed Aug 09 22:00:46 2006 +0100
@@ -107,6 +107,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_conte
 DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
 
 struct arch_shared_info {
+    uint64_t pad[32];
 };
 
 struct arch_vcpu_info {
diff -r b60ea69932b1 -r e24df0e22cdf xen/include/public/arch-x86_32.h
--- a/xen/include/public/arch-x86_32.h	Wed Aug 09 18:04:20 2006 +0100
+++ b/xen/include/public/arch-x86_32.h	Wed Aug 09 22:00:46 2006 +0100
@@ -191,6 +191,7 @@ struct arch_shared_info {
     /* Frame containing list of mfns containing list of mfns containing p2m. */
     xen_pfn_t     pfn_to_mfn_frame_list_list;
     unsigned long nmi_reason;
+    uint64_t pad[32];
 };
 typedef struct arch_shared_info arch_shared_info_t;
 
diff -r b60ea69932b1 -r e24df0e22cdf xen/include/public/arch-x86_64.h
--- a/xen/include/public/arch-x86_64.h	Wed Aug 09 18:04:20 2006 +0100
+++ b/xen/include/public/arch-x86_64.h	Wed Aug 09 22:00:46 2006 +0100
@@ -261,6 +261,7 @@ struct arch_shared_info {
     /* Frame containing list of mfns containing list of mfns containing p2m. */
     xen_pfn_t     pfn_to_mfn_frame_list_list;
     unsigned long nmi_reason;
+    uint64_t pad[32];
 };
 typedef struct arch_shared_info arch_shared_info_t;
 
diff -r b60ea69932b1 -r e24df0e22cdf xen/include/public/xen.h
--- a/xen/include/public/xen.h	Wed Aug 09 18:04:20 2006 +0100
+++ b/xen/include/public/xen.h	Wed Aug 09 22:00:46 2006 +0100
@@ -376,7 +376,11 @@ typedef struct vcpu_info vcpu_info_t;
 
 /*
  * Xen/kernel shared data -- pointer provided in start_info.
- * NB. We expect that this struct is smaller than a page.
+ *
+ * This structure is defined to be both smaller than a page, and the
+ * only data on the page, but may vary in actual size even within
+ * compatible Xen versions; domains should never rely on the actual
+ * size of this structure.
  */
 struct shared_info {
     struct vcpu_info vcpu_info[MAX_VIRT_CPUS];

Re: [PATCH] shared_info size and padding fixes

[Tristan Gingold]

Le Mercredi 09 Août 2006 23:05, John Levon a écrit :
> # HG changeset patch
> # User john.levon@sun.com
> # Date 1155157246 -3600
> # Node ID e24df0e22cdfe6daaa0d4d40c88e13c0427a1515
> # Parent  b60ea69932b1a4d10c3aae945a1ce1aa160c689b
> sizeof(shared_info_t) can change even in compatible hypervisor versions.
> Add a clear warning about this, and fix the few places relying on it.
> In addition, add some padding to arch_shared_info, to allow future
> additions to the structure without breaking known offsets of members
> following it in the shared info.
Hi,

Comment for ia64:

[...]
> diff -r b60ea69932b1 -r e24df0e22cdf xen/arch/ia64/asm-offsets.c
> --- a/xen/arch/ia64/asm-offsets.c	Wed Aug 09 18:04:20 2006 +0100
> +++ b/xen/arch/ia64/asm-offsets.c	Wed Aug 09 22:00:46 2006 +0100
> @@ -31,7 +31,6 @@ void foo(void)
>  	DEFINE(IA64_SWITCH_STACK_SIZE, sizeof (struct switch_stack));
>  	DEFINE(IA64_CPU_SIZE, sizeof (struct cpuinfo_ia64));
>  	DEFINE(UNW_FRAME_INFO_SIZE, sizeof (struct unw_frame_info));
> -	DEFINE(SHARED_INFO_SIZE, sizeof (struct shared_info));
>
>  	BLANK();
>  	DEFINE(IA64_MCA_CPU_INIT_STACK_OFFSET, offsetof (struct ia64_mca_cpu,
> init_stack)); diff -r b60ea69932b1 -r e24df0e22cdf
[...]

Please, do not remove this define.  It is used to check the struct is not 
bigger than a page!

Tristan.

Re: [PATCH] shared_info size and padding fixes

[Jan Beulich]

>>  	DEFINE(IA64_SWITCH_STACK_SIZE, sizeof (struct switch_stack));
>>  	DEFINE(IA64_CPU_SIZE, sizeof (struct cpuinfo_ia64));
>>  	DEFINE(UNW_FRAME_INFO_SIZE, sizeof (struct unw_frame_info));
>> -	DEFINE(SHARED_INFO_SIZE, sizeof (struct shared_info));
>>
>>  	BLANK();
>>  	DEFINE(IA64_MCA_CPU_INIT_STACK_OFFSET, offsetof (struct
ia64_mca_cpu,
>
>Please, do not remove this define.  It is used to check the struct is
not 
>bigger than a page!

Checking this should be done with BUILD_BUG_ON() in a .c file, and
hence
not require an entry in asm-offsets.c.

Jan

Re: [PATCH] shared_info size and padding fixes

[Tristan Gingold]

Le Jeudi 10 Août 2006 10:31, Jan Beulich a écrit :
> >>  	DEFINE(IA64_SWITCH_STACK_SIZE, sizeof (struct switch_stack));
> >>  	DEFINE(IA64_CPU_SIZE, sizeof (struct cpuinfo_ia64));
> >>  	DEFINE(UNW_FRAME_INFO_SIZE, sizeof (struct unw_frame_info));
> >> -	DEFINE(SHARED_INFO_SIZE, sizeof (struct shared_info));
> >>
> >>  	BLANK();
> >>  	DEFINE(IA64_MCA_CPU_INIT_STACK_OFFSET, offsetof (struct
>
> ia64_mca_cpu,
>
> >Please, do not remove this define.  It is used to check the struct is
>
> not
>
> >bigger than a page!
>
> Checking this should be done with BUILD_BUG_ON() in a .c file, and
> hence
> not require an entry in asm-offsets.c.
Thank you for the tip.
I will write a patch for this.

Tristan.

Re: [PATCH] shared_info size and padding fixes

[Jan Beulich]

>>> John Levon <levon@movementarian.org> 09.08.06 23:05 >>>
>sizeof(shared_info_t) can change even in compatible hypervisor
versions.
>Add a clear warning about this, and fix the few places relying on it.
>In addition, add some padding to arch_shared_info, to allow future
>additions to the structure without breaking known offsets of members
>following it in the shared info.

Shouldn't you also bump the ABI version with these compatibility
breaking
changes to the shared info structure?

Jan

Re: [PATCH] shared_info size and padding fixes

[John Levon]

On Thu, Aug 10, 2006 at 10:33:01AM +0200, Jan Beulich wrote:

> >sizeof(shared_info_t) can change even in compatible hypervisor
> versions.
> >Add a clear warning about this, and fix the few places relying on it.
> >In addition, add some padding to arch_shared_info, to allow future
> >additions to the structure without breaking known offsets of members
> >following it in the shared info.
> 
> Shouldn't you also bump the ABI version with these compatibility
> breaking
> changes to the shared info structure?

No, see the previous discussion with Keir - changing the size of the
shared info structure is apparently not part of the ABI (hence the
reason for the patch and the explanation above!)

Personally this seems unusual at best to me, but that's the way it is,
and furthermore has already happened with "nmi_reason".

john

Re: [PATCH] shared_info size and padding fixes

[Keir Fraser]

On 10/8/06 1:13 pm, "John Levon" <levon@movementarian.org> wrote:

> No, see the previous discussion with Keir - changing the size of the
> shared info structure is apparently not part of the ABI (hence the
> reason for the patch and the explanation above!)
> 
> Personally this seems unusual at best to me, but that's the way it is,
> and furthermore has already happened with "nmi_reason".

Your patch looked fine, by the way, except that adding padding to every
arch-specific portion of shared info seems unnecessary.

 -- Keir

Re: [PATCH] shared_info size and padding fixes

[John Levon]

On Thu, Aug 10, 2006 at 02:29:25PM +0100, Keir Fraser wrote:

> > No, see the previous discussion with Keir - changing the size of the
> > shared info structure is apparently not part of the ABI (hence the
> > reason for the patch and the explanation above!)
> > 
> > Personally this seems unusual at best to me, but that's the way it is,
> > and furthermore has already happened with "nmi_reason".
> 
> Your patch looked fine, by the way, except that adding padding to every
> arch-specific portion of shared info seems unnecessary.

Imagine we want to add a new field to shared_info. It has to go after
the "arch" member, or offsets break. Once this happens, none of the
arch_shared_info's can be modified at all, as this will break the offset
of the new member.

It seems vastly better to provide some headroom *now* than to make a
mistake later.

regards
john

Re: [PATCH] shared_info size and padding fixes

[John Levon]

On Thu, Aug 10, 2006 at 02:29:25PM +0100, Keir Fraser wrote:

> Your patch looked fine, by the way, except that adding padding to every
> arch-specific portion of shared info seems unnecessary.

On the presumption that silence means no, here's a patch without the
padding. I sincerely hope whoever next changes these structures
remembers the padding!

regards
john

# HG changeset patch
# User levon@movementarian.org
# Date 1155298435 -3600
# Node ID 43cc94ddfa1c31b1c0d288344ec3147fd61180e5
# Parent  b60ea69932b1a4d10c3aae945a1ce1aa160c689b
Add a clear warning that shared_info_t can change in size even in
compatible ABI revisions. Fix a few places relying on its size.

Signed-off-by: John Levon <john.levon@sun.com>

diff -r b60ea69932b1 -r 43cc94ddfa1c tools/libxc/xc_hvm_build.c
--- a/tools/libxc/xc_hvm_build.c	Wed Aug 09 18:04:20 2006 +0100
+++ b/tools/libxc/xc_hvm_build.c	Fri Aug 11 13:13:55 2006 +0100
@@ -304,7 +304,7 @@ static int setup_guest(int xc_handle,
               xc_handle, dom, PAGE_SIZE, PROT_READ | PROT_WRITE,
               shared_info_frame)) == 0 )
         goto error_out;
-    memset(shared_info, 0, sizeof(shared_info_t));
+    memset(shared_info, 0, PAGE_SIZE);
     /* Mask all upcalls... */
     for ( i = 0; i < MAX_VIRT_CPUS; i++ )
         shared_info->vcpu_info[i].evtchn_upcall_mask = 1;
diff -r b60ea69932b1 -r 43cc94ddfa1c tools/libxc/xc_linux_build.c
--- a/tools/libxc/xc_linux_build.c	Wed Aug 09 18:04:20 2006 +0100
+++ b/tools/libxc/xc_linux_build.c	Fri Aug 11 13:13:55 2006 +0100
@@ -593,7 +593,7 @@ static int setup_guest(int xc_handle,
         xc_handle, dom, PAGE_SIZE, PROT_READ|PROT_WRITE, shared_info_frame);
     printf("shared_info = %p, err=%s frame=%lx\n",
            shared_info, strerror (errno), shared_info_frame);
-    //memset(shared_info, 0, sizeof(shared_info_t));
+    //memset(shared_info, 0, PAGE_SIZE);
     /* Mask all upcalls... */
     for ( i = 0; i < MAX_VIRT_CPUS; i++ )
         shared_info->vcpu_info[i].evtchn_upcall_mask = 1;
@@ -1064,7 +1064,7 @@ static int setup_guest(int xc_handle,
     /* shared_info page starts its life empty. */
     shared_info = xc_map_foreign_range(
         xc_handle, dom, PAGE_SIZE, PROT_READ|PROT_WRITE, shared_info_frame);
-    memset(shared_info, 0, sizeof(shared_info_t));
+    memset(shared_info, 0, PAGE_SIZE);
     /* Mask all upcalls... */
     for ( i = 0; i < MAX_VIRT_CPUS; i++ )
         shared_info->vcpu_info[i].evtchn_upcall_mask = 1;
diff -r b60ea69932b1 -r 43cc94ddfa1c tools/libxc/xc_linux_restore.c
--- a/tools/libxc/xc_linux_restore.c	Wed Aug 09 18:04:20 2006 +0100
+++ b/tools/libxc/xc_linux_restore.c	Fri Aug 11 13:13:55 2006 +0100
@@ -737,7 +737,7 @@ int xc_linux_restore(int xc_handle, int 
     /* Copy saved contents of shared-info page. No checking needed. */
     page = xc_map_foreign_range(
         xc_handle, dom, PAGE_SIZE, PROT_WRITE, shared_info_frame);
-    memcpy(page, shared_info, sizeof(shared_info_t));
+    memcpy(page, shared_info, PAGE_SIZE);
     munmap(page, PAGE_SIZE);
 
     /* Uncanonicalise the pfn-to-mfn table frame-number list. */
diff -r b60ea69932b1 -r 43cc94ddfa1c xen/include/public/xen.h
--- a/xen/include/public/xen.h	Wed Aug 09 18:04:20 2006 +0100
+++ b/xen/include/public/xen.h	Fri Aug 11 13:13:55 2006 +0100
@@ -376,7 +376,11 @@ typedef struct vcpu_info vcpu_info_t;
 
 /*
  * Xen/kernel shared data -- pointer provided in start_info.
- * NB. We expect that this struct is smaller than a page.
+ *
+ * This structure is defined to be both smaller than a page, and the
+ * only data on the page, but may vary in actual size even within
+ * compatible Xen versions; domains should never rely on the actual
+ * size of this structure.
  */
 struct shared_info {
     struct vcpu_info vcpu_info[MAX_VIRT_CPUS];