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

[Vijay Kilari <vijay.kilari@gmail.com>]

On Sat, Aug 1, 2015 at 9:21 PM, Julien Grall <julien.grall@citrix.com> wrote:
> 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.
>

  One possible way that I can think of is

In MAPVI command is sent by guest, we know vlpi. Store col_id in the
pending_irq structure
for the vLPI and use this collection id to retrieve correct vCPU on
which we can take vgic lock.

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

My intention was to convey that any LPI interrupt ID outside of number
of ID bits
set in GICR_PROPBASER.id_bits is treated by GIC as out of range.

Also the GICR_PROPBASER.id_bits should be more than 13.

So three cases are

1) If GICR_PROPBASER.id_bits is greater vgic.id_bits
then we can limit LPI property table size to vgic.id_bits

2) If GICR_PROPBASER.id_bits is greater than 13 and less than vgic.id_bits
then we can limit vgic.id_bits to GICR_PROPBASER.id_bits and LPI property table
size to GICR_PROPBASER.id_bits

3) If GIC_PROPBASER.id_bits is less than 13
then we can limit vgic.id_bits to GIC_PROPBASER.id_bits and disable LPI support
for the guest by setting.

OR do not allow any guest that is setting GICR_PROPBASER.id_bits less than
vgic.id_bits

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

 Check is required before accessing LPI property table if property
table is available
or not?.

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