Re: [PATCH 5/5] KVM: x86/xen: Explicitly pad struct compat_vcpu_info to 64 bytes

[Woodhouse, David]

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



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.