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

[David Kiarie]

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

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