Re: Xen/arm: Virtual ITS command queue handling

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

On 19/05/15 15:48, Ian Campbell wrote:
>> A software would be buggy if no INV/INVALL is sent after change the LPI
>> configuration table.
> 
> Specifically _guest_ software.
> 
> AIUI the ITS is not required to reread the LPI cfg table unless an
> INV/INVALL is issued, but it is allowed to do so if it wants, i.e. it
> could pickup the config change at any point after the write to the cfg
> table. Is that correct?

Yes.

> If so then as long as it cannot blow up in Xen's face (i.e. an interrupt
> storm) I think between a write to the LPI config table and the next
> associated INV/INVALL we are entitled either continue using the old
> config until the INV/INVALL, to immediately enact the change or anything
> in the middle. I think this gives a fair bit of flexibility.

The interrupt is deprivileged by Xen and EOI by the guest. I don't think
it's possible to produce an interrupt storm.

> You've proposed something at the "immediately enact" end of the
> spectrum.

Yes, it one suggestion among another.

>> As suggested on a previous mail, I think we can get rid of sending
>> INV/INVALL command to the pITS by trapping the LPI configuration table:
> 
> The motivation here is simply to avoid the potential negative impact on
> the system of a guest which fills its command queue with INVALL
> commands?

Right.

> I think we don't especially care about INV since they are targeted. We
> care about INVALL because they are global. INV handling comes along for
> the ride though.
> 
>> For every write access, when the vLPIs is valid (i.e associated to a
>> device/interrupt), Xen will toggle the enable bit in the hardware LPIs
>> configuration table, send an INV * and sync his internal state. This
>> requiring to be able to translate the vLPIs to a (device,ID).
> 
> "INV *"? You don't mean INVALL I think, but rather INV of the specific
> device?

Yes, I mean INV command.

> 
> One possible downside is that you will convert this guest vits
> interaction:
>         for all LPIs
>                 enable LPI
>         INVALL
> 
> Into this pits interaction:
>         for all LPIs
>                 enable LPI
>                 INV LPI
> 
> Also sequences of events with toggle things back and forth before
> invalidating are similarly made more synchronous. (Such sequences seem
> dumb to me, but kernel side abstractions sometimes lead to such things).

Correct, this will result to send much more command to the ITS.

>> INVALL/INV command could be ignored and directly increment CREADR (with
>> some care) because it only ensure that the command has been executed,
>> not fully completed. A SYNC would be required from the guest in order to
>> ensure the completion.
>>
>> Therefore we would need more care for the SYNC. Maybe by injecting a
>> SYNC when it's necessary.
>>
>> Note that we would need Xen to send command on behalf of the guest (i.e
>> not part of the command queue).
> 
> A guest may do this:
>         Enqueue command A
>         Enqueue command B
>         Change LPI1 cfg table
>         Change LPI2 cfg table
>         Enqueue command C
>         Enqueue command D
>         Enqueue INV LPI2
>         Enqueue INV LPI1
> 
> With your change this would end up going to the PITS as:
>         Enqueue command A
>         Enqueue command B
>         Change LPI1 cfg table
>         Enqueue INV LPI1
>         Change LPI2 cfg table
>         Enqueue INV LPI2
>         Enqueue command C
>         Enqueue command D
> 
> Note that the INV's have been reordered WRT command C and D as well as
> each other. Are there sequences of commands where this may make a
> semantic difference?

AFAICT, the commands don't change their semantics following the state of
LPI configuration.

> What if command C is a SYNC for example?

That would not be a problem. As soon as the OS write into the LPI
configuration it can expect that the ITS will take the change a anytime.

>> With this solution, it would be possible to have a small amount of time
>> where the pITS doesn't use the correct the configuration (i.e the
>> interrupt not yet enabled/disabled). Xen is able to cooperate with that
>> and will queue the interrupt to the guest.
> 
> I think it is inherent in the h/w design that an LPI may still be
> delivered after the cfg table has changed or even the INV enqueued, it
> is only guaranteed to take effect with a sync following the INV.

Right.

> I had in mind a lazier scheme which I'll mention for completeness not
> because I necessarily think it is better.

I wasn't expected to have a correct solution from the beginning ;). I
was more a first step for a better one such as yours.

> For each vits we maintain a bit map which marks LPI cfg table entries as
> dirty. Possibly a count of dirty entries too.
> 
> On trap of cfg table write we propagate the change to the physical table
> and set the corresponding dirty bit (and count++ if we are doing that)
> 
> On INV we insert the corresponding INV to the PITS iff
> test_and_clear(dirty, LPI) and count--. If the bit is not set then we
> just eat the INV.

The bitmap is global to the host, right?

If not we may end up to send multiple INVALL from different domain in a
row. Although, I don't know if we could improve it.

> On INVALL we insert INVALL iff there are bits set in the bitmap (or use
> count is not 0 instead, if we chose to maintain that) and clear the
> bitmap. If no bits are set in the bitmap then we eat the INVALL.
> 
> Extension: If we are tracking count then we may choose to switch INVAL
> into one or more INV's up to some threshold of dirtiness.

It may be more expensive to do the conversion LPIs -> (Dev, ID) than
doing the INVALL.

> I've been trying to think of ways of extending this to reduce the
> number/impact of guest SYNC. Other than tracking whether there have been
> 0 or !0 commands since the last SYNC (and squashing the extras) I
> haven't thought of a cunning scheme.

Also this discussion remind me another point.  So far, we've assume that
Xen doesn't send a single command. Although, when a guest is destroyed
we may need to wipe a part of the LPI configuration table and therefore
sending an INVALL.

Regards,

-- 
Julien Grall