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

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Thu, Jul 28, 2016 at 3:01 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
>
>
> On 28/07/2016 21:48, Tamas K Lengyel wrote:
>>
>> On Thu, Jul 28, 2016 at 2:41 PM, Andrew Cooper
>> <andrew.cooper3@xxxxxxxxxx> wrote:
>>>
>>> On 28/07/2016 21:36, Tamas K Lengyel wrote:
>>>>
>>>> On Thu, Jul 28, 2016 at 2:26 PM, Andrew Cooper
>>>> <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>>
>>>>> On 28/07/2016 21:05, Tamas K Lengyel wrote:
>>>>>>
>>>>>> Add support for getting/setting registers through vm_event on ARM.
>>>>>> The set of registers can be expanded in the future to include other
>>>>>> registers
>>>>>> as well if necessary but for now it is limited to TTB/CR/R0/R1, PC and
>>>>>> CPSR.
>>>>>>
>>>>>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
>>>>>> ---
>>>>>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>>>>> Cc: Julien Grall <julien.grall@xxxxxxx>
>>>>>> Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>>>>>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>>>>>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>>
>>>>> For the x86 and common bits, Reviewed-by: Andrew Cooper
>>>>> <andrew.cooper3@xxxxxxxxxx>
>>>>>
>>>>> However,
>>>>>
>>>>>> +#include <xen/sched.h>
>>>>>> +#include <asm/vm_event.h>
>>>>>> +
>>>>>> +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.pc = regs->pc;
>>>>>> +    req->data.regs.arm.ttbcr = READ_SYSREG(TCR_EL1);
>>>>>> +    req->data.regs.arm.ttbr0 = READ_SYSREG64(TTBR0_EL1);
>>>>>> +    req->data.regs.arm.ttbr1 = READ_SYSREG64(TTBR1_EL1);
>>>>>> +}
>>>>>> +
>>>>>> +void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
>>>>>> +{
>>>>>> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
>>>>>> +
>>>>>> +    regs->cpsr = rsp->data.regs.arm.cpsr;
>>>>>> +    regs->pc = rsp->data.regs.arm.pc;
>>>>>> +    v->arch.ttbcr = rsp->data.regs.arm.ttbcr;
>>>>>> +    v->arch.ttbr0 = rsp->data.regs.arm.ttbr0;
>>>>>> +    v->arch.ttbr1 = rsp->data.regs.arm.ttbr1;
>>>>>
>>>>> Not knowing anything about ARM, but this looks like it is missing some
>>>>> sanity/plausibility checks (to protect Xen against accidental
>>>>> clobbering
>>>>> from the vm_event listener), and some WRITE_SYSREG() to reload the new
>>>>> values (unless this is done unconditionally later, at which point you
>>>>> should at least leave a comment here saying so).
>>>>>
>>>> This function only ever gets called if the vm_event response
>>>> specifically has the VM_EVENT_FLAG_SET_REGISTERS set, so accidental
>>>> clobbering is not possible.
>>>
>>>
>>> That isn't my point.  Are there any reserved bits in the registers
>>> themselves which could cause Xen to fault when it tries to reload?  If
>>> all that happens is a domain_crash() then ok, but if Xen falls over with
>>> a fatal fault, that should be avoided.
>
>
> The TTBR*_EL1 are registers that can be set by the guest without any trap to
> the hypervisor. So they will not cause Xen to fault even writing to any
> reserved bit.
>
>>
>> I agree. At the moment the only register I actually need access
>> through vm_event setting is PC so I'll just leave the other registers
>> out and document it in the vm_event header.
>
>
> I am starting to be really annoyed with this kind of sentence. It is not
> difficult to get things correct from the beginning.
>
> You either set/get them or do not expose them at all. But please avoid to
> have half of an implementation just because your use case does not need it.

That's not how we do it with vm_event. Even on x86 we only selectively
set registers using the VM_EVENT_FLAG_SET_REGISTERS flag (albeit it
not being documented in the header). As for "not exposing them" it's a
waste to declare separate structures for getting and setting. I'll
change my mind about that if Razvan is on the side that we should
start doing that, but I don't think that's the case at the moment.

Cheers,
Tamas

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