From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37409) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLE47-0005JH-2Z for qemu-devel@nongnu.org; Mon, 18 Jan 2016 12:58:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLE43-00018W-05 for qemu-devel@nongnu.org; Mon, 18 Jan 2016 12:58:06 -0500 Received: from mail-wm0-x230.google.com ([2a00:1450:400c:c09::230]:35182) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLE42-00018Q-M5 for qemu-devel@nongnu.org; Mon, 18 Jan 2016 12:58:02 -0500 Received: by mail-wm0-x230.google.com with SMTP id r129so61140533wmr.0 for ; Mon, 18 Jan 2016 09:58:02 -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> <569D176B.9010904@gmail.com> <569D209A.80502@redhat.com> From: David Kiarie Message-ID: <569D2799.3050407@gmail.com> Date: Mon, 18 Jan 2016 20:57:45 +0300 MIME-Version: 1.0 In-Reply-To: <569D209A.80502@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 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 >>>> --- >>>> 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"); >>>> >>> >> >