Hi Peter, On 09/10/2017 20:17, Peter Maydell wrote: > On 27 September 2017 at 15:56, Eric Auger <eric.au...@redhat.com> wrote: >> At the moment the ITS is not properly reset. On System reset or >> reboot, previous ITS register values and caches are left >> unchanged. Some of the registers might point to some guest RAM >> tables which are not provisionned. This leads to state >> inconsistencies that are detected by the kernel save/restore >> code. And eventually this may cause qemu abort on source or >> destination. >> >> The 1st patch, suggested to be cc'ed stable proposes to remove >> the abort in case of table save/restore failure. This is >> definitively not ideal but looks the most reasonable until we >> get a proper way to reset the ITS. Still a message is emitted >> to report the save/restore did not happen correctly. >> >> Subsequent patches add the support of explicit reset using >> a new kvm device group/attribute combo. The associated kernel >> series is not upstream [1], hence the RFC. >> >> ITS specification is not very clear about reset. There is no >> reset wire. Some register fields are documented to have >> architecturally defined reset values and we use those here: >> Most importantly the Valid bit of GITS_CBASER and GITS_BASER >> are cleared and the GITS_CTLR.Enabled bit is cleared as well. > > The ITS is a device, not part of the CPU, so its reset is > implementation-defined but typically via some signal that's > controlled by the SoC (this is no different to the other > parts of the GICv3 which aren't denoted as being in the > reset domain of the CPU). For instance if you look at the TRM > for the GIC-500 it has a single 'resetn' reset signal which > resets all of the GIC/ITS apart from the CPU interface parts: > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.100336_0002_01_en/index.html > > The GICv3 spec 6.6 indicates what is supposed to happen to > the ITS on powerup. For QEMU's purposes reset is generally > considered to be a cold reset and we should always reset our > state to the same thing it was when QEMU first started. > > It's not clear to me why we need a new KVM device attribute > for doing ITS reset. The usual approach for this is: > * system reset causes QEMU's device model reset code > to reset state structure values to whatever their > reset value is > * we write those values up into the kernel, which is > sufficient to reset it > > What goes wrong with that in the case of the ITS? > In particular, if GITS_CTLR.Enabled is 0 then I think the > kernel should not be trying to read guest memory tables.
We discussed that on the kernel thread and Christoffer seemed to prefer an IOTCL instead of individual register writes (https://www.spinics.net/lists/kvm-arm/msg27211.html) The IOCTL empties the ITS cached data (list of collection, list of devices and list of LPIs) while it is not obvious the reset of BASER<n> would mandate that. Eventually in my kernel series I also voided the list on BASER<n>.valid toggling from 1 to 0. Spec also says: If a write to GITS.CTLR changes the Enabled field from 1 to 0, the Interrupt Translation Space must ensure that both: • Any caches containing mapping data are made consistent with external memory. • GITS_CTLR.Quiescent == 0 until all caches are consistent with external memory. I aknowledge I don't really get at which moment we are supposed to void the caches. Nevertheless, personnally I think we can achieve the same reset functionality with individual register writes. Thanks Eric > > thanks > -- PMM >
