Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank

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

On Thu, 2015-10-08 at 14:46 +0100, Julien Grall wrote:
> On 08/10/15 13:23, Ian Campbell wrote:
> > On Thu, 2015-10-08 at 12:36 +0100, Stefano Stabellini wrote:
> > > On Thu, 8 Oct 2015, Ian Campbell wrote:
> > > > On Wed, 2015-10-07 at 19:16 +0100, Julien Grall wrote:
> > > > 
> > > > > Furthermore, based on the spec (4.3.12 in IHI 0048B.b): "A
> > > > > register
> > > > > field corresponding to an unimplemented interrupt is RAZ/WI."
> > > > > 
> > > > > If the user knows that an interrupt is not implemented, he may
> > > > > decide
> > > > > to
> > > > > write 0 in the corresponding byte. With the current solution, the
> > > > > whole
> > > > > write access is ignored.
> > > > > 
> > > > > The solution suggested in this patch is less restrictive and will
> > > > > just
> > > > > ignore the corresponding byte if it's 0.
> > > > 
> > > > I think this (a 32-bit register covering both implemented and non
> > > > -implemented interrupts) is a compelling reason to only ignore the
> > > > specific
> > > > zero bytes and not the whole word.
> > > 
> > > I agree that zero writes to unimplemented interrupts should be
> > > allowed.
> > > However allowing them for everything encourages 32-bit writes with
> > > just
> > > one byte set, the one that the OS actually wants to write. It doesn't
> > > seem correct to me. Something like:
> > > 
> > > uint32_t val = 0x2 << 8;
> > > write32(ITARGETSR + something, val);
> > > 
> > > which I don't think is supposed to work. That said, I recognize that
> > > this is a minor issue, so I won't insist.
> > 
> > Right.
> > 
> > The underlying issue here is that we can't cope with interrupts which
> > are
> > not routed to any CPU at all, which is the expected semantics for a
> > write
> > of 0 to the TARGET, right? (such interrupts essentially remain pending
> > in
> > the distributor).
> > 
> > How hard would it be to actually implement that and therefore avoid
> > this
> > whole issue?
> 
> It will take sometime to figure all the place which take a vcpu and
> expect it valid and fix it.
> 
> While this should be done at some point to respect the GICv3 and GICv2
> spec, I think the correct support of 0 byte in GICD_ITARGETSR is not
> that important because it likely would  hit people trying to disable an
> interrupt via GICD_ITARGETSR rather than properly using GICD_ICENABLER
> register to disable it.
> 
> I agree that this patch is by side effect moving from a wrong behavior
> to another wrong behavior (though slightly less).

Right.

>  Although, I don't want
> to fix it properly in this series because I can't fix everything in a
> single series. The main purpose here is to fix access size to emulated
> register. This has to be fixed now as it prevents real guest to properly
> boot on Xen.

This is fair enough.

> If the concern is the behavior is changed, I'm happy to rework this code
> to keep exactly the same behavior. I.e any 32-bit write containing
> a 0 byte will be ignored. This is not optimal but at least I'm not
> opening the pandora box of fixing every single error in the code touch
> by this series.

I'm okay with the new behaviour, I think Stefano was willing to tolerate it
(based on <alpine.DEB.2.02.1510081220190.1179@kaball.uk.xensource.com>).

So if we aren't going to fix it to DTRT WRT writing zero to a target then I
think we can go with the current variant and not change to ignoring any
word with a zero byte in it.

Ian.