[PATCH v5 01/22] KVM: arm/arm64: Add ITS save/restore API documentation

Thu May 4 00:40:40 PDT 2017

On 04/05/17 08:00, Auger Eric wrote:
> Hi Christoffer,
> 
> On 27/04/2017 16:45, Christoffer Dall wrote:
>> Hi Eric,
>>
>> On Thu, Apr 27, 2017 at 02:51:00PM +0200, Auger Eric wrote:
>>> On 27/04/2017 13:02, Christoffer Dall wrote:
>>>> On Thu, Apr 27, 2017 at 11:33:39AM +0200, Auger Eric wrote:
>>>>> On 27/04/2017 10:57, Christoffer Dall wrote:
>>>>>> On Wed, Apr 26, 2017 at 05:48:32PM +0200, Auger Eric wrote:
>>>>>>> On 26/04/2017 14:31, Christoffer Dall wrote:
>>>>>>>> On Fri, Apr 14, 2017 at 12:15:13PM +0200, Eric Auger wrote:
>>>>>>>>> Add description for how to access ITS registers and how to save/restore
>>>>>>>>> ITS tables into/from memory.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Eric Auger <eric.auger at redhat.com>
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>> v4 -> v5:
>>>>>>>>> - take into account Christoffer's comments
>>>>>>>>> - pending table save on GICV3 side now
>>>>>>>>>
>>>>>>>>> v3 -> v4:
>>>>>>>>> - take into account Peter's comments:
>>>>>>>>>   - typos
>>>>>>>>>   - KVM_DEV_ARM_VGIC_GRP_ITS_TABLES kvm_device_attr = 0
>>>>>>>>>   - add a validity bit in DTE
>>>>>>>>>   - document all fields in CTE and ITE
>>>>>>>>>   - document ABI revision
>>>>>>>>> - take into account Andre's comments:
>>>>>>>>>   - document restrictions about GITS_CREADR writing and GITS_IIDR
>>>>>>>>>   - document -EBUSY error if one or more VCPUS are runnning
>>>>>>>>>   - document 64b registers only can be accessed with 64b access
>>>>>>>>> - itt_addr field matches bits [51:8] of the itt_addr
>>>>>>>>>
>>>>>>>>> v1 -> v2:
>>>>>>>>> - DTE and ITE now are 8 bytes
>>>>>>>>> - DTE and ITE now indexed by deviceid/eventid
>>>>>>>>> - use ITE name instead of ITTE
>>>>>>>>> - mentions ITT_addr matches bits [51:8] of the actual address
>>>>>>>>> - mentions LE layout
>>>>>>>>> ---
>>>>>>>>>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 99 ++++++++++++++++++++++
>>>>>>>>>  1 file changed, 99 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>>>>>>>> index 6081a5b..b5f010d 100644
>>>>>>>>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>>>>>>>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>>>>>>>> @@ -32,7 +32,106 @@ Groups:
>>>>>>>>>      KVM_DEV_ARM_VGIC_CTRL_INIT
>>>>>>>>>        request the initialization of the ITS, no additional parameter in
>>>>>>>>>        kvm_device_attr.addr.
>>>>>>>>> +
>>>>>>>>> +    KVM_DEV_ARM_ITS_SAVE_TABLES
>>>>>>>>> +      save the ITS table data into guest RAM, at the location provisioned
>>>>>>>>> +      by the guest in corresponding registers/table entries.
>>>>>>>>> +
>>>>>>>>> +      The layout of the tables in guest memory defines an ABI. The entries
>>>>>>>>> +      are laid out in little endian format as described in the last paragraph.
>>>>>>>>> +
>>>>>>>>> +    KVM_DEV_ARM_ITS_RESTORE_TABLES
>>>>>>>>> +      restore the ITS tables from guest RAM to ITS internal structures.
>>>>>>>>> +
>>>>>>>>> +      The GICV3 must be restored before the ITS and all ITS registers but
>>>>>>>>> +      the GITS_CTLR must be restored before restoring the ITS tables.
>>>>>>>>> +
>>>>>>>>> +      The GITS_IIDR read-only register must also be restored before
>>>>>>>>> +      the table restore as the IIDR revision field encodes the ABI revision.
>>>>>>>>> +
>>>>>>>>
>>>>>>>> what is the expected sequence of operations.  For example, to restore
>>>>>>>> the ITS, do I call KVM_DEV_ARM_VGIC_CTRL_INIT first, then restore all
>>>>>>>> the memory and registers, and finally call KVM_DEV_ARM_ITS_RESTORE_TABLES?
>>>>>>> Yes KVM_DEV_ARM_VGIC_CTRL_INIT comes first, then restore all registers
>>>>>>> except GITS_CTLR, then table restore, then GITS_CTLR
>>>>>>>>
>>>>>>>> Is there any interaction between when you call KVM_DEV_ARM_ITS_RESTORE_TABLES
>>>>>>>> and restore GITS_CTLR (which enables the ITS)?
>>>>>>>
>>>>>>> Yep, when GITS_CTLR is set, LPIs may be enabled and this on that event
>>>>>>> that the pending table is read. But the whole pending table is not read
>>>>>>> as we only iterate on registered LPIs. So the ITT must have been
>>>>>>> restored previously.
>>>>>>>
>>>>>>> I became aware that the pending table sync is done twice, once in the
>>>>>>> pending table restore,  and once in the GITS_CTLR restore. So if we
>>>>>>> leave this order specification, I should be able to remove the sync on
>>>>>>> table restore. This was the original reason why GITS_CTLR restore has
>>>>>>> been done at the very end.
>>>>>>
>>>>>> I'm sorry, I'm a bit confused.  Do we not need
>>>>>> KVM_DEV_ARM_ITS_RESTORE_TABLES at all then?
>>>>>
>>>>> Yes you do. I was talking about the RDIST pending table sync. The save
>>>>> is explicit using GICV3 device KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES.
>>>>> However the sync is implicit on GITS_CTLR restore if LPIs are enabled.
>>>>> and today I do it also on ITS device KVM_DEV_ARM_ITS_RESTORE_TABLES
>>>>> which is not requested I think since GITS_CTLR restore does it already.
>>>>
>>>> Shouldn't restoring the pending tables happen when restoring some
>>>> redeistributor state and not anything related to the ITS?
>>>
>>> Marc wrote:
>>> "
>>> I don't think you necessarily need a coarse map. When restoring the ITS
>>> tables, you can always read the pending bit when creating the LPI
>>> structure (it has been written to RAM at save time). Note that we
>>> already do something like this in vgic_enable_lpis().
>>> "
>>>
>>> This is currently what is implemented I think. the pending tables are
>>> currently sync'ed on GITS_CTLR set (if LPI are enabled) + erroneously
>>> also on on ITS table restore
>>>
>>> The problematic is: Either you know in advance which LPI INTIDare used
>>> or you need to parse the whole pending table (possibly using the 1st kB
>>> as coarse mapping).
>>>
>>> If you don't know the LPI INTIDs in advance it is only possible to
>>> restore the pending bit of pending LPIs. At that time you would
>>> re-allocate those pending LPI (vgic_add_lpi) and when you restore the
>>> ITS ITT you would do the same for those which were not pending. Looks
>>> really heavy to me: coarse mapping + dual vgic_add_lpi path.
>>>
>>> Otherwise we would need to add another dependency between RDIST pending
>>> table restore and ITS table restore but this looks even more weird, no?
>>>
>>>
>> So I just sat down with Andre and Marc and we tried to work through this
>> and came up with the best scheme.  I apologize in advance for the
>> one-way nature of this e-mail, and I am of course open to discussing the
>> following proposal again if you do not agree.
>>
>> What I think this document should say, is that the following ordering
>> must be followed when restoring the GIC and the ITS:
>>
>>   First, restore all guest memory
>>
>>   Second, restore ALL redistributors
>>
>>   Third, restore the ITS, in the following order:
>>     1. Initialize the ITS (KVM_DEV_ARM_VGIC_CTRL_INIT)
>>     2. Restore GITS_CBASER
>>     3. Restore all other GITS_ registers, except GITS_CTLR!
>>     4. Load the ITS table data (KVM_DEV_ARM_ITS_RESTORE_TABLES)
>>     5. Restore GITS_CTLR
>>
>> The rationale is that we really want the redistributor and the ITS
>> restore to be independent and follow the architecture.  This means that
>> our ABI for the redistributor should still work without restoring an ITS
>> (if we ever decide to support LPIs for KVM without the ITS).
>>
>> In terms of our current implementation this means that vgic_add_lpi()
>> should ask the redistributor what the state of the LPI is (priority,
>> enabled, pending).  I suggest you do the pending check by adding a
>> function called something like vgic_v3_lpi_is_pending() which scans the
>> bit in memory, clears the memory bit, and returns the value.  Clearing
>> the pending bit in memory when moving it to the struct irq is nice,
>> because you then don't have to clear out the entire pending table later
>> and we don't keep 'consumed' data lying around.  This change should be
>> implemented in its_sync_lpi_pending_table() as well, but note that you
>> need never call that function in the normal restore path using this
>> design.
>>
>> I hope this makes sense.
> 
> I am dubious about the above changes at the moment.
> its_sync_lpi_pending_table() gets called on GITS_CTLR setting which is
> documented to be the last step of the restoration. I wonder why the
> above changes cannot be part of another series later on.

I think that's one of the issues. See below.

> Consuming the RAM bit status means we record it in irq->pending_latch so
> I guess we should have the irq->pending_latch setting in the same
> function as the one that retrieves the bit status in guest RAM. So I
> would rename vgic_v3_lpi_is_pending into something like
> int vgic_v3_sync_lpi_pending_status(struct kvm *kvm, u32 intid)
> Since this covers a single LPI, the removes the byte access optimization
> found in its_sync_lpi_pending_table

Well, never mind the optimization. How many LPIs are we restoring in a
typical VM? 10? 1000? That's just one byte access per LPI. Of course,
I'd rather have fewer guest memory accesses, but a restore is an
incredibly rare event, so I'm not too bothered about the extra usec! ;-)

> 
> Also if I understand it correctly this means the sync will be done on
> both add_lpi and GITS_CTLR setting

Why GITS_CTLR? The Enable bit only controls the effect of
GITS_TRANSLATER... I believe that vgic_add_lpi() is the only point where
we should snapshot the pending state.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...