Re: [Qemu-devel] [V7 3/4] hw/core: Add AMD IOMMU to machine properties

[Marcel Apfelbaum]

On 03/14/2016 02:24 AM, David Kiarie wrote:
> Add IOMMU as a string to machine properties which is
> used to control whether and the type of IOMMU to emulate
>
> Signed-off-by: David Kiarie <address@hidden>
> ---
>   hw/core/machine.c   | 27 ++++++++++++++++++++++++---
>   include/hw/boards.h |  1 +
>   qemu-options.hx     |  7 +++++--
>   util/qemu-config.c  |  4 ++--
>   4 files changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index a8c4680..ce23b3d 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -14,6 +14,8 @@
>   #include "hw/boards.h"
>   #include "qapi-visit.h"
>   #include "qapi/visitor.h"
> +#include "hw/i386/amd_iommu.h"
> +#include "hw/i386/intel_iommu.h"
>   #include "hw/sysbus.h"
>   #include "sysemu/sysemu.h"
>   #include "qemu/error-report.h"
> @@ -298,6 +300,20 @@ static void machine_set_iommu(Object *obj, bool value, 
> Error **errp)
>       ms->iommu = value;
>   }
>
> +static bool machine_get_amd_iommu_override(Object *obj, Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    return ms->amd_iommu_type;
> +}
> +
> +static void machine_set_amd_iommu_override(Object *obj, bool value, Error 
> **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    ms->amd_iommu_type = value;
> +}
> +
>   static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp)
>   {
>       MachineState *ms = MACHINE(obj);
> @@ -471,10 +487,15 @@ static void machine_initfn(Object *obj)
>                                       "Firmware image",
>                                       NULL);
>       object_property_add_bool(obj, "iommu",
> -                             machine_get_iommu,
> -                             machine_set_iommu, NULL);
> +                            machine_get_iommu, machine_set_iommu, NULL);
>       object_property_set_description(obj, "iommu",
> -                                    "Set on/off to enable/disable Intel IOMMU 
> (VT-d)",
> +                                    "Set on to enable IOMMU emulation",
> +                                    NULL);
> +    object_property_add_bool(obj, "amd-iommu",
> +                            machine_get_amd_iommu_override,
> +                            machine_set_amd_iommu_override, NULL);
> +    object_property_set_description(obj, "amd-iommu",
> +                                    "Set on to override emulated IOMMU to AMD 
> IOMMU",
>                                       NULL);
>       object_property_add_bool(obj, "suppress-vmdesc",
>                                machine_get_suppress_vmdesc,
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index b5d7eae..5bdd0bb 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -126,6 +126,7 @@ struct MachineState {
>       bool igd_gfx_passthru;
>       char *firmware;
>       bool iommu;
> +    bool amd_iommu_type;
>       bool suppress_vmdesc;
>       bool enforce_config_section;
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 0cf7bb9..de3f02e 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -38,7 +38,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>       "                kvm_shadow_mem=size of KVM shadow MMU\n"
>       "                dump-guest-core=on|off include guest memory in a core dump 
> (default=on)\n"
>       "                mem-merge=on|off controls memory merge support (default: 
> on)\n"
> -    "                iommu=on|off controls emulated Intel IOMMU (VT-d) support 
> (default=off)\n"
> +    "                iommu=on|off controls emulated IOMMU support(default: 
> off)\n"
> +    "                amd-iommu=on|off overrides emulated IOMMU to AMD IOMMU 
> (default: off)\n"

Hi David,

I think this is a step backward from the last series (in my opinion).
Summarizing what Jan and Michael said, it should be:
  iommu=on|off controls emulated IOMMU support(default: off)
  x-iommu-type=intel|amd controls the IOMMU emulation type(default: intel)

In this way:
  -machine iommu=on => it works like before
  -machine iommu=on,x-iommu-type=intel => ask for intel emulation
  -machine iommu=off,x-iommu-type=amd => ask for amd emulation

The "x-" prefix I suppose is for "tech preview" property, right Michael?

Of course -device iommu,type="intel|amd" would be preferred, but I understand
is not in the scope of this series.

Thanks,
Marcel





>       "                igd-passthru=on|off controls IGD GFX passthrough support 
> (default=off)\n"
>       "                aes-key-wrap=on|off controls support for AES key wrapping 
> (default=on)\n"
>       "                dea-key-wrap=on|off controls support for DEA key wrapping 
> (default=on)\n"
> @@ -74,7 +75,9 @@ Enables or disables memory merge support. This feature, when 
> supported by
>   the host, de-duplicates identical memory pages among VMs instances
>   (enabled by default).
>   @item iommu=on|off
> -Enables or disables emulated Intel IOMMU (VT-d) support. The default is off.
> +Enables and disables IOMMU emulation. The default is off.
> address@hidden amd-iommu=on|off
> +Overrides emulated IOMMU from AMD IOMMU. By default Intel IOMMU is emulated.
>   @item aes-key-wrap=on|off
>   Enables or disables AES key wrapping support on s390-ccw hosts. This feature
>   controls whether AES wrapping keys will be created to allow
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index fb97307..f1b5a3b 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -213,8 +213,8 @@ static QemuOptsList machine_opts = {
>               .help = "firmware image",
>           },{
>               .name = "iommu",
> -            .type = QEMU_OPT_BOOL,
> -            .help = "Set on/off to enable/disable Intel IOMMU (VT-d)",
> +            .type =  QEMU_OPT_STRING,
> +            .help = "Enables IOMMU and sets the emulated type",
>           },{
>               .name = "suppress-vmdesc",
>               .type = QEMU_OPT_BOOL,