Re: [Qemu-devel] [V4 2/4] hw/core: Add AMD IO MMU to machine properties

[David Kiarie <davidkiarie4@gmail.com>]

On 1/18/2016 8:27 PM, Marcel Apfelbaum wrote:
> On 01/18/2016 06:48 PM, David Kiarie wrote:
>>
>>
>> On 1/18/2016 7:21 PM, Marcel Apfelbaum wrote:
>>> On 01/18/2016 05:25 PM, David Kiarie wrote:
>>>> Add IO MMU as a string to machine properties which
>>>> is used to control whether and the type of IO MMU
>>>> to emulate
>>>>
>>>> Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
>>>> ---
>>>>   hw/core/machine.c             | 17 +++++++++--------
>>>>   include/hw/boards.h           |  3 ++-
>>>>   include/hw/i386/intel_iommu.h |  1 +
>>>>   qemu-options.hx               |  6 +++---
>>>>   util/qemu-config.c            |  4 ++--
>>>>   vl.c                          |  8 ++++++++
>>>>   6 files changed, 25 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>>> index c46ddc7..cb309aa 100644
>>>> --- a/hw/core/machine.c
>>>> +++ b/hw/core/machine.c
>>>> @@ -283,18 +283,19 @@ 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);
>>>>
>>>> -    ms->iommu = value;
>>>> +    g_free(ms->iommu);
>>>> +    ms->iommu = g_strdup(value);
>>>>   }
>>>>
>>>>   static void machine_set_suppress_vmdesc(Object *obj, bool value, 
>>>> Error **errp)
>>>> @@ -454,11 +455,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,
>>>> @@ -484,6 +484,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/include/hw/i386/intel_iommu.h 
>>>> b/include/hw/i386/intel_iommu.h
>>>> index 5dbadb7..0b32bd6 100644
>>>> --- a/include/hw/i386/intel_iommu.h
>>>> +++ b/include/hw/i386/intel_iommu.h
>>>> @@ -27,6 +27,7 @@
>>>>   #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
>>>>   #define INTEL_IOMMU_DEVICE(obj) \
>>>>        OBJECT_CHECK(IntelIOMMUState, (obj), TYPE_INTEL_IOMMU_DEVICE)
>>>> +#define INTEL_IOMMU_STR "intel"
>>>>
>>>>   /* DMAR Hardware Unit Definition address (IOMMU unit) */
>>>>   #define Q35_HOST_BRIDGE_IOMMU_ADDR  0xfed90000ULL
>>>> diff --git a/qemu-options.hx b/qemu-options.hx
>>>> index 215d00d..ac327c8 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 IO MMU (default: off)\n"
>>>>       "                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 IO MMU. 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 687fd34..f79b98c 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 IO MMU and sets the emulated type",
>>>>           },{
>>>>               .name = "suppress-vmdesc",
>>>>               .type = QEMU_OPT_BOOL,
>>>> diff --git a/vl.c b/vl.c
>>>> index b7a083e..4e39caa 100644
>>>> --- a/vl.c
>>>> +++ b/vl.c
>>>> @@ -65,6 +65,8 @@ int main(int argc, char **argv)
>>>>   #include "sysemu/accel.h"
>>>>   #include "hw/usb.h"
>>>>   #include "hw/i386/pc.h"
>>>> +#include "hw/i386/amd_iommu.h"
>>>> +#include "hw/i386/intel_iommu.h"
>>>>   #include "hw/isa/isa.h"
>>>>   #include "hw/bt.h"
>>>>   #include "sysemu/watchdog.h"
>>>> @@ -3677,6 +3679,12 @@ int main(int argc, char **argv, char **envp)
>>>>                   if (!opts) {
>>>>                       exit(1);
>>>>                   }
>>>> +                const char *iommu = qemu_opt_get(opts, "iommu");
>>>> +                if (iommu && !(g_strcmp0(iommu, AMD_IOMMU_STR) ||
>>>> +                    g_strcmp0(iommu, INTEL_IOMMU_STR))) {
>>>
>>> Hi,
>>>
>>> I am sorry for bugging you, please don't re-post before
>>> you get more meaningful reviews, that being said:
>>>
>>> I don't know if the above will work as
>>>    (g_strcmp0(iommu, AMD_IOMMU_STR) || g_strcmp0(iommu, 
>>> INTEL_IOMMU_STR))
>>> will always return 1. Right :) ?
>>
>> Yep, always true.
>>
>> Above fails though if the string doesn't match any 'iommu' type string.
>
> If you agree that the above is always true, !() is always false =>
> if statement always false, meaning you never get the error message.

You right :)

Queued for a fix.

>
> Thanks,
> Marcel
>
>>
>>>
>>> However, you should put this check in machine_set_iommu. You fill in 
>>> the errp
>>> and you will get a nice error message when the machine properties 
>>> are parsed.
>>>
>>> Thanks,
>>> Marcel
>>>
>>>
>>>
>>>> +                    fprintf(stderr, "Invalid IO MMU type %s\n", 
>>>> iommu);
>>>> +                    exit(1);
>>>> +                }
>>>>                   break;
>>>>                case QEMU_OPTION_no_kvm:
>>>>                   olist = qemu_find_opts("machine");
>>>>
>>>
>>
>