Re: [Xen-devel] [PATCH v10 09/20] x86/VPMU: Add public xenpmu.h

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.09.14 at 18:51, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 09/11/2014 11:59 AM, Jan Beulich wrote:
>>>>> On 11.09.14 at 17:26, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> On 09/11/2014 10:55 AM, Jan Beulich wrote:
>>>>>>> On 11.09.14 at 15:54, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>>> On 09/11/2014 02:39 AM, Jan Beulich wrote:
>>>>>>>>> On 10.09.14 at 19:23, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>>>>> On 09/10/2014 10:45 AM, Jan Beulich wrote:
>>>>>>>>>>> On 04.09.14 at 05:41, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>>>>>>> +struct xen_pmu_arch {
>>>>>>>>> +    union {
>>>>>>>>> +        struct cpu_user_regs regs;
>>>>>>>>> +        uint8_t pad[256];
>>>>>>>>> +    } r;
>>>>>>>> Can you remind me again what you need the union and padding for
>>>>>>>> here?
>>>>>>> This structure is laid out in a shared page with a (possibly 32-bit)
>>>>>>> guest who need to access fields that follow this union.
>>>>>> Hmm, okay. But how would such a guest make reasonable use of
>>>>>> the regs field then?
>>>>> When hypervisor is preparing this data for 32-bit consumer in
>>>>> vpmu_do_interrupts() it translates registers to 32-bit version:
>>>>>
>>>>>            struct compat_cpu_user_regs *cmp;
>>>>>            gregs = guest_cpu_user_regs();
>>>>>            cmp = (void *)&vpmu->xenpmu_data->pmu.r.regs;
>>>>>            XLAT_cpu_user_regs(cmp, gregs);
>>>>>
>>>>> I remember struggling trying to figure a better way of presenting this
>>>>> but ended up with the (void *) cast. IIRC I tried putting
>>>>> compat_cpu_user_regs into the union but something didn't quite work
>>>>> (with compilation).
>>>> Of course that can't work - the compat structure simply doesn't
>>>> exist for public headers.
>>>>
>>>>>> And then - why 256 and not 200? struct
>>>>>> cpu_user_regs can't change size anyway. Plus, finally, why do
>>>>>> you expose the GPRs but not any of the other register state?
>>>>> I wanted to leave some padding in case we decide to add non-GPR
>>>>> registers and keep major version of the interface unchanged (only minor
>>>>> version will bumped). TBH though, I can't think of any non-GPR registers
>>>>> to be ever useful.
>>>> Then what do you need the GPRs for here? I don't think they're
>>>> any better or worse than, say, XMM ones. I could see you needing/
>>>> wanting some basic stuff like CS:RIP and SS:RSP and maybe EFLAGS,
>>>> but that's about it.
>>> I believe some perf sub-tools (tracing-related if I am not mistaken)
>>> want to have access to traced function's arguments.
>> And function arguments on x86-64 can very well live in XMM
>> registers... Hence no, I still don't see why the registers get
>> exposed here in an incomplete/inconsistent fashion.
> 
> Linux perf handler takes struct pt_regs as the its sole argument. If we 
> pass only few selected registers from hypervisor to the guest then I 
> will be passing garbage (partly) to perf.

Okay, so you're tailoring the hypervisor interface to Linux. That's
not what we generally aim for, and hence I continue to think this
isn't the right approach.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel