Hi Peter, On 12/10/2017 12:57, Peter Maydell wrote: > 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." Sure I will add this explanation. Thanks Eric > > >> 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 >
