[Qemu-devel] [RFC v2 0/4] vITS Reset

[Qemu-devel] [RFC v2 0/4] vITS Reset

[Eric Auger]

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 valid anymore. This leads to state
inconsistencies that are detected by the kernel save/restore
code. And eventually this may cause qemu abort.

The two first patches would need to be cc'ed stable. Assuming
patches 1-5 of "[PATCH v5 00/10] vITS Migration fixes and reset" 
 also are cc'ed stable, they fix the above issue, without 
implementing a dedicated ITS KVM device reset IOCTL.
Patches 3-4 use the new reset IOCTL which clarifies the reset
process.

The series is in RFC state as it depends on:
[1] [PATCH v5 00/10] vITS Migration fixes and reset

Best Regards

Eric

The series is available at:
https://github.com/eauger/qemu/tree/v2.10-its-reset-v2

History:
v1 -> v2:
- Clarify why abort should be removed for save. Leave abort
  for restore.
- Adopt the same reset infra as vgic
- introduce "hw/intc/arm_gicv3_its: Implement a minimalist reset"
  which perform individual register writes. This is sufficient to
  fix the issues without ioctl

Eric Auger (4):
  hw/intc/arm_gicv3_its: Don't abort on table save failure
  hw/intc/arm_gicv3_its: Implement a minimalist reset
  linux-headers: Partial header update for ITS reset
  hw/intc/arm_gicv3_its: Implement full reset

 hw/intc/arm_gicv3_its_kvm.c   | 53 +++++++++++++++++++++++++++++++++++++------
 linux-headers/asm-arm/kvm.h   |  1 +
 linux-headers/asm-arm64/kvm.h |  1 +
 3 files changed, 48 insertions(+), 7 deletions(-)

-- 
2.5.5

[Qemu-devel] [RFC v2 1/4] hw/intc/arm_gicv3_its: Don't abort on table save failure

[Eric Auger]

The ITS is not fully properly reset at the moment. Caches are
not emptied.

After a reset, in case we attempt to save the state before
the bound devices have registered their MSIs and after the
1st level table has been allocated by the ITS driver
(device BASER is valid), the first level entries are still
invalid. If the device cache is not empty (devices registered
before the reset), vgic_its_save_device_tables fails with -EINVAL.
This causes a QEMU abort().

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reported-by: wanghaibin <wanghaibin.wang@huawei.com>

---

this patch would deserve being cc'ed stable (2.10)
This goes along with patches 1-5 of
[PATCH v5 00/10] vITS Migration fixes and reset, candidate
for being cc'ed stable
---
 hw/intc/arm_gicv3_its_kvm.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
index 39903d5..1ae205f 100644
--- a/hw/intc/arm_gicv3_its_kvm.c
+++ b/hw/intc/arm_gicv3_its_kvm.c
@@ -64,20 +64,16 @@ static void vm_change_state_handler(void *opaque, int running,
 {
     GICv3ITSState *s = (GICv3ITSState *)opaque;
     Error *err = NULL;
-    int ret;
 
     if (running) {
         return;
     }
 
-    ret = kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
-                            KVM_DEV_ARM_ITS_SAVE_TABLES, NULL, true, &err);
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
+                      KVM_DEV_ARM_ITS_SAVE_TABLES, NULL, true, &err);
     if (err) {
         error_report_err(err);
     }
-    if (ret < 0 && ret != -EFAULT) {
-        abort();
-    }
 }
 
 static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
-- 
2.5.5

Re: [Qemu-devel] [RFC v2 1/4] hw/intc/arm_gicv3_its: Don't abort on table save failure

[Peter Maydell]

On 23 October 2017 at 16:35, Eric Auger <eric.auger@redhat.com> wrote:
> The ITS is not fully properly reset at the moment. Caches are
> not emptied.
>
> After a reset, in case we attempt to save the state before
> the bound devices have registered their MSIs and after the
> 1st level table has been allocated by the ITS driver
> (device BASER is valid), the first level entries are still
> invalid. If the device cache is not empty (devices registered
> before the reset), vgic_its_save_device_tables fails with -EINVAL.
> This causes a QEMU abort().
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>
>
> ---
>
> this patch would deserve being cc'ed stable (2.10)
> This goes along with patches 1-5 of
> [PATCH v5 00/10] vITS Migration fixes and reset, candidate
> for being cc'ed stable
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

We could/should put this patch into qemu now, right (it's
the rest of the series that's RFC) ?

thanks
-- PMM

Re: [Qemu-devel] [RFC v2 1/4] hw/intc/arm_gicv3_its: Don't abort on table save failure

[Auger Eric]

Hi Peter,

On 02/11/2017 13:53, Peter Maydell wrote:
> On 23 October 2017 at 16:35, Eric Auger <eric.auger@redhat.com> wrote:
>> The ITS is not fully properly reset at the moment. Caches are
>> not emptied.
>>
>> After a reset, in case we attempt to save the state before
>> the bound devices have registered their MSIs and after the
>> 1st level table has been allocated by the ITS driver
>> (device BASER is valid), the first level entries are still
>> invalid. If the device cache is not empty (devices registered
>> before the reset), vgic_its_save_device_tables fails with -EINVAL.
>> This causes a QEMU abort().
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>
>>
>> ---
>>
>> this patch would deserve being cc'ed stable (2.10)
>> This goes along with patches 1-5 of
>> [PATCH v5 00/10] vITS Migration fixes and reset, candidate
>> for being cc'ed stable
>> ---
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> We could/should put this patch into qemu now, right (it's
> the rest of the series that's RFC) ?

Yes that's correct.

Thanks

Eric
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [RFC v2 1/4] hw/intc/arm_gicv3_its: Don't abort on table save failure

[Peter Maydell]

On 6 November 2017 at 10:09, Auger Eric <eric.auger@redhat.com> wrote:
> Hi Peter,
>
> On 02/11/2017 13:53, Peter Maydell wrote:
>> On 23 October 2017 at 16:35, Eric Auger <eric.auger@redhat.com> wrote:
>>> The ITS is not fully properly reset at the moment. Caches are
>>> not emptied.
>>>
>>> After a reset, in case we attempt to save the state before
>>> the bound devices have registered their MSIs and after the
>>> 1st level table has been allocated by the ITS driver
>>> (device BASER is valid), the first level entries are still
>>> invalid. If the device cache is not empty (devices registered
>>> before the reset), vgic_its_save_device_tables fails with -EINVAL.
>>> This causes a QEMU abort().
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>
>>>
>>> ---
>>>
>>> this patch would deserve being cc'ed stable (2.10)
>>> This goes along with patches 1-5 of
>>> [PATCH v5 00/10] vITS Migration fixes and reset, candidate
>>> for being cc'ed stable
>>> ---
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> We could/should put this patch into qemu now, right (it's
>> the rest of the series that's RFC) ?
>
> Yes that's correct.

OK. I have applied 1/4 (and none of the rest) to target-arm.next,
with a cc-stable annotation.

thanks
-- PMM

[Qemu-devel] [RFC v2 2/4] hw/intc/arm_gicv3_its: Implement a minimalist reset

[Eric Auger]

At the moment the ITS is not properly reset and this causes
various bugs on save/restore. We implement a minimalist cold reset
through individual register writes. This does not void the ITS cache
though. This full reset will be addressed through a specific ioctl.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

this patch would deserve being cc'ed stable (2.10)
This goes along with patches 1-5 of
[PATCH v5 00/10] vITS Migration fixes and reset, candidate
for being cc'ed stable
---
 hw/intc/arm_gicv3_its_kvm.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
index 1ae205f..537cea1 100644
--- a/hw/intc/arm_gicv3_its_kvm.c
+++ b/hw/intc/arm_gicv3_its_kvm.c
@@ -28,6 +28,16 @@
 
 #define TYPE_KVM_ARM_ITS "arm-its-kvm"
 #define KVM_ARM_ITS(obj) OBJECT_CHECK(GICv3ITSState, (obj), TYPE_KVM_ARM_ITS)
+#define KVM_ARM_ITS_CLASS(klass) \
+     OBJECT_CLASS_CHECK(KVMARMITSClass, (klass), TYPE_KVM_ARM_ITS)
+#define KVM_ARM_ITS_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(KVMARMITSClass, (obj), TYPE_KVM_ARM_ITS)
+
+typedef struct KVMARMITSClass {
+    GICv3ITSCommonClass parent_class;
+    void (*parent_reset)(DeviceState *dev);
+} KVMARMITSClass;
+
 
 static int kvm_its_send_msi(GICv3ITSState *s, uint32_t value, uint16_t devid)
 {
@@ -153,12 +163,25 @@ static void kvm_arm_its_pre_save(GICv3ITSState *s)
  */
 static void kvm_arm_its_post_load(GICv3ITSState *s)
 {
+    bool cold_reset = !s->iidr;
     int i;
 
-    if (!s->iidr) {
+    if (cold_reset) {
+        kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                          GITS_CTLR, &s->ctlr, true, &error_abort);
+
+        kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                          GITS_CBASER, &s->cbaser, true, &error_abort);
+
+        for (i = 0; i < 8; i++) {
+            kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                              GITS_BASER + i * 8, &s->baser[i], true,
+                              &error_abort);
+        }
         return;
     }
 
+    /* restore */
     kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
                       GITS_IIDR, &s->iidr, true, &error_abort);
 
@@ -190,6 +213,13 @@ static void kvm_arm_its_post_load(GICv3ITSState *s)
                       GITS_CTLR, &s->ctlr, true, &error_abort);
 }
 
+static void kvm_arm_its_reset(DeviceState *dev)
+{
+    KVMARMITSClass *c = KVM_ARM_ITS_GET_CLASS(s);
+
+    c->parent_reset(dev);
+}
+
 static Property kvm_arm_its_props[] = {
     DEFINE_PROP_LINK("parent-gicv3", GICv3ITSState, gicv3, "kvm-arm-gicv3",
                      GICv3State *),
@@ -200,12 +230,15 @@ static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     GICv3ITSCommonClass *icc = ARM_GICV3_ITS_COMMON_CLASS(klass);
+    KVMARMITSClass *ic = KVM_ARM_ITS_CLASS(klass);
 
     dc->realize = kvm_arm_its_realize;
     dc->props   = kvm_arm_its_props;
+    ic->parent_reset = dc->reset;
     icc->send_msi = kvm_its_send_msi;
     icc->pre_save = kvm_arm_its_pre_save;
     icc->post_load = kvm_arm_its_post_load;
+    dc->reset = kvm_arm_its_reset;
 }
 
 static const TypeInfo kvm_arm_its_info = {
-- 
2.5.5

Re: [Qemu-devel] [RFC v2 2/4] hw/intc/arm_gicv3_its: Implement a minimalist reset

[Peter Maydell]

On 23 October 2017 at 16:35, Eric Auger <eric.auger@redhat.com> wrote:
> At the moment the ITS is not properly reset and this causes
> various bugs on save/restore. We implement a minimalist cold reset
> through individual register writes. This does not void the ITS cache
> though. This full reset will be addressed through a specific ioctl.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>

> diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
> index 1ae205f..537cea1 100644
> --- a/hw/intc/arm_gicv3_its_kvm.c
> +++ b/hw/intc/arm_gicv3_its_kvm.c
> @@ -28,6 +28,16 @@
>
>  #define TYPE_KVM_ARM_ITS "arm-its-kvm"
>  #define KVM_ARM_ITS(obj) OBJECT_CHECK(GICv3ITSState, (obj), TYPE_KVM_ARM_ITS)
> +#define KVM_ARM_ITS_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(KVMARMITSClass, (klass), TYPE_KVM_ARM_ITS)
> +#define KVM_ARM_ITS_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(KVMARMITSClass, (obj), TYPE_KVM_ARM_ITS)
> +
> +typedef struct KVMARMITSClass {
> +    GICv3ITSCommonClass parent_class;
> +    void (*parent_reset)(DeviceState *dev);
> +} KVMARMITSClass;
> +

The infrastructure here for having a place to hang reset is
all fine, but it's a bit out of place because this patch doesn't
actually use any of it.

>  static int kvm_its_send_msi(GICv3ITSState *s, uint32_t value, uint16_t devid)
>  {
> @@ -153,12 +163,25 @@ static void kvm_arm_its_pre_save(GICv3ITSState *s)
>   */
>  static void kvm_arm_its_post_load(GICv3ITSState *s)
>  {
> +    bool cold_reset = !s->iidr;
>      int i;
>
> -    if (!s->iidr) {
> +    if (cold_reset) {
> +        kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +                          GITS_CTLR, &s->ctlr, true, &error_abort);
> +
> +        kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +                          GITS_CBASER, &s->cbaser, true, &error_abort);
> +
> +        for (i = 0; i < 8; i++) {
> +            kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +                              GITS_BASER + i * 8, &s->baser[i], true,
> +                              &error_abort);
> +        }
>          return;
>      }

I don't understand why we need to treat the "iidr zero" case
differently here. Why can't we just restore the register state
like we do now? (Also, all resets in QEMU are cold resets,
and post_load isn't called for reset, so the flag naming is
a bit odd.)

(I don't understand why the code in master at the moment does
nothing for iidr == 0, for that matter. We should always tell
the kernel the state of the device regs.)

thanks
-- PMM

[Qemu-devel] [RFC v2 3/4] linux-headers: Partial header update for ITS reset

[Eric Auger]

This aims at importing the KVM_DEV_ARM_ITS_CTRL_RESET attribute
which allows to trigger a reset of the in-kernel emulated ITS.

This is not yet upstream but can be found on the
follwing branch:
https://github.com/eauger/linux/tree/v4.14-rc2-its-reset-v2

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 linux-headers/asm-arm/kvm.h   | 1 +
 linux-headers/asm-arm64/kvm.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/linux-headers/asm-arm/kvm.h b/linux-headers/asm-arm/kvm.h
index fa9fae8..8dd0ba7 100644
--- a/linux-headers/asm-arm/kvm.h
+++ b/linux-headers/asm-arm/kvm.h
@@ -215,6 +215,7 @@ struct kvm_arch_memory_slot {
 #define   KVM_DEV_ARM_ITS_SAVE_TABLES		1
 #define   KVM_DEV_ARM_ITS_RESTORE_TABLES	2
 #define   KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES	3
+#define   KVM_DEV_ARM_ITS_CTRL_RESET		4
 
 /* KVM_IRQ_LINE irq field index values */
 #define KVM_ARM_IRQ_TYPE_SHIFT		24
diff --git a/linux-headers/asm-arm64/kvm.h b/linux-headers/asm-arm64/kvm.h
index d254700..2585a50 100644
--- a/linux-headers/asm-arm64/kvm.h
+++ b/linux-headers/asm-arm64/kvm.h
@@ -227,6 +227,7 @@ struct kvm_arch_memory_slot {
 #define   KVM_DEV_ARM_ITS_SAVE_TABLES           1
 #define   KVM_DEV_ARM_ITS_RESTORE_TABLES        2
 #define   KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES	3
+#define   KVM_DEV_ARM_ITS_CTRL_RESET		4
 
 /* Device Control API on vcpu fd */
 #define KVM_ARM_VCPU_PMU_V3_CTRL	0
-- 
2.5.5

[Qemu-devel] [RFC v2 4/4] hw/intc/arm_gicv3_its: Implement full reset

[Eric Auger]

Voiding the ITS caches is not supposed to happen via
individual register writes. So we introduced a dedicated
ITS KVM device ioctl to perform a cold reset of the ITS:
KVM_DEV_ARM_VGIC_GRP_CTRL/KVM_DEV_ARM_ITS_CTRL_RESET. Let's
use this latter if the kernel supports it.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/intc/arm_gicv3_its_kvm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
index 537cea1..73e2530 100644
--- a/hw/intc/arm_gicv3_its_kvm.c
+++ b/hw/intc/arm_gicv3_its_kvm.c
@@ -215,9 +215,19 @@ static void kvm_arm_its_post_load(GICv3ITSState *s)
 
 static void kvm_arm_its_reset(DeviceState *dev)
 {
+    GICv3ITSState *s = ARM_GICV3_ITS_COMMON(dev);
     KVMARMITSClass *c = KVM_ARM_ITS_GET_CLASS(s);
 
     c->parent_reset(dev);
+
+    if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
+                               KVM_DEV_ARM_ITS_CTRL_RESET)) {
+        error_report("ITS KVM: reset is not supported by the kernel");
+        return;
+    }
+
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
+                      KVM_DEV_ARM_ITS_CTRL_RESET, NULL, true, &error_abort);
 }
 
 static Property kvm_arm_its_props[] = {
-- 
2.5.5

Re: [Qemu-devel] [RFC v2 4/4] hw/intc/arm_gicv3_its: Implement full reset

[Peter Maydell]

On 23 October 2017 at 16:35, Eric Auger <eric.auger@redhat.com> wrote:
> Voiding the ITS caches is not supposed to happen via
> individual register writes. So we introduced a dedicated
> ITS KVM device ioctl to perform a cold reset of the ITS:
> KVM_DEV_ARM_VGIC_GRP_CTRL/KVM_DEV_ARM_ITS_CTRL_RESET. Let's
> use this latter if the kernel supports it.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/intc/arm_gicv3_its_kvm.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
> index 537cea1..73e2530 100644
> --- a/hw/intc/arm_gicv3_its_kvm.c
> +++ b/hw/intc/arm_gicv3_its_kvm.c
> @@ -215,9 +215,19 @@ static void kvm_arm_its_post_load(GICv3ITSState *s)
>
>  static void kvm_arm_its_reset(DeviceState *dev)
>  {
> +    GICv3ITSState *s = ARM_GICV3_ITS_COMMON(dev);
>      KVMARMITSClass *c = KVM_ARM_ITS_GET_CLASS(s);
>
>      c->parent_reset(dev);
> +
> +    if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +                               KVM_DEV_ARM_ITS_CTRL_RESET)) {
> +        error_report("ITS KVM: reset is not supported by the kernel");

Best to say "host kernel" so users know we're not complaining about
the guest kernel.

> +        return;
> +    }
> +
> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +                      KVM_DEV_ARM_ITS_CTRL_RESET, NULL, true, &error_abort);
>  }
>
>  static Property kvm_arm_its_props[] = {
> --

This would be the right patch for the boilerplate for adding
the parent class and hooking into the reset function chain.

thanks
-- PMM