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

[Christoffer Dall <cdall@linaro.org>]

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.

> +
> +- 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:

> +  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 ?

> -- 
> 2.5.5
> 

Thanks, (and sorry for being pedantic about ABI wordings)
-Christoffer