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 <[email protected]>
---
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
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 <[email protected]>
>
> ---
>
> 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
On Thu, Sep 14 2017 at 10:57:28 am BST, Eric Auger <[email protected]> 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 <[email protected]>
>
> ---
>
> 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.
Hi,
On 14/09/2017 19:06, Marc Zyngier wrote:
> On Thu, Sep 14 2017 at 10:57:28 am BST, Eric Auger <[email protected]> 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 <[email protected]>
>>
>> ---
>>
>> 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.
>
On Fri, Sep 15, 2017 at 5:26 AM, Auger Eric <[email protected]> wrote:
> Hi,
>
> On 14/09/2017 19:06, Marc Zyngier wrote:
>> On Thu, Sep 14 2017 at 10:57:28 am BST, Eric Auger <[email protected]> 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 <[email protected]>
>>>
>>> ---
>>>
>>> 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
On Fri, Sep 15 2017 at 10:56:06 am BST, Christoffer Dall <[email protected]> wrote:
> On Fri, Sep 15, 2017 at 5:26 AM, Auger Eric <[email protected]> wrote:
>> Hi,
>>
>> On 14/09/2017 19:06, Marc Zyngier wrote:
>>> On Thu, Sep 14 2017 at 10:57:28 am BST, Eric Auger <[email protected]> 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 <[email protected]>
>>>>
>>>> ---
>>>>
>>>> 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.