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

[Peter Maydell]

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."


> 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