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

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

On 30/05/2016 21:37, Tamas K Lengyel wrote:
> On Mon, May 30, 2016 at 2:20 PM, Julien Grall <julien.grall@arm.com> wrote:
>> 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.
>
> Well, as we discussed it in the previous revision, there is no
> hard-set rule of what can and cannot be transmitted here. The only
> thing to keep in mind is to not grow this struct to be too large. The
> registers sent right now represent a "best guess" of what may be
> useful for performance-sensitive vm_event applications on ARM. It can
> be adjusted in the future if applications require other registers.
> Right now there are no applications at all in this space so we don't
> have any specifications to rely on for making this selection. I'm
> happy to make adjustments to the selection if others have a better
> idea what to send, the only registers I certainly find very useful are
> TTBR0/1, cpsr and pc at this time.

Please log it in the commit message and the code. If someone emitted 
multiple time the same concern on previous version, it likely means that 
your commit message was not clear enough and should be updated.

The number of patch to review on Xen-devel is very consequence, so we 
cannot really afford to spend a lot of time digging into previous 
threads. As a maintainer of a subsystem, you should be aware of that.

We are trying, at least on ARM, to get as much details as possible in 
the commit message and document any possible unclear code to help 
developer understanding why it has been done like that. It also helps us 
(the reviewers and maintainers) to give useful advice later on.

Regards,

-- 
Julien Grall

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