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

2016-05-01 Thread Marcel Apfelbaum 

On 04/30/2016 01:42 AM, David Kiarie wrote:

Added an enum, subject to review, to machine properties which
it used to override iommu emulated from Intel to AMD.

Signed-off-by: David Kiarie 
---
  hw/core/machine.c | 33 ++---
  include/hw/boards.h   |  1 +
  include/hw/i386/intel_iommu.h |  1 +
  qemu-options.hx   |  7 +--
  util/qemu-config.c|  8 ++--
  5 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 6dbbc85..ff830f0 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -15,6 +15,8 @@
  #include "qapi/error.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"
@@ -300,6 +302,27 @@ static void machine_set_iommu(Object *obj, bool value, 
Error **errp)
  ms->iommu = value;
  }

+static void machine_set_iommu_override(Object *obj, const char *value,
+   Error **errp)
+{
+MachineState *ms = MACHINE(obj);
+Error *err = NULL;
+
+ms->iommu_type = TYPE_INTEL;
+/* ensure a valid iommu type */
+if (g_strcmp0(value, AMD_IOMMU_STR) == 0) {
+} else if(g_strcmp0(value, INTEL_IOMMU_STR) == 0) {
+} else {
+error_setg(errp, "Invalid IOMMU type %s", value);
+error_propagate(errp, err);


You don't need 'error_propagate' and the err pointer(is not used),
error_setg does the job right.

Also you declared the IommuType as enum  (thanks for that, I really like it!) :
   typedef enum IommuType {
  TYPE_AMD,
  TYPE_INTEL,
  TYPE_NONE
  } IommuType;
What happens if the user does not set this property?
ms->iommu_type will default to AMD and -machine iommu=on
will enable AMD by default.
You can choose between swicthing the values in enum
or add  ms->iommu_type = TYPE_INTEL; to machine_init.

While at it, I would re-write it in a more standard way:

if (g_strcmp0(value, INTEL_IOMMU_STR) == 0) {
ms->iommu_type = TYPE_INTEL;
} else if(g_strcmp0(value, AMD_IOMMU_STR) == 0) {
ms->iommu_type = TYPE_AMD;
} else {
error_setg(errp, "Invalid IOMMU type %s", value);
}


Other than that the patch is ready, thank you for taking the time
to address the comments,
Marcel


+return;
+}
+
+if ((g_strcmp0(value, AMD_IOMMU_STR) == 0)) {
+ms->iommu_type = TYPE_AMD;
+}
+}
+
  static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp)
  {
  MachineState *ms = MACHINE(obj);
@@ -473,10 +496,14 @@ 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_str(obj, "x-iommu-type",
+NULL, machine_set_iommu_override, NULL);
+object_property_set_description(obj, "x-iommu-type",
+"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 dbe6745..5b7eeda 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -158,6 +158,7 @@ struct MachineState {
  bool igd_gfx_passthru;
  char *firmware;
  bool iommu;
+IommuType iommu_type;
  bool suppress_vmdesc;
  bool enforce_config_section;

diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index b024ffa..7e511e1 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -24,6 +24,7 @@
  #include "hw/qdev.h"
  #include "sysemu/dma.h"

+#define INTEL_IOMMU_STR "intel"
  #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
  #define INTEL_IOMMU_DEVICE(obj) \
   OBJECT_CHECK(IntelIOMMUState, (obj), TYPE_INTEL_IOMMU_DEVICE)
diff --git a/qemu-options.hx b/qemu-options.hx
index 6106520..81217d3 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-

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

2016-04-29 Thread David Kiarie 
Added an enum, subject to review, to machine properties which
it used to override iommu emulated from Intel to AMD.

Signed-off-by: David Kiarie 
---
 hw/core/machine.c | 33 ++---
 include/hw/boards.h   |  1 +
 include/hw/i386/intel_iommu.h |  1 +
 qemu-options.hx   |  7 +--
 util/qemu-config.c|  8 ++--
 5 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 6dbbc85..ff830f0 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -15,6 +15,8 @@
 #include "qapi/error.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"
@@ -300,6 +302,27 @@ static void machine_set_iommu(Object *obj, bool value, 
Error **errp)
 ms->iommu = value;
 }
 
+static void machine_set_iommu_override(Object *obj, const char *value,
+   Error **errp)
+{
+MachineState *ms = MACHINE(obj);
+Error *err = NULL;
+
+ms->iommu_type = TYPE_INTEL;
+/* ensure a valid iommu type */
+if (g_strcmp0(value, AMD_IOMMU_STR) == 0) {
+} else if(g_strcmp0(value, INTEL_IOMMU_STR) == 0) {
+} else {
+error_setg(errp, "Invalid IOMMU type %s", value);
+error_propagate(errp, err);
+return;
+}
+
+if ((g_strcmp0(value, AMD_IOMMU_STR) == 0)) {
+ms->iommu_type = TYPE_AMD;
+}
+}
+
 static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp)
 {
 MachineState *ms = MACHINE(obj);
@@ -473,10 +496,14 @@ 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_str(obj, "x-iommu-type",
+NULL, machine_set_iommu_override, NULL);
+object_property_set_description(obj, "x-iommu-type",
+"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 dbe6745..5b7eeda 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -158,6 +158,7 @@ struct MachineState {
 bool igd_gfx_passthru;
 char *firmware;
 bool iommu;
+IommuType iommu_type;
 bool suppress_vmdesc;
 bool enforce_config_section;
 
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index b024ffa..7e511e1 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -24,6 +24,7 @@
 #include "hw/qdev.h"
 #include "sysemu/dma.h"
 
+#define INTEL_IOMMU_STR "intel"
 #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
 #define INTEL_IOMMU_DEVICE(obj) \
  OBJECT_CHECK(IntelIOMMUState, (obj), TYPE_INTEL_IOMMU_DEVICE)
diff --git a/qemu-options.hx b/qemu-options.hx
index 6106520..81217d3 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"
+"x-iommu-type=amd|intel overrides emulated IOMMU to AMD 
IOMMU (default: intel)\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 x-iommu-type=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 w

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

2016-04-24 Thread Peter Xu 
On Mon, Apr 25, 2016 at 01:12:56AM +0300, David Kiarie wrote:
> Added a bool, subject to review to machine properties which
> it used to override iommu emulated from Intel to AMD.
> 
> Signed-off-by: David Kiarie 
> ---
>  hw/core/machine.c | 32 +---
>  include/hw/boards.h   |  1 +
>  include/hw/i386/intel_iommu.h |  1 +
>  qemu-options.hx   |  7 +--
>  util/qemu-config.c|  8 ++--
>  5 files changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 6dbbc85..792641b 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -15,6 +15,8 @@
>  #include "qapi/error.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"
> @@ -300,6 +302,26 @@ static void machine_set_iommu(Object *obj, bool value, 
> Error **errp)
>  ms->iommu = value;
>  }
>  
> +static void machine_set_iommu_override(Object *obj, const char *value,
> +   Error **errp)
> +{
> +MachineState *ms = MACHINE(obj);
> +Error *err = NULL;
> +
> +ms->x_iommu_type = false;
> +/* ensure a valid iommu type */
> +if (g_strcmp0(value, AMD_IOMMU_STR) &&
> +g_strcmp0(value, INTEL_IOMMU_STR)) {
> +error_setg(errp, "Invalid IOMMU type %s", value);
> +error_propagate(errp, err);
> +return;
> +}
> +
> +if ((g_strcmp0(value, AMD_IOMMU_STR) == 0)) {
> +ms->x_iommu_type = true;
> +}

Would it be clearer to use:

if (g_strcmp0(value, AMD_IOMMU_STR) == 0) {
...
} else if (g_strcmp0(value, INTEL_IOMMU_STR) == 0) {
...
} else {
error...
}

?

Thanks.

-- peterx

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

2016-04-24 Thread David Kiarie 
Added a bool, subject to review to machine properties which
it used to override iommu emulated from Intel to AMD.

Signed-off-by: David Kiarie 
---
 hw/core/machine.c | 32 +---
 include/hw/boards.h   |  1 +
 include/hw/i386/intel_iommu.h |  1 +
 qemu-options.hx   |  7 +--
 util/qemu-config.c|  8 ++--
 5 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 6dbbc85..792641b 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -15,6 +15,8 @@
 #include "qapi/error.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"
@@ -300,6 +302,26 @@ static void machine_set_iommu(Object *obj, bool value, 
Error **errp)
 ms->iommu = value;
 }
 
+static void machine_set_iommu_override(Object *obj, const char *value,
+   Error **errp)
+{
+MachineState *ms = MACHINE(obj);
+Error *err = NULL;
+
+ms->x_iommu_type = false;
+/* ensure a valid iommu type */
+if (g_strcmp0(value, AMD_IOMMU_STR) &&
+g_strcmp0(value, INTEL_IOMMU_STR)) {
+error_setg(errp, "Invalid IOMMU type %s", value);
+error_propagate(errp, err);
+return;
+}
+
+if ((g_strcmp0(value, AMD_IOMMU_STR) == 0)) {
+ms->x_iommu_type = true;
+}
+}
+
 static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp)
 {
 MachineState *ms = MACHINE(obj);
@@ -473,10 +495,14 @@ 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_str(obj, "x-iommu-type",
+NULL, machine_set_iommu_override, NULL);
+object_property_set_description(obj, "x-iommu-type",
+"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 8d4fe56..5a12a1c 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -152,6 +152,7 @@ struct MachineState {
 bool igd_gfx_passthru;
 char *firmware;
 bool iommu;
+bool x_iommu_type;
 bool suppress_vmdesc;
 bool enforce_config_section;
 
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index b024ffa..7e511e1 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -24,6 +24,7 @@
 #include "hw/qdev.h"
 #include "sysemu/dma.h"
 
+#define INTEL_IOMMU_STR "intel"
 #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
 #define INTEL_IOMMU_DEVICE(obj) \
  OBJECT_CHECK(IntelIOMMUState, (obj), TYPE_INTEL_IOMMU_DEVICE)
diff --git a/qemu-options.hx b/qemu-options.hx
index 6106520..81217d3 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"
+"x-iommu-type=amd|intel overrides emulated IOMMU to AMD 
IOMMU (default: intel)\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 x-iommu-type=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. Th