From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45774) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLCzJ-0003Iz-5r for qemu-devel@nongnu.org; Mon, 18 Jan 2016 11:49:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLCzE-0000Ai-46 for qemu-devel@nongnu.org; Mon, 18 Jan 2016 11:49:05 -0500 Received: from mail-wm0-x22c.google.com ([2a00:1450:400c:c09::22c]:33403) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLCzD-0000Ac-OQ for qemu-devel@nongnu.org; Mon, 18 Jan 2016 11:49:00 -0500 Received: by mail-wm0-x22c.google.com with SMTP id 123so59836441wmz.0 for ; Mon, 18 Jan 2016 08:48:59 -0800 (PST) References: <1453130745-25793-1-git-send-email-davidkiarie4@gmail.com> <1453130745-25793-3-git-send-email-davidkiarie4@gmail.com> <569D1120.7020202@redhat.com> From: David Kiarie Message-ID: <569D176B.9010904@gmail.com> Date: Mon, 18 Jan 2016 19:48:43 +0300 MIME-Version: 1.0 In-Reply-To: <569D1120.7020202@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [V4 2/4] hw/core: Add AMD IO MMU to machine properties List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum , qemu-devel@nongnu.org Cc: valentine.sinitsyn@gmail.com, jan.kiszka@web.de, crosthwaitepeter@gmail.com, mst@redhat.com 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 >> --- >> 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. > > 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"); >> >