[PATCH v5 01/22] KVM: arm/arm64: Add ITS save/restore API documentation
Auger Eric
eric.auger at redhat.com
Thu Apr 27 13:27:22 EDT 2017
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. Obviously this is implicit and should
not be documented. Is that what you meant? btw this is not documented
atm I think.
>
> Btw., I think it's GITS_CREADR, not GITS_READR.
right.
Thanks
Eric
>
>>>
>>> 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).
>> this practically means I move update_lpi_config call from
>> vgic_its_restore_ite to vgic_add_lpi(). OK
>
> Pretty much, yes.
>
> Thanks,
> -Christoffer
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
More information about the linux-arm-kernel
mailing list