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 Wed, 2015-09-30 at 19:11 +0100, Julien Grall wrote:
> On 29/09/15 15:23, Ian Campbell wrote:
> > 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 ;-)
> 
> I think our implementation could fold into 4.3.12 IHI 0048B:
> 
> "It is IMPLEMENTATION DEFINED which, if any, SPIs are statically
> configured in hardware. The CPU targets field for such an SPI is
> read-only, and returns a value that indicates the CPU targets for the
> interrupt."
> 
> We are implementing a weird read-only but at least an OS should not
> trust the written value.

It's certainly a weird read-only, in that it isn't at all read-only, but I
agree its likely to be tolerated by most OSes.

Note though that an OS written by a pedant would be quite entitled to do:

   old_targets = readb(GICD_TARGETR+irq)
   writeb(GICD_TARGET+irq, new_targets)
   foo = readb(GICD_TARGETR+irq)
    ASSERT(foo==old_targets || foo==new_targets)

That would be rather retentive though.

Ian.