Re: [Qemu-arm] [PATCH v9 14/14] hw/arm/virt: Handle iommu in 2.12 machine type

[Peter Maydell]

On 17 February 2018 at 18:46, Eric Auger <address@hidden> wrote:
> The new machine type exposes a new "iommu" virt machine option.
> The SMMUv3 IOMMU is instantiated using -machine virt,iommu=smmuv3.
>
> Signed-off-by: Eric Auger <address@hidden>
>
> ---
> v7 -> v8:
> - Revert to machine option, now dubbed "iommu", preparing for virtio
>   instantiation.
>
> v5 -> v6: machine 2_11
>
> Another alternative would be to use the -device option as
> done on x86. As the smmu is a sysbus device, we would need to
> use the platform bus framework.
> ---
>  hw/arm/virt.c         | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/virt.h |  1 +
>  2 files changed, 46 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index e9dca0d..607c7e1 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1547,6 +1547,34 @@ static void virt_set_gic_version(Object *obj, const 
> char *value, Error **errp)
>      }
>  }
>
> +static char *virt_get_iommu(Object *obj, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    switch (vms->iommu) {
> +    case VIRT_IOMMU_NONE:
> +        return g_strdup("none");
> +    case VIRT_IOMMU_SMMUV3:
> +        return g_strdup("smmuv3");
> +    default:
> +        return g_strdup("none");

Isn't this a "can't happen" case? If so, g_assert_not_reached().

> +    }
> +}
> +
> +static void virt_set_iommu(Object *obj, const char *value, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    if (!strcmp(value, "smmuv3")) {
> +        vms->iommu = VIRT_IOMMU_SMMUV3;
> +    } else if (!strcmp(value, "none")) {
> +        vms->iommu = VIRT_IOMMU_NONE;
> +    } else {
> +        error_setg(errp, "Invalid iommu value");
> +        error_append_hint(errp, "Valid value are none, smmuv3\n");

"Valid values are", and add trailing "." to the hint string (matches
virt_set_gic_version() phrasing.)

> +    }
> +}
> +
>  static CpuInstanceProperties
>  virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
>  {
> @@ -1679,6 +1707,19 @@ static void virt_2_12_instance_init(Object *obj)
>                                          NULL);
>      }
>
> +    if (vmc->no_iommu) {
> +        vms->iommu = VIRT_IOMMU_NONE;
> +    } else {
> +        /* Default disallows smmu instantiation */
> +        vms->iommu = VIRT_IOMMU_NONE;

If the default is "none" then you don't need the no_iommu field
in the VirtMachineClass. You can just have the property exist
on all virt machines, the way we do for "secure" and "virtualization".
It's only if we want to have the default for newer virt-n.nn versions
be something other than what the older machines had that we need to
have the VirtMachineClass field that lets us distinguish the older
and newer machine types.

> +        object_property_add_str(obj, "iommu", virt_get_iommu,
> +                                 virt_set_iommu, NULL);
> +        object_property_set_description(obj, "iommu",
> +                                        "Set the IOMMU model among "
> +                                        "none, smmuv3 (default none)",
> +                                        NULL);
> +    }
> +
>      vms->memmap = a15memmap;
>      vms->irqmap = a15irqmap;
>  }
> @@ -1698,8 +1739,12 @@ static void virt_2_11_instance_init(Object *obj)
>
>  static void virt_machine_2_11_options(MachineClass *mc)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
>      virt_machine_2_12_options(mc);
>      SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_11);
> +
> +    vmc->no_iommu = true;
>  }
>  DEFINE_VIRT_MACHINE(2, 11)
>
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 13d3724..3a92fc3 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -92,6 +92,7 @@ typedef struct {
>      bool disallow_affinity_adjustment;
>      bool no_its;
>      bool no_pmu;
> +    bool no_iommu;
>      bool claim_edge_triggered_timers;
>  } VirtMachineClass;
>
> --
> 2.5.5
>

thanks
-- PMM