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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > 
> > Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > 
> > 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" ?


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