[RFC] KVM: arm/arm64: Introduce KVM_DEV_ARM_ITS_CTRL_RESET

[RFC] KVM: arm/arm64: Introduce KVM_DEV_ARM_ITS_CTRL_RESET

[Eric Auger]

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@redhat.com>

---

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.
---
 Documentation/virtual/kvm/devices/arm-vgic-its.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
index eb06beb..ebb15c5 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
@@ -33,6 +33,9 @@ Groups:
       request the initialization of the ITS, no additional parameter in
       kvm_device_attr.addr.
 
+    KVM_DEV_ARM_ITS_CTRL_RESET
+      reset the ITS, no additional parameter in kvm_device_attr.addr.
+
     KVM_DEV_ARM_ITS_SAVE_TABLES
       save the ITS table data into guest RAM, at the location provisioned
       by the guest in corresponding registers/table entries.
-- 
2.5.5

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

[Christoffer Dall]

On Thu, Sep 14, 2017 at 10:57:28AM +0200, Eric Auger 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

It's more about freeing the cached data structures than what the BASERn
registers point to, really, but ok.

> and reset the relevant registers.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> 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 crazy about that idea.

> ---
>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> index eb06beb..ebb15c5 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> @@ -33,6 +33,9 @@ Groups:
>        request the initialization of the ITS, no additional parameter in
>        kvm_device_attr.addr.
>  
> +    KVM_DEV_ARM_ITS_CTRL_RESET
> +      reset the ITS, no additional parameter in kvm_device_attr.addr.
> +

I can't find information in the spec about what 'reset the ITS' means.
So I think we need to describe this a little more carefully.  Which
assumptions does a user have after calling this.

>      KVM_DEV_ARM_ITS_SAVE_TABLES
>        save the ITS table data into guest RAM, at the location provisioned
>        by the guest in corresponding registers/table entries.
> -- 
> 2.5.5
> 

Thanks,
-Christoffer

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

[Marc Zyngier]

On Thu, Sep 14 2017 at 10:57:28 am BST, Eric Auger <eric.auger@redhat.com> 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@redhat.com>
>
> ---
>
> 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.

Thoughts?

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

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

[Auger Eric]

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@redhat.com> 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@redhat.com>
>>
>> ---
>>
>> 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 will follow up with some patches.

Thanks

Eric
> 
> Thoughts?
> 
> 	M.
> 

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@redhat.com> 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@redhat.com> 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@redhat.com>
>>>
>>> ---
>>>
>>> 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

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@linaro.org> wrote:
> On Fri, Sep 15, 2017 at 5:26 AM, Auger Eric <eric.auger@redhat.com> 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@redhat.com> 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@redhat.com>
>>>>
>>>> ---
>>>>
>>>> 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.