Re: [Qemu-arm] [Qemu-devel] [RFC 0/3] vITS Reset

[Auger Eric <eric.auger@redhat.com>]

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
>