Re: [PATCH v2 1/4] x86/kvm/hyper-v: Align the hcall param for kvm_hyperv_exit

Jon Doron
2020-03-06T14:43:13Z

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@redhat.com> wrote:
>
> Paolo Bonzini <pbonzini@redhat.com> 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
>