Re: [RFC] KVM: arm/arm64: Introduce KVM_DEV_ARM_ITS_CTRL_RESET

[Christoffer Dall]

On Fri, Sep 15, 2017 at 5:26 AM, Auger Eric <eric.auger@xxxxxxxxxx> wrote:
> Hi,
>
> On 14/09/2017 19:06, Marc Zyngier wrote:
>> On Thu, Sep 14 2017 at 10:57:28 am BST, 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 emptied.
>>>
>>> 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 introduces a new attribute, KVM_DEV_ARM_ITS_CTRL_RESET in
>>> the KVM_DEV_ARM_VGIC_GRP_CTRL group.
>>>
>>> Upon this action, we can invalidate the various memory structures
>>> pointed by GITS_BASERn and GITS_CBASER, free the ITS internal caches
>>> and reset the relevant registers.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>>>
>>> ---
>>>
>>> An alternative would consist in having the userspace writing
>>> individual registers with default values: GITS_BASERn, GITS_CBASER
>>> and GITS_CTLR. On kernel side we would reset related lists when
>>> detecting the valid bit is set to false.
>>
>> I'm not sure this is necessarily a "either/or" situation. It looks to me
>> that we're not completely doing the right thing when writing to the
>> GITS_BASER registers, and that writing a new value (with the valid bit
>> set or not) should have an action of some sort on the fate of the
>> existing mappings.
>
> I agree. I think whenever the GITS_BASERn or GITS_CBASER validity bit is
> reset, we should empty the internal lists and assure the code does not
> attempt to read the data structures in caches/RAM anymore.
>

I don't think that is likely to match the behavior suggested in the
GIC/ITS spec.  I doubt that hardware implementations will support
software changing the BASERs without turning off the GIC, and
therefore I don't think we'll see drivers doing this any time soon,
and I don't think we need to support that.

What I do think we should support is the ITS power management sequence
pointed out in Section 6.6 in the spec.  But I don't think this is
urgent, as we don't seem to have any guests that power down and power
up the ITS yet.


Thanks,
-Christoffer