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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

Hi Julien,

On Mon, Aug 3, 2015 at 6:31 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 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.

 If GICR_PROPBASER.id_bits is always subset of GITS_TYPER then
we should always limit size of LPI property table to id_bits or
vgic.id_bits whichever
is smaller.

While injecting LPI drop which is beyond LPI property table or LPI
property table is not
set.

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

  Yes, you can provide the patch as you have better understanding of this code.

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

   Same as above solution.  Ignore LPI as we don't have LPI property
information to inject.

Regards
Vijay

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