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

[Marc Zyngier]

On Fri, Sep 15 2017 at 10:56:06 am BST, Christoffer Dall <cdall@xxxxxxxxxx> wrote:
> 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.

I've managed to check this, and at least one implementation does clear
its caches on write to the corresponding BASERn registers, which makes
some sense. It is slightly annoying that the spec doesn't outline this,
but I'll enquire to see if that can be clarified.

> 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.

+1 on both point.

	M.
-- 
Jazz is not dead. It just smells funny.