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

[Julien Grall <julien.grall@citrix.com>]

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

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.

> Another approach btw would be to insist that nr_spis was a multiple of at
> least 4, then you don't have registers which are a mixture of implemented
> and unimplemented, which might simplify the logic.

GICD_TYPER.ITLines is always a multiple of 32 (and therefore 4).
Although, this is only representing the number of SPIs supported by the
Distributor.

Some of them may not be wired and any write to the corresponding byte
should be ignored.

Those interrupt can be detected using the d->arch.vgic.allocated_irqs
bitmap.

Regards,

-- 
Julien Grall