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

Thu Apr 27 15:27:46 EDT 2017


On 27/04/2017 19:54, Christoffer Dall wrote:
> On Thu, Apr 27, 2017 at 07:27:22PM +0200, Auger Eric wrote:
>> Hi Christoffer,
>>
>> On 27/04/2017 18:38, Christoffer Dall wrote:
>>> On Thu, Apr 27, 2017 at 05:29:35PM +0200, Auger Eric wrote:
>>>>
>>>>
>>>> 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).
>>>>
>>>> OK. Note I already mentioned that GICv3 must be restored before the ITS.
>>>> To me this comprised the RDIST.
>>>
>>> Possibly, but I think it's good to write out the whole thing so we
>>> clearly understand the flow.  That could better be achieved by
>>> correcting my proposed text above to say something like "Second, restore
>>> ALL redistributors to ensure the pending and configuration tables can be
>>> read."
>>>
>>>>
>>>> I understand the above description of the ordering comes in addition to
>>>> the existing text, right? 
>>>
>>> Yes
>>>
>>>> in other words I keep the GITS_READR,
>>>> GITS_IIDR specific text as well as KVM_DEV_ARM_ITS_SAVE/RESTORE_TABLES
>>>> section.
>>>>
>>>
>>> Yes.  But you don't need to do any reading of the pending table on any
>>> of the restore operations.
>> well you told me to do it on vgic_add_lpi(). This is obviously called on
>> ITS table restore. /me confused. 
> 
> Sorry, I meant you do not need to scan the entire table independently
> from restoring other state that requires building the data structures.
> 
>> Obviously this is implicit and should
>> not be documented. Is that what you meant? btw this is not documented
>> atm I think.
> 
> What I care about is that the ABI is clear and represents what the
> architecture does.  So in terms of documentation in the ABI, we don't
> need to mention anything about when this is done, but we also do not
> need to specify any interaction between the pending tables and the ITS,
> beyond that the redestributors and memory must be restored before the
> ITS.
> 
> Hope this clarifies.

yes it does

Thanks

Eric
> 
> Thanks,
> -Christoffer
>