Re: [PATCH v4 5/8] arm/vm_event: get/set registers

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

Hi Tamas,

On 30/05/2016 20:47, Tamas K Lengyel wrote:
> On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>> +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 fp;
>>> +    uint64_t pc;
>>> +    uint64_t sp_el0;
>>> +    uint64_t sp_el1;
>>> +    uint32_t spsr_el1;
>>> +    uint32_t _pad;
>>> +};
>>
>> My ARM knowledge is certainly quite limited, but the incomplete set
>> of GPRs here is quite obvious: Is there a reason to not make all of
>> them available here? And if there is, could the criteria of which
>> registers got put here please be documented in a comment (so that
>> the next person noticing the incomplete set won't ask again)?
>
> There already is a comment in place that explains why we are not
> sending the full set of registers here.

Your comment only says:
"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." it does not 
explain the criteria of which registers got put here.

I already mentioned it on a previous version, and it seems to have been 
dropped from the commit message.

Anyway, we should avoid to be stingy with comments. They are helpful for 
anyone interested by the vm_event subsystem and even you in few years if 
you decide to modify this code.

>>
>> I'm also puzzled by fp and lr - I'm not aware of registers of those
>> names (and gas also doesn't accept them afaict).
>
> Not sure why but Xen names x29 fp and x30 lr. See
> include/asm-arm/arm64/processor.h.

They are not officially called "lr" and "fp" in the ARM ARM. However the 
AAPCS64 [1] (5.1.1) defines x29 and x30 as special registers holding 
resp. "lr" and "fp".

The convention is used internally in Xen for convenience. However, the 
public header uses x29 and x30 (see include/public/arch-arm.h).

I would prefer if you use the official name in the public headers.

Regards,

[1] 
http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf


-- 
Julien Grall

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