Re: [Qemu-devel] [PATCH 1/4] hw/core: Add iommu to machine properties

[Marcel Apfelbaum <marcel.apfelbaum@gmail.com>]

On 10/09/2015 05:53 AM, David Kiarie wrote:
> From: David <davidkiarie4@gmail.com>
>
>   Add iommu to machine properties in preparation of introducing
>   AMD IOMMU
>
> Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
> ---
>   hw/core/machine.c   | 25 +++++++++++++++++++++++++
>   include/hw/boards.h |  2 ++
>   2 files changed, 27 insertions(+)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 51ed6b2..8cc7461 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -269,6 +269,20 @@ static void machine_set_iommu(Object *obj, bool value, Error **errp)
>       ms->iommu = value;
>   }
>
> +static bool machine_get_amd_iommu(Object *obj, Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    return ms->amd_iommu;
> +}
> +
> +static void machine_set_amd_iommu(Object *obj, bool value, Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    ms->amd_iommu = value;
> +}
> +
>   static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp)
>   {
>       MachineState *ms = MACHINE(obj);
> @@ -420,6 +434,12 @@ static void machine_initfn(Object *obj)
>       object_property_set_description(obj, "iommu",
>                                       "Set on/off to enable/disable Intel IOMMU (VT-d)",
>                                       NULL);
> +    object_property_add_bool(obj, "amd-iommu",
> +                             machine_get_amd_iommu,
> +                             machine_set_amd_iommu, NULL);
> +    object_property_set_description(obj, "amd-iommu",
> +                                    "Set on/off to enable/disable AMD-Vi",
> +                                    NULL);
>       object_property_add_bool(obj, "suppress-vmdesc",
>                                machine_get_suppress_vmdesc,
>                                machine_set_suppress_vmdesc, NULL);
> @@ -456,6 +476,11 @@ bool machine_iommu(MachineState *machine)
>       return machine->iommu;
>   }
>
> +bool machine_amd_iommu(MachineState *machine)
> +{
> +    return machine->amd_iommu;
> +}
> +
>   bool machine_kernel_irqchip_allowed(MachineState *machine)
>   {
>       return machine->kernel_irqchip_allowed;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 566a5ca..c8424f7 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -54,6 +54,7 @@ extern MachineState *current_machine;
>
>   bool machine_usb(MachineState *machine);
>   bool machine_iommu(MachineState *machine);
> +bool machine_amd_iommu(MachineState *machine);
>   bool machine_kernel_irqchip_allowed(MachineState *machine);
>   bool machine_kernel_irqchip_required(MachineState *machine);
>   int machine_kvm_shadow_mem(MachineState *machine);
> @@ -140,6 +141,7 @@ struct MachineState {
>       bool igd_gfx_passthru;
>       char *firmware;
>       bool iommu;
> +    bool amd_iommu;
>       bool suppress_vmdesc;
>
>       ram_addr_t ram_size;
>

Hi,

I think we should add the property to PC_MACHINE class because it make sense only
for PC and Q35 machines (right?).
MACHINE is the base class for all the architectures.

On the same note, "iommu" refers to intel_iommu and maybe we should
move it also to the PC_MACHINE.
I understand that it is not in the scope of your series, we can add a patch later.

Another possibility would be to keep the same "iommu" property, but change it from boolean
to off|intel|amd|... . This will break backward compatibility, on the other hand having
iommu referring to Intel only when we have  multiple iommu implementations is ugly IMHO.

Thanks,
Marcel