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

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

On Tue, 2015-09-29 at 14:36 +0100, Julien Grall wrote:
> On 29/09/15 14:07, Ian Campbell wrote:
> > On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
> > > Xen is currently directly storing the value of register
> > > GICD_ITARGETSR
> > > (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 IRQ more complex.
> > > 
> > > While the target vCPU of an IRQ is retrieved everytime an IRQ 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 introduce 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.
> > > 
> > > Note that with these changes, any read to those registers will list
> > > only
> > > the target vCPU used by Xen. This is fine because the GIC spec
> > > doesn't
> > > require to return exactly the value written and it can be seen as if
> > > we
> > > decide to implement the register read-only.
> > 
> > I think this is probably OK, but skirting round what the spec actually
> > says
> > a fair bit.
> 
> Well, nothing in the spec clearly explain the behavior of a read access
> on the register. An implementation could decide to make some bits RO or
> even not store everything.
> 
> FWIW, KVM is using the same trick.

At least we'll both get screwed by a picky OS then ;-)

> > Although I'm not 100% sure what that comment is trying to say. From the
> > code I think you mean aligned to the appropriate 4 byte boundary?
> 
> Yes. What about: "Note the offset will be aligned to the appropriate 4
> byte boundary"

Yes, or just "the appropriate boundary".

> I will rename to vgic_fetch_itargetsr(...).

(and for the other similar functions I assume).

> > I don't see anything which is handling the PPI and SGI special case
> > (which
> > is that they are r/o).
> > 
> > Likewise on read they are supposed to always reflect the value of the
> > CPU
> > doing the read.
> > 
> > Are both of those handled elsewhere in some way I'm missing?
> 
> A write to those registers is ignored and handled by a separate case
> (GICD_ITARGETSR ... GICD_ITARGETSR + 7).

Ah, good!

And a read just happens as normal due to the initialisation of the rank's
fields I saw at some point. Good.

> The only places where rank->vcpu[...] is set are vgic_store_* and at the
> initialization time (always routed to vCPU 0).
> Before the new value is stored, we check the validity of the vCPU the
> value should always be valid here.

Good.