[PATCH] arm/vm_event: get/set registers

[PATCH] arm/vm_event: get/set registers

[Tamas K Lengyel]

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@zentific.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/arm/Makefile          |  1 +
 xen/arch/arm/vm_event.c        | 53 ++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/vm_event.h | 11 ---------
 xen/include/asm-x86/vm_event.h |  4 ----
 xen/include/public/vm_event.h  | 14 +++++++++--
 xen/include/xen/vm_event.h     |  3 +++
 6 files changed, 69 insertions(+), 17 deletions(-)
 create mode 100644 xen/arch/arm/vm_event.c

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index b264ed4..5752830 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -41,6 +41,7 @@ obj-y += traps.o
 obj-y += vgic.o
 obj-y += vgic-v2.o
 obj-$(CONFIG_ARM_64) += vgic-v3.o
+obj-y += vm_event.o
 obj-y += vtimer.o
 obj-y += vpsci.o
 obj-y += vuart.o
diff --git a/xen/arch/arm/vm_event.c b/xen/arch/arm/vm_event.c
new file mode 100644
index 0000000..5e4bee1
--- /dev/null
+++ b/xen/arch/arm/vm_event.c
@@ -0,0 +1,53 @@
+/*
+ * arch/arm/vm_event.c
+ *
+ * Architecture-specific vm_event handling routines
+ *
+ * Copyright (c) 2016 Tamas K Lengyel (tamas.lengyel@zentific.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#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;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
index ccc4b60..9482636 100644
--- a/xen/include/asm-arm/vm_event.h
+++ b/xen/include/asm-arm/vm_event.h
@@ -45,15 +45,4 @@ void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp)
     /* Not supported on ARM. */
 }
 
-static inline
-void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
-{
-    /* Not supported on ARM. */
-}
-
-static inline void vm_event_fill_regs(vm_event_request_t *req)
-{
-    /* Not supported on ARM. */
-}
-
 #endif /* __ASM_ARM_VM_EVENT_H__ */
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index 7e6adff..294def6 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -39,8 +39,4 @@ void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v);
 
 void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp);
 
-void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp);
-
-void vm_event_fill_regs(vm_event_request_t *req);
-
 #endif /* __ASM_X86_VM_EVENT_H__ */
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 64e6857..1e3195d 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -132,8 +132,8 @@
 #define VM_EVENT_X86_XCR0   3
 
 /*
- * Using a custom struct (not hvm_hw_cpu) so as to not fill
- * the vm_event ring buffer too quickly.
+ * Using custom vCPU structs (i.e. not hvm_hw_cpu) for both x86 and ARM
+ * so as to not fill the vm_event ring buffer too quickly.
  */
 struct vm_event_regs_x86 {
     uint64_t rax;
@@ -171,6 +171,15 @@ struct vm_event_regs_x86 {
     uint32_t _pad;
 };
 
+struct vm_event_regs_arm {
+    uint64_t ttbr0;
+    uint64_t ttbr1;
+    uint64_t ttbcr;
+    uint64_t pc;
+    uint32_t cpsr;
+    uint32_t _pad;
+};
+
 /*
  * mem_access flag definitions
  *
@@ -273,6 +282,7 @@ typedef struct vm_event_st {
     union {
         union {
             struct vm_event_regs_x86 x86;
+            struct vm_event_regs_arm arm;
         } regs;
 
         struct vm_event_emul_read_data emul_read_data;
diff --git a/xen/include/xen/vm_event.h b/xen/include/xen/vm_event.h
index c09f723..4f088c8 100644
--- a/xen/include/xen/vm_event.h
+++ b/xen/include/xen/vm_event.h
@@ -75,6 +75,9 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
 void vm_event_vcpu_pause(struct vcpu *v);
 void vm_event_vcpu_unpause(struct vcpu *v);
 
+void vm_event_fill_regs(vm_event_request_t *req);
+void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp);
+
 #endif /* __VM_EVENT_H__ */
 
 /*
-- 
2.8.1


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

Re: [PATCH] arm/vm_event: get/set registers

[Andrew Cooper]

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@zentific.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>

For the x86 and common bits, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

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).

~Andrew

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

Re: [PATCH] arm/vm_event: get/set registers

[Tamas K Lengyel]

On Thu, Jul 28, 2016 at 2:26 PM, Andrew Cooper
<andrew.cooper3@citrix.com> 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@zentific.com>
>> ---
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>
> For the x86 and common bits, Reviewed-by: Andrew Cooper
> <andrew.cooper3@citrix.com>
>
> 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. Also, using WRITE_SYSREG() is not safe at
this point because current != v. However, I have another issue here
with regs which should actually be:

    struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs;

I'll fix that shortly.

Thanks,
Tamas

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

Re: [PATCH] arm/vm_event: get/set registers

[Julien Grall]

On 28/07/2016 21:36, Tamas K Lengyel wrote:
> On Thu, Jul 28, 2016 at 2:26 PM, Andrew Cooper
> <andrew.cooper3@citrix.com> 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@zentific.com>
>>> ---
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>> Cc: Julien Grall <julien.grall@arm.com>
>>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> For the x86 and common bits, Reviewed-by: Andrew Cooper
>> <andrew.cooper3@citrix.com>
>>
>> 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. Also, using WRITE_SYSREG() is not safe at
> this point because current != v. However, I have another issue here
> with regs which should actually be:

What if the vCPU is running? If it cannot happen, please document and 
add an ASSERT/BUG_ON.

>
>     struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs;
>
> I'll fix that shortly.
>
> Thanks,
> Tamas
>

-- 
Julien Grall

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

Re: [PATCH] arm/vm_event: get/set registers

[Tamas K Lengyel]

On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote:
>
>
> On 28/07/2016 21:36, Tamas K Lengyel wrote:
>>
>> On Thu, Jul 28, 2016 at 2:26 PM, Andrew Cooper
>> <andrew.cooper3@citrix.com> 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@zentific.com>
>>>> ---
>>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>>> Cc: Julien Grall <julien.grall@arm.com>
>>>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>> Cc: Jan Beulich <jbeulich@suse.com>
>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>
>>>
>>> For the x86 and common bits, Reviewed-by: Andrew Cooper
>>> <andrew.cooper3@citrix.com>
>>>
>>> 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. Also, using WRITE_SYSREG() is not safe at
>> this point because current != v. However, I have another issue here
>> with regs which should actually be:
>
>
> What if the vCPU is running? If it cannot happen, please document and add an
> ASSERT/BUG_ON.

vCPU is guaranteed to be paused because we check
atomic_read(&v->vm_event_pause_count) before calling this. Adding an
ASSERT should be fine though.

Tamas

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

Re: [PATCH] arm/vm_event: get/set registers

[Andrew Cooper]

On 28/07/2016 21:36, Tamas K Lengyel wrote:
> On Thu, Jul 28, 2016 at 2:26 PM, Andrew Cooper
> <andrew.cooper3@citrix.com> 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@zentific.com>
>>> ---
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>> Cc: Julien Grall <julien.grall@arm.com>
>>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> For the x86 and common bits, Reviewed-by: Andrew Cooper
>> <andrew.cooper3@citrix.com>
>>
>> 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@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] arm/vm_event: get/set registers

[Tamas K Lengyel]

On Thu, Jul 28, 2016 at 2:41 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 28/07/2016 21:36, Tamas K Lengyel wrote:
>> On Thu, Jul 28, 2016 at 2:26 PM, Andrew Cooper
>> <andrew.cooper3@citrix.com> 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@zentific.com>
>>>> ---
>>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>>> Cc: Julien Grall <julien.grall@arm.com>
>>>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>> Cc: Jan Beulich <jbeulich@suse.com>
>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>> For the x86 and common bits, Reviewed-by: Andrew Cooper
>>> <andrew.cooper3@citrix.com>
>>>
>>> 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 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.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?
>

AFAIK during scheduling the registers get loaded from this save state.

Tamas

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

Re: [PATCH] arm/vm_event: get/set registers

[Julien Grall]

On 28/07/2016 21:48, Tamas K Lengyel wrote:
> On Thu, Jul 28, 2016 at 2:41 PM, Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>> On 28/07/2016 21:36, Tamas K Lengyel wrote:
>>> On Thu, Jul 28, 2016 at 2:26 PM, Andrew Cooper
>>> <andrew.cooper3@citrix.com> 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@zentific.com>
>>>>> ---
>>>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>>>> Cc: Julien Grall <julien.grall@arm.com>
>>>>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>>> Cc: Jan Beulich <jbeulich@suse.com>
>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> For the x86 and common bits, Reviewed-by: Andrew Cooper
>>>> <andrew.cooper3@citrix.com>
>>>>
>>>> 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@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] arm/vm_event: get/set registers

[Tamas K Lengyel]

On Thu, Jul 28, 2016 at 3:01 PM, Julien Grall <julien.grall@arm.com> wrote:
>
>
> On 28/07/2016 21:48, Tamas K Lengyel wrote:
>>
>> On Thu, Jul 28, 2016 at 2:41 PM, Andrew Cooper
>> <andrew.cooper3@citrix.com> wrote:
>>>
>>> On 28/07/2016 21:36, Tamas K Lengyel wrote:
>>>>
>>>> On Thu, Jul 28, 2016 at 2:26 PM, Andrew Cooper
>>>> <andrew.cooper3@citrix.com> 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@zentific.com>
>>>>>> ---
>>>>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>>>>> Cc: Julien Grall <julien.grall@arm.com>
>>>>>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>>>> Cc: Jan Beulich <jbeulich@suse.com>
>>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>
>>>>> For the x86 and common bits, Reviewed-by: Andrew Cooper
>>>>> <andrew.cooper3@citrix.com>
>>>>>
>>>>> 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@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] arm/vm_event: get/set registers

[Julien Grall]

On 28/07/2016 22:05, Tamas K Lengyel wrote:
> On Thu, Jul 28, 2016 at 3:01 PM, Julien Grall <julien.grall@arm.com> wrote:
> 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.

Is there any rationale to only set a subset of the information you 
retrieved?

-- 
Julien Grall

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

Re: [PATCH] arm/vm_event: get/set registers

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 1087 bytes --]

On Jul 28, 2016 15:25, "Julien Grall" <julien.grall@arm.com> wrote:
>
>
>
> On 28/07/2016 22:05, Tamas K Lengyel wrote:
>>
>> On Thu, Jul 28, 2016 at 3:01 PM, Julien Grall <julien.grall@arm.com>
wrote:
>> 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.
>
>
> Is there any rationale to only set a subset of the information you
retrieved?
>

I just did a testrun with setting every register through this method to 0
other then pc and it resulted in hypervisor crash. Not sure if it's just my
setup or not though so I'm still poking at it. However, I don't really see
a usecase where setting ttbr regs to be required either via the fast method
so it simply may not worth digging into it more at this time.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1458 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH] arm/vm_event: get/set registers

[Julien Grall]

On 28/07/2016 22:33, Tamas K Lengyel wrote:
> On Jul 28, 2016 15:25, "Julien Grall" <julien.grall@arm.com
> <mailto:julien.grall@arm.com>> wrote:
>>
>>
>>
>> On 28/07/2016 22:05, Tamas K Lengyel wrote:
>>>
>>> On Thu, Jul 28, 2016 at 3:01 PM, Julien Grall <julien.grall@arm.com
> <mailto:julien.grall@arm.com>> wrote:
>>> 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.
>>
>>
>> Is there any rationale to only set a subset of the information you
> retrieved?
>>
>
> I just did a testrun with setting every register through this method to
> 0 other then pc and it resulted in hypervisor crash. Not sure if it's
> just my setup or not though so I'm still poking at it. However, I don't
> really see a usecase where setting ttbr regs to be required either via
> the fast method so it simply may not worth digging into it more at this
> time.

To confirm, do you mean setting CPSR, TTBR0_EL1, TTBR1_EL1 to 0?

TTBR*_EL1 are safe to set to any values (they are directly accessible by 
the guest anyway). However this is not the case of CPSR. From my 
understanding of the ARM ARM (B1-1150 in ARM DDI 0406C.b) is writing 0 
to M[4:0] will lead to an unpredictable behavior (that could cause an 
hypervisor trap).

Can you copy/paste the hypervisor crash log?

Thank you,

-- 
Julien Grall

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

Re: [PATCH] arm/vm_event: get/set registers

[Tamas K Lengyel]

On Thu, Jul 28, 2016 at 4:03 PM, Julien Grall <julien.grall@arm.com> wrote:
>
>
> On 28/07/2016 22:33, Tamas K Lengyel wrote:
>>
>> On Jul 28, 2016 15:25, "Julien Grall" <julien.grall@arm.com
>> <mailto:julien.grall@arm.com>> wrote:
>>>
>>>
>>>
>>>
>>> On 28/07/2016 22:05, Tamas K Lengyel wrote:
>>>>
>>>>
>>>> On Thu, Jul 28, 2016 at 3:01 PM, Julien Grall <julien.grall@arm.com
>>
>> <mailto:julien.grall@arm.com>> wrote:
>>>>
>>>> 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.
>>>
>>>
>>>
>>> Is there any rationale to only set a subset of the information you
>>
>> retrieved?
>>>
>>>
>>
>> I just did a testrun with setting every register through this method to
>> 0 other then pc and it resulted in hypervisor crash. Not sure if it's
>> just my setup or not though so I'm still poking at it. However, I don't
>> really see a usecase where setting ttbr regs to be required either via
>> the fast method so it simply may not worth digging into it more at this
>> time.
>
>
> To confirm, do you mean setting CPSR, TTBR0_EL1, TTBR1_EL1 to 0?
>
> TTBR*_EL1 are safe to set to any values (they are directly accessible by the
> guest anyway). However this is not the case of CPSR. From my understanding
> of the ARM ARM (B1-1150 in ARM DDI 0406C.b) is writing 0 to M[4:0] will lead
> to an unpredictable behavior (that could cause an hypervisor trap).
>
> Can you copy/paste the hypervisor crash log?
>

OK, the issue I had was that right now mem_access on ARM doesn't send
the registers (yet) as it needs my monitor_traps work from the other
patch so I kept setting all registers to 0. Rebasing on top of my
other patch now I was able to verify that indeed only setting cpsr to
arbitrary values (like 0) can cause hypervisor crash as follows:

(XEN) CPU1: Unexpected Trap: Prefetch Abort
(XEN) ----[ Xen-4.8-unstable  arm32  debug=y  Not tainted ]----
(XEN) CPU:    1
(XEN) PC:     c09195f4
(XEN) CPSR:   0000001a MODE:Hypervisor
(XEN)      R0: 600f0013 R1: 390038ff R2: c0dd4540 R3: 38ff38ff
(XEN)      R4: 00000000 R5: c0dd4240 R6: c7fb0a64 R7: 00000001
(XEN)      R8: c7eaaf60 R9: c0dd4240 R10:c7eaaf58 R11:00000000 R12:00000000
(XEN) USR: SP: beb4e20c LR: 7f5883ed
(XEN) SVC: SP: c277ddc0 LR: c02caf58 SPSR:800f0030
(XEN) ABT: SP: c0dd9a4c LR: c02147c0 SPSR:80070193
(XEN) UND: SP: c0dd9a58 LR: c0214880 SPSR:20060093
(XEN) IRQ: SP: c0dd9a40 LR: c0214800 SPSR:600f0193
(XEN) FIQ: SP: c0dd9a64 LR: c0dd9a64 SPSR:00000000
(XEN) FIQ: R8: 00000000 R9: 00000000 R10:00000000 R11:00000000 R12:00000000
(XEN)
(XEN)      SCTLR: 10c5387d
(XEN)        TCR: 00000000
(XEN)      TTBR0: 00000000426b806a
(XEN)      TTBR1: 000000004020406a
(XEN)       IFAR: b6e75480, IFSR: 00000007
(XEN)       DFAR: 7f5ab024, DFSR: 00000017
(XEN)
(XEN)   VTCR_EL2: 80003558
(XEN)  VTTBR_EL2: 00020000bfa86000
(XEN)
(XEN)  SCTLR_EL2: 30cd187f
(XEN)    HCR_EL2: 000000000038663f
(XEN)  TTBR0_EL2: 00000000bdfea000
(XEN)
(XEN)    ESR_EL2: 84000006
(XEN)  HPFAR_EL2: 000000000001c810
(XEN)      HDFAR: e0800f00
(XEN)      HIFAR: c09195f4
(XEN)
(XEN) Xen BUG at traps.c:946
(XEN) ----[ Xen-4.8-unstable  arm32  debug=y  Not tainted ]----
(XEN) CPU:    1
(XEN) PC:     0025ee4c traps.c#show_guest_stack+0xcc/0x274
(XEN) CPSR:   8000011a MODE:Hypervisor
(XEN)      R0: 40064000 R1: 43fcff58 R2: 43ce9d00 R3: 0000000a
(XEN)      R4: 43fcff9c R5: 40064000 R6: 00282608 R7: 43fcff58
(XEN)      R8: c7eaaf60 R9: c0dd4240 R10:c7eaaf58 R11:43fcff0c R12:00000000
(XEN) HYP: SP: 43fcfedc LR: 0025f69c
(XEN)
(XEN)   VTCR_EL2: 80003558
(XEN)  VTTBR_EL2: 00020000bfa86000
(XEN)
(XEN)  SCTLR_EL2: 30cd187f
(XEN)    HCR_EL2: 000000000038663f
(XEN)  TTBR0_EL2: 00000000bdfea000
(XEN)
(XEN)    ESR_EL2: 00000000
(XEN)  HPFAR_EL2: 000000000001c810
(XEN)      HDFAR: e0800f00
(XEN)      HIFAR: c09195f4
(XEN)
(XEN) Xen stack trace from sp=43fcfedc:
(XEN)    00282608 43fcff58 c7eaaf60 c0dd4240 43fcff9c 00281a74 00282608 43fcff58
(XEN)    c7eaaf60 c0dd4240 c7eaaf58 43fcff2c 0025f69c 43fcff58 00281a74 00282608
(XEN)    43fcff58 c7eaaf60 c0dd4240 43fcff3c 0025f848 0030b614 00281a74 43fcff4c
(XEN)    0025f94c 00000000 00000001 43fcff54 002650f0 43fcff58 00264e90 600f0013
(XEN)    390038ff c0dd4540 38ff38ff 00000000 c0dd4240 c7fb0a64 00000001 c7eaaf60
(XEN)    c0dd4240 c7eaaf58 00000000 00000000 43fcff9c 7f5883ed c09195f4 0000001a
(XEN)    00000007 beb4e20c c0dd9a40 c0214800 c277ddc0 c02caf58 c0dd9a4c c02147c0
(XEN)    c0dd9a58 c0214880 00000000 00000000 00000000 00000000 00000000 c0dd9a64
(XEN)    c0dd9a64 800f0030 80070193 20060093 600f0193 00000000 00000000 00000000
(XEN)    00000000
(XEN) Xen call trace:
(XEN)    [<0025ee4c>] traps.c#show_guest_stack+0xcc/0x274 (PC)
(XEN)    [<0025f69c>] show_stack+0x78/0x204 (LR)
(XEN)    [<0025f69c>] show_stack+0x78/0x204
(XEN)    [<0025f848>] show_execution_state+0x20/0x34
(XEN)    [<0025f94c>] do_unexpected_trap+0x48/0x5c
(XEN)    [<002650f0>] do_trap_data_abort+0/0x1c
(XEN)    [<00264e90>] entry.o#return_from_trap+0/0x4
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 1:
(XEN) Xen BUG at traps.c:946
(XEN) ****************************************
(XEN)
(XEN) Reboot in five seconds...

Setting ttb/cr/r0/r1 is fine however as it only causes an in-guest
kernel crash. So we still need to selectively set registers and as at
this point only PC makes sense we will start with just that.

Tamas

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

Re: [PATCH] arm/vm_event: get/set registers

[Razvan Cojocaru]

On 07/29/16 00:25, Julien Grall wrote:
> 
> 
> On 28/07/2016 22:05, Tamas K Lengyel wrote:
>> On Thu, Jul 28, 2016 at 3:01 PM, Julien Grall <julien.grall@arm.com>
>> wrote:
>> 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.
> 
> Is there any rationale to only set a subset of the information you
> retrieved?

The perennial speed optimization, but mainly that setting everything can
have side-effects (on x86 I remember that at the time I wrote the
initial patch this had something to do with the control registers - if
you'd like I can try to follow the code again and try to remember what
the exact issue was).

My main use-case at the time was to simply set EIP (I needed to be able
to skip the current instruction if it happened to be deemed to be
malicious by the introspection engine). I believe it has been assumed at
the time that setting the GPRS is enough, and that can be extended in
the future by interested parties.


Thanks,
Razvan

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

Re: [PATCH] arm/vm_event: get/set registers

[Julien Grall]

Hi Tamas,

On 28/07/16 23:45, Tamas K Lengyel wrote:
> On Thu, Jul 28, 2016 at 4:03 PM, Julien Grall <julien.grall@arm.com> wrote:
>>
>>
>> On 28/07/2016 22:33, Tamas K Lengyel wrote:
>>>
>>> On Jul 28, 2016 15:25, "Julien Grall" <julien.grall@arm.com
>>> <mailto:julien.grall@arm.com>> wrote:
>>>>
>>>>
>>>>
>>>>
>>>> On 28/07/2016 22:05, Tamas K Lengyel wrote:
>>>>>
>>>>>
>>>>> On Thu, Jul 28, 2016 at 3:01 PM, Julien Grall <julien.grall@arm.com
>>>
>>> <mailto:julien.grall@arm.com>> wrote:
>>>>>
>>>>> 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.
>>>>
>>>>
>>>>
>>>> Is there any rationale to only set a subset of the information you
>>>
>>> retrieved?
>>>>
>>>>
>>>
>>> I just did a testrun with setting every register through this method to
>>> 0 other then pc and it resulted in hypervisor crash. Not sure if it's
>>> just my setup or not though so I'm still poking at it. However, I don't
>>> really see a usecase where setting ttbr regs to be required either via
>>> the fast method so it simply may not worth digging into it more at this
>>> time.
>>
>>
>> To confirm, do you mean setting CPSR, TTBR0_EL1, TTBR1_EL1 to 0?
>>
>> TTBR*_EL1 are safe to set to any values (they are directly accessible by the
>> guest anyway). However this is not the case of CPSR. From my understanding
>> of the ARM ARM (B1-1150 in ARM DDI 0406C.b) is writing 0 to M[4:0] will lead
>> to an unpredictable behavior (that could cause an hypervisor trap).
>>
>> Can you copy/paste the hypervisor crash log?
>>
>
> OK, the issue I had was that right now mem_access on ARM doesn't send
> the registers (yet) as it needs my monitor_traps work from the other
> patch so I kept setting all registers to 0. Rebasing on top of my
> other patch now I was able to verify that indeed only setting cpsr to
> arbitrary values (like 0) can cause hypervisor crash as follows:

Thank you for the stack trace. I managed to reproduce it and did further 
testing.

We need to sanitize the CPSR mode in Xen before writing it. So far, this 
can be only set by XEN_DOMCTL_setvcpucontext.

[...]

> Setting ttb/cr/r0/r1 is fine however as it only causes an in-guest
> kernel crash. So we still need to selectively set registers and as at
> this point only PC makes sense we will start with just that.

Which lead to my next question. For instance, we have an app A which is 
built for Xen 4.N and they want to also support Xen 4.(N + 1) which will 
set more registers and take advantage of it. How the app will 
differentiate the 2 versions?

Regards,

-- 
Julien Grall

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

Re: [PATCH] arm/vm_event: get/set registers

[Razvan Cojocaru]

On 07/29/2016 05:11 PM, Julien Grall wrote:
> Which lead to my next question. For instance, we have an app A which is
> built for Xen 4.N and they want to also support Xen 4.(N + 1) which will
> set more registers and take advantage of it. How the app will
> differentiate the 2 versions?

We currenly have macros like VM_EVENT_INTERFACE_VERSION that can be
incremented so that applications can know at compile-time which
interface they get.

At runtime, I suppose we could use code similar to what runs when "xl
info" is called (which outputs xen_major, xen_minor and xen_extra, or
the complete xen_version).


Thanks,
Razvan

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

Re: [PATCH] arm/vm_event: get/set registers

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 926 bytes --]

On Jul 29, 2016 08:21, "Razvan Cojocaru" <rcojocaru@bitdefender.com> wrote:
>
> On 07/29/2016 05:11 PM, Julien Grall wrote:
> > Which lead to my next question. For instance, we have an app A which is
> > built for Xen 4.N and they want to also support Xen 4.(N + 1) which will
> > set more registers and take advantage of it. How the app will
> > differentiate the 2 versions?
>
> We currenly have macros like VM_EVENT_INTERFACE_VERSION that can be
> incremented so that applications can know at compile-time which
> interface they get.
>
> At runtime, I suppose we could use code similar to what runs when "xl
> info" is called (which outputs xen_major, xen_minor and xen_extra, or
> the complete xen_version).
>
>

The vm_event interface version is included in all requests and responses so
the user knows at runtime what it is running on. We enforce on the Xen side
that the response matches the interface expected.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1190 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH] arm/vm_event: get/set registers

[Tamas K Lengyel]

On Thu, Jul 28, 2016 at 10:16 PM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> On 07/29/16 00:25, Julien Grall wrote:
>>
>>
>> On 28/07/2016 22:05, Tamas K Lengyel wrote:
>>> On Thu, Jul 28, 2016 at 3:01 PM, Julien Grall <julien.grall@arm.com>
>>> wrote:
>>> 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.
>>
>> Is there any rationale to only set a subset of the information you
>> retrieved?
>
> The perennial speed optimization, but mainly that setting everything can
> have side-effects (on x86 I remember that at the time I wrote the
> initial patch this had something to do with the control registers - if
> you'd like I can try to follow the code again and try to remember what
> the exact issue was).
>
> My main use-case at the time was to simply set EIP (I needed to be able
> to skip the current instruction if it happened to be deemed to be
> malicious by the introspection engine). I believe it has been assumed at
> the time that setting the GPRS is enough, and that can be extended in
> the future by interested parties.
>

Yeap, it's pretty much the same case here, we just need to be able to
set the address of the next instruction without needing to get and set
all registers. Changing the translation table underneath the OS or
other control registers is just not necessary (even if technically it
would be possible).

Thanks,
Tamas

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