shared_info size and padding fixes

PATCH In review John Levon 4 participants 9 messages Aug 9, 2006 – Aug 11, 2006

Share & tools

John Levon

author

2006-08-09 21:05 UTC #0 lore raw

PATCH 1/1 shared_info size and padding fixes

# HG changeset patch
# User [email protected]
# 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 <[email protected]>

tools/libxc/xc_hvm_build.c +1 -1

diff -r b60ea69932b1 -r e24df0e22cdf tools/libxc/xc_hvm_build.c
@@ -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;

tools/libxc/xc_linux_build.c +2 -2

diff -r b60ea69932b1 -r e24df0e22cdf tools/libxc/xc_linux_build.c
@@ -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;

tools/libxc/xc_linux_restore.c +1 -1

diff -r b60ea69932b1 -r e24df0e22cdf tools/libxc/xc_linux_restore.c
@@ -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. */

xen/arch/ia64/asm-offsets.c +0 -1

diff -r b60ea69932b1 -r e24df0e22cdf xen/arch/ia64/asm-offsets.c
@@ -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));

xen/include/public/arch-ia64.h +2 -0

diff -r b60ea69932b1 -r e24df0e22cdf xen/include/public/arch-ia64.h
@@ -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;

xen/include/public/arch-powerpc.h +1 -0

diff -r b60ea69932b1 -r e24df0e22cdf xen/include/public/arch-powerpc.h
@@ -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 {

xen/include/public/arch-x86_32.h +1 -0

diff -r b60ea69932b1 -r e24df0e22cdf xen/include/public/arch-x86_32.h
@@ -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;

xen/include/public/arch-x86_64.h +1 -0

diff -r b60ea69932b1 -r e24df0e22cdf xen/include/public/arch-x86_64.h
@@ -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;

xen/include/public/xen.h +5 -1

diff -r b60ea69932b1 -r e24df0e22cdf xen/include/public/xen.h
@@ -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];

Tristan Gingold

2006-08-10 08:03 UTC #1 lore raw

Quoted 10 lines · John Levon

# HG changeset patch
# User [email protected]
# 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:

[...]

Quoted 11 lines

diff -r b60ea69932b1 -r e24df0e22cdf xen/arch/ia64/asm-offsets.c

xen/arch/ia64/asm-offsets.c +0 -1

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

Quoted 1 line

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.

Jan Beulich

2006-08-10 08:31 UTC #2 lore raw

Quoted 7 lines

xen/arch/ia64/asm-offsets.c void foo(void)

Quoted 7 lines

xen/arch/ia64/asm-offsets.c +0 -1

 	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,

Quoted 2 lines

Please, do not remove this define. It is used to check the struct is

not

Quoted 1 line

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

Tristan Gingold

2006-08-10 08:42 UTC #3 lore raw

Quoted 12 lines · Jan Beulich

Quoted 7 lines
xen/arch/ia64/asm-offsets.c void foo(void)
Quoted 7 lines
xen/arch/ia64/asm-offsets.c +0 -1
 	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,
Quoted 1 line
Please, do not remove this define. It is used to check the struct is

Quoted 7 lines

not
Quoted 1 line
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.

Jan Beulich

2006-08-10 08:33 UTC #4 lore raw

In reply to John Levon

Quoted 2 lines

Quoted 1 line
Quoted 1 line
John Levon <[email protected]> 09.08.06 23:05 >>>
sizeof(shared_info_t) can change even in compatible hypervisor

versions.

Quoted 4 lines

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

John Levon

author

2006-08-10 12:13 UTC #5 lore raw

Quoted 10 lines · Jan Beulich

Quoted 1 line
sizeof(shared_info_t) can change even in compatible hypervisor
versions.
Quoted 4 lines
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

Keir Fraser

2006-08-10 13:29 UTC #6 lore raw

Quoted 6 lines · John Levon

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

John Levon

author

2006-08-10 13:34 UTC #7 lore raw

Quoted 9 lines · Keir Fraser

Quoted 6 lines
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

John Levon

author

2006-08-11 13:30 UTC #8 lore raw

In reply to Keir Fraser

Quoted 2 lines · Keir Fraser

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 [email protected]
# 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 <[email protected]>

tools/libxc/xc_hvm_build.c +1 -1

diff -r b60ea69932b1 -r 43cc94ddfa1c tools/libxc/xc_hvm_build.c
@@ -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;

tools/libxc/xc_linux_build.c +2 -2

diff -r b60ea69932b1 -r 43cc94ddfa1c tools/libxc/xc_linux_build.c
@@ -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;

tools/libxc/xc_linux_restore.c +1 -1

diff -r b60ea69932b1 -r 43cc94ddfa1c tools/libxc/xc_linux_restore.c
@@ -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. */

xen/include/public/xen.h +5 -1

diff -r b60ea69932b1 -r 43cc94ddfa1c xen/include/public/xen.h
@@ -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];