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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

(i.e. there should be no bit pattern a vm_event listener could ever set
which causes a crash of the hypervisor itself)

>  Also, using WRITE_SYSREG() is not safe at
> this point because current != v.

Ok, but how do these new values end up getting propagated into hardware?

~Andrew

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