Re: [Xen-devel] [RFC PATCH 21/24] ARM: vITS: handle INVALL command

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

sorry for chiming in late ....

I've been spending some time thinking about this, and I think we can in
fact get away without ever propagating command from domains to the host.

I made a list of all commands that possible require host ITS command
propagation. There are two groups:
1: enabling/disabling LPIs: INV, INVALL
2: mapping/unmapping devices/events: DISCARD, MAPD, MAPTI.

The second group can be handled by mapping all required devices up
front, I will elaborate on that in a different email.

For the first group, read below ...

On 01/12/16 01:19, Stefano Stabellini wrote:
> On Fri, 25 Nov 2016, Julien Grall wrote:
>> Hi,
>>
>> On 18/11/16 18:39, Stefano Stabellini wrote:
>>> On Fri, 11 Nov 2016, Stefano Stabellini wrote:
>>>> On Fri, 11 Nov 2016, Julien Grall wrote:
>>>>> On 10/11/16 20:42, Stefano Stabellini wrote:
>>>>> That's why in the approach we had on the previous series was "host ITS
>>>>> command
>>>>> should be limited when emulating guest ITS command". From my recall, in
>>>>> that
>>>>> series the host and guest LPIs was fully separated (enabling a guest
>>>>> LPIs was
>>>>> not enabling host LPIs).
>>>>
>>>> I am interested in reading what Ian suggested to do when the physical
>>>> ITS queue is full, but I cannot find anything specific about it in the
>>>> doc.
>>>>
>>>> Do you have a suggestion for this?
>>>>
>>>> The only things that come to mind right now are:
>>>>
>>>> 1) check if the ITS queue is full and busy loop until it is not (spin_lock
>>>> style)
>>>> 2) check if the ITS queue is full and sleep until it is not (mutex style)
>>>
>>> Another, probably better idea, is to map all pLPIs of a device when the
>>> device is assigned to a guest (including Dom0). This is what was written
>>> in Ian's design doc. The advantage of this approach is that Xen doesn't
>>> need to take any actions on the physical ITS command queue when the
>>> guest issues virtual ITS commands, therefore completely solving this
>>> problem at the root. (Although I am not sure about enable/disable
>>> commands: could we avoid issuing enable/disable on pLPIs?)
>>
>> In the previous design document (see [1]), the pLPIs are enabled when the
>> device is assigned to the guest. This means that it is not necessary to send
>> command there. This is also means we may receive a pLPI before the associated
>> vLPI has been configured.
>>
>> That said, given that LPIs are edge-triggered, there is no deactivate state
>> (see 4.1 in ARM IHI 0069C). So as soon as the priority drop is done, the same
>> LPIs could potentially be raised again. This could generate a storm.
> 
> Thank you for raising this important point. You are correct.
>
>> The priority drop is necessary if we don't want to block the reception of
>> interrupt for the current physical CPU.
>>
>> What I am more concerned about is this problem can also happen in normal
>> running (i.e the pLPI is bound to an vLPI and the vLPI is enabled). For
>> edge-triggered interrupt, there is no way to prevent them to fire again. 
>> Maybe
>> it is time to introduce rate-limit interrupt for ARM. Any opinions?
> 
> Yes. It could be as simple as disabling the pLPI when Xen receives a
> second pLPI before the guest EOIs the first corresponding vLPI, which
> shouldn't happen in normal circumstances.
> 
> We need a simple per-LPI inflight counter, incremented when a pLPI is
> received, decremented when the corresponding vLPI is EOIed (the LR is
> cleared).
> 
> When the counter > 1, we disable the pLPI and request a maintenance
> interrupt for the corresponding vLPI.

So why do we need a _counter_? This is about edge triggered interrupts,
I think we can just accumulate all of them into one.

So here is what I think:
- We use the guest provided pending table to hold a pending bit for each
VLPI. We can unmap the memory from the guest, since software is not
supposed to access this table as per the spec.
- We use the guest provided property table, without trapping it. There
is nothing to be "validated" in that table, since it's a really tight
encoding and every value written in there is legal. We only look at bit
0 for this exercise here anyway.
- Upon reception of a physical LPI, we look it up to find the VCPU and
virtual LPI number. This is what we need to do anyway and it's a quick
two-level table lookup at the moment.
- We use the VCPU's domain and the VLPI number to index the guest's
property table and read the enabled bit. Again a quick table lookup.
 - If the VLPI is enabled, we EOI it on the host and inject it.
 - If the VLPI is disabled, we set the pending bit in the VCPU's
   pending table and EOI on the host - to allow other IRQs.
- On a guest INV command, we check whether that vLPI is now enabled:
 - If it is disabled now, we don't need to do anything.
 - If it is enabled now, we check the pending bit for that VLPI:
  - If it is 0, we don't do anything.
  - If it is 1, we inject the VLPI and clear the pending bit.
- On a guest INVALL command, we just need to iterate over the virtual
LPIs. If you look at the conditions above, the only immediate action is
when a VLPI gets enabled _and_ its pending bit is set. So we can do
64-bit read accesses over the whole pending table to find non-zero words
and thus set bits, which should be rare in practice. We can store the
highest mapped VLPI to avoid iterating over the whole of the table.
Ideally the guest has no direct control over the pending bits, since
this is what the device generates. Also we limit the number of VLPIs in
total per guest anyway.

If that still sounds like a DOS vector, we could additionally rate-limit
INVALLs, and/or track additions to the pending table after the last
INVALL: if there haven't been any new pending bits since the last scan,
INVALL is a NOP.

Does that makes sense so far?

So that just leaves us with this IRQ storm issue, which I am thinking
about now. But I guess this is not a show stopper given we can disable
the physical LPI if we sense this situation.

> When we receive the maintenance interrupt and we clear the LR of the
> vLPI, Xen should re-enable the pLPI.
> Given that the state of the LRs is sync'ed before calling gic_interrupt,
> we can be sure to know exactly in what state the vLPI is at any given
> time. But for this to work correctly, it is important to configure the
> pLPI to be delivered to the same pCPU running the vCPU which handles
> the vLPI (as it is already the case today for SPIs).

Why would that be necessary?

Cheers,
Andre

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