From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54760) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afPWX-0007hb-HS for qemu-devel@nongnu.org; Mon, 14 Mar 2016 06:14:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1afPWU-0004XM-3v for qemu-devel@nongnu.org; Mon, 14 Mar 2016 06:14:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37446) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afPWT-0004XD-Rs for qemu-devel@nongnu.org; Mon, 14 Mar 2016 06:14:50 -0400 References: <1457915099-11174-1-git-send-email-davidkiarie4@gmail.com> <1457915099-11174-4-git-send-email-davidkiarie4@gmail.com> <56E678FC.3060700@redhat.com> From: Marcel Apfelbaum Message-ID: <56E68F16.1090308@redhat.com> Date: Mon, 14 Mar 2016 12:14:46 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [V7 3/4] hw/core: Add AMD IOMMU to machine properties List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Kiarie Cc: Valentine Sinitsyn , Jan Kiszka , QEMU Developers , "Michael S. Tsirkin" On 03/14/2016 11:34 AM, David Kiarie wrote: > On Mon, Mar 14, 2016 at 11:40 AM, Marcel Apfelbaum 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 >>> --- >>> 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, >>> >>