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

[Ian Campbell <ian.campbell@citrix.com>]

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)?

Might be worth clarifying the comment?

Ian.