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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On May 16, 2016 09:59, "Julien Grall" <julien.grall@xxxxxxx> wrote:
>
> Hi Tamas,
>
>
> On 16/05/16 16:37, Tamas K Lengyel wrote:
>>
>>
>> On May 16, 2016 04:14, "Julien Grall" <julien.grall@xxxxxxx
>> <mailto:julien.grall@xxxxxxx>> 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.
>

Hm fair point. It complicates things a bit though either way as the event subscriber wouldn't know which mode it received the event from without doing some extra checks. So I guess Xen should transmit that information too, at which point it could also just pick the right struct to fill with the current setup.

Tamas

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