Re: [PATCH v6 4/6] xen/arm: vgic: Optimize the way to store the target vCPU in the rank

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

On 30/11/15 13:55, Ian Campbell wrote:
> On Mon, 2015-11-30 at 13:32 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 25/11/15 11:37, Ian Campbell wrote:
>>> On Wed, 2015-11-18 at 16:42 +0000, Julien Grall wrote:
>>>> Xen is currently directly storing the value of GICD_ITARGETSR
>>>> register
>>>> (for GICv2) and GICD_IROUTER (for GICv3) in the rank. This makes the
>>>> emulation of the registers access very simple but makes the code to
>>>> get
>>>> the target vCPU for a given vIRQ more complex.
>>>>
>>>> While the target vCPU of an vIRQ is retrieved every time an vIRQ is
>>>> injected to the guest, the access to the register occurs less often.
>>>>
>>>> So the data structure should be optimized for the most common case
>>>> rather than the inverse.
>>>>
>>>> This patch introduces the usage of an array to store the target vCPU
>>>> for
>>>> every interrupt in the rank. This will make the code to get the
>>>> target
>>>> very quick. The emulation code will now have to generate the
>>>> GICD_ITARGETSR
>>>> and GICD_IROUTER register for read access and split it to store in a
>>>> convenient way.
>>>>
>>>> With the new way to store the target vCPU, the structure
>>>> vgic_irq_rank
>>>> is shrunk down from 320 bytes to 92 bytes. This is saving about 228
>>>> bytes of memory allocated separately per vCPU.
>>>>
>>>> Note that with these changes, any read to those register will list
>>>> only
>>>> the target vCPU used by Xen. As the spec is not clear whether this is
>>>> a
>>>> valid choice or not, OSes which have a different interpretation of
>>>> the
>>>> spec (i.e OSes which perform read-modify-write operations on these
>>>> registers) may not boot anymore on Xen. Although, I think this is
>>>> fair
>>>> trade between memory usage in Xen (1KB less on a domain using 4 vCPUs
>>>> with no SPIs) and a strict interpretation of the spec (though all the
>>>> cases are not clearly defined).
>>>>
>>>> Furthermore, the implementation of the callback get_target_vcpu is
>>>> now
>>>> exactly the same. Consolidate the implementation in the common vGIC
>>>> code
>>>> and drop the callback.
>>>>
>>>> Finally take the opportunity to fix coding style and replace "irq" by
>>>> "virq" to make clear that we are dealing with virtual IRQ in section
>>>> we
>>>> are modifying.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>>>
>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>
>>> I have one clarifying question, which may or may not be worth a
>>> followup:
>>>
>>>> + * Fetch an ITARGETSR register based on the offset from ITARGETSR0.
>>>
>>> Is the offset here in terms of bytes or in terms of entire ITARGETSR<n>
>>> registers (i.e. 4 bytes)?
>>
>> The offset is in term of bytes.
>>
>>> Might be worth clarifying the comment?
>>
>> I'm not sure, I think it's implicit with the following sentence in the
>> comment:
>>
>> "Note the offset will be aligned to the appropriate boundary."
> 
> It's very implicit, since without knowing the answer it's not clear what an
> appropriate boundary is. 
> 
> How about: "Note the byte offset will be aligned to an ITARGETSR<n>"
> boundary" ?

Ok. I will do the same for the comment on top of vgic_*_irouter.

Regards,

-- 
Julien Grall