linux-kernel - Re: [PATCH v2 09/10] 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: <4c1801e9-5198-b920-369d-6c6fad38a905@redhat.com>
Date:   Thu, 12 Oct 2017 13:34:59 +0200
From:   Auger Eric <eric.auger@...hat.com>
To:     Peter Maydell <peter.maydell@...aro.org>
Cc:     eric.auger.pro@...il.com,
        lkml - Kernel Mailing List <linux-kernel@...r.kernel.org>,
        kvm-devel <kvm@...r.kernel.org>,
        Marc Zyngier <marc.zyngier@....com>,
        Christoffer Dall <cdall@...aro.org>,
        Andre Przywara <andre.przywara@....com>,
        wanghaibin.wang@...wei.com, wu.wubin@...wei.com
Subject: Re: [PATCH v2 09/10] KVM: arm/arm64: Document
 KVM_DEV_ARM_ITS_CTRL_RESET

Hi Peter,

On 12/10/2017 12:57, Peter Maydell wrote:
> On 27 September 2017 at 14:28, Eric Auger <eric.auger@...hat.com> 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@...hat.com>
>> Reported-by: wanghaibin <wanghaibin.wang@...wei.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.
> 
> 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
>