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

[Christoffer Dall]

On Fri, Oct 13, 2017 at 05:41:10PM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 13/10/2017 17:26, Christoffer Dall wrote:
> > 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@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.
> >> +
> >>      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.
> 
> yes this is after reset.
> > 
> >> +
> >> +- 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:
> referenced?

I think used is more plainspoken and intuitively understood.

> > 
> >> +  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 ?
> Yes the ABI version stays the same.
> > 
> >> -- 
> >> 2.5.5
> >>
> > 
> > Thanks, (and sorry for being pedantic about ABI wordings)
> no worries, I know it is important ;-)
> 
Thanks :)
-Christoffer