Re: [Xen-devel] [PATCH v8 4/6] arm/vm_event: get/set registers

[Julien Grall <julien.grall@xxxxxxx>]

On 07/07/16 09:23, Jan Beulich wrote:
> > > > On 06.07.16 at 21:12, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> > On Wed, Jul 6, 2016 at 12:04 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> > > On 05/07/16 19:37, Tamas K Lengyel wrote:
> > > > +void vm_event_fill_regs(vm_event_request_t *req)
> > > > +{
> > > > +    const struct cpu_user_regs *regs = guest_cpu_user_regs();
> > > > +
> > > > +    req->data.regs.arm.cpsr = regs->cpsr;
> > > > +    req->data.regs.arm.ttbr0 = READ_SYSREG64(TTBR0_EL1);
> > > > +    req->data.regs.arm.ttbr1 = READ_SYSREG64(TTBR1_EL1);
> > > > +
> > > > +    if ( psr_mode_is_32bit(regs->cpsr) )
> > > > +    {
> > > > +        req->data.regs.arm.arch.arm32.r0 = regs->r0;
> > > > +        req->data.regs.arm.arch.arm32.r1 = regs->r1;
> > > > +        req->data.regs.arm.arch.arm32.r2 = regs->r2;
> > > > +        req->data.regs.arm.arch.arm32.r3 = regs->r3;
> > > > +        req->data.regs.arm.arch.arm32.r4 = regs->r4;
> > > > +        req->data.regs.arm.arch.arm32.r5 = regs->r5;
> > > > +        req->data.regs.arm.arch.arm32.r6 = regs->r6;
> > > > +        req->data.regs.arm.arch.arm32.r7 = regs->r7;
> > > > +        req->data.regs.arm.arch.arm32.r8 = regs->r8;
> > > > +        req->data.regs.arm.arch.arm32.r9 = regs->r9;
> > > > +        req->data.regs.arm.arch.arm32.r10 = regs->r10;
> > > > +        req->data.regs.arm.arch.arm32.r11 = regs->fp;
> > > > +        req->data.regs.arm.arch.arm32.r12 = regs->r12;
> > > > +        req->data.regs.arm.arch.arm32.r13 = regs->sp;
> > >
> > > Please look at the description of "sp" in cpu_user_regs. You will notice
> > > this is only valid for the hypervisor.
> > >
> > > There are multiple stack pointers for the guest depending on the running
> > > mode (see B9.2.1 in ARM DDI 0406C.c), so you may want to pass all of them.
> > >
> > > > +        req->data.regs.arm.arch.arm32.r14 = regs->lr;
> > >
> > > Whilst lr is an union with lr_usr on ARM32 port, for the ARM64 port they are
> > > distinct (see cpu_users). So you would use the wrong register here.
> > >
> > > However, as for sp, there are multiple lr pointers for the guest depending
> > > on the running mode. So you will pass the wrong lr if the guest is running
> > > in another mode than user.
> > This patch is becoming a lot more work then what I need it for so I'm
> > inclined to just reduce it to the bare minimum my use-case requires,
> > which is only cpsr, pc and ttbr0 and ttbr1. I had reservation about
> > growing the data field in the vm_event struct anyway, especially since
> > there is no use-case that requires it. In case someone has an actual
> > use-case in the future that requires other registers to be submitted
> > through vm_event, the interface is available for extension.
> Rather than dropping this patch entirely (as you suggest in your
> other reply) I am, fwiw, fine with a register set not including any
> GPRs at all. Not sure about Julien though.
Well, the SMC instructions are used to communicate with the secure mode 
(usually a trusted firmware).
In general, if you want to trap the SMC instructions it is for emulating 
them and not using them for breakpoint in the guest (as for Tamas's 
use-case). In this case, you want to have the set of GPRs available in 
the vm_event request to avoid the overhead of the DOMCTL_getvcpucontext 
(assuming the vCPU has been paused).
Adding the GPRs in the vm_event will likely require to bump the version 
number (VM_EVENT_INTERFACE_VERSION) because the ABI will not be 
compatible. If we consider that it is ok to ask developers to rebuild 
their vm-event app, then fine.
Anyway, I think this patch was in a good state (though few registers 
were needed clarification). Assuming it is ok to break the compatibility 
later on, I will not oppose to have a reduce set.
Regards,

--
Julien Grall

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