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

[Auger Eric]

Hi Peter,

On 12/10/2017 12:57, Peter Maydell wrote:
> On 27 September 2017 at 14:28, Eric Auger <eric.auger@xxxxxxxxxx> 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.
>>
>> 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.
> 
> I think we could probably expand this to explain why our usual
> approach to reset doesn't work, something like:
> 
> "The usual approach for device reset of having userspace write
> the reset values of the registers to the kernel via the register
> read/write APIs doesn't work for the ITS because it has some
> internal state (caches) which is not exposed as registers,
> and there is no register interface for "drop cached data without
> writing it back to RAM". So we need a KVM API which mimics the
> hardware's reset line, to provide the equivalent behaviour to
> a "pull the power cord out of the back of the machine" reset."

Sure I will add this explanation.

Thanks

Eric
> 
> 
>> 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.
> 
> Why is the init attribute for the ITS named
> KVM_DEV_ARM_VGIC_CTRL_INIT, but the reset attribute is
> KVM_DEV_ARM_ITS_CTRL_RESET ? Seems a bit inconsistent.
> 
>> +
>>      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:
>> + ----------------
>> +
>> +- the ITS is not enabled and quiescent:
>> +  GITS_CTLR.Enabled = 0 .Quiescent=1
>> +- caches are empty
>> +- No collection or device table is provisionned
> 
> "provisioned"
> 
>> +  GITS_BASER<n>.Valid = 0
>> +- the command queue is not allocated:
>> +  GITS_CBASER = 0, GITS_CREADR = 0, GITS_CWRITER = 0
>> +- The ABI version corresponds to the one set before reset
> 
> Rather than trying to spell out all the reset values here,
> it might be better to just say that RESET returns the ITS to
> the same state that it is in after it is first created and
> inited. (That is the behaviour that userspace wants.)
> 
> Happy with the general principle of the new userspace ABI.
> 
> thanks
> -- PMM
>