[Qemu-devel] [RFC 0/3] vITS Reset

[Qemu-devel] [RFC 0/3] 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 provisionned. This leads to state
inconsistencies that are detected by the kernel save/restore
code. And eventually this may cause qemu abort on source or
destination.

The 1st patch, suggested to be cc'ed stable proposes to remove
the abort in case of table save/restore failure. This is
definitively not ideal but looks the most reasonable until we
get a proper way to reset the ITS. Still a message is emitted
to report the save/restore did not happen correctly.

Subsequent patches add the support of explicit reset using
a new kvm device group/attribute combo. The associated kernel
series is not upstream [1], hence the RFC.

ITS specification is not very clear about reset. There is no
reset wire. Some register fields are documented to have
architecturally defined reset values and we use those here:
Most importantly the Valid bit of GITS_CBASER and GITS_BASER
are cleared and the GITS_CTLR.Enabled bit is cleared as well.

Best Regards

Eric

Host Kernel dependencies:
- [1] [PATCH 0/10 v2] vITS Migration fixes and reset

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


Eric Auger (3):
  hw/intc/arm_gicv3_its: Don't abort on table save/restore
  linux-headers: Partial header update for ITS reset
  hw/intc/arm_gicv3_its: Implement reset

 hw/intc/arm_gicv3_its_common.c         |  5 ++---
 hw/intc/arm_gicv3_its_kvm.c            | 37 ++++++++++++++++++++++++----------
 include/hw/intc/arm_gicv3_its_common.h |  1 +
 linux-headers/asm-arm/kvm.h            |  1 +
 linux-headers/asm-arm64/kvm.h          |  1 +
 5 files changed, 31 insertions(+), 14 deletions(-)

-- 
2.5.5

[Qemu-devel] [RFC 1/3] hw/intc/arm_gicv3_its: Don't abort on table save/restore

[Eric Auger]

The ITS is not properly reset at the moment. It is possible the
GITS_BASER<n>.valid is set and the in-kernel ITS caches are not
empty (list of devices, collections, LPIs) while data structures
in guest RAM are invalid/inconsistent.

For instance, this happens after a guest shutdown -r now or a
system reset, if we save the state before the guest re-writes
the ITS registers or map devices, the table save ioctl may
produce a QEMU abort.

Until there is a proper reset implemented, let's unplug the
consistency error checking.

The reset issue will be fixed in subsequent patches.

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)
---
 hw/intc/arm_gicv3_its_kvm.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
index 39903d5..120b86d 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)
@@ -157,6 +153,7 @@ static void kvm_arm_its_pre_save(GICv3ITSState *s)
  */
 static void kvm_arm_its_post_load(GICv3ITSState *s)
 {
+    Error *err = NULL;
     int i;
 
     if (!s->iidr) {
@@ -188,7 +185,11 @@ static void kvm_arm_its_post_load(GICv3ITSState *s)
 
     kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
                       KVM_DEV_ARM_ITS_RESTORE_TABLES, NULL, true,
-                      &error_abort);
+                      &err);
+
+    if (err) {
+        error_report_err(err);
+    }
 
     kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
                       GITS_CTLR, &s->ctlr, true, &error_abort);
-- 
2.5.5

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

[Peter Maydell]

On 27 September 2017 at 15:56, Eric Auger <eric.auger@redhat.com> wrote:
> The ITS is not properly reset at the moment. It is possible the
> GITS_BASER<n>.valid is set and the in-kernel ITS caches are not
> empty (list of devices, collections, LPIs) while data structures
> in guest RAM are invalid/inconsistent.
>
> For instance, this happens after a guest shutdown -r now or a
> system reset, if we save the state before the guest re-writes
> the ITS registers or map devices, the table save ioctl may
> produce a QEMU abort.
>
> Until there is a proper reset implemented, let's unplug the
> consistency error checking.
>
> The reset issue will be fixed in subsequent patches.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>

When in particular does this cause an abort -- when we're
trying to save the state in these edge cases, or when we're
trying to restore it? What does the kernel do -- is it just
rejecting the attempt, or might it actually have done bad
things to guest memory ?

thanks
-- PMM

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

[Auger Eric]

Hi Peter,
On 12/10/2017 13:54, Peter Maydell wrote:
> On 27 September 2017 at 15:56, Eric Auger <eric.auger@redhat.com> wrote:
>> The ITS is not properly reset at the moment. It is possible the
>> GITS_BASER<n>.valid is set and the in-kernel ITS caches are not
>> empty (list of devices, collections, LPIs) while data structures
>> in guest RAM are invalid/inconsistent.
>>
>> For instance, this happens after a guest shutdown -r now or a
>> system reset, if we save the state before the guest re-writes
>> the ITS registers or map devices, the table save ioctl may
>> produce a QEMU abort.
>>
>> Until there is a proper reset implemented, let's unplug the
>> consistency error checking.
>>
>> The reset issue will be fixed in subsequent patches.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>
> 
> When in particular does this cause an abort -- when we're
> trying to save the state in these edge cases, or when we're
> trying to restore it?
Both.

After a guest reset, device GITS_BASER<n> may point to an L1 device
table that is not valid anymore. In that case device table save IOTCL
returned -EINVAL and we aborted.

On restore we had a bug in case all data in the table is invalid. In
that case as well we currently abort.

 What does the kernel do -- is it just
> rejecting the attempt, or might it actually have done bad
> things to guest memory ?
On Save, in case the GITS_BASER points to an invalid linear device tree
table but the page still corresponds to a visible gfn window, this
memory could be overwritten by the kernel.

Thanks

Eric
> 
> thanks
> -- PMM
> 

[Qemu-devel] [RFC 2/3] 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 3/3] hw/intc/arm_gicv3_its: Implement reset

[Eric Auger]

Currently the ITS is not reset and this causes trouble
when state backup is initiated before the guest has initialized
the ITS registers and especially GITS_CBASER<n>.

We are likely to save register values set before the reset/
restart. The register values may not be consistent with the
data structures in RAM.

So let's use the ITS KVM device new combo,
KVM_DEV_ARM_VGIC_GRP_CTRL/KVM_DEV_ARM_ITS_CTRL_RESET
to explicitly force the in-kernel emulated reset.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/intc/arm_gicv3_its_common.c         |  5 ++---
 hw/intc/arm_gicv3_its_kvm.c            | 22 ++++++++++++++++++----
 include/hw/intc/arm_gicv3_its_common.h |  1 +
 3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/hw/intc/arm_gicv3_its_common.c b/hw/intc/arm_gicv3_its_common.c
index 68b20fc..a2fe561 100644
--- a/hw/intc/arm_gicv3_its_common.c
+++ b/hw/intc/arm_gicv3_its_common.c
@@ -129,15 +129,14 @@ static void gicv3_its_common_reset(DeviceState *dev)
     s->creadr = 0;
     s->iidr = 0;
     memset(&s->baser, 0, sizeof(s->baser));
-
-    gicv3_its_post_load(s, 0);
 }
 
 static void gicv3_its_common_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    GICv3ITSCommonClass *c = ARM_GICV3_ITS_COMMON_CLASS(klass);
 
-    dc->reset = gicv3_its_common_reset;
+    c->parent_reset = gicv3_its_common_reset;
     dc->vmsd = &vmstate_its;
 }
 
diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
index 120b86d..3c2e724 100644
--- a/hw/intc/arm_gicv3_its_kvm.c
+++ b/hw/intc/arm_gicv3_its_kvm.c
@@ -156,10 +156,6 @@ static void kvm_arm_its_post_load(GICv3ITSState *s)
     Error *err = NULL;
     int i;
 
-    if (!s->iidr) {
-        return;
-    }
-
     kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
                       GITS_IIDR, &s->iidr, true, &error_abort);
 
@@ -195,6 +191,23 @@ static void kvm_arm_its_post_load(GICv3ITSState *s)
                       GITS_CTLR, &s->ctlr, true, &error_abort);
 }
 
+static void kvm_arm_its_reset(DeviceState *dev)
+{
+    GICv3ITSState *s = ARM_GICV3_ITS_COMMON(dev);
+    GICv3ITSCommonClass *c = ARM_GICV3_ITS_COMMON_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[] = {
     DEFINE_PROP_LINK("parent-gicv3", GICv3ITSState, gicv3, "kvm-arm-gicv3",
                      GICv3State *),
@@ -211,6 +224,7 @@ static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
     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 = {
diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
index fd1fe64..c158e9f 100644
--- a/include/hw/intc/arm_gicv3_its_common.h
+++ b/include/hw/intc/arm_gicv3_its_common.h
@@ -79,6 +79,7 @@ struct GICv3ITSCommonClass {
     int (*send_msi)(GICv3ITSState *s, uint32_t data, uint16_t devid);
     void (*pre_save)(GICv3ITSState *s);
     void (*post_load)(GICv3ITSState *s);
+    void (*parent_reset)(DeviceState *dev);
 };
 
 typedef struct GICv3ITSCommonClass GICv3ITSCommonClass;
-- 
2.5.5

Re: [Qemu-devel] [RFC 3/3] hw/intc/arm_gicv3_its: Implement reset

[Peter Maydell]

On 27 September 2017 at 15:56, Eric Auger <eric.auger@redhat.com> wrote:
> Currently the ITS is not reset and this causes trouble
> when state backup is initiated before the guest has initialized
> the ITS registers and especially GITS_CBASER<n>.
>
> We are likely to save register values set before the reset/
> restart. The register values may not be consistent with the
> data structures in RAM.
>
> So let's use the ITS KVM device new combo,
> KVM_DEV_ARM_VGIC_GRP_CTRL/KVM_DEV_ARM_ITS_CTRL_RESET
> to explicitly force the in-kernel emulated reset.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/intc/arm_gicv3_its_common.c         |  5 ++---
>  hw/intc/arm_gicv3_its_kvm.c            | 22 ++++++++++++++++++----
>  include/hw/intc/arm_gicv3_its_common.h |  1 +
>  3 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_its_common.c b/hw/intc/arm_gicv3_its_common.c
> index 68b20fc..a2fe561 100644
> --- a/hw/intc/arm_gicv3_its_common.c
> +++ b/hw/intc/arm_gicv3_its_common.c
> @@ -129,15 +129,14 @@ static void gicv3_its_common_reset(DeviceState *dev)
>      s->creadr = 0;
>      s->iidr = 0;
>      memset(&s->baser, 0, sizeof(s->baser));
> -
> -    gicv3_its_post_load(s, 0);

This doesn't look right as it means we won't write the QEMU
initial device register values up to the kernel. I think we
want to do that as well as call the specific reset ioctl,
so that both userspace and the kernel are consistent in
their idea of what's going on.

>  }
>
>  static void gicv3_its_common_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    GICv3ITSCommonClass *c = ARM_GICV3_ITS_COMMON_CLASS(klass);
>
> -    dc->reset = gicv3_its_common_reset;
> +    c->parent_reset = gicv3_its_common_reset;
>      dc->vmsd = &vmstate_its;
>  }

This isn't how we handle this for the arm_gicv3_kvm.c and arm_gic_kvm.c
code which has a subclass reset/parent class reset. What we do there is:
 * the parent_reset field is in the subclass's Class struct
 * the subclass's reset function calls the parent_reset function
 * the subclass's class_init function sets parent_reset to whatever
   the old dc->reset was before setting dc->reset to its own reset
   function

I think we should be consistent in how we do this.

> diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
> index 120b86d..3c2e724 100644
> --- a/hw/intc/arm_gicv3_its_kvm.c
> +++ b/hw/intc/arm_gicv3_its_kvm.c
> @@ -156,10 +156,6 @@ static void kvm_arm_its_post_load(GICv3ITSState *s)
>      Error *err = NULL;
>      int i;
>
> -    if (!s->iidr) {
> -        return;
> -    }
> -

This looks like an unrelated change, or at least not one mentioned
in the commit message?

>      kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
>                        GITS_IIDR, &s->iidr, true, &error_abort);
>
> @@ -195,6 +191,23 @@ static void kvm_arm_its_post_load(GICv3ITSState *s)
>                        GITS_CTLR, &s->ctlr, true, &error_abort);
>  }
>
> +static void kvm_arm_its_reset(DeviceState *dev)
> +{
> +    GICv3ITSState *s = ARM_GICV3_ITS_COMMON(dev);
> +    GICv3ITSCommonClass *c = ARM_GICV3_ITS_COMMON_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[] = {
>      DEFINE_PROP_LINK("parent-gicv3", GICv3ITSState, gicv3, "kvm-arm-gicv3",
>                       GICv3State *),
> @@ -211,6 +224,7 @@ static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
>      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 = {
> diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
> index fd1fe64..c158e9f 100644
> --- a/include/hw/intc/arm_gicv3_its_common.h
> +++ b/include/hw/intc/arm_gicv3_its_common.h
> @@ -79,6 +79,7 @@ struct GICv3ITSCommonClass {
>      int (*send_msi)(GICv3ITSState *s, uint32_t data, uint16_t devid);
>      void (*pre_save)(GICv3ITSState *s);
>      void (*post_load)(GICv3ITSState *s);
> +    void (*parent_reset)(DeviceState *dev);
>  };
>
>  typedef struct GICv3ITSCommonClass GICv3ITSCommonClass;
> --
> 2.5.5
>

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/3] vITS Reset

[Peter Maydell]

On 27 September 2017 at 15:56, Eric Auger <eric.auger@redhat.com> wrote:
> 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 provisionned. This leads to state
> inconsistencies that are detected by the kernel save/restore
> code. And eventually this may cause qemu abort on source or
> destination.
>
> The 1st patch, suggested to be cc'ed stable proposes to remove
> the abort in case of table save/restore failure. This is
> definitively not ideal but looks the most reasonable until we
> get a proper way to reset the ITS. Still a message is emitted
> to report the save/restore did not happen correctly.
>
> Subsequent patches add the support of explicit reset using
> a new kvm device group/attribute combo. The associated kernel
> series is not upstream [1], hence the RFC.
>
> ITS specification is not very clear about reset. There is no
> reset wire. Some register fields are documented to have
> architecturally defined reset values and we use those here:
> Most importantly the Valid bit of GITS_CBASER and GITS_BASER
> are cleared and the GITS_CTLR.Enabled bit is cleared as well.

The ITS is a device, not part of the CPU, so its reset is
implementation-defined but typically via some signal that's
controlled by the SoC (this is no different to the other
parts of the GICv3 which aren't denoted as being in the
reset domain of the CPU). For instance if you look at the TRM
for the GIC-500 it has a single 'resetn' reset signal which
resets all of the GIC/ITS apart from the CPU interface parts:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.100336_0002_01_en/index.html

The GICv3 spec 6.6 indicates what is supposed to happen to
the ITS on powerup. For QEMU's purposes reset is generally
considered to be a cold reset and we should always reset our
state to the same thing it was when QEMU first started.

It's not clear to me why we need a new KVM device attribute
for doing ITS reset. The usual approach for this is:
 * system reset causes QEMU's device model reset code
   to reset state structure values to whatever their
   reset value is
 * we write those values up into the kernel, which is
   sufficient to reset it

What goes wrong with that in the case of the ITS?
In particular, if GITS_CTLR.Enabled is 0 then I think the
kernel should not be trying to read guest memory tables.

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/3] vITS Reset

[Auger Eric]

Hi Peter,

On 09/10/2017 20:17, Peter Maydell wrote:
> On 27 September 2017 at 15:56, Eric Auger <eric.auger@redhat.com> wrote:
>> 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 provisionned. This leads to state
>> inconsistencies that are detected by the kernel save/restore
>> code. And eventually this may cause qemu abort on source or
>> destination.
>>
>> The 1st patch, suggested to be cc'ed stable proposes to remove
>> the abort in case of table save/restore failure. This is
>> definitively not ideal but looks the most reasonable until we
>> get a proper way to reset the ITS. Still a message is emitted
>> to report the save/restore did not happen correctly.
>>
>> Subsequent patches add the support of explicit reset using
>> a new kvm device group/attribute combo. The associated kernel
>> series is not upstream [1], hence the RFC.
>>
>> ITS specification is not very clear about reset. There is no
>> reset wire. Some register fields are documented to have
>> architecturally defined reset values and we use those here:
>> Most importantly the Valid bit of GITS_CBASER and GITS_BASER
>> are cleared and the GITS_CTLR.Enabled bit is cleared as well.
> 
> The ITS is a device, not part of the CPU, so its reset is
> implementation-defined but typically via some signal that's
> controlled by the SoC (this is no different to the other
> parts of the GICv3 which aren't denoted as being in the
> reset domain of the CPU). For instance if you look at the TRM
> for the GIC-500 it has a single 'resetn' reset signal which
> resets all of the GIC/ITS apart from the CPU interface parts:
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.100336_0002_01_en/index.html
> 
> The GICv3 spec 6.6 indicates what is supposed to happen to
> the ITS on powerup. For QEMU's purposes reset is generally
> considered to be a cold reset and we should always reset our
> state to the same thing it was when QEMU first started.
> 
> It's not clear to me why we need a new KVM device attribute
> for doing ITS reset. The usual approach for this is:
>  * system reset causes QEMU's device model reset code
>    to reset state structure values to whatever their
>    reset value is
>  * we write those values up into the kernel, which is
>    sufficient to reset it
> 
> What goes wrong with that in the case of the ITS?
> In particular, if GITS_CTLR.Enabled is 0 then I think the
> kernel should not be trying to read guest memory tables.

We discussed that on the kernel thread and Christoffer seemed to prefer
an IOTCL instead of individual register writes
(https://www.spinics.net/lists/kvm-arm/msg27211.html)

The IOCTL empties the ITS cached data (list of collection, list of
devices and list of LPIs) while it is not obvious the reset of BASER<n>
would mandate that. Eventually in my kernel series I also voided the
list on BASER<n>.valid toggling from 1 to 0.

Spec also says:
If a write to GITS.CTLR changes the Enabled field from 1 to 0, the
Interrupt Translation Space must ensure
that both:
• Any caches containing mapping data are made consistent with external
memory.
• GITS_CTLR.Quiescent == 0 until all caches are consistent with external
memory.

I aknowledge I don't really get at which moment we are supposed to void
the caches.

Nevertheless, personnally I think we can achieve the same reset
functionality with individual register writes.

Thanks

Eric






> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [RFC 0/3] vITS Reset

[Peter Maydell]

On 11 October 2017 at 17:06, Auger Eric <eric.auger@redhat.com> wrote:
> On 09/10/2017 20:17, Peter Maydell wrote:
>> It's not clear to me why we need a new KVM device attribute
>> for doing ITS reset. The usual approach for this is:
>>  * system reset causes QEMU's device model reset code
>>    to reset state structure values to whatever their
>>    reset value is
>>  * we write those values up into the kernel, which is
>>    sufficient to reset it
>>
>> What goes wrong with that in the case of the ITS?
>> In particular, if GITS_CTLR.Enabled is 0 then I think the
>> kernel should not be trying to read guest memory tables.
>
> We discussed that on the kernel thread and Christoffer seemed to prefer
> an IOTCL instead of individual register writes
> (https://www.spinics.net/lists/kvm-arm/msg27211.html)
>
> The IOCTL empties the ITS cached data (list of collection, list of
> devices and list of LPIs) while it is not obvious the reset of BASER<n>
> would mandate that. Eventually in my kernel series I also voided the
> list on BASER<n>.valid toggling from 1 to 0.

OK, I guess that makes sense -- in hardware, the device
has extra internal state (the cached stuff) which isn't
visible in its register interface. So we want to provide
an API which is morally equivalent to the hardware reset line
to tell the ITS to drop that internal state.

> Spec also says:
> If a write to GITS.CTLR changes the Enabled field from 1 to 0, the
> Interrupt Translation Space must ensure
> that both:
> • Any caches containing mapping data are made consistent with external
> memory.
> • GITS_CTLR.Quiescent == 0 until all caches are consistent with external
> memory.
>
> I aknowledge I don't really get at which moment we are supposed to void
> the caches.

This is the interface for a software controlled clean powerdown:
 * OS writes 0 to Enabled
 * ITS finishes anything it is currently in the middle of
 * ITS updates any info that it currently has only in its internal
   cache so that it is also reflected in the external memory
   data structures
 * ITS sets Quiescent to 1 to tell the OS it has finished this process
   (and may no longer access guest RAM after this point)
 * OS can now unmap the RAM that the ITS tables are in,
   tell the h/w to power off the GIC/ITS, etc

It's a multistep process where the ITS gets the opportunity to
write memory in the middle. I think in theory there's no obligation
to actually drop the cached data when you set Quiescent to 1,
provided that you validate that when the OS sets Enabled back to
1 all your cached data is still valid, because that's
indistinguishable from dropping the cache and then repopulating it.

For the unclean powerdown (yanking the cord out of the back of the
machine), which is what qemu's reset is, we don't give the ITS
the option to write memory, it just has to effectively drop
any internal data structures it was using. So it needs a KVM
API that's different from the register-access API.

thanks
-- PMM