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

[Marcel Apfelbaum]

On 03/14/2016 11:34 AM, David Kiarie wrote:
> On Mon, Mar 14, 2016 at 11:40 AM, Marcel Apfelbaum <address@hidden> wrote:
> > 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
>
> I thought the idea was to get rid of the "ugly" option parsing which
> will remain if I have it as above...

Hi,

Well, we can limit the parsing to one place, when the option is set in machine.
Let's say the machine has a string property "x-iommu-type" (with only "set" property 
implemented, no "get")

    object_property_add_str(obj, "x-iommu-type",
                            NULL, machine_set_iommu_type, NULL);

You can parse and validate the type *only inside* machine_set_iommu_type
and use an enum as a machine field. The "intel"/"amd" strings and parsing would
be isolated to machine.c file which is reasonable.

You defined already the enum in patch 2/4: "iommu_type". Maybe you can rename 
it to MachineIommuType
or something and add it to include/hw/boards.h.

From this point forward you can use only the enum value in code.

Thanks,
Marcel


> > 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,