On May 16, 2016 09:59, "Julien Grall" <julien.gr...@arm.com> wrote: > > Hi Tamas, > > > On 16/05/16 16:37, Tamas K Lengyel wrote: >> >> >> On May 16, 2016 04:14, "Julien Grall" <julien.gr...@arm.com >> <mailto:julien.gr...@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. >
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@lists.xen.org http://lists.xen.org/xen-devel