hw/intc/arm_gicv3_its_common.c | 5 ++--- hw/intc/arm_gicv3_its_kvm.c | 37 ++++++++++++++++++++++++---------- include/hw/intc/arm_gicv3_its_common.h | 1 + linux-headers/asm-arm/kvm.h | 1 + linux-headers/asm-arm64/kvm.h | 1 + 5 files changed, 31 insertions(+), 14 deletions(-)
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. Best Regards Eric Host Kernel dependencies: - [1] [PATCH 0/10 v2] vITS Migration fixes and reset The series is available at: https://github.com/eauger/qemu/tree/v2.10-its-reset-v1 Eric Auger (3): hw/intc/arm_gicv3_its: Don't abort on table save/restore linux-headers: Partial header update for ITS reset hw/intc/arm_gicv3_its: Implement reset hw/intc/arm_gicv3_its_common.c | 5 ++--- hw/intc/arm_gicv3_its_kvm.c | 37 ++++++++++++++++++++++++---------- include/hw/intc/arm_gicv3_its_common.h | 1 + linux-headers/asm-arm/kvm.h | 1 + linux-headers/asm-arm64/kvm.h | 1 + 5 files changed, 31 insertions(+), 14 deletions(-) -- 2.5.5
On 27 September 2017 at 15:56, Eric Auger <eric.auger@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. thanks -- PMM
Hi Peter, On 09/10/2017 20:17, Peter Maydell wrote: > On 27 September 2017 at 15:56, Eric Auger <eric.auger@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 >
On 11 October 2017 at 17:06, Auger Eric <eric.auger@redhat.com> wrote: > On 09/10/2017 20:17, Peter Maydell wrote: >> 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. OK, I guess that makes sense -- in hardware, the device has extra internal state (the cached stuff) which isn't visible in its register interface. So we want to provide an API which is morally equivalent to the hardware reset line to tell the ITS to drop that internal state. > 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. This is the interface for a software controlled clean powerdown: * OS writes 0 to Enabled * ITS finishes anything it is currently in the middle of * ITS updates any info that it currently has only in its internal cache so that it is also reflected in the external memory data structures * ITS sets Quiescent to 1 to tell the OS it has finished this process (and may no longer access guest RAM after this point) * OS can now unmap the RAM that the ITS tables are in, tell the h/w to power off the GIC/ITS, etc It's a multistep process where the ITS gets the opportunity to write memory in the middle. I think in theory there's no obligation to actually drop the cached data when you set Quiescent to 1, provided that you validate that when the OS sets Enabled back to 1 all your cached data is still valid, because that's indistinguishable from dropping the cache and then repopulating it. For the unclean powerdown (yanking the cord out of the back of the machine), which is what qemu's reset is, we don't give the ITS the option to write memory, it just has to effectively drop any internal data structures it was using. So it needs a KVM API that's different from the register-access API. thanks -- PMM
© 2016 - 2024 Red Hat, Inc.