[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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.

Regards,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.