Re: [PATCH v2 09/10] KVM: arm/arm64: Document KVM_DEV_ARM_ITS_CTRL_RESET

Auger Eric
2017-10-13T15:41:17Z

Hi Christoffer,

On 13/10/2017 17:26, Christoffer Dall wrote:
> Hi Eric,
>
> On Wed, Sep 27, 2017 at 03:28:39PM +0200, Eric Auger wrote:
>> At the moment, the in-kernel emulated ITS is not properly reset.
>> On guest restart/reset some registers keep their old values and
>> internal structures like device, ITE, collection lists are not freed.
>
> , and collection ...
>>
>> This may lead to various bugs. Among them, we can have incorrect state
>> backup or failure when saving the ITS state at early guest boot stage.
>>
>> This patch documents a new attribute, KVM_DEV_ARM_ITS_CTRL_RESET in
>> the KVM_DEV_ARM_VGIC_GRP_CTRL group.
>>
>> Upon this action, we can reset registers and especially those
>> pointing to tables previously allocated by the guest and free
>> the internal data structures storing the list of devices, collections
>> and lpis.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>
>>
>> ---
>>
>> v1 -> v2:
>> - Describe architecturally-defined reset values
>> ---
>> Documentation/virtual/kvm/devices/arm-vgic-its.txt | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> index eb06beb..047358c 100644
>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> @@ -33,6 +33,10 @@ Groups:
>> request the initialization of the ITS, no additional parameter in
>> kvm_device_attr.addr.
>>
>> + KVM_DEV_ARM_ITS_CTRL_RESET
>> + reset the ITS, no additional parameter in kvm_device_attr.addr.
>> + See "ITS Reset State" section.
>> +
>> 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.
>> @@ -157,3 +161,15 @@ Then vcpus can be started.
>> - pINTID is the physical LPI ID; if zero, it means the entry is not valid
>> and other fields are not meaningful.
>> - ICID is the collection ID
>> +
>> + ITS Reset State:
>> + ----------------
>
> Are the bullet points below the state after the device has been reset
> (after KVM_DEV_ARM_ITS_CTRL_RESET returns) or ? I think this should be
> clarified.

yes this is after reset.
>
>> +
>> +- the ITS is not enabled and quiescent:
>
> The
>
>> + GITS_CTLR.Enabled = 0 .Quiescent=1
>> +- caches are empty
> Caches (would it make more sense to say that there is no
> internally cached state?)
>> +- No collection or device table is provisionned
>
> are used:
referenced?
>
>> + GITS_BASER<n>.Valid = 0
>> +- the command queue is not allocated:
>
> The
>
>> + GITS_CBASER = 0, GITS_CREADR = 0, GITS_CWRITER = 0
>> +- The ABI version corresponds to the one set before reset
>
> Do you mean that resetting the ITS cannot change the ABI version used
> for save/restore, and therefore remains the same as it the version
> configured when the device was first created ?
Yes the ABI version stays the same.
>
>> --
>> 2.5.5
>>
>
> Thanks, (and sorry for being pedantic about ABI wordings)
no worries, I know it is important ;-)

Thanks

Eric
> -Christoffer
>