Re: [PATCH v5 12/22] xen/arm: ITS: Add GICR register emulation

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

Hi Vijay,

On 01/08/2015 11:25, Vijay Kilari wrote:
>> I guess you mean vgic_enable_irqs? And no what you've implemented is
>> definitely not the same as vgic_enable_irqs.
>>
>> vgic_enable_irqs is locking the pending_irq structure using the vgic
>> lock of the targeting VCPU (see v_target = ... ->get_target_cpu(...)).
>>
>> Here, you are locking with the current vCPU, i.e the vCPU which wrote
>> into the LPI property table.
>>
>> All the vGIC code is doing the same, so using the wrong locking won't
>> protect this structure.
>
>     With just vlpi, we cannot get target vcpu without devid. Now
> question is there a
> need to call gic_raise_guest_irq() for inflight LPIs?

Yes it's necessary. Physical LPIs can come up at any time before the 
guest enables the virtual LPI. This is because we enable the physical 
LPIs and route to the guest as soon as the device is assigned to it. You 
may be interesting by the reading of [1].

You will have to find a way to get the correct vCPU because this may 
occur more often than you think.

>>>>
>>>>> +    id_bits = ((vits->propbase & GICR_PROPBASER_IDBITS_MASK) + 1);
>>>>> +
>>>>> +    if ( id_bits > d->arch.vgic.id_bits )
>>>>> +        id_bits = d->arch.vgic.id_bits;
>>>>
>>>> As said on v4, you are allowing the possibility to have a smaller
>>>> property table than the effective number of LPIs.
>>>>
>>>> An ASSERT in vits_get_priority (patch #16) doesn't ensure the validity
>>>> of the size of the property table provided by the guest. This will
>>>> surely crash Xen in debug mode, and who knows what will happen in
>>>> production mode.
>>>
>>>   lpi_size is calculated based on id_bits. If it is smaller, the lpi_size will be
>>> smaller where only size of lpi_size is considered.
>>
>> Where id_bits is based on what the guest provides in GICR_PROPBASER.
>> Although the guest, malicious or not, can decide to setup this id_bits
>> smaller than the number of vLPIs effectively supported by the gic-v3.
>>
>> In this case, if a vLPI higher than this number is injected you will hit
>> the ASSERT(vlpi < vits->prop_size) in vits_get_priority. A guest
>> *should* not be able to crash Xen because it decides to send valid input.
>>
>
>  From 8.11.19, if id_bits is < 8192 (as below statement), GIC treats
> LPIs as out of range.

When you quote the spec, please give both the section and which spec. I 
have about 5 different docs for the GICv3, and I had to guess which one 
you were using.

>
> "If the value of this field is less than 0b1101, indicating that the
> largest interrupt ID is less than 8192
> (the smallest LPI interrupt ID), the GIC will behave as if all
> physical LPIs are out of range."

Thank you for the quoting. I helps me to not a latent bug in your LPI
property table emulation. I should have read more carefully the spec.

The field IDBits gives you the number of interrupt ID bits supported. 
The LPI property table size is only describing the LPI, i.e the offset 0 
of the table is the IntID 8192.

So when you compute the size of the table, you have to substract 8192. 
Otherwise you will remove 2 4KB pages from the guest which could be used 
by it. You will also, possible Xen unsafe as we may read out of the 
pending IRQ array (we are trusting the handler to be registered on valid 
input).

> Based on this, we should make a check on this GICR_PROPBASER.id_bits
> before injecting LPI to domain  when LPI is received.

And doing what? The paragraph you quote doesn't say anything on what 
happen when the LPI interrupt ID is higher than the number of bits. It 
only explain what happen the this number if higher and smaller than a bound.

What would happen if the LPI is injected before the GICR_PROPBASER is 
enabled? See for more details on the problem here [1]

I think, you just have to make sure that the function reading the 
priority (i.e vits_get_priority) is not trying to read out of the array 
and return a dummy value (??).

>>> I added it because, after updating my Linux kernel driver, I see that it
>>>   is making 32-bit access to TYPER register
>>
>> This change should go in a separate patch then. It's not related to this
>> patch.
>
>    Why separate patch?.  This change could be part of GICR* reg emulation
> done for LPI

Because this change is not part of the re-distributor emulation done for 
LPI. You don't even mention it in your commit message. I discovered it 
by comparing on what you did with the previous version.

Furthermore, as said earlier, if you handle 32-bit access for GICR_TYPER 
you have to do it for every others registers.

Anyway, I will send a patch myself to handle 32-bit access on 64-bit 
registers.

Regards,

[1] 
http://lists.xenproject.org/archives/html/xen-devel/2015-06/msg02591.html

-- 
Julien Grall