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

[Julien Grall <julien.grall@arm.com>]

On 07/07/16 09:23, Jan Beulich wrote:
>>>> On 06.07.16 at 21:12, <tamas.lengyel@zentific.com> wrote:
>> On Wed, Jul 6, 2016 at 12:04 PM, Julien Grall <julien.grall@arm.com> 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@lists.xen.org
https://lists.xen.org/xen-devel