Re: [PATCH v3 4/9] arm/vm_event: get/set registers

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

Hi Tamas,

On 16/05/16 16:37, Tamas K Lengyel wrote:
>
> On May 16, 2016 04:14, "Julien Grall" <julien.grall@arm.com
> <mailto:julien.grall@arm.com>> wrote:
>  > On 04/05/16 15:51, Tamas K Lengyel wrote:
>  >>
>  >> diff --git a/xen/include/asm-arm/vm_event.h
> b/xen/include/asm-arm/vm_event.h
>  >> index a3fc4ce..814d0da 100644
>  >> --- a/xen/include/asm-arm/vm_event.h
>  >> +++ b/xen/include/asm-arm/vm_event.h
>  >> @@ -48,15 +48,10 @@ 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. */
>  >> -}
>  >> +void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp);
>  >>
>  >> -static inline void vm_event_fill_regs(vm_event_request_t *req)
>  >> -{
>  >> -    /* Not supported on ARM. */
>  >> -}
>  >> +void vm_event_fill_regs(vm_event_request_t *req,
>  >> +                        const struct cpu_user_regs *regs,
>  >> +                        struct domain *d);
>  >>
>  >>   #endif /* __ASM_ARM_VM_EVENT_H__ */
>  >> diff --git a/xen/include/public/vm_event.h
> b/xen/include/public/vm_event.h
>  >> index 3acf217..fabeee8 100644
>  >> --- a/xen/include/public/vm_event.h
>  >> +++ b/xen/include/public/vm_event.h
>  >> @@ -129,8 +129,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
>  >
>  >
>  > You may want to rework this sentence as hvm_hw_cpu does not exist on
> ARM/ARM64.
>
> IMHO the point gets across even if we don't name the ARM structs
> specifically.

It was more for more correctness from an ARM point of view. But fair enough.

>
>  >
>  >> + * so as to not fill the vm_event ring buffer too quickly.
>  >>    */
>  >>   struct vm_event_regs_x86 {
>  >>       uint64_t rax;
>  >> @@ -168,6 +168,54 @@ struct vm_event_regs_x86 {
>  >>       uint32_t _pad;
>  >>   };
>  >>
>  >> +struct vm_event_regs_arm32 {
>  >> +    uint32_t r0_usr;
>  >> +    uint32_t r1_usr;
>  >> +    uint32_t r2_usr;
>  >> +    uint32_t r3_usr;
>  >> +    uint32_t r4_usr;
>  >> +    uint32_t r5_usr;
>  >> +    uint32_t r6_usr;
>  >> +    uint32_t r7_usr;
>  >> +    uint32_t r8_usr;
>  >> +    uint32_t r9_usr;
>  >> +    uint32_t r10_usr;
>  >> +    uint32_t r12_usr;
>  >> +    uint32_t lr_usr;
>  >> +    uint32_t sp_usr;
>  >> +    uint32_t sp_svc;
>  >> +    uint32_t spsr_svc;
>  >> +    uint32_t fp;
>  >> +    uint32_t pc;
>  >> +    uint32_t cpsr;
>  >> +    uint32_t ttbr0;
>  >> +    uint32_t ttbr1;
>  >> +};
>  >> +
>  >> +struct vm_event_regs_arm64 {
>  >> +    uint64_t x0;
>  >> +    uint64_t x1;
>  >> +    uint64_t x2;
>  >> +    uint64_t x3;
>  >> +    uint64_t x4;
>  >> +    uint64_t x5;
>  >> +    uint64_t x6;
>  >> +    uint64_t x7;
>  >> +    uint64_t x8;
>  >> +    uint64_t x9;
>  >> +    uint64_t x10;
>  >> +    uint64_t x16;
>  >> +    uint64_t lr;
>  >> +    uint64_t sp_el0;
>  >> +    uint64_t sp_el1;
>  >> +    uint32_t spsr_el1;
>  >> +    uint64_t fp;
>  >> +    uint64_t pc;
>  >> +    uint32_t cpsr;
>  >> +    uint64_t ttbr0;
>  >> +    uint64_t ttbr1;
>  >> +};
>  >
>  >
>  > By defining 2 distinct structures, it will be more difficult for the
> introspection tools to inspect registers of an Aarch64 domain running in
> AArch32 mode. They wouldn't be able to re-use code for AArch32 domain
> because the structure fields are different.
>  >
>  > The ARM ARM (see D1.20.1 in ARM DDI 0487A.i) provides a mapping
> between AArch32 state and AArch64 state. If you use it to define the
> layout of a common structure, the support of AArch32 state for AArch64
> domain will come free.
>  >
>
> If the guest is running in 32-bit mode Xen will fill the 32-bit struct,
> so doing a common struct is not strictly necessary. It also requires a
> bunch if union declarations to match the names between that I would
> prefer to avoid. IMHO it's cleaner to do the struct definitions
> separately and then do a union on top.

That is not true. is_domain_32bit will check if the domain is configured 
to run 32-bit or 64-bit in EL1 (i.e the kernel level).

So if you have a guest with 64-bit kernel and 32-bit userspace, Xen will 
always fill the 64-bit structure, even when the userspace is running.

Regards,

-- 
Julien Grall

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