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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Vijay,

On 03/08/15 10:36, Vijay Kilari wrote:
> On Sat, Aug 1, 2015 at 9:21 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
>> 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.

MAPVI wouldn't be the only command where would have to hook up for
storing the col_id. MOVI is another candidate.

Anyway, unfortunately your solution won't work in complicate case.

The vLPI could be associated to multiple (DeviceID, EventID), this means
that different call to MAPVI may target to different collections (i.e
different VCPU).

So we would end up to protect the pending_irq structure with different
lock which could result to corrupt the Xen state.

There is a similar problem coming if the collection is moving from one
vCPU to another while an interrupt is inflight.

The only way I can see to get accurately is to store it in the
vgic_vcpu_inject_lpi. Of course this would be done only when the vLPI is
not inflight/pending.

Stefano, can you confirm this would work?

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

Ack. This is what you already do.

> 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

What do you mean by limit vgic.id_bits? You mean changing value of it?

If so, this is wrong for 2 reasons:
        1) The guest may decide to setup a bigger property table later.
        2) vgic.id_bits should never be touched. The GITS_TYPER is static and
defined before the guest is booted.

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

Same remark as 2).

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

Nack. It's valid to have GICR_PROPBASER.id_bits smaller than the number
of vgic.id_bits. For instance the guest may decide to handle less LPIs
than the number supported by the vGIC.

I may have a solution, but it would require some rework in
vgic_vcpu_inject_irq. Currently, the priority is retrieved in order to
add the IRQ in the inflight list by priority order.

The inflight list is used in various place in order to know whether we
need to inject an IRQ to the guest and/or evict an IRQ in favor of an
higher one.

Although, having non-enabled IRQ in here is IHMO pointless and is less
efficient. While this is ok currently, SPIs are disabled when the guest
is not using them, with your series LPIs may come at any point even
though the guest didn't enable it (this is because we don't replicate
the virtual enable bit in the hardware).

Furthermore, an LPI can't be enabled if there is a non-corresponding
byte in the LPI property table. This means that the priority won't be
there too. Therefore we can only safely retrieve the priority when the
IRQ is enabled.

I think we could solve the 2 problems I mentioned by a single solution.
Rather having one list for all the inflights IRQ, we could introduce a
new one to link only inflight enabled IRQ. The IRQ would be added in
this list in:
        - vgic_vcpu_inject_irq if the IRQ is already enabled
        - when the guest is enabling the IRQ and we need to inject the IRQ.

I'd like Stefano inputs on it here too before doing any modification.

FIY, I would be happy to provide a patch to implement what I'm saying if
you want.

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

You didn't answer to my question... What would you do if the property is
not available? Ignoring the LPI? Would the LPI can fire back after the
property table is set and the LPI enabled?

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel