Re: [Xen-devel] [PATCH v3 3/5] x86/hyperv: provide percpu hypercall input page

[Wei Liu <wl@xen.org>]

On Tue, Jan 07, 2020 at 06:08:19PM +0100, Jan Beulich wrote:
> On 07.01.2020 17:33, Wei Liu wrote:
> > On Mon, Jan 06, 2020 at 11:27:18AM +0100, Jan Beulich wrote:
> >> On 05.01.2020 17:47, Wei Liu wrote:
> >>> Hyper-V's input / output argument must be 8 bytes aligned an not cross
> >>> page boundary. The easiest way to satisfy those requirements is to use
> >>> percpu page.
> >>
> >> I'm not sure "easiest" is really true here. Others could consider adding
> >> __aligned() attributes as easy or even easier (by being even more
> >> transparent to use sites). Could we settle on "One way ..."?
> > 
> > Do you mean something like
> > 
> >    struct foo __aligned(8);
> 
> If this is in a header and ...
> 
> >    hv_do_hypercall(OP, virt_to_maddr(&foo), ...);
> 
> ... this in actual code, then yes.
> 
> > ?
> > 
> > I don't think this is transparent to user sites. Plus, foo is on stack
> > which is 1) difficult to get its maddr,
> 
> It being on the stack may indeed complicate getting its machine address
> (if not now, then down the road) - valid point.
> 
> > 2) may cross page boundary.
> 
> The __aligned() of course needs to be large enough to avoid this
> happening.

For this alignment to be large enough, it will need to be of PAGE_SIZE,
right? Wouldn't that blow up Xen's stack easily?  Given we only have two
pages for that.

In light of these restrictions, the approach I take in the original
patch should be okay.

I'm fine with changing the wording to "One way ..." -- if that's the
only objection you have after this mail.

> 
> >> Also, while looking at this I notice that - despite my earlier
> >> comment when giving the respective, sort-of-conditional ack -
> >> there are (still) many apparently pointless __packed attributes
> >> in hyperv-tlfs.h. Care to comment on this?
> > 
> > Again, that's a straight import from Linux. I tried not to deviate too
> > much. A commit in Linux (ec084491727b0) claims "compiler can add
> > alignment padding to structures or reorder struct members for
> > randomization and optimization".
> 
> Would a compiler doing so (without explicitly being told to) even
> be in line with the C spec? I'd buy such a claim only if I see an
> example proving it.
> 
> > I just checked all the packed structures. They seem to have all the
> > required manual paddings already. I can only assume they tried to erred
> > on the safe side.
> 
> And you surely recall we had to remove quite a few instances of
> __packed for gcc 9 compatibility?

Fair enough. I will write a patch to drop those __packed attributes.

Wei.

> 
> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel