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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 09/12/2014 02:50 AM, Jan Beulich wrote:
> > > > 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.
But we know that at least one consumer of this interface (Linux/perf) 
does want to have full GPR set. So why not provide it? Especially since 
(I suspect that) doing memcpy may be faster than copying only selected 
fields.
If you are categorically against this I can certainly rework this and 
pass RIP, RSP, CS and EFLAGS only.
-boris

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