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

[Jan Beulich <jbeulich@suse.com>]

On 07.01.2020 17:45, Michael Kelley wrote:
> From: Wei Liu <wl@xen.org> Sent: Tuesday, January 7, 2020 8:34 AM
>>
>> 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);
>>
>>    hv_do_hypercall(OP, virt_to_maddr(&foo), ...);
>>
>> ?
>>
>> I don't think this is transparent to user sites. Plus, foo is on stack
>> which is 1) difficult to get its maddr, 2) may cross page boundary.
>>
>> If I misunderstood what you meant, please give me an example here.
>>
>>>
>>>> @@ -83,14 +84,33 @@ static void __init setup_hypercall_page(void)
>>>>      wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>>>>  }
>>>>
>>>> +static void setup_hypercall_pcpu_arg(void)
>>>> +{
>>>> +    void *mapping;
>>>> +
>>>> +    mapping = alloc_xenheap_page();
>>>> +    if ( !mapping )
>>>> +        panic("Failed to allocate hypercall input page for %u\n",
>>>
>>> "... for CPU%u\n" please.
>>>
>>>> +              smp_processor_id());
>>>> +
>>>> +    this_cpu(hv_pcpu_input_arg) = mapping;
>>>
>>> When offlining and then re-onlining a CPU, the prior page will be
>>> leaked.
>>
>> Right. Thanks for catching this one.
>>
>>>
>>>> --- a/xen/include/asm-x86/guest/hyperv.h
>>>> +++ b/xen/include/asm-x86/guest/hyperv.h
>>>> @@ -51,6 +51,8 @@ static inline uint64_t hv_scale_tsc(uint64_t tsc, uint64_t scale,
>>>>
>>>>  #ifdef CONFIG_HYPERV_GUEST
>>>>
>>>> +#include <xen/percpu.h>
>>>> +
>>>>  #include <asm/guest/hypervisor.h>
>>>>
>>>>  struct ms_hyperv_info {
>>>> @@ -63,6 +65,8 @@ struct ms_hyperv_info {
>>>>  };
>>>>  extern struct ms_hyperv_info ms_hyperv;
>>>>
>>>> +DECLARE_PER_CPU(void *, hv_pcpu_input_arg);
>>>
>>> Will this really be needed outside of the file that defines it?
>>>
>>
>> This can live in a private header for the time being.
>>
>>> 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".
>>
>> 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.
> 
> Correct.  The __packed attribute was added only about a year ago
> after somebody on LKML noticed that the structures were not packed.
> Some discussion ensued, but the consensus was to add __packed due
> to general  paranoia about what the compiler might do even though
> individual fields are aligned to their natural boundary.

Which, as wwe've found the hard way, then leads to problems (with at
least gcc 9) when wanting to take the address of a member of such a
structure, as the compiler then (mostly validly) assumes the pointer
won't be naturally aligned. Hence, as also said in reply to Wei
elsewhere, we found it necessary to drop various __packed in order
to be able to build Xen with gcc 9.

Jan

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