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>]

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."

Regards,

-- 
Julien Grall