[Qemu-devel] [RFC 0/3] vITS Reset

Eric Auger posted 3 patches 6 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1506524205-20763-1-git-send-email-eric.auger@redhat.com
Test checkpatch passed
Test docker passed
Test s390x passed
There is a newer version of this series
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(-)
[Qemu-devel] [RFC 0/3] vITS Reset
Posted by Eric Auger 6 years, 6 months ago
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


Re: [Qemu-devel] [RFC 0/3] vITS Reset
Posted by Peter Maydell 6 years, 5 months ago
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

Re: [Qemu-devel] [RFC 0/3] vITS Reset
Posted by Auger Eric 6 years, 5 months ago
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
> 

Re: [Qemu-devel] [RFC 0/3] vITS Reset
Posted by Peter Maydell 6 years, 5 months ago
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