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

[Auger Eric]

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@xxxxxxxxxx>
>> Reported-by: wanghaibin <wanghaibin.wang@xxxxxxxxxx>
>>
>> ---
>>
>> 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
>