Re: [PATCH v2] x86/hyper-v: Mark TLFS structures packed

From: Roman Kagan <rkagan@virtuozzo.com>
To: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: "Nadav Amit" <nadav.amit@gmail.com>,
	"kvm list" <kvm@vger.kernel.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Radim Krčmář" <rkrcmar@redhat.com>,
	LKML <linux-kernel@vger.kernel.org>,
	"K. Y. Srinivasan" <kys@microsoft.com>,
	"Haiyang Zhang" <haiyangz@microsoft.com>,
	"Stephen Hemminger" <sthemmin@microsoft.com>,
	"x86@kernel.org" <x86@kernel.org>,
	"Michael Kelley (EOSG)" <Michael.H.Kelley@microsoft.com>,
	"Thomas Gleixner" <tglx@linutronix.de>
Subject: Re: [PATCH v2] x86/hyper-v: Mark TLFS structures packed
Date: Mon, 3 Dec 2018 07:15:25 +0000	[thread overview]
Message-ID: <20181203071520.GA17278@rkaganb.sw.ru> (raw)
In-Reply-To: <87k1krh588.fsf@vitty.brq.redhat.com>

On Mon, Dec 03, 2018 at 12:35:35AM +0100, Vitaly Kuznetsov wrote:
> Nadav Amit <nadav.amit@gmail.com> writes:
> 
> [skip]
> 
> >
> > Having said that, something else is sort of strange in the TLFS definitions,
> > I think (I really know little about this whole protocol). Look at the
> > following definitions from hyperv-tlfs.h:
> >
> >> struct hv_vpset {
> >>         u64 format;
> >>         u64 valid_bank_mask;
> >>         u64 bank_contents[];
> >> };
> >> 
> >> struct hv_tlb_flush_ex {
> >>         u64 address_space;
> >>         u64 flags;
> >>         struct hv_vpset hv_vp_set;
> >>         u64 gva_list[];
> >> };
> >
> > It seems you have two flexible array members at the end of hv_tlb_flush_ex.
> > This causes bank_contents[x] and gva_list[x] to overlap. So unless they have
> > the same meaning, this asks for trouble IMHO.
> >
> 
> This is weird but intentional :-) We're just following Hyper-V spec
> here.
> 
> E.g. HvFlushVirtualAddressListEx hypercall has the following input ABI:
> 
> [Fixed len head][[Fixed len VP set spec]Var len VP set][Var len addr List]
> 
> "Fixed len VP set spec" defines the true length of "Var len VP set" and
> "Address List" starts right after that. The length of the whole
> structure is also known.
> 
> So bank_contents[] and gva_list[] do overlap (and have different
> meaning). We take special precautions when forming the structure
> (e.g. fill_gva_list() takes 'offset').

This basically means that the argument of this hypercall can't be
represented by a C struct.  gva_list just can't be used.  So I'd rather
remove it from the struct (but leave a comment to that end perhaps), and
construct the message in place (as is done now anyway).

Roman.