| Message ID | 20210210182609.435200-6-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86/xen: Selftest fixes and a cleanup | expand |
On Wed, 2021-02-10 at 10:26 -0800, Sean Christopherson wrote: > Add a 2 byte pad to struct compat_vcpu_info so that the sum size of its > fields is actually 64 bytes. The effective size without the padding is > also 64 bytes due to the compiler aligning evtchn_pending_sel to a 4-byte > boundary, but depending on compiler alignment is subtle and unnecessary. I think there's at least one BUILD_BUG_ON() which would have triggered if the compiler ever did stop honouring the ELF ABI. And in fact in a parallel universe where the ABI permits such packing, the padding would be *wrong*, since the original Xen struct doesn't have the padding. It *does* have an explicit uint8_t to replace evtchn_upcall_mask but it doesn't have the following two bytes; canonically we *are* supposed to take our chances with the ABI there. Although of course the relevant ABI is the *32-bit* ABI in the compat case, not the 64-bit ABI. They both align 32-bit values to 32 bits though. uint8_t evtchn_upcall_pending; #ifdef XEN_HAVE_PV_UPCALL_MASK uint8_t evtchn_upcall_mask; #else /* XEN_HAVE_PV_UPCALL_MASK */ uint8_t pad0; #endif /* XEN_HAVE_PV_UPCALL_MASK */ xen_ulong_t evtchn_pending_sel; struct arch_vcpu_info arch; struct vcpu_time_info time; }; /* 64 bytes (x86) */ So it isn't clear the additionally padding really buys us anything; if we play this game without knowing the ABI we'd be screwed anyway. But it doesn't hurt. > Opportunistically replace spaces with tables in the other fields. That part I certainly approve of. Reviewed-by: David Woodhouse <dwmw@amazon.co.uk> Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
On Wed, Feb 10, 2021, Woodhouse, David wrote: > On Wed, 2021-02-10 at 10:26 -0800, Sean Christopherson wrote: > So it isn't clear the additionally padding really buys us anything; if > we play this game without knowing the ABI we'd be screwed anyway. But > it doesn't hurt. Ya, this is purely for folks reading the code and wondering how 62==64.
On Wed, 2021-02-10 at 13:13 -0800, Sean Christopherson wrote: > On Wed, Feb 10, 2021, Woodhouse, David wrote: > > On Wed, 2021-02-10 at 10:26 -0800, Sean Christopherson wrote: > > So it isn't clear the additionally padding really buys us anything; if > > we play this game without knowing the ABI we'd be screwed anyway. But > > it doesn't hurt. > > Ya, this is purely for folks reading the code and wondering how 62==64. Fair enough. That kind of thing is why I littered the code with assertions based on sizeof() and offsetof() :) Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
diff --git a/arch/x86/kvm/xen.h b/arch/x86/kvm/xen.h index 4b32489c0cec..b66a921776f4 100644 --- a/arch/x86/kvm/xen.h +++ b/arch/x86/kvm/xen.h @@ -49,11 +49,12 @@ struct compat_arch_vcpu_info { }; struct compat_vcpu_info { - uint8_t evtchn_upcall_pending; - uint8_t evtchn_upcall_mask; - uint32_t evtchn_pending_sel; - struct compat_arch_vcpu_info arch; - struct pvclock_vcpu_time_info time; + uint8_t evtchn_upcall_pending; + uint8_t evtchn_upcall_mask; + uint16_t pad; + uint32_t evtchn_pending_sel; + struct compat_arch_vcpu_info arch; + struct pvclock_vcpu_time_info time; }; /* 64 bytes (x86) */ struct compat_arch_shared_info {
Add a 2 byte pad to struct compat_vcpu_info so that the sum size of its fields is actually 64 bytes. The effective size without the padding is also 64 bytes due to the compiler aligning evtchn_pending_sel to a 4-byte boundary, but depending on compiler alignment is subtle and unnecessary. Opportunistically replace spaces with tables in the other fields. Cc: David Woodhouse <dwmw@amazon.co.uk> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/xen.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)