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

[Marcel Apfelbaum <marcel@redhat.com>]

On 03/14/2016 11:34 AM, David Kiarie wrote:
> On Mon, Mar 14, 2016 at 11:40 AM, Marcel Apfelbaum <marcel@redhat.com> 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 <davidkiarie4@gmail.com>
>>> ---
>>>    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.
>>> +@item 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,
>>>
>>