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

From: Tamas K Lengyel <tamas@tklengyel.com>
To: Julien Grall <julien.grall@arm.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Razvan Cojocaru <rcojocaru@bitdefender.com>
Subject: Re: [PATCH v3 4/9] arm/vm_event: get/set registers
Date: Mon, 16 May 2016 09:37:37 -0600	[thread overview]
Message-ID: <CABfawhk-YFO+cwD4Q9hCkqxpbJWWhL62_3jjWbWWgwFv35UbfQ@mail.gmail.com> (raw)
In-Reply-To: <57399D92.1030905@arm.com>

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

On May 16, 2016 04:14, "Julien Grall" <julien.grall@arm.com> wrote:
>
> Hi Tamas,
>
>
> 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.

>
>> + * 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.

Tamas

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

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

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