linux-kernel - Re: [PATCH v4 10/11] KVM: arm/arm64: Document KVM_DEV_ARM_ITS_CTRL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171017223836.GJ8326@lvm>
Date:   Wed, 18 Oct 2017 00:38:36 +0200
From:   Christoffer Dall <cdall@...aro.org>
To:     Eric Auger <eric.auger@...hat.com>
Cc:     eric.auger.pro@...il.com, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, kvmarm@...ts.cs.columbia.edu,
        marc.zyngier@....com, peter.maydell@...aro.org,
        andre.przywara@....com, wanghaibin.wang@...wei.com,
        wu.wubin@...wei.com, drjones@...hat.com, wei@...hat.com
Subject: Re: [PATCH v4 10/11] KVM: arm/arm64: Document
 KVM_DEV_ARM_ITS_CTRL_RESET

On Tue, Oct 17, 2017 at 09:10:08AM +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, and 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.
> 
> 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@...hat.com>
> Reported-by: wanghaibin <wanghaibin.wang@...wei.com>
> 
> ---
> v2 -> v3:
> - reword commit message, credit to Peter Maydell.
> - take into account Christoffer rewording comments but still
>   kept details. Added Peter's comment but still kept details.
>   Peter may disagree.
> 
> v1 -> v2:
> - Describe architecturally-defined reset values
> ---
>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> index eb06beb..4e9bb6f 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,19 @@ 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:
> + ----------------
> +
> +RESET returns the ITS to the same state that it was when first created and
> +inited:

initialized.  When the RESET command returns, the following things are
guaranteed:

> +
> +- The ITS is not enabled and quiescent
> +  GITS_CTLR.Enabled = 0 .Quiescent=1
> +- There is no internally cache state

internally cached,  or internal cache

> +- No collection or device table are used
> +  GITS_BASER<n>.Valid = 0
> +- The command queue is not allocated:
> +  GITS_CBASER = 0, GITS_CREADR = 0, GITS_CWRITER = 0
> +- The ABI version is unchanged and remains the one set when the ITS
> +  device was first created.
> -- 
> 2.5.5
> 
With those changes, this looks ok to me.

Thanks,
-Christoffer