In 2.9 ITS will block save/restore and migration use cases. As
such let's introduce a user option that disallows its instantiation
along with the GICv3. With no-its option turned true, migration will
be possible, obviously at the expense of MSI support (with GICv3).

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

---

v1 -> v2: fix omitted coma in object_property_set_description

With this patch the option also is added in virt 2.8. I don't know
if it is acceptable.
---
 hw/arm/virt.c         | 28 +++++++++++++++++++++++++++-
 include/hw/arm/virt.h |  1 +
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f3440f2..c08deb0 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -605,7 +605,7 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
 
     fdt_add_gic_node(vms);
 
-    if (type == 3 && !vmc->no_its) {
+    if (type == 3 && !vmc->no_its && !vms->no_its) {
         create_its(vms, gicdev);
     } else if (type == 2) {
         create_v2m(vms, pic);
@@ -1480,6 +1480,21 @@ static void virt_set_highmem(Object *obj, bool value, Error **errp)
     vms->highmem = value;
 }
 
+static bool virt_get_no_its(Object *obj, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    return vms->no_its;
+}
+
+static void virt_set_no_its(Object *obj, bool value, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    vms->no_its = value;
+}
+
+
 static char *virt_get_gic_version(Object *obj, Error **errp)
 {
     VirtMachineState *vms = VIRT_MACHINE(obj);
@@ -1540,6 +1555,7 @@ type_init(machvirt_machine_init);
 static void virt_2_9_instance_init(Object *obj)
 {
     VirtMachineState *vms = VIRT_MACHINE(obj);
+    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
 
     /* EL3 is disabled by default on virt: this makes us consistent
      * between KVM and TCG for this board, and it also allows us to
@@ -1579,6 +1595,16 @@ static void virt_2_9_instance_init(Object *obj)
                                     "Set GIC version. "
                                     "Valid values are 2, 3 and host", NULL);
 
+    /* Default allows ITS instantiation */
+    if (!vmc->no_its) {
+        object_property_add_bool(obj, "no-its", virt_get_no_its,
+                                 virt_set_no_its, NULL);
+        vms->no_its = false;
+        object_property_set_description(obj, "no-its",
+                                        "Disallow the ITS instantiation",
+                                        NULL);
+    }
+
     vms->memmap = a15memmap;
     vms->irqmap = a15irqmap;
 }
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 58ce74e..5e73be9 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -93,6 +93,7 @@ typedef struct {
     FWCfgState *fw_cfg;
     bool secure;
     bool highmem;
+    bool no_its;
     bool virt;
     int32_t gic_version;
     struct arm_boot_info bootinfo;
-- 
2.5.5

On 17 February 2017 at 14:25, Eric Auger <eric.auger@redhat.com> wrote:
> In 2.9 ITS will block save/restore and migration use cases. As
> such let's introduce a user option that disallows its instantiation
> along with the GICv3. With no-its option turned true, migration will
> be possible, obviously at the expense of MSI support (with GICv3).
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>
> v1 -> v2: fix omitted coma in object_property_set_description
>
> With this patch the option also is added in virt 2.8. I don't know
> if it is acceptable.

I think that's OK.

> ---
>  hw/arm/virt.c         | 28 +++++++++++++++++++++++++++-
>  include/hw/arm/virt.h |  1 +
>  2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index f3440f2..c08deb0 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -605,7 +605,7 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>
>      fdt_add_gic_node(vms);
>
> -    if (type == 3 && !vmc->no_its) {
> +    if (type == 3 && !vmc->no_its && !vms->no_its) {

Making the board creation code check both a vmc field and
a vms field seems a bit awkward...

> +    /* Default allows ITS instantiation */
> +    if (!vmc->no_its) {
> +        object_property_add_bool(obj, "no-its", virt_get_no_its,
> +                                 virt_set_no_its, NULL);
> +        vms->no_its = false;
> +        object_property_set_description(obj, "no-its",
> +                                        "Disallow the ITS instantiation",
> +                                        NULL);

Can we have the property be "its" with true-for-enabled,
rather than "no-its", please? (See later for rationale.)

> +    }

...if we have this code have an else {} clause we can
make it set the vms field appropriately, and then the
board creation code only needs to check the vms field.

> +
>      vms->memmap = a15memmap;
>      vms->irqmap = a15irqmap;
>  }
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 58ce74e..5e73be9 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -93,6 +93,7 @@ typedef struct {
>      FWCfgState *fw_cfg;
>      bool secure;
>      bool highmem;
> +    bool no_its;

Can we make this "bool its" where true means "has an ITS", please?
The fields in VirtMachineClass are all "no_foo" because we require
the "false" case to be the "new modern behaviour" and the "true"
case to be "legacy like old boards" for convenience of the
virt_machine_*_options() function chain, but we don't need to
do this for VirtMachineState fields. (So VMS 'secure', 'highmem'
and 'virt' are all true-for-feature-enabled.)
The user-facing property should also be "its" for the same reason.

An alternative to having the property be always default true
and only existing on newer boards would be to have the VMC field
be "no_default_its", expose the "its" property in all cases and
just have its default value be different for old virt-* machine
versions. I have no opinion here about which would be better.

>      bool virt;
>      int32_t gic_version;
>      struct arm_boot_info bootinfo;

thanks
-- PMM