Re: [Qemu-devel] [PATCH v2] hw/arm/virt: Add a user option to disallow ITS instantiation

[Peter Maydell]

On 17 February 2017 at 14:25, Eric Auger <address@hidden> 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 <address@hidden>
>
> ---
>
> 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