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

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Feb 21, 2016 at 09:10:58PM +0300, 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 <davidkiarie4@gmail.com>
> ---
>  hw/core/machine.c   | 28 ++++++++++++++++++++--------
>  include/hw/boards.h |  3 ++-
>  qemu-options.hx     |  6 +++---
>  util/qemu-config.c  |  4 ++--
>  4 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 6d1a0d8..001ace9 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"
> @@ -284,18 +286,28 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp)
>      ms->firmware = g_strdup(value);
>  }
>  
> -static bool machine_get_iommu(Object *obj, Error **errp)
> +static char *machine_get_iommu(Object *obj, Error **errp)
>  {
>      MachineState *ms = MACHINE(obj);
>  
> -    return ms->iommu;
> +    return g_strdup(ms->iommu);
>  }
>  
> -static void machine_set_iommu(Object *obj, bool value, Error **errp)
> +static void machine_set_iommu(Object *obj, const char *value, Error **errp)
>  {
>      MachineState *ms = MACHINE(obj);
> +    Error *err = NULL;
> +
> +    g_free(ms->iommu);
> +
> +    if (g_strcmp0(value, AMD_IOMMU_STR) &&
> +            g_strcmp0(value, INTEL_IOMMU_STR)) {
> +        error_setg(errp, "Invalid IOMMU type %s", value);
> +        error_propagate(errp, err);
> +        return;
> +    }
>  
> -    ms->iommu = value;
> +    ms->iommu = g_strdup(value);
>  }
>  
>  static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp)
> @@ -455,11 +467,10 @@ static void machine_initfn(Object *obj)
>      object_property_set_description(obj, "firmware",
>                                      "Firmware image",
>                                      NULL);
> -    object_property_add_bool(obj, "iommu",
> -                             machine_get_iommu,
> -                             machine_set_iommu, NULL);
> +    object_property_add_str(obj, "iommu",
> +                            machine_get_iommu, machine_set_iommu, NULL);
>      object_property_set_description(obj, "iommu",
> -                                    "Set on/off to enable/disable Intel IOMMU (VT-d)",
> +                                    "IOMMU list",
>                                      NULL);
>      object_property_add_bool(obj, "suppress-vmdesc",
>                               machine_get_suppress_vmdesc,
> @@ -485,6 +496,7 @@ static void machine_finalize(Object *obj)
>      g_free(ms->dumpdtb);
>      g_free(ms->dt_compatible);
>      g_free(ms->firmware);
> +    g_free(ms->iommu);
>  }
>  
>  bool machine_usb(MachineState *machine)
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 0f30959..b119245 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -36,6 +36,7 @@ bool machine_usb(MachineState *machine);
>  bool machine_kernel_irqchip_allowed(MachineState *machine);
>  bool machine_kernel_irqchip_required(MachineState *machine);
>  bool machine_kernel_irqchip_split(MachineState *machine);
> +bool machine_amd_iommu(MachineState *machine);
>  int machine_kvm_shadow_mem(MachineState *machine);
>  int machine_phandle_start(MachineState *machine);
>  bool machine_dump_guest_core(MachineState *machine);
> @@ -126,7 +127,7 @@ struct MachineState {
>      bool usb_disabled;
>      bool igd_gfx_passthru;
>      char *firmware;
> -    bool iommu;
> +    char *iommu;
>      bool suppress_vmdesc;
>  
>      ram_addr_t ram_size;
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 2f0465e..dad160f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -38,7 +38,7 @@ 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=amd|intel enables and selects the emulated IOMMU (default: off)\n"

Besides what Jan said (it says default:off so off should be
available as an option), "on" should be available as an option
too, to
1. make qemu select an automatic model if user does not care
2. keep compatibility with existing users

Given this code is still kind of rough, how about a separate
x-iommu-type property to override type from intel to AMD?


>      "                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"
> @@ -72,8 +72,8 @@ Include guest memory in a core dump. The default is on.
>  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.
> +@item iommu=intel|amd
> +Enables and selects the emulated IOMMU. The default is off.
>  @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,
> -- 
> 2.1.4