Thanks Vitaly and Paoloo I'll fix the 1st patch and wait for the final review on the other 3 and submit v3, I'll also look into adding the proper test for the Hypercall patch to https://git.kernel.org/pub/scm/virt/kvm/kvm-unit-tests.git, and submit a separate patch to that repository. Thanks, -- Jon. On Fri, Mar 6, 2020 at 12:30 PM Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> wrote: > > Paolo Bonzini <pbonzini@xxxxxxxxxx> writes: > > > On 05/03/20 15:53, Jon Doron wrote: > >> Vitaly recommended we will align the struct to 64bit... > > > > Oh, then I think you actually should add a padding after "__u32 type;" > > and "__u32 msr;" if you want to make it explicit. The patch, as is, is > > not aligning anything, hence my confusion. > > > > Right, > > the problem I tried to highlight is that without propper padding ABI may > change, e.g. > > #include <stdio.h> > #include <stdint.h> > #include <stddef.h> > > #define __u32 uint32_t > #define __u64 uint64_t > > struct kvm_hyperv_exit { > __u32 type; > union { > struct { > __u32 msr; > __u64 control; > __u64 evt_page; > __u64 msg_page; > } synic; > struct { > __u64 input; > __u64 result; > __u64 params[2]; > } hcall; > } u; > }; > > int main() { > printf("%d\n", offsetof(struct kvm_hyperv_exit, u.synic.control)); > printf("%d\n", offsetof(struct kvm_hyperv_exit, u.hcall.input)); > > return 0; > } > > $ gcc -m32 1.c -o 1 > $ ./1 > 8 > 4 > > $ gcc 1.c -o 1 > $ ./1 > 16 > 8 > > if we add a padding after 'type' and 'msr' we'll get > $ gcc -m32 1.c -o 1 > $ ./1 > 16 > 8 > > $ gcc 1.c -o 1 > $ ./1 > 16 > 8 > > which is much better. Technically, this is an ABI change on 32 bit but > I'm pretty sure noone cares (famous last words!). > > -- > Vitaly >
