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

2016-03-14 Thread Marcel Apfelbaum 

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

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

2016-03-14 Thread David Kiarie 
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...

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

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

2016-03-14 Thread Marcel Apfelbaum 

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

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

2016-03-13 Thread David Kiarie 
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"
 "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,
-- 
2.1.4