[Qemu-devel] [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[Qemu-devel] [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[Lan Tianyu]

This patchset is to add Xen vIOMMU device model and handle
irq remapping stuffs. Xen vIOMMU emulation is in the Xen hypervisor
and the new device module in Qemu works as hypercall wrappers to
create and destroy vIOMMU in hypervisor.

Xen only supports emulated I440 and so we enable vIOMMU with emulated
I440 chipset. This works on Linux and Windows guest.


Chao Gao (4):
  I440: Allow adding sysbus devices with -device on I440
  Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen
  xen-pt: bind/unbind interrupt remapping format MSI
  msi: taking interrupt format into consideration during judging a pirq
    is binded with a event channel

 hw/i386/pc_piix.c             |   1 +
 hw/pci/msi.c                  |   5 +-
 hw/pci/msix.c                 |   4 +-
 hw/xen/Makefile.objs          |   1 +
 hw/xen/xen_pt_msi.c           |  38 ++++++++++----
 hw/xen/xen_viommu.c           | 116 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/apic-msidef.h |   1 +
 include/hw/xen/xen.h          |   2 +-
 xen-hvm-stub.c                |   2 +-
 xen-hvm.c                     |   7 ++-
 10 files changed, 162 insertions(+), 15 deletions(-)
 create mode 100644 hw/xen/xen_viommu.c

-- 
1.8.3.1

[RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[Lan Tianyu]

This patchset is to add Xen vIOMMU device model and handle
irq remapping stuffs. Xen vIOMMU emulation is in the Xen hypervisor
and the new device module in Qemu works as hypercall wrappers to
create and destroy vIOMMU in hypervisor.

Xen only supports emulated I440 and so we enable vIOMMU with emulated
I440 chipset. This works on Linux and Windows guest.


Chao Gao (4):
  I440: Allow adding sysbus devices with -device on I440
  Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen
  xen-pt: bind/unbind interrupt remapping format MSI
  msi: taking interrupt format into consideration during judging a pirq
    is binded with a event channel

 hw/i386/pc_piix.c             |   1 +
 hw/pci/msi.c                  |   5 +-
 hw/pci/msix.c                 |   4 +-
 hw/xen/Makefile.objs          |   1 +
 hw/xen/xen_pt_msi.c           |  38 ++++++++++----
 hw/xen/xen_viommu.c           | 116 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/apic-msidef.h |   1 +
 include/hw/xen/xen.h          |   2 +-
 xen-hvm-stub.c                |   2 +-
 xen-hvm.c                     |   7 ++-
 10 files changed, 162 insertions(+), 15 deletions(-)
 create mode 100644 hw/xen/xen_viommu.c

-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Qemu-devel] [RFC PATCH 1/4] I440: Allow adding sysbus devices with -device on I440

[Lan Tianyu]

From: Chao Gao <chao.gao@intel.com>

xen-viommu will be a sysbus device and the device model will
be enabled via "-device" parameter.

Signed-off-by: Chao Gao <chao.gao@intel.com>
Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 hw/i386/pc_piix.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a07dc81..3289593 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -436,6 +436,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
     m->hot_add_cpu = pc_hot_add_cpu;
     m->default_machine_opts = "firmware=bios-256k.bin";
     m->default_display = "std";
+    m->has_dynamic_sysbus = true;
 }
 
 static void pc_i440fx_2_7_machine_options(MachineClass *m)
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH 1/4] I440: Allow adding sysbus devices with -device on I440

[Eduardo Habkost]

On Fri, Mar 17, 2017 at 07:29:14PM +0800, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@intel.com>
> 
> xen-viommu will be a sysbus device and the device model will
> be enabled via "-device" parameter.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>

I'm worried about the bugs we may expose by accepting all the
other sysbus devices in the command-line in addition to
xen-viommu.

I am working on a RFC to replace "has_dynamic_sysbus" with a
whitelist of sysbus device classes. This way we could enable only
xen-viommu on i440fx, instead of suddenly enabling all sysbus
devices just because of xen-viommu.

> ---
>  hw/i386/pc_piix.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index a07dc81..3289593 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -436,6 +436,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
>      m->hot_add_cpu = pc_hot_add_cpu;
>      m->default_machine_opts = "firmware=bios-256k.bin";
>      m->default_display = "std";
> +    m->has_dynamic_sysbus = true;
>  }
>  
>  static void pc_i440fx_2_7_machine_options(MachineClass *m)
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC PATCH 1/4] I440: Allow adding sysbus devices with -device on I440

[Lan Tianyu]

Hi Eduardo:
	Thanks for your review.
On 2017年03月21日 03:49, Eduardo Habkost wrote:
> On Fri, Mar 17, 2017 at 07:29:14PM +0800, Lan Tianyu wrote:
>> From: Chao Gao <chao.gao@intel.com>
>>
>> xen-viommu will be a sysbus device and the device model will
>> be enabled via "-device" parameter.
>>
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> 
> I'm worried about the bugs we may expose by accepting all the
> other sysbus devices in the command-line in addition to
> xen-viommu.
> 
> I am working on a RFC to replace "has_dynamic_sysbus" with a
> whitelist of sysbus device classes. This way we could enable only
> xen-viommu on i440fx, instead of suddenly enabling all sysbus
> devices just because of xen-viommu.

That sounds reasonable. Thanks for the job and we rebase on your patches.



> 
>> ---
>>  hw/i386/pc_piix.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index a07dc81..3289593 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -436,6 +436,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
>>      m->hot_add_cpu = pc_hot_add_cpu;
>>      m->default_machine_opts = "firmware=bios-256k.bin";
>>      m->default_display = "std";
>> +    m->has_dynamic_sysbus = true;
>>  }
>>  
>>  static void pc_i440fx_2_7_machine_options(MachineClass *m)
>> -- 
>> 1.8.3.1
>>
> 


-- 
Best regards
Tianyu Lan

[Qemu-devel] [RFC PATCH 2/4] Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen

[Lan Tianyu]

From: Chao Gao <chao.gao@intel.com>

Since adding a dynamic sysbus device is forbidden, so choose TYPE_DEVICE
as parent class.

Signed-off-by: Chao Gao <chao.gao@intel.com>
Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 hw/xen/Makefile.objs |   1 +
 hw/xen/xen_viommu.c  | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+)
 create mode 100644 hw/xen/xen_viommu.c

diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs
index d367094..e37d808 100644
--- a/hw/xen/Makefile.objs
+++ b/hw/xen/Makefile.objs
@@ -3,3 +3,4 @@ common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o xen_devconfig.o
 
 obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
 obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_graphics.o xen_pt_msi.o
+obj-$(CONFIG_XEN) += xen_viommu.o
diff --git a/hw/xen/xen_viommu.c b/hw/xen/xen_viommu.c
new file mode 100644
index 0000000..9bf9158
--- /dev/null
+++ b/hw/xen/xen_viommu.c
@@ -0,0 +1,116 @@
+/*
+ * Xen virtual IOMMU (virtual VT-D)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+
+#include "hw/qdev-core.h"
+#include "hw/sysbus.h"
+#include "hw/xen/xen.h"
+#include "hw/xen/xen_backend.h"
+
+#define TYPE_XEN_VIOMMU_DEVICE "xen_viommu"
+#define  XEN_VIOMMU_DEVICE(obj) \
+    OBJECT_CHECK(XenVIOMMUState, (obj), TYPE_XEN_VIOMMU_DEVICE)
+
+typedef struct XenVIOMMUState XenVIOMMUState;
+
+struct XenVIOMMUState {
+    DeviceState dev;
+    uint32_t id;
+    uint64_t cap;
+    uint64_t base_addr;
+};
+
+static void xen_viommu_realize(DeviceState *dev, Error **errp)
+{
+    int rc;
+    uint64_t cap;
+    char *dom;
+    char viommu_path[1024];
+    XenVIOMMUState *s = XEN_VIOMMU_DEVICE(dev);
+
+    s->id = -1;
+    
+    /* Read vIOMMU attributes from Xenstore. */
+    dom = xs_get_domain_path(xenstore, xen_domid);
+    snprintf(viommu_path, sizeof(viommu_path), "%s/viommu", dom);
+    rc = xenstore_read_uint64(viommu_path, "base_addr", &s->base_addr);  
+    if (rc) {
+        error_report("Can't get base address of vIOMMU");
+        exit(1);
+    }
+
+    rc = xenstore_read_uint64(viommu_path, "cap", &s->cap);
+    if (rc) {
+        error_report("Can't get capabilities of vIOMMU");
+        exit(1);
+    }
+
+    rc = xc_viommu_query_cap(xen_xc, xen_domid, &cap);
+    if (rc) {
+        exit(1);
+    }
+
+
+    if ((cap & s->cap) != cap) {
+        error_report("xen: Unsupported capability %lx", s->cap);
+        exit(1);
+    }
+
+    rc = xc_viommu_create(xen_xc, xen_domid, s->base_addr, s->cap, &s->id);
+    if (rc) {
+        s->id = -1;
+        error_report("xen: failed(%d) to create viommu ", rc);
+        exit(1);
+    }
+}
+
+static void xen_viommu_instance_finalize(Object *o)
+{
+    int rc;
+    XenVIOMMUState *s = XEN_VIOMMU_DEVICE(o);
+
+    if (s->id != -1) {
+        rc = xc_viommu_destroy(xen_xc, xen_domid, s->id); 
+        if (rc) {
+            error_report("xen: failed(%d) to destroy viommu ", rc);
+        }
+    }
+}
+
+static void xen_viommu_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    dc->hotpluggable = false;
+    dc->realize = xen_viommu_realize;
+}
+
+static const TypeInfo xen_viommu_info = {
+    .name              = TYPE_XEN_VIOMMU_DEVICE,
+    .parent            = TYPE_SYS_BUS_DEVICE,
+    .instance_size     = sizeof(XenVIOMMUState),
+    .instance_finalize = xen_viommu_instance_finalize,
+    .class_init        = xen_viommu_class_init,
+};
+
+static void xen_viommu_register_types(void)
+{
+    type_register_static(&xen_viommu_info); 
+}
+
+type_init(xen_viommu_register_types);
-- 
1.8.3.1

[RFC PATCH 2/4] Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen

[Lan Tianyu]

From: Chao Gao <chao.gao@intel.com>

Since adding a dynamic sysbus device is forbidden, so choose TYPE_DEVICE
as parent class.

Signed-off-by: Chao Gao <chao.gao@intel.com>
Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 hw/xen/Makefile.objs |   1 +
 hw/xen/xen_viommu.c  | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+)
 create mode 100644 hw/xen/xen_viommu.c

diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs
index d367094..e37d808 100644
--- a/hw/xen/Makefile.objs
+++ b/hw/xen/Makefile.objs
@@ -3,3 +3,4 @@ common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o xen_devconfig.o
 
 obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
 obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_graphics.o xen_pt_msi.o
+obj-$(CONFIG_XEN) += xen_viommu.o
diff --git a/hw/xen/xen_viommu.c b/hw/xen/xen_viommu.c
new file mode 100644
index 0000000..9bf9158
--- /dev/null
+++ b/hw/xen/xen_viommu.c
@@ -0,0 +1,116 @@
+/*
+ * Xen virtual IOMMU (virtual VT-D)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+
+#include "hw/qdev-core.h"
+#include "hw/sysbus.h"
+#include "hw/xen/xen.h"
+#include "hw/xen/xen_backend.h"
+
+#define TYPE_XEN_VIOMMU_DEVICE "xen_viommu"
+#define  XEN_VIOMMU_DEVICE(obj) \
+    OBJECT_CHECK(XenVIOMMUState, (obj), TYPE_XEN_VIOMMU_DEVICE)
+
+typedef struct XenVIOMMUState XenVIOMMUState;
+
+struct XenVIOMMUState {
+    DeviceState dev;
+    uint32_t id;
+    uint64_t cap;
+    uint64_t base_addr;
+};
+
+static void xen_viommu_realize(DeviceState *dev, Error **errp)
+{
+    int rc;
+    uint64_t cap;
+    char *dom;
+    char viommu_path[1024];
+    XenVIOMMUState *s = XEN_VIOMMU_DEVICE(dev);
+
+    s->id = -1;
+    
+    /* Read vIOMMU attributes from Xenstore. */
+    dom = xs_get_domain_path(xenstore, xen_domid);
+    snprintf(viommu_path, sizeof(viommu_path), "%s/viommu", dom);
+    rc = xenstore_read_uint64(viommu_path, "base_addr", &s->base_addr);  
+    if (rc) {
+        error_report("Can't get base address of vIOMMU");
+        exit(1);
+    }
+
+    rc = xenstore_read_uint64(viommu_path, "cap", &s->cap);
+    if (rc) {
+        error_report("Can't get capabilities of vIOMMU");
+        exit(1);
+    }
+
+    rc = xc_viommu_query_cap(xen_xc, xen_domid, &cap);
+    if (rc) {
+        exit(1);
+    }
+
+
+    if ((cap & s->cap) != cap) {
+        error_report("xen: Unsupported capability %lx", s->cap);
+        exit(1);
+    }
+
+    rc = xc_viommu_create(xen_xc, xen_domid, s->base_addr, s->cap, &s->id);
+    if (rc) {
+        s->id = -1;
+        error_report("xen: failed(%d) to create viommu ", rc);
+        exit(1);
+    }
+}
+
+static void xen_viommu_instance_finalize(Object *o)
+{
+    int rc;
+    XenVIOMMUState *s = XEN_VIOMMU_DEVICE(o);
+
+    if (s->id != -1) {
+        rc = xc_viommu_destroy(xen_xc, xen_domid, s->id); 
+        if (rc) {
+            error_report("xen: failed(%d) to destroy viommu ", rc);
+        }
+    }
+}
+
+static void xen_viommu_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    dc->hotpluggable = false;
+    dc->realize = xen_viommu_realize;
+}
+
+static const TypeInfo xen_viommu_info = {
+    .name              = TYPE_XEN_VIOMMU_DEVICE,
+    .parent            = TYPE_SYS_BUS_DEVICE,
+    .instance_size     = sizeof(XenVIOMMUState),
+    .instance_finalize = xen_viommu_instance_finalize,
+    .class_init        = xen_viommu_class_init,
+};
+
+static void xen_viommu_register_types(void)
+{
+    type_register_static(&xen_viommu_info); 
+}
+
+type_init(xen_viommu_register_types);
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [RFC PATCH 2/4] Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen

[Anthony PERARD]

Hi,

On Fri, Mar 17, 2017 at 07:29:15PM +0800, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@intel.com>
> 
> Since adding a dynamic sysbus device is forbidden, so choose TYPE_DEVICE
> as parent class.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  hw/xen/Makefile.objs |   1 +
>  hw/xen/xen_viommu.c  | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 117 insertions(+)
>  create mode 100644 hw/xen/xen_viommu.c
> 
> diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs
> index d367094..e37d808 100644
> --- a/hw/xen/Makefile.objs
> +++ b/hw/xen/Makefile.objs
> @@ -3,3 +3,4 @@ common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o xen_devconfig.o
>  
>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_graphics.o xen_pt_msi.o
> +obj-$(CONFIG_XEN) += xen_viommu.o
> diff --git a/hw/xen/xen_viommu.c b/hw/xen/xen_viommu.c
> new file mode 100644
> index 0000000..9bf9158
> --- /dev/null
> +++ b/hw/xen/xen_viommu.c
> @@ -0,0 +1,116 @@
> +/*
> + * Xen virtual IOMMU (virtual VT-D)
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> +
> +#include "hw/qdev-core.h"
> +#include "hw/sysbus.h"
> +#include "hw/xen/xen.h"
> +#include "hw/xen/xen_backend.h"
> +
> +#define TYPE_XEN_VIOMMU_DEVICE "xen_viommu"
> +#define  XEN_VIOMMU_DEVICE(obj) \
> +    OBJECT_CHECK(XenVIOMMUState, (obj), TYPE_XEN_VIOMMU_DEVICE)
> +
> +typedef struct XenVIOMMUState XenVIOMMUState;
> +
> +struct XenVIOMMUState {
> +    DeviceState dev;
> +    uint32_t id;
> +    uint64_t cap;
> +    uint64_t base_addr;
> +};
> +
> +static void xen_viommu_realize(DeviceState *dev, Error **errp)
> +{
> +    int rc;
> +    uint64_t cap;
> +    char *dom;
> +    char viommu_path[1024];
> +    XenVIOMMUState *s = XEN_VIOMMU_DEVICE(dev);
> +
> +    s->id = -1;
> +    
> +    /* Read vIOMMU attributes from Xenstore. */
> +    dom = xs_get_domain_path(xenstore, xen_domid);
> +    snprintf(viommu_path, sizeof(viommu_path), "%s/viommu", dom);
> +    rc = xenstore_read_uint64(viommu_path, "base_addr", &s->base_addr);  

Could these informations (base_addr and cap) be read from the command
line instead of via xenstore?
Any reason for these informations to be on xenstore?

> +    if (rc) {
> +        error_report("Can't get base address of vIOMMU");

I think error_setg should be used instead of error_report.

> +        exit(1);

Also, exit should be remove, and return instead. error_setg would be
enough to signal that the device can not work.

> +    }
> +
> +    rc = xenstore_read_uint64(viommu_path, "cap", &s->cap);
> +    if (rc) {
> +        error_report("Can't get capabilities of vIOMMU");
> +        exit(1);
> +    }
> +
> +    rc = xc_viommu_query_cap(xen_xc, xen_domid, &cap);

Since xc_viommu_* seems to be new, you should use
xendevicemodel_viommu_* instead, also you will need to define a stub for
them to be able to compile QEMU against older version of Xen.


The patch looks good otherwise.

Thanks,

-- 
Anthony PERARD

Re: [RFC PATCH 2/4] Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen

[Anthony PERARD]

Hi,

On Fri, Mar 17, 2017 at 07:29:15PM +0800, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@intel.com>
> 
> Since adding a dynamic sysbus device is forbidden, so choose TYPE_DEVICE
> as parent class.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  hw/xen/Makefile.objs |   1 +
>  hw/xen/xen_viommu.c  | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 117 insertions(+)
>  create mode 100644 hw/xen/xen_viommu.c
> 
> diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs
> index d367094..e37d808 100644
> --- a/hw/xen/Makefile.objs
> +++ b/hw/xen/Makefile.objs
> @@ -3,3 +3,4 @@ common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o xen_devconfig.o
>  
>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_graphics.o xen_pt_msi.o
> +obj-$(CONFIG_XEN) += xen_viommu.o
> diff --git a/hw/xen/xen_viommu.c b/hw/xen/xen_viommu.c
> new file mode 100644
> index 0000000..9bf9158
> --- /dev/null
> +++ b/hw/xen/xen_viommu.c
> @@ -0,0 +1,116 @@
> +/*
> + * Xen virtual IOMMU (virtual VT-D)
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> +
> +#include "hw/qdev-core.h"
> +#include "hw/sysbus.h"
> +#include "hw/xen/xen.h"
> +#include "hw/xen/xen_backend.h"
> +
> +#define TYPE_XEN_VIOMMU_DEVICE "xen_viommu"
> +#define  XEN_VIOMMU_DEVICE(obj) \
> +    OBJECT_CHECK(XenVIOMMUState, (obj), TYPE_XEN_VIOMMU_DEVICE)
> +
> +typedef struct XenVIOMMUState XenVIOMMUState;
> +
> +struct XenVIOMMUState {
> +    DeviceState dev;
> +    uint32_t id;
> +    uint64_t cap;
> +    uint64_t base_addr;
> +};
> +
> +static void xen_viommu_realize(DeviceState *dev, Error **errp)
> +{
> +    int rc;
> +    uint64_t cap;
> +    char *dom;
> +    char viommu_path[1024];
> +    XenVIOMMUState *s = XEN_VIOMMU_DEVICE(dev);
> +
> +    s->id = -1;
> +    
> +    /* Read vIOMMU attributes from Xenstore. */
> +    dom = xs_get_domain_path(xenstore, xen_domid);
> +    snprintf(viommu_path, sizeof(viommu_path), "%s/viommu", dom);
> +    rc = xenstore_read_uint64(viommu_path, "base_addr", &s->base_addr);  

Could these informations (base_addr and cap) be read from the command
line instead of via xenstore?
Any reason for these informations to be on xenstore?

> +    if (rc) {
> +        error_report("Can't get base address of vIOMMU");

I think error_setg should be used instead of error_report.

> +        exit(1);

Also, exit should be remove, and return instead. error_setg would be
enough to signal that the device can not work.

> +    }
> +
> +    rc = xenstore_read_uint64(viommu_path, "cap", &s->cap);
> +    if (rc) {
> +        error_report("Can't get capabilities of vIOMMU");
> +        exit(1);
> +    }
> +
> +    rc = xc_viommu_query_cap(xen_xc, xen_domid, &cap);

Since xc_viommu_* seems to be new, you should use
xendevicemodel_viommu_* instead, also you will need to define a stub for
them to be able to compile QEMU against older version of Xen.


The patch looks good otherwise.

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [RFC PATCH 2/4] Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen

[Chao Gao]

On Thu, Mar 30, 2017 at 05:24:52PM +0100, Anthony PERARD wrote:
>Hi,
>
>On Fri, Mar 17, 2017 at 07:29:15PM +0800, Lan Tianyu wrote:
>> From: Chao Gao <chao.gao@intel.com>
>> 
>> Since adding a dynamic sysbus device is forbidden, so choose TYPE_DEVICE
>> as parent class.
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>>  hw/xen/Makefile.objs |   1 +
>>  hw/xen/xen_viommu.c  | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 117 insertions(+)
>>  create mode 100644 hw/xen/xen_viommu.c
>> 
>> +static void xen_viommu_realize(DeviceState *dev, Error **errp)
>> +{
>> +    int rc;
>> +    uint64_t cap;
>> +    char *dom;
>> +    char viommu_path[1024];
>> +    XenVIOMMUState *s = XEN_VIOMMU_DEVICE(dev);
>> +
>> +    s->id = -1;
>> +    
>> +    /* Read vIOMMU attributes from Xenstore. */
>> +    dom = xs_get_domain_path(xenstore, xen_domid);
>> +    snprintf(viommu_path, sizeof(viommu_path), "%s/viommu", dom);
>> +    rc = xenstore_read_uint64(viommu_path, "base_addr", &s->base_addr);  
>
>Could these informations (base_addr and cap) be read from the command
>line instead of via xenstore?
>Any reason for these informations to be on xenstore?

Actually, we passed both via command line at first. We just concerned
whether it was ok to pass a address through command line since
we find no device does the similar thing.

>
>> +    if (rc) {
>> +        error_report("Can't get base address of vIOMMU");
>
>I think error_setg should be used instead of error_report.
>
>> +        exit(1);
>
>Also, exit should be remove, and return instead. error_setg would be
>enough to signal that the device can not work.
>
>> +    }
>> +
>> +    rc = xenstore_read_uint64(viommu_path, "cap", &s->cap);
>> +    if (rc) {
>> +        error_report("Can't get capabilities of vIOMMU");
>> +        exit(1);
>> +    }
>> +
>> +    rc = xc_viommu_query_cap(xen_xc, xen_domid, &cap);
>
>Since xc_viommu_* seems to be new, you should use
>xendevicemodel_viommu_* instead, also you will need to define a stub for
>them to be able to compile QEMU against older version of Xen.

Will follow your suggestions above.

Thanks
Chao

>
>
>The patch looks good otherwise.
>
>Thanks,
>
>-- 
>Anthony PERARD

Re: [RFC PATCH 2/4] Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen

[Chao Gao]

On Thu, Mar 30, 2017 at 05:24:52PM +0100, Anthony PERARD wrote:
>Hi,
>
>On Fri, Mar 17, 2017 at 07:29:15PM +0800, Lan Tianyu wrote:
>> From: Chao Gao <chao.gao@intel.com>
>> 
>> Since adding a dynamic sysbus device is forbidden, so choose TYPE_DEVICE
>> as parent class.
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>>  hw/xen/Makefile.objs |   1 +
>>  hw/xen/xen_viommu.c  | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 117 insertions(+)
>>  create mode 100644 hw/xen/xen_viommu.c
>> 
>> +static void xen_viommu_realize(DeviceState *dev, Error **errp)
>> +{
>> +    int rc;
>> +    uint64_t cap;
>> +    char *dom;
>> +    char viommu_path[1024];
>> +    XenVIOMMUState *s = XEN_VIOMMU_DEVICE(dev);
>> +
>> +    s->id = -1;
>> +    
>> +    /* Read vIOMMU attributes from Xenstore. */
>> +    dom = xs_get_domain_path(xenstore, xen_domid);
>> +    snprintf(viommu_path, sizeof(viommu_path), "%s/viommu", dom);
>> +    rc = xenstore_read_uint64(viommu_path, "base_addr", &s->base_addr);  
>
>Could these informations (base_addr and cap) be read from the command
>line instead of via xenstore?
>Any reason for these informations to be on xenstore?

Actually, we passed both via command line at first. We just concerned
whether it was ok to pass a address through command line since
we find no device does the similar thing.

>
>> +    if (rc) {
>> +        error_report("Can't get base address of vIOMMU");
>
>I think error_setg should be used instead of error_report.
>
>> +        exit(1);
>
>Also, exit should be remove, and return instead. error_setg would be
>enough to signal that the device can not work.
>
>> +    }
>> +
>> +    rc = xenstore_read_uint64(viommu_path, "cap", &s->cap);
>> +    if (rc) {
>> +        error_report("Can't get capabilities of vIOMMU");
>> +        exit(1);
>> +    }
>> +
>> +    rc = xc_viommu_query_cap(xen_xc, xen_domid, &cap);
>
>Since xc_viommu_* seems to be new, you should use
>xendevicemodel_viommu_* instead, also you will need to define a stub for
>them to be able to compile QEMU against older version of Xen.

Will follow your suggestions above.

Thanks
Chao

>
>
>The patch looks good otherwise.
>
>Thanks,
>
>-- 
>Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Qemu-devel] [RFC PATCH 3/4] xen-pt: bind/unbind interrupt remapping format MSI

[Lan Tianyu]

From: Chao Gao <chao.gao@intel.com>

If a vIOMMU is exposed to guest, guest will configure the msi to remapping
format. The original code isn't suitable to the new format. A new pair
bind/unbind interfaces are added for this usage. This patch recognizes
this case and use new interfaces to bind/unbind msi.

Signed-off-by: Chao Gao <chao.gao@intel.com>
Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 hw/xen/xen_pt_msi.c           | 36 ++++++++++++++++++++++++++++--------
 include/hw/i386/apic-msidef.h |  1 +
 2 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index 62add06..8b0d7fc 100644
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -161,6 +161,7 @@ static int msi_msix_update(XenPCIPassthroughState *s,
     uint8_t gvec = msi_vector(data);
     uint32_t gflags = msi_gflags(data, addr);
     int rc = 0;
+    bool ir = !!(addr & MSI_ADDR_IM_MASK);
     uint64_t table_addr = 0;
 
     XEN_PT_LOG(d, "Updating MSI%s with pirq %d gvec %#x gflags %#x"
@@ -171,8 +172,14 @@ static int msi_msix_update(XenPCIPassthroughState *s,
         table_addr = s->msix->mmio_base_addr;
     }
 
-    rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
+    if (ir) {
+        rc = xc_domain_update_msi_irq_remapping(xen_xc, xen_domid, pirq,
+                                    d->devfn, data, addr, table_addr);
+    }
+    else {
+        rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
                                   pirq, gflags, table_addr);
+    }
 
     if (rc) {
         XEN_PT_ERR(d, "Updating of MSI%s failed. (err: %d)\n",
@@ -204,13 +211,26 @@ static int msi_msix_disable(XenPCIPassthroughState *s,
     }
 
     if (is_binded) {
-        XEN_PT_LOG(d, "Unbind MSI%s with pirq %d, gvec %#x\n",
-                   is_msix ? "-X" : "", pirq, gvec);
-        rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
-        if (rc) {
-            XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
-                       is_msix ? "-X" : "", errno, pirq, gvec);
-            return rc;
+        if ( addr & MSI_ADDR_IM_MASK ) {
+            XEN_PT_LOG(d, "Unbinding of MSI%s . ( pirq: %d, data: %x, addr: %lx)\n",
+                       is_msix ? "-X" : "", pirq, data, addr);
+            rc = xc_domain_unbind_msi_irq_remapping(xen_xc, xen_domid, pirq,
+                                                    d->devfn, data, addr);
+            if (rc) {
+                XEN_PT_ERR(d, "Unbinding of MSI%s . (error: %d, pirq: %d, data: %x, addr: %lx)\n",
+                           is_msix ? "-X" : "", rc, pirq, data, addr);
+                return rc;
+            }
+
+        } else {
+            XEN_PT_LOG(d, "Unbind MSI%s with pirq %d, gvec %#x\n",
+                       is_msix ? "-X" : "", pirq, gvec);
+            rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
+            if (rc) {
+                XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
+                           is_msix ? "-X" : "", errno, pirq, gvec);
+                return rc;
+            }
         }
     }
 
diff --git a/include/hw/i386/apic-msidef.h b/include/hw/i386/apic-msidef.h
index 8b4d4cc..08b584f 100644
--- a/include/hw/i386/apic-msidef.h
+++ b/include/hw/i386/apic-msidef.h
@@ -27,5 +27,6 @@
 #define MSI_ADDR_DEST_ID_SHIFT          12
 #define MSI_ADDR_DEST_IDX_SHIFT         4
 #define  MSI_ADDR_DEST_ID_MASK          0x00ffff0
+#define  MSI_ADDR_IM_MASK               0x00000010
 
 #endif /* HW_APIC_MSIDEF_H */
-- 
1.8.3.1

[RFC PATCH 3/4] xen-pt: bind/unbind interrupt remapping format MSI

[Lan Tianyu]

From: Chao Gao <chao.gao@intel.com>

If a vIOMMU is exposed to guest, guest will configure the msi to remapping
format. The original code isn't suitable to the new format. A new pair
bind/unbind interfaces are added for this usage. This patch recognizes
this case and use new interfaces to bind/unbind msi.

Signed-off-by: Chao Gao <chao.gao@intel.com>
Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 hw/xen/xen_pt_msi.c           | 36 ++++++++++++++++++++++++++++--------
 include/hw/i386/apic-msidef.h |  1 +
 2 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index 62add06..8b0d7fc 100644
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -161,6 +161,7 @@ static int msi_msix_update(XenPCIPassthroughState *s,
     uint8_t gvec = msi_vector(data);
     uint32_t gflags = msi_gflags(data, addr);
     int rc = 0;
+    bool ir = !!(addr & MSI_ADDR_IM_MASK);
     uint64_t table_addr = 0;
 
     XEN_PT_LOG(d, "Updating MSI%s with pirq %d gvec %#x gflags %#x"
@@ -171,8 +172,14 @@ static int msi_msix_update(XenPCIPassthroughState *s,
         table_addr = s->msix->mmio_base_addr;
     }
 
-    rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
+    if (ir) {
+        rc = xc_domain_update_msi_irq_remapping(xen_xc, xen_domid, pirq,
+                                    d->devfn, data, addr, table_addr);
+    }
+    else {
+        rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
                                   pirq, gflags, table_addr);
+    }
 
     if (rc) {
         XEN_PT_ERR(d, "Updating of MSI%s failed. (err: %d)\n",
@@ -204,13 +211,26 @@ static int msi_msix_disable(XenPCIPassthroughState *s,
     }
 
     if (is_binded) {
-        XEN_PT_LOG(d, "Unbind MSI%s with pirq %d, gvec %#x\n",
-                   is_msix ? "-X" : "", pirq, gvec);
-        rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
-        if (rc) {
-            XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
-                       is_msix ? "-X" : "", errno, pirq, gvec);
-            return rc;
+        if ( addr & MSI_ADDR_IM_MASK ) {
+            XEN_PT_LOG(d, "Unbinding of MSI%s . ( pirq: %d, data: %x, addr: %lx)\n",
+                       is_msix ? "-X" : "", pirq, data, addr);
+            rc = xc_domain_unbind_msi_irq_remapping(xen_xc, xen_domid, pirq,
+                                                    d->devfn, data, addr);
+            if (rc) {
+                XEN_PT_ERR(d, "Unbinding of MSI%s . (error: %d, pirq: %d, data: %x, addr: %lx)\n",
+                           is_msix ? "-X" : "", rc, pirq, data, addr);
+                return rc;
+            }
+
+        } else {
+            XEN_PT_LOG(d, "Unbind MSI%s with pirq %d, gvec %#x\n",
+                       is_msix ? "-X" : "", pirq, gvec);
+            rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
+            if (rc) {
+                XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
+                           is_msix ? "-X" : "", errno, pirq, gvec);
+                return rc;
+            }
         }
     }
 
diff --git a/include/hw/i386/apic-msidef.h b/include/hw/i386/apic-msidef.h
index 8b4d4cc..08b584f 100644
--- a/include/hw/i386/apic-msidef.h
+++ b/include/hw/i386/apic-msidef.h
@@ -27,5 +27,6 @@
 #define MSI_ADDR_DEST_ID_SHIFT          12
 #define MSI_ADDR_DEST_IDX_SHIFT         4
 #define  MSI_ADDR_DEST_ID_MASK          0x00ffff0
+#define  MSI_ADDR_IM_MASK               0x00000010
 
 #endif /* HW_APIC_MSIDEF_H */
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [RFC PATCH 3/4] xen-pt: bind/unbind interrupt remapping format MSI

[Anthony PERARD]

On Fri, Mar 17, 2017 at 07:29:16PM +0800, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@intel.com>
> 
> If a vIOMMU is exposed to guest, guest will configure the msi to remapping
> format. The original code isn't suitable to the new format. A new pair
> bind/unbind interfaces are added for this usage. This patch recognizes
> this case and use new interfaces to bind/unbind msi.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  hw/xen/xen_pt_msi.c           | 36 ++++++++++++++++++++++++++++--------
>  include/hw/i386/apic-msidef.h |  1 +
>  2 files changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> index 62add06..8b0d7fc 100644
> --- a/hw/xen/xen_pt_msi.c
> +++ b/hw/xen/xen_pt_msi.c
> @@ -161,6 +161,7 @@ static int msi_msix_update(XenPCIPassthroughState *s,
>      uint8_t gvec = msi_vector(data);
>      uint32_t gflags = msi_gflags(data, addr);
>      int rc = 0;
> +    bool ir = !!(addr & MSI_ADDR_IM_MASK);
>      uint64_t table_addr = 0;
>  
>      XEN_PT_LOG(d, "Updating MSI%s with pirq %d gvec %#x gflags %#x"
> @@ -171,8 +172,14 @@ static int msi_msix_update(XenPCIPassthroughState *s,
>          table_addr = s->msix->mmio_base_addr;
>      }
>  
> -    rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
> +    if (ir) {

You could maybe use add&MSI_ADDR_IM_MASK instead of going through a
variable.

> +        rc = xc_domain_update_msi_irq_remapping(xen_xc, xen_domid, pirq,
> +                                    d->devfn, data, addr, table_addr);

Do you also want to update the XEN_PT_LOG above? Since it does not
always reflect the update_msi call anymore.

> +    }
> +    else {
> +        rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
>                                    pirq, gflags, table_addr);
> +    }
>  
>      if (rc) {
>          XEN_PT_ERR(d, "Updating of MSI%s failed. (err: %d)\n",
> @@ -204,13 +211,26 @@ static int msi_msix_disable(XenPCIPassthroughState *s,
>      }
>  
>      if (is_binded) {
> -        XEN_PT_LOG(d, "Unbind MSI%s with pirq %d, gvec %#x\n",
> -                   is_msix ? "-X" : "", pirq, gvec);
> -        rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
> -        if (rc) {
> -            XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
> -                       is_msix ? "-X" : "", errno, pirq, gvec);
> -            return rc;
> +        if ( addr & MSI_ADDR_IM_MASK ) {
> +            XEN_PT_LOG(d, "Unbinding of MSI%s . ( pirq: %d, data: %x, addr: %lx)\n",

For addr, it should be PRIx64 instead of %lx.

> +                       is_msix ? "-X" : "", pirq, data, addr);
> +            rc = xc_domain_unbind_msi_irq_remapping(xen_xc, xen_domid, pirq,
> +                                                    d->devfn, data, addr);
> +            if (rc) {
> +                XEN_PT_ERR(d, "Unbinding of MSI%s . (error: %d, pirq: %d, data: %x, addr: %lx)\n",
> +                           is_msix ? "-X" : "", rc, pirq, data, addr);
> +                return rc;
> +            }
> +
> +        } else {
> +            XEN_PT_LOG(d, "Unbind MSI%s with pirq %d, gvec %#x\n",
> +                       is_msix ? "-X" : "", pirq, gvec);
> +            rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
> +            if (rc) {
> +                XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
> +                           is_msix ? "-X" : "", errno, pirq, gvec);
> +                return rc;
> +            }
>          }
>      }
>  
> diff --git a/include/hw/i386/apic-msidef.h b/include/hw/i386/apic-msidef.h
> index 8b4d4cc..08b584f 100644
> --- a/include/hw/i386/apic-msidef.h
> +++ b/include/hw/i386/apic-msidef.h
> @@ -27,5 +27,6 @@
>  #define MSI_ADDR_DEST_ID_SHIFT          12
>  #define MSI_ADDR_DEST_IDX_SHIFT         4
>  #define  MSI_ADDR_DEST_ID_MASK          0x00ffff0

Could you add a 0 to dest_id here? So their will be 8 digit and it those
not look weird when compared to the next define.

> +#define  MSI_ADDR_IM_MASK               0x00000010

Is the definition of MSI_ADDR_IM_MASK available somewhere? In the Intel
SDM I've only found this bit to be reserved.

Thanks,

-- 
Anthony PERARD

Re: [RFC PATCH 3/4] xen-pt: bind/unbind interrupt remapping format MSI

[Anthony PERARD]

On Fri, Mar 17, 2017 at 07:29:16PM +0800, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@intel.com>
> 
> If a vIOMMU is exposed to guest, guest will configure the msi to remapping
> format. The original code isn't suitable to the new format. A new pair
> bind/unbind interfaces are added for this usage. This patch recognizes
> this case and use new interfaces to bind/unbind msi.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  hw/xen/xen_pt_msi.c           | 36 ++++++++++++++++++++++++++++--------
>  include/hw/i386/apic-msidef.h |  1 +
>  2 files changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> index 62add06..8b0d7fc 100644
> --- a/hw/xen/xen_pt_msi.c
> +++ b/hw/xen/xen_pt_msi.c
> @@ -161,6 +161,7 @@ static int msi_msix_update(XenPCIPassthroughState *s,
>      uint8_t gvec = msi_vector(data);
>      uint32_t gflags = msi_gflags(data, addr);
>      int rc = 0;
> +    bool ir = !!(addr & MSI_ADDR_IM_MASK);
>      uint64_t table_addr = 0;
>  
>      XEN_PT_LOG(d, "Updating MSI%s with pirq %d gvec %#x gflags %#x"
> @@ -171,8 +172,14 @@ static int msi_msix_update(XenPCIPassthroughState *s,
>          table_addr = s->msix->mmio_base_addr;
>      }
>  
> -    rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
> +    if (ir) {

You could maybe use add&MSI_ADDR_IM_MASK instead of going through a
variable.

> +        rc = xc_domain_update_msi_irq_remapping(xen_xc, xen_domid, pirq,
> +                                    d->devfn, data, addr, table_addr);

Do you also want to update the XEN_PT_LOG above? Since it does not
always reflect the update_msi call anymore.

> +    }
> +    else {
> +        rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
>                                    pirq, gflags, table_addr);
> +    }
>  
>      if (rc) {
>          XEN_PT_ERR(d, "Updating of MSI%s failed. (err: %d)\n",
> @@ -204,13 +211,26 @@ static int msi_msix_disable(XenPCIPassthroughState *s,
>      }
>  
>      if (is_binded) {
> -        XEN_PT_LOG(d, "Unbind MSI%s with pirq %d, gvec %#x\n",
> -                   is_msix ? "-X" : "", pirq, gvec);
> -        rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
> -        if (rc) {
> -            XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
> -                       is_msix ? "-X" : "", errno, pirq, gvec);
> -            return rc;
> +        if ( addr & MSI_ADDR_IM_MASK ) {
> +            XEN_PT_LOG(d, "Unbinding of MSI%s . ( pirq: %d, data: %x, addr: %lx)\n",

For addr, it should be PRIx64 instead of %lx.

> +                       is_msix ? "-X" : "", pirq, data, addr);
> +            rc = xc_domain_unbind_msi_irq_remapping(xen_xc, xen_domid, pirq,
> +                                                    d->devfn, data, addr);
> +            if (rc) {
> +                XEN_PT_ERR(d, "Unbinding of MSI%s . (error: %d, pirq: %d, data: %x, addr: %lx)\n",
> +                           is_msix ? "-X" : "", rc, pirq, data, addr);
> +                return rc;
> +            }
> +
> +        } else {
> +            XEN_PT_LOG(d, "Unbind MSI%s with pirq %d, gvec %#x\n",
> +                       is_msix ? "-X" : "", pirq, gvec);
> +            rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
> +            if (rc) {
> +                XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
> +                           is_msix ? "-X" : "", errno, pirq, gvec);
> +                return rc;
> +            }
>          }
>      }
>  
> diff --git a/include/hw/i386/apic-msidef.h b/include/hw/i386/apic-msidef.h
> index 8b4d4cc..08b584f 100644
> --- a/include/hw/i386/apic-msidef.h
> +++ b/include/hw/i386/apic-msidef.h
> @@ -27,5 +27,6 @@
>  #define MSI_ADDR_DEST_ID_SHIFT          12
>  #define MSI_ADDR_DEST_IDX_SHIFT         4
>  #define  MSI_ADDR_DEST_ID_MASK          0x00ffff0

Could you add a 0 to dest_id here? So their will be 8 digit and it those
not look weird when compared to the next define.

> +#define  MSI_ADDR_IM_MASK               0x00000010

Is the definition of MSI_ADDR_IM_MASK available somewhere? In the Intel
SDM I've only found this bit to be reserved.

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [RFC PATCH 3/4] xen-pt: bind/unbind interrupt remapping format MSI

[Chao Gao]

On Thu, Mar 30, 2017 at 05:51:45PM +0100, Anthony PERARD wrote:
>On Fri, Mar 17, 2017 at 07:29:16PM +0800, Lan Tianyu wrote:
>> From: Chao Gao <chao.gao@intel.com>
>> 
>> If a vIOMMU is exposed to guest, guest will configure the msi to remapping
>> format. The original code isn't suitable to the new format. A new pair
>> bind/unbind interfaces are added for this usage. This patch recognizes
>> this case and use new interfaces to bind/unbind msi.
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>>  hw/xen/xen_pt_msi.c           | 36 ++++++++++++++++++++++++++++--------
>>  include/hw/i386/apic-msidef.h |  1 +
>>  2 files changed, 29 insertions(+), 8 deletions(-)
>> 
>> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
>> index 62add06..8b0d7fc 100644
>> --- a/hw/xen/xen_pt_msi.c
>> +++ b/hw/xen/xen_pt_msi.c
>> @@ -161,6 +161,7 @@ static int msi_msix_update(XenPCIPassthroughState *s,
>>      uint8_t gvec = msi_vector(data);
>>      uint32_t gflags = msi_gflags(data, addr);
>>      int rc = 0;
>> +    bool ir = !!(addr & MSI_ADDR_IM_MASK);
>>      uint64_t table_addr = 0;
>>  
>>      XEN_PT_LOG(d, "Updating MSI%s with pirq %d gvec %#x gflags %#x"
>> @@ -171,8 +172,14 @@ static int msi_msix_update(XenPCIPassthroughState *s,
>>          table_addr = s->msix->mmio_base_addr;
>>      }
>>  
>> -    rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
>> +    if (ir) {
>
>You could maybe use add&MSI_ADDR_IM_MASK instead of going through a
>variable.
>
>> +        rc = xc_domain_update_msi_irq_remapping(xen_xc, xen_domid, pirq,
>> +                                    d->devfn, data, addr, table_addr);
>
>Do you also want to update the XEN_PT_LOG above? Since it does not
>always reflect the update_msi call anymore.

Yes. I adjust the output.

>
>> +    }
>> +    else {
>> +        rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
>>                                    pirq, gflags, table_addr);
>> +    }
>>  
>>      if (rc) {
>>          XEN_PT_ERR(d, "Updating of MSI%s failed. (err: %d)\n",
>> @@ -204,13 +211,26 @@ static int msi_msix_disable(XenPCIPassthroughState *s,
>>      }
>>  
>>      if (is_binded) {
>> -        XEN_PT_LOG(d, "Unbind MSI%s with pirq %d, gvec %#x\n",
>> -                   is_msix ? "-X" : "", pirq, gvec);
>> -        rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
>> -        if (rc) {
>> -            XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
>> -                       is_msix ? "-X" : "", errno, pirq, gvec);
>> -            return rc;
>> +        if ( addr & MSI_ADDR_IM_MASK ) {
>> +            XEN_PT_LOG(d, "Unbinding of MSI%s . ( pirq: %d, data: %x, addr: %lx)\n",
>
>For addr, it should be PRIx64 instead of %lx.
>
>> +                       is_msix ? "-X" : "", pirq, data, addr);
>> +            rc = xc_domain_unbind_msi_irq_remapping(xen_xc, xen_domid, pirq,
>> +                                                    d->devfn, data, addr);
>> +            if (rc) {
>> +                XEN_PT_ERR(d, "Unbinding of MSI%s . (error: %d, pirq: %d, data: %x, addr: %lx)\n",
>> +                           is_msix ? "-X" : "", rc, pirq, data, addr);
>> +                return rc;
>> +            }
>> +
>> +        } else {
>> +            XEN_PT_LOG(d, "Unbind MSI%s with pirq %d, gvec %#x\n",
>> +                       is_msix ? "-X" : "", pirq, gvec);
>> +            rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
>> +            if (rc) {
>> +                XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
>> +                           is_msix ? "-X" : "", errno, pirq, gvec);
>> +                return rc;
>> +            }
>>          }
>>      }
>>  
>> diff --git a/include/hw/i386/apic-msidef.h b/include/hw/i386/apic-msidef.h
>> index 8b4d4cc..08b584f 100644
>> --- a/include/hw/i386/apic-msidef.h
>> +++ b/include/hw/i386/apic-msidef.h
>> @@ -27,5 +27,6 @@
>>  #define MSI_ADDR_DEST_ID_SHIFT          12
>>  #define MSI_ADDR_DEST_IDX_SHIFT         4
>>  #define  MSI_ADDR_DEST_ID_MASK          0x00ffff0
>
>Could you add a 0 to dest_id here? So their will be 8 digit and it those
>not look weird when compared to the next define.
>

Will do.

>> +#define  MSI_ADDR_IM_MASK               0x00000010
>
>Is the definition of MSI_ADDR_IM_MASK available somewhere? In the Intel
>SDM I've only found this bit to be reserved.

Yes, it is defined in VT-d spec 5.1.5.2 MSI and MSI-X Register Programming.
I made a mistake here. I should use MSI_ADDR_IF_MASK. 

Thanks
Chao

>
>Thanks,
>
>-- 
>Anthony PERARD

Re: [RFC PATCH 3/4] xen-pt: bind/unbind interrupt remapping format MSI

[Chao Gao]

On Thu, Mar 30, 2017 at 05:51:45PM +0100, Anthony PERARD wrote:
>On Fri, Mar 17, 2017 at 07:29:16PM +0800, Lan Tianyu wrote:
>> From: Chao Gao <chao.gao@intel.com>
>> 
>> If a vIOMMU is exposed to guest, guest will configure the msi to remapping
>> format. The original code isn't suitable to the new format. A new pair
>> bind/unbind interfaces are added for this usage. This patch recognizes
>> this case and use new interfaces to bind/unbind msi.
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>>  hw/xen/xen_pt_msi.c           | 36 ++++++++++++++++++++++++++++--------
>>  include/hw/i386/apic-msidef.h |  1 +
>>  2 files changed, 29 insertions(+), 8 deletions(-)
>> 
>> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
>> index 62add06..8b0d7fc 100644
>> --- a/hw/xen/xen_pt_msi.c
>> +++ b/hw/xen/xen_pt_msi.c
>> @@ -161,6 +161,7 @@ static int msi_msix_update(XenPCIPassthroughState *s,
>>      uint8_t gvec = msi_vector(data);
>>      uint32_t gflags = msi_gflags(data, addr);
>>      int rc = 0;
>> +    bool ir = !!(addr & MSI_ADDR_IM_MASK);
>>      uint64_t table_addr = 0;
>>  
>>      XEN_PT_LOG(d, "Updating MSI%s with pirq %d gvec %#x gflags %#x"
>> @@ -171,8 +172,14 @@ static int msi_msix_update(XenPCIPassthroughState *s,
>>          table_addr = s->msix->mmio_base_addr;
>>      }
>>  
>> -    rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
>> +    if (ir) {
>
>You could maybe use add&MSI_ADDR_IM_MASK instead of going through a
>variable.
>
>> +        rc = xc_domain_update_msi_irq_remapping(xen_xc, xen_domid, pirq,
>> +                                    d->devfn, data, addr, table_addr);
>
>Do you also want to update the XEN_PT_LOG above? Since it does not
>always reflect the update_msi call anymore.

Yes. I adjust the output.

>
>> +    }
>> +    else {
>> +        rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
>>                                    pirq, gflags, table_addr);
>> +    }
>>  
>>      if (rc) {
>>          XEN_PT_ERR(d, "Updating of MSI%s failed. (err: %d)\n",
>> @@ -204,13 +211,26 @@ static int msi_msix_disable(XenPCIPassthroughState *s,
>>      }
>>  
>>      if (is_binded) {
>> -        XEN_PT_LOG(d, "Unbind MSI%s with pirq %d, gvec %#x\n",
>> -                   is_msix ? "-X" : "", pirq, gvec);
>> -        rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
>> -        if (rc) {
>> -            XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
>> -                       is_msix ? "-X" : "", errno, pirq, gvec);
>> -            return rc;
>> +        if ( addr & MSI_ADDR_IM_MASK ) {
>> +            XEN_PT_LOG(d, "Unbinding of MSI%s . ( pirq: %d, data: %x, addr: %lx)\n",
>
>For addr, it should be PRIx64 instead of %lx.
>
>> +                       is_msix ? "-X" : "", pirq, data, addr);
>> +            rc = xc_domain_unbind_msi_irq_remapping(xen_xc, xen_domid, pirq,
>> +                                                    d->devfn, data, addr);
>> +            if (rc) {
>> +                XEN_PT_ERR(d, "Unbinding of MSI%s . (error: %d, pirq: %d, data: %x, addr: %lx)\n",
>> +                           is_msix ? "-X" : "", rc, pirq, data, addr);
>> +                return rc;
>> +            }
>> +
>> +        } else {
>> +            XEN_PT_LOG(d, "Unbind MSI%s with pirq %d, gvec %#x\n",
>> +                       is_msix ? "-X" : "", pirq, gvec);
>> +            rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
>> +            if (rc) {
>> +                XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
>> +                           is_msix ? "-X" : "", errno, pirq, gvec);
>> +                return rc;
>> +            }
>>          }
>>      }
>>  
>> diff --git a/include/hw/i386/apic-msidef.h b/include/hw/i386/apic-msidef.h
>> index 8b4d4cc..08b584f 100644
>> --- a/include/hw/i386/apic-msidef.h
>> +++ b/include/hw/i386/apic-msidef.h
>> @@ -27,5 +27,6 @@
>>  #define MSI_ADDR_DEST_ID_SHIFT          12
>>  #define MSI_ADDR_DEST_IDX_SHIFT         4
>>  #define  MSI_ADDR_DEST_ID_MASK          0x00ffff0
>
>Could you add a 0 to dest_id here? So their will be 8 digit and it those
>not look weird when compared to the next define.
>

Will do.

>> +#define  MSI_ADDR_IM_MASK               0x00000010
>
>Is the definition of MSI_ADDR_IM_MASK available somewhere? In the Intel
>SDM I've only found this bit to be reserved.

Yes, it is defined in VT-d spec 5.1.5.2 MSI and MSI-X Register Programming.
I made a mistake here. I should use MSI_ADDR_IF_MASK. 

Thanks
Chao

>
>Thanks,
>
>-- 
>Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Qemu-devel] [RFC PATCH 4/4] msi: taking interrupt format into consideration during judging a pirq is binded with a event channel

[Lan Tianyu]

From: Chao Gao <chao.gao@intel.com>

As remapping format interrupt has been introduced, the vector in msi remapping
format can also be 0, same as a interrupt is binded with a event channel.
So we can't just use whether vector is 0 or not to judge a msi is binded
to a event channel or not.

This patch takes the msi interrupt format into consideration.

Signed-off-by: Chao Gao <chao.gao@intel.com>
Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 hw/pci/msi.c         | 5 +++--
 hw/pci/msix.c        | 4 +++-
 hw/xen/xen_pt_msi.c  | 2 +-
 include/hw/xen/xen.h | 2 +-
 xen-hvm-stub.c       | 2 +-
 xen-hvm.c            | 7 ++++++-
 6 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index a87b227..8d1ac9e 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -289,7 +289,7 @@ void msi_reset(PCIDevice *dev)
 static bool msi_is_masked(const PCIDevice *dev, unsigned int vector)
 {
     uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
-    uint32_t mask, data;
+    uint32_t mask, data, addr_lo;
     bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
     assert(vector < PCI_MSI_VECTORS_MAX);
 
@@ -298,7 +298,8 @@ static bool msi_is_masked(const PCIDevice *dev, unsigned int vector)
     }
 
     data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));
-    if (xen_is_pirq_msi(data)) {
+    addr_lo = pci_get_word(dev->config + msi_address_lo_off(dev));
+    if (xen_is_pirq_msi(data, addr_lo)) {
         return false;
     }
 
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 0ec1cb1..6b8045a 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -81,9 +81,11 @@ static bool msix_vector_masked(PCIDevice *dev, unsigned int vector, bool fmask)
 {
     unsigned offset = vector * PCI_MSIX_ENTRY_SIZE;
     uint8_t *data = &dev->msix_table[offset + PCI_MSIX_ENTRY_DATA];
+    uint8_t *addr_lo = &dev->msix_table[offset + PCI_MSIX_ENTRY_LOWER_ADDR];
     /* MSIs on Xen can be remapped into pirqs. In those cases, masking
      * and unmasking go through the PV evtchn path. */
-    if (xen_enabled() && xen_is_pirq_msi(pci_get_long(data))) {
+    if (xen_enabled() && xen_is_pirq_msi(pci_get_long(data),
+                                         pci_get_long(addr_lo))) {
         return false;
     }
     return fmask || dev->msix_table[offset + PCI_MSIX_ENTRY_VECTOR_CTRL] &
diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index 8b0d7fc..f799fed 100644
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -114,7 +114,7 @@ static int msi_msix_setup(XenPCIPassthroughState *s,
 
     assert((!is_msix && msix_entry == 0) || is_msix);
 
-    if (xen_is_pirq_msi(data)) {
+    if (xen_is_pirq_msi(data, (uint32_t)(addr & 0xffffffff))) {
         *ppirq = msi_ext_dest_id(addr >> 32) | msi_dest_id(addr);
         if (!*ppirq) {
             /* this probably identifies an misconfiguration of the guest,
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index a8f3afb..c15beb5 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -33,7 +33,7 @@ int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num);
 void xen_piix3_set_irq(void *opaque, int irq_num, int level);
 void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len);
 void xen_hvm_inject_msi(uint64_t addr, uint32_t data);
-int xen_is_pirq_msi(uint32_t msi_data);
+int xen_is_pirq_msi(uint32_t msi_data, uint32_t msi_addr_lo);
 
 qemu_irq *xen_interrupt_controller_init(void);
 
diff --git a/xen-hvm-stub.c b/xen-hvm-stub.c
index c500325..dae421c 100644
--- a/xen-hvm-stub.c
+++ b/xen-hvm-stub.c
@@ -31,7 +31,7 @@ void xen_hvm_inject_msi(uint64_t addr, uint32_t data)
 {
 }
 
-int xen_is_pirq_msi(uint32_t msi_data)
+int xen_is_pirq_msi(uint32_t msi_data, uint32_t msi_addr_lo)
 {
     return 0;
 }
diff --git a/xen-hvm.c b/xen-hvm.c
index 2f348ed..9e78b23 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -146,8 +146,13 @@ void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len)
     }
 }
 
-int xen_is_pirq_msi(uint32_t msi_data)
+int xen_is_pirq_msi(uint32_t msi_data, uint32_t msi_addr_lo)
 {
+    /* If msi address is configurate to remapping format, the msi will not
+     * remapped into a pirq.
+     */
+    if ( msi_addr_lo & 0x10 )
+        return 0;
     /* If vector is 0, the msi is remapped into a pirq, passed as
      * dest_id.
      */
-- 
1.8.3.1

[RFC PATCH 4/4] msi: taking interrupt format into consideration during judging a pirq is binded with a event channel

[Lan Tianyu]

From: Chao Gao <chao.gao@intel.com>

As remapping format interrupt has been introduced, the vector in msi remapping
format can also be 0, same as a interrupt is binded with a event channel.
So we can't just use whether vector is 0 or not to judge a msi is binded
to a event channel or not.

This patch takes the msi interrupt format into consideration.

Signed-off-by: Chao Gao <chao.gao@intel.com>
Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 hw/pci/msi.c         | 5 +++--
 hw/pci/msix.c        | 4 +++-
 hw/xen/xen_pt_msi.c  | 2 +-
 include/hw/xen/xen.h | 2 +-
 xen-hvm-stub.c       | 2 +-
 xen-hvm.c            | 7 ++++++-
 6 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index a87b227..8d1ac9e 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -289,7 +289,7 @@ void msi_reset(PCIDevice *dev)
 static bool msi_is_masked(const PCIDevice *dev, unsigned int vector)
 {
     uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
-    uint32_t mask, data;
+    uint32_t mask, data, addr_lo;
     bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
     assert(vector < PCI_MSI_VECTORS_MAX);
 
@@ -298,7 +298,8 @@ static bool msi_is_masked(const PCIDevice *dev, unsigned int vector)
     }
 
     data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));
-    if (xen_is_pirq_msi(data)) {
+    addr_lo = pci_get_word(dev->config + msi_address_lo_off(dev));
+    if (xen_is_pirq_msi(data, addr_lo)) {
         return false;
     }
 
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 0ec1cb1..6b8045a 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -81,9 +81,11 @@ static bool msix_vector_masked(PCIDevice *dev, unsigned int vector, bool fmask)
 {
     unsigned offset = vector * PCI_MSIX_ENTRY_SIZE;
     uint8_t *data = &dev->msix_table[offset + PCI_MSIX_ENTRY_DATA];
+    uint8_t *addr_lo = &dev->msix_table[offset + PCI_MSIX_ENTRY_LOWER_ADDR];
     /* MSIs on Xen can be remapped into pirqs. In those cases, masking
      * and unmasking go through the PV evtchn path. */
-    if (xen_enabled() && xen_is_pirq_msi(pci_get_long(data))) {
+    if (xen_enabled() && xen_is_pirq_msi(pci_get_long(data),
+                                         pci_get_long(addr_lo))) {
         return false;
     }
     return fmask || dev->msix_table[offset + PCI_MSIX_ENTRY_VECTOR_CTRL] &
diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index 8b0d7fc..f799fed 100644
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -114,7 +114,7 @@ static int msi_msix_setup(XenPCIPassthroughState *s,
 
     assert((!is_msix && msix_entry == 0) || is_msix);
 
-    if (xen_is_pirq_msi(data)) {
+    if (xen_is_pirq_msi(data, (uint32_t)(addr & 0xffffffff))) {
         *ppirq = msi_ext_dest_id(addr >> 32) | msi_dest_id(addr);
         if (!*ppirq) {
             /* this probably identifies an misconfiguration of the guest,
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index a8f3afb..c15beb5 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -33,7 +33,7 @@ int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num);
 void xen_piix3_set_irq(void *opaque, int irq_num, int level);
 void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len);
 void xen_hvm_inject_msi(uint64_t addr, uint32_t data);
-int xen_is_pirq_msi(uint32_t msi_data);
+int xen_is_pirq_msi(uint32_t msi_data, uint32_t msi_addr_lo);
 
 qemu_irq *xen_interrupt_controller_init(void);
 
diff --git a/xen-hvm-stub.c b/xen-hvm-stub.c
index c500325..dae421c 100644
--- a/xen-hvm-stub.c
+++ b/xen-hvm-stub.c
@@ -31,7 +31,7 @@ void xen_hvm_inject_msi(uint64_t addr, uint32_t data)
 {
 }
 
-int xen_is_pirq_msi(uint32_t msi_data)
+int xen_is_pirq_msi(uint32_t msi_data, uint32_t msi_addr_lo)
 {
     return 0;
 }
diff --git a/xen-hvm.c b/xen-hvm.c
index 2f348ed..9e78b23 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -146,8 +146,13 @@ void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len)
     }
 }
 
-int xen_is_pirq_msi(uint32_t msi_data)
+int xen_is_pirq_msi(uint32_t msi_data, uint32_t msi_addr_lo)
 {
+    /* If msi address is configurate to remapping format, the msi will not
+     * remapped into a pirq.
+     */
+    if ( msi_addr_lo & 0x10 )
+        return 0;
     /* If vector is 0, the msi is remapped into a pirq, passed as
      * dest_id.
      */
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [RFC PATCH 4/4] msi: taking interrupt format into consideration during judging a pirq is binded with a event channel

[Anthony PERARD]

On Fri, Mar 17, 2017 at 07:29:17PM +0800, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@intel.com>
> Subject: msi: taking interrupt format into consideration during
> judging a pirq is binded with a event channel

This is quite a long title, I think we can make it shorter. Maybe "msi:
Handle MSI remapping format.

> 
> As remapping format interrupt has been introduced, the vector in msi remapping
> format can also be 0, same as a interrupt is binded with a event channel.
> So we can't just use whether vector is 0 or not to judge a msi is binded
> to a event channel or not.
> 
> This patch takes the msi interrupt format into consideration.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  hw/pci/msi.c         | 5 +++--
>  hw/pci/msix.c        | 4 +++-
>  hw/xen/xen_pt_msi.c  | 2 +-
>  include/hw/xen/xen.h | 2 +-
>  xen-hvm-stub.c       | 2 +-
>  xen-hvm.c            | 7 ++++++-
>  6 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index a87b227..8d1ac9e 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -289,7 +289,7 @@ void msi_reset(PCIDevice *dev)
>  static bool msi_is_masked(const PCIDevice *dev, unsigned int vector)
>  {
>      uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
> -    uint32_t mask, data;
> +    uint32_t mask, data, addr_lo;
>      bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
>      assert(vector < PCI_MSI_VECTORS_MAX);
>  
> @@ -298,7 +298,8 @@ static bool msi_is_masked(const PCIDevice *dev, unsigned int vector)
>      }
>  
>      data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));
> -    if (xen_is_pirq_msi(data)) {
> +    addr_lo = pci_get_word(dev->config + msi_address_lo_off(dev));

This could be get_long, so addr_lo will actually have the all low bits
of the addr.

> +    if (xen_is_pirq_msi(data, addr_lo)) {
>          return false;
>      }
>  
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 0ec1cb1..6b8045a 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -81,9 +81,11 @@ static bool msix_vector_masked(PCIDevice *dev, unsigned int vector, bool fmask)
>  {
>      unsigned offset = vector * PCI_MSIX_ENTRY_SIZE;
>      uint8_t *data = &dev->msix_table[offset + PCI_MSIX_ENTRY_DATA];
> +    uint8_t *addr_lo = &dev->msix_table[offset + PCI_MSIX_ENTRY_LOWER_ADDR];
>      /* MSIs on Xen can be remapped into pirqs. In those cases, masking
>       * and unmasking go through the PV evtchn path. */
> -    if (xen_enabled() && xen_is_pirq_msi(pci_get_long(data))) {
> +    if (xen_enabled() && xen_is_pirq_msi(pci_get_long(data),
> +                                         pci_get_long(addr_lo))) {
>          return false;
>      }
>      return fmask || dev->msix_table[offset + PCI_MSIX_ENTRY_VECTOR_CTRL] &
> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> index 8b0d7fc..f799fed 100644
> --- a/hw/xen/xen_pt_msi.c
> +++ b/hw/xen/xen_pt_msi.c
> @@ -114,7 +114,7 @@ static int msi_msix_setup(XenPCIPassthroughState *s,
>  
>      assert((!is_msix && msix_entry == 0) || is_msix);
>  
> -    if (xen_is_pirq_msi(data)) {
> +    if (xen_is_pirq_msi(data, (uint32_t)(addr & 0xffffffff))) {

I don't think the cast is necessary. Also the & 0xffffffff is probably
not needed as well.

>          *ppirq = msi_ext_dest_id(addr >> 32) | msi_dest_id(addr);
>          if (!*ppirq) {
>              /* this probably identifies an misconfiguration of the guest,
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index a8f3afb..c15beb5 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -33,7 +33,7 @@ int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num);
>  void xen_piix3_set_irq(void *opaque, int irq_num, int level);
>  void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len);
>  void xen_hvm_inject_msi(uint64_t addr, uint32_t data);
> -int xen_is_pirq_msi(uint32_t msi_data);
> +int xen_is_pirq_msi(uint32_t msi_data, uint32_t msi_addr_lo);
>  
>  qemu_irq *xen_interrupt_controller_init(void);
>  
> diff --git a/xen-hvm-stub.c b/xen-hvm-stub.c
> index c500325..dae421c 100644
> --- a/xen-hvm-stub.c
> +++ b/xen-hvm-stub.c
> @@ -31,7 +31,7 @@ void xen_hvm_inject_msi(uint64_t addr, uint32_t data)
>  {
>  }
>  
> -int xen_is_pirq_msi(uint32_t msi_data)
> +int xen_is_pirq_msi(uint32_t msi_data, uint32_t msi_addr_lo)
>  {
>      return 0;
>  }
> diff --git a/xen-hvm.c b/xen-hvm.c
> index 2f348ed..9e78b23 100644
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -146,8 +146,13 @@ void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len)
>      }
>  }
>  
> -int xen_is_pirq_msi(uint32_t msi_data)
> +int xen_is_pirq_msi(uint32_t msi_data, uint32_t msi_addr_lo)
>  {
> +    /* If msi address is configurate to remapping format, the msi will not
> +     * remapped into a pirq.
> +     */
> +    if ( msi_addr_lo & 0x10 )

That's a magic number, is 0x10 MSI_ADDR_IM_MASK?

Thanks,

-- 
Anthony PERARD

Re: [RFC PATCH 4/4] msi: taking interrupt format into consideration during judging a pirq is binded with a event channel

[Anthony PERARD]

On Fri, Mar 17, 2017 at 07:29:17PM +0800, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@intel.com>
> Subject: msi: taking interrupt format into consideration during
> judging a pirq is binded with a event channel

This is quite a long title, I think we can make it shorter. Maybe "msi:
Handle MSI remapping format.

> 
> As remapping format interrupt has been introduced, the vector in msi remapping
> format can also be 0, same as a interrupt is binded with a event channel.
> So we can't just use whether vector is 0 or not to judge a msi is binded
> to a event channel or not.
> 
> This patch takes the msi interrupt format into consideration.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  hw/pci/msi.c         | 5 +++--
>  hw/pci/msix.c        | 4 +++-
>  hw/xen/xen_pt_msi.c  | 2 +-
>  include/hw/xen/xen.h | 2 +-
>  xen-hvm-stub.c       | 2 +-
>  xen-hvm.c            | 7 ++++++-
>  6 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index a87b227..8d1ac9e 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -289,7 +289,7 @@ void msi_reset(PCIDevice *dev)
>  static bool msi_is_masked(const PCIDevice *dev, unsigned int vector)
>  {
>      uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
> -    uint32_t mask, data;
> +    uint32_t mask, data, addr_lo;
>      bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
>      assert(vector < PCI_MSI_VECTORS_MAX);
>  
> @@ -298,7 +298,8 @@ static bool msi_is_masked(const PCIDevice *dev, unsigned int vector)
>      }
>  
>      data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));
> -    if (xen_is_pirq_msi(data)) {
> +    addr_lo = pci_get_word(dev->config + msi_address_lo_off(dev));

This could be get_long, so addr_lo will actually have the all low bits
of the addr.

> +    if (xen_is_pirq_msi(data, addr_lo)) {
>          return false;
>      }
>  
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 0ec1cb1..6b8045a 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -81,9 +81,11 @@ static bool msix_vector_masked(PCIDevice *dev, unsigned int vector, bool fmask)
>  {
>      unsigned offset = vector * PCI_MSIX_ENTRY_SIZE;
>      uint8_t *data = &dev->msix_table[offset + PCI_MSIX_ENTRY_DATA];
> +    uint8_t *addr_lo = &dev->msix_table[offset + PCI_MSIX_ENTRY_LOWER_ADDR];
>      /* MSIs on Xen can be remapped into pirqs. In those cases, masking
>       * and unmasking go through the PV evtchn path. */
> -    if (xen_enabled() && xen_is_pirq_msi(pci_get_long(data))) {
> +    if (xen_enabled() && xen_is_pirq_msi(pci_get_long(data),
> +                                         pci_get_long(addr_lo))) {
>          return false;
>      }
>      return fmask || dev->msix_table[offset + PCI_MSIX_ENTRY_VECTOR_CTRL] &
> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> index 8b0d7fc..f799fed 100644
> --- a/hw/xen/xen_pt_msi.c
> +++ b/hw/xen/xen_pt_msi.c
> @@ -114,7 +114,7 @@ static int msi_msix_setup(XenPCIPassthroughState *s,
>  
>      assert((!is_msix && msix_entry == 0) || is_msix);
>  
> -    if (xen_is_pirq_msi(data)) {
> +    if (xen_is_pirq_msi(data, (uint32_t)(addr & 0xffffffff))) {

I don't think the cast is necessary. Also the & 0xffffffff is probably
not needed as well.

>          *ppirq = msi_ext_dest_id(addr >> 32) | msi_dest_id(addr);
>          if (!*ppirq) {
>              /* this probably identifies an misconfiguration of the guest,
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index a8f3afb..c15beb5 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -33,7 +33,7 @@ int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num);
>  void xen_piix3_set_irq(void *opaque, int irq_num, int level);
>  void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len);
>  void xen_hvm_inject_msi(uint64_t addr, uint32_t data);
> -int xen_is_pirq_msi(uint32_t msi_data);
> +int xen_is_pirq_msi(uint32_t msi_data, uint32_t msi_addr_lo);
>  
>  qemu_irq *xen_interrupt_controller_init(void);
>  
> diff --git a/xen-hvm-stub.c b/xen-hvm-stub.c
> index c500325..dae421c 100644
> --- a/xen-hvm-stub.c
> +++ b/xen-hvm-stub.c
> @@ -31,7 +31,7 @@ void xen_hvm_inject_msi(uint64_t addr, uint32_t data)
>  {
>  }
>  
> -int xen_is_pirq_msi(uint32_t msi_data)
> +int xen_is_pirq_msi(uint32_t msi_data, uint32_t msi_addr_lo)
>  {
>      return 0;
>  }
> diff --git a/xen-hvm.c b/xen-hvm.c
> index 2f348ed..9e78b23 100644
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -146,8 +146,13 @@ void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len)
>      }
>  }
>  
> -int xen_is_pirq_msi(uint32_t msi_data)
> +int xen_is_pirq_msi(uint32_t msi_data, uint32_t msi_addr_lo)
>  {
> +    /* If msi address is configurate to remapping format, the msi will not
> +     * remapped into a pirq.
> +     */
> +    if ( msi_addr_lo & 0x10 )

That's a magic number, is 0x10 MSI_ADDR_IM_MASK?

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [RFC PATCH 4/4] msi: taking interrupt format into consideration during judging a pirq is binded with a event channel

[Chao Gao]

On Thu, Mar 30, 2017 at 06:29:29PM +0100, Anthony PERARD wrote:
>On Fri, Mar 17, 2017 at 07:29:17PM +0800, Lan Tianyu wrote:
>> From: Chao Gao <chao.gao@intel.com>
>> Subject: msi: taking interrupt format into consideration during
>> judging a pirq is binded with a event channel
>
>This is quite a long title, I think we can make it shorter. Maybe "msi:
>Handle MSI remapping format.

OK.

>
>>      data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));
>> -    if (xen_is_pirq_msi(data)) {
>> +    addr_lo = pci_get_word(dev->config + msi_address_lo_off(dev));
>
>This could be get_long, so addr_lo will actually have the all low bits
>of the addr.

yes.

>
>> +int xen_is_pirq_msi(uint32_t msi_data, uint32_t msi_addr_lo)
>>  {
>> +    /* If msi address is configurate to remapping format, the msi will not
>> +     * remapped into a pirq.
>> +     */
>> +    if ( msi_addr_lo & 0x10 )
>
>That's a magic number, is 0x10 MSI_ADDR_IM_MASK?

Yes.

Really thanks for your comments.

>
>Thanks,
>
>-- 
>Anthony PERARD

Re: [RFC PATCH 4/4] msi: taking interrupt format into consideration during judging a pirq is binded with a event channel

[Chao Gao]

On Thu, Mar 30, 2017 at 06:29:29PM +0100, Anthony PERARD wrote:
>On Fri, Mar 17, 2017 at 07:29:17PM +0800, Lan Tianyu wrote:
>> From: Chao Gao <chao.gao@intel.com>
>> Subject: msi: taking interrupt format into consideration during
>> judging a pirq is binded with a event channel
>
>This is quite a long title, I think we can make it shorter. Maybe "msi:
>Handle MSI remapping format.

OK.

>
>>      data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));
>> -    if (xen_is_pirq_msi(data)) {
>> +    addr_lo = pci_get_word(dev->config + msi_address_lo_off(dev));
>
>This could be get_long, so addr_lo will actually have the all low bits
>of the addr.

yes.

>
>> +int xen_is_pirq_msi(uint32_t msi_data, uint32_t msi_addr_lo)
>>  {
>> +    /* If msi address is configurate to remapping format, the msi will not
>> +     * remapped into a pirq.
>> +     */
>> +    if ( msi_addr_lo & 0x10 )
>
>That's a magic number, is 0x10 MSI_ADDR_IM_MASK?

Yes.

Really thanks for your comments.

>
>Thanks,
>
>-- 
>Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 1489750157-17401-1-git-send-email-tianyu.lan@intel.com
Subject: [Qemu-devel] [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1489750157-17401-1-git-send-email-tianyu.lan@intel.com -> patchew/1489750157-17401-1-git-send-email-tianyu.lan@intel.com
Switched to a new branch 'test'
0f16022 msi: taking interrupt format into consideration during judging a pirq is binded with a event channel
62b6353 xen-pt: bind/unbind interrupt remapping format MSI
0900f2b Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen
7cb3e06 I440: Allow adding sysbus devices with -device on I440

=== OUTPUT BEGIN ===
Checking PATCH 1/4: I440: Allow adding sysbus devices with -device on I440...
Checking PATCH 2/4: Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen...
ERROR: trailing whitespace
#75: FILE: hw/xen/xen_viommu.c:48:
+    $

ERROR: trailing whitespace
#79: FILE: hw/xen/xen_viommu.c:52:
+    rc = xenstore_read_uint64(viommu_path, "base_addr", &s->base_addr);  $

ERROR: trailing whitespace
#116: FILE: hw/xen/xen_viommu.c:89:
+        rc = xc_viommu_destroy(xen_xc, xen_domid, s->id); $

ERROR: trailing whitespace
#140: FILE: hw/xen/xen_viommu.c:113:
+    type_register_static(&xen_viommu_info); $

total: 4 errors, 0 warnings, 120 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/4: xen-pt: bind/unbind interrupt remapping format MSI...
ERROR: else should follow close brace '}'
#36: FILE: hw/xen/xen_pt_msi.c:179:
+    }
+    else {

ERROR: space prohibited after that open parenthesis '('
#54: FILE: hw/xen/xen_pt_msi.c:214:
+        if ( addr & MSI_ADDR_IM_MASK ) {

ERROR: space prohibited before that close parenthesis ')'
#54: FILE: hw/xen/xen_pt_msi.c:214:
+        if ( addr & MSI_ADDR_IM_MASK ) {

WARNING: line over 80 characters
#55: FILE: hw/xen/xen_pt_msi.c:215:
+            XEN_PT_LOG(d, "Unbinding of MSI%s . ( pirq: %d, data: %x, addr: %lx)\n",

ERROR: line over 90 characters
#60: FILE: hw/xen/xen_pt_msi.c:220:
+                XEN_PT_ERR(d, "Unbinding of MSI%s . (error: %d, pirq: %d, data: %x, addr: %lx)\n",

WARNING: line over 80 characters
#68: FILE: hw/xen/xen_pt_msi.c:228:
+            rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);

ERROR: line over 90 characters
#70: FILE: hw/xen/xen_pt_msi.c:230:
+                XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",

total: 5 errors, 2 warnings, 61 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 4/4: msi: taking interrupt format into consideration during judging a pirq is binded with a event channel...
ERROR: space prohibited after that open parenthesis '('
#111: FILE: xen-hvm.c:154:
+    if ( msi_addr_lo & 0x10 )

ERROR: space prohibited before that close parenthesis ')'
#111: FILE: xen-hvm.c:154:
+    if ( msi_addr_lo & 0x10 )

ERROR: braces {} are necessary for all arms of this statement
#111: FILE: xen-hvm.c:154:
+    if ( msi_addr_lo & 0x10 )
[...]

total: 3 errors, 0 warnings, 67 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 1489750157-17401-1-git-send-email-tianyu.lan@intel.com
Subject: [Qemu-devel] [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1489750157-17401-1-git-send-email-tianyu.lan@intel.com -> patchew/1489750157-17401-1-git-send-email-tianyu.lan@intel.com
Switched to a new branch 'test'
0f16022 msi: taking interrupt format into consideration during judging a pirq is binded with a event channel
62b6353 xen-pt: bind/unbind interrupt remapping format MSI
0900f2b Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen
7cb3e06 I440: Allow adding sysbus devices with -device on I440

=== OUTPUT BEGIN ===
Checking PATCH 1/4: I440: Allow adding sysbus devices with -device on I440...
Checking PATCH 2/4: Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen...
ERROR: trailing whitespace
#75: FILE: hw/xen/xen_viommu.c:48:
+    $

ERROR: trailing whitespace
#79: FILE: hw/xen/xen_viommu.c:52:
+    rc = xenstore_read_uint64(viommu_path, "base_addr", &s->base_addr);  $

ERROR: trailing whitespace
#116: FILE: hw/xen/xen_viommu.c:89:
+        rc = xc_viommu_destroy(xen_xc, xen_domid, s->id); $

ERROR: trailing whitespace
#140: FILE: hw/xen/xen_viommu.c:113:
+    type_register_static(&xen_viommu_info); $

total: 4 errors, 0 warnings, 120 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/4: xen-pt: bind/unbind interrupt remapping format MSI...
ERROR: else should follow close brace '}'
#36: FILE: hw/xen/xen_pt_msi.c:179:
+    }
+    else {

ERROR: space prohibited after that open parenthesis '('
#54: FILE: hw/xen/xen_pt_msi.c:214:
+        if ( addr & MSI_ADDR_IM_MASK ) {

ERROR: space prohibited before that close parenthesis ')'
#54: FILE: hw/xen/xen_pt_msi.c:214:
+        if ( addr & MSI_ADDR_IM_MASK ) {

WARNING: line over 80 characters
#55: FILE: hw/xen/xen_pt_msi.c:215:
+            XEN_PT_LOG(d, "Unbinding of MSI%s . ( pirq: %d, data: %x, addr: %lx)\n",

ERROR: line over 90 characters
#60: FILE: hw/xen/xen_pt_msi.c:220:
+                XEN_PT_ERR(d, "Unbinding of MSI%s . (error: %d, pirq: %d, data: %x, addr: %lx)\n",

WARNING: line over 80 characters
#68: FILE: hw/xen/xen_pt_msi.c:228:
+            rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);

ERROR: line over 90 characters
#70: FILE: hw/xen/xen_pt_msi.c:230:
+                XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",

total: 5 errors, 2 warnings, 61 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 4/4: msi: taking interrupt format into consideration during judging a pirq is binded with a event channel...
ERROR: space prohibited after that open parenthesis '('
#111: FILE: xen-hvm.c:154:
+    if ( msi_addr_lo & 0x10 )

ERROR: space prohibited before that close parenthesis ')'
#111: FILE: xen-hvm.c:154:
+    if ( msi_addr_lo & 0x10 )

ERROR: braces {} are necessary for all arms of this statement
#111: FILE: xen-hvm.c:154:
+    if ( msi_addr_lo & 0x10 )
[...]

total: 3 errors, 0 warnings, 67 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[Paolo Bonzini]

On 17/03/2017 12:29, Lan Tianyu wrote:
> This patchset is to add Xen vIOMMU device model and handle
> irq remapping stuffs. Xen vIOMMU emulation is in the Xen hypervisor
> and the new device module in Qemu works as hypercall wrappers to
> create and destroy vIOMMU in hypervisor.
> 
> Xen only supports emulated I440 and so we enable vIOMMU with emulated
> I440 chipset. This works on Linux and Windows guest.

Any plans to change this?  Why is Xen not able to use Q35 with Intel
IOMMU, with only special hooks for interrupt remapping?

Paolo

> Chao Gao (4):
>   I440: Allow adding sysbus devices with -device on I440
>   Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen
>   xen-pt: bind/unbind interrupt remapping format MSI
>   msi: taking interrupt format into consideration during judging a pirq
>     is binded with a event channel

Re: [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[Paolo Bonzini]

On 17/03/2017 12:29, Lan Tianyu wrote:
> This patchset is to add Xen vIOMMU device model and handle
> irq remapping stuffs. Xen vIOMMU emulation is in the Xen hypervisor
> and the new device module in Qemu works as hypercall wrappers to
> create and destroy vIOMMU in hypervisor.
> 
> Xen only supports emulated I440 and so we enable vIOMMU with emulated
> I440 chipset. This works on Linux and Windows guest.

Any plans to change this?  Why is Xen not able to use Q35 with Intel
IOMMU, with only special hooks for interrupt remapping?

Paolo

> Chao Gao (4):
>   I440: Allow adding sysbus devices with -device on I440
>   Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen
>   xen-pt: bind/unbind interrupt remapping format MSI
>   msi: taking interrupt format into consideration during judging a pirq
>     is binded with a event channel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[Stefano Stabellini]

On Fri, 17 Mar 2017, Paolo Bonzini wrote:
> On 17/03/2017 12:29, Lan Tianyu wrote:
> > This patchset is to add Xen vIOMMU device model and handle
> > irq remapping stuffs. Xen vIOMMU emulation is in the Xen hypervisor
> > and the new device module in Qemu works as hypercall wrappers to
> > create and destroy vIOMMU in hypervisor.
> > 
> > Xen only supports emulated I440 and so we enable vIOMMU with emulated
> > I440 chipset. This works on Linux and Windows guest.
> 
> Any plans to change this?  Why is Xen not able to use Q35 with Intel
> IOMMU, with only special hooks for interrupt remapping?

CC'ing Anthony that worked on it in the past


> > Chao Gao (4):
> >   I440: Allow adding sysbus devices with -device on I440
> >   Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen
> >   xen-pt: bind/unbind interrupt remapping format MSI
> >   msi: taking interrupt format into consideration during judging a pirq
> >     is binded with a event channel
> 

Re: [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[Stefano Stabellini]

On Fri, 17 Mar 2017, Paolo Bonzini wrote:
> On 17/03/2017 12:29, Lan Tianyu wrote:
> > This patchset is to add Xen vIOMMU device model and handle
> > irq remapping stuffs. Xen vIOMMU emulation is in the Xen hypervisor
> > and the new device module in Qemu works as hypercall wrappers to
> > create and destroy vIOMMU in hypervisor.
> > 
> > Xen only supports emulated I440 and so we enable vIOMMU with emulated
> > I440 chipset. This works on Linux and Windows guest.
> 
> Any plans to change this?  Why is Xen not able to use Q35 with Intel
> IOMMU, with only special hooks for interrupt remapping?

CC'ing Anthony that worked on it in the past


> > Chao Gao (4):
> >   I440: Allow adding sysbus devices with -device on I440
> >   Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen
> >   xen-pt: bind/unbind interrupt remapping format MSI
> >   msi: taking interrupt format into consideration during judging a pirq
> >     is binded with a event channel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[Lan Tianyu]

On 2017年03月17日 22:48, Paolo Bonzini wrote:
> 
> 
> On 17/03/2017 12:29, Lan Tianyu wrote:
>> This patchset is to add Xen vIOMMU device model and handle
>> irq remapping stuffs. Xen vIOMMU emulation is in the Xen hypervisor
>> and the new device module in Qemu works as hypercall wrappers to
>> create and destroy vIOMMU in hypervisor.
>>
>> Xen only supports emulated I440 and so we enable vIOMMU with emulated
>> I440 chipset. This works on Linux and Windows guest.
> 
> Any plans to change this?  Why is Xen not able to use Q35 with Intel
> IOMMU, with only special hooks for interrupt remapping?
> 
> Paolo
> 

Hi Paolo:
Thanks for review. For Xen side, we won't reuse Intel IOMMU device model
in Qemu and create counterpart in Xen hypervisor. The reasons are
 1) Avoid round trips between Qemu and Xen hypervisor
 2) Ease of integration with the rest part of the hypervisor(vIOAPIC,
vMSI and so on).

We don't have plan to enable Q35 for Xen now. There is no dependency
between Q35 emulation and vIOMMU implementation from our test. As
Stefano mentioned, Anthony worked on the Q35 support before. These two
tasks can be done in parallel.

-- 
Best regards
Tianyu Lan

Re: [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[Lan Tianyu]

On 2017年03月17日 22:48, Paolo Bonzini wrote:
> 
> 
> On 17/03/2017 12:29, Lan Tianyu wrote:
>> This patchset is to add Xen vIOMMU device model and handle
>> irq remapping stuffs. Xen vIOMMU emulation is in the Xen hypervisor
>> and the new device module in Qemu works as hypercall wrappers to
>> create and destroy vIOMMU in hypervisor.
>>
>> Xen only supports emulated I440 and so we enable vIOMMU with emulated
>> I440 chipset. This works on Linux and Windows guest.
> 
> Any plans to change this?  Why is Xen not able to use Q35 with Intel
> IOMMU, with only special hooks for interrupt remapping?
> 
> Paolo
> 

Hi Paolo:
Thanks for review. For Xen side, we won't reuse Intel IOMMU device model
in Qemu and create counterpart in Xen hypervisor. The reasons are
 1) Avoid round trips between Qemu and Xen hypervisor
 2) Ease of integration with the rest part of the hypervisor(vIOAPIC,
vMSI and so on).

We don't have plan to enable Q35 for Xen now. There is no dependency
between Q35 emulation and vIOMMU implementation from our test. As
Stefano mentioned, Anthony worked on the Q35 support before. These two
tasks can be done in parallel.

-- 
Best regards
Tianyu Lan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[Paolo Bonzini]

On 20/03/2017 03:40, Lan Tianyu wrote:
>>> Xen only supports emulated I440 and so we enable vIOMMU with emulated
>>> I440 chipset. This works on Linux and Windows guest.
>> Any plans to change this?  Why is Xen not able to use Q35 with Intel
>> IOMMU, with only special hooks for interrupt remapping?
>>
>> Paolo
>>
> Hi Paolo:
> Thanks for review. For Xen side, we won't reuse Intel IOMMU device model
> in Qemu and create counterpart in Xen hypervisor. The reasons are
>  1) Avoid round trips between Qemu and Xen hypervisor
>  2) Ease of integration with the rest part of the hypervisor(vIOAPIC,
> vMSI and so on).

Fair enough, though I'd be worried about increasing the attack surface
of the hypervisor.  For KVM, for example, IOMMU emulation requires using
the "split irqchip" feature to move the PIC and IOAPIC out of the kernel
and back to QEMU.

Also, I think this series is missing changes to support IOMMU
translation in the vIOMMU device model.

Paolo

> We don't have plan to enable Q35 for Xen now. There is no dependency
> between Q35 emulation and vIOMMU implementation from our test. As
> Stefano mentioned, Anthony worked on the Q35 support before. These two
> tasks can be done in parallel.

Re: [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[Paolo Bonzini]

On 20/03/2017 03:40, Lan Tianyu wrote:
>>> Xen only supports emulated I440 and so we enable vIOMMU with emulated
>>> I440 chipset. This works on Linux and Windows guest.
>> Any plans to change this?  Why is Xen not able to use Q35 with Intel
>> IOMMU, with only special hooks for interrupt remapping?
>>
>> Paolo
>>
> Hi Paolo:
> Thanks for review. For Xen side, we won't reuse Intel IOMMU device model
> in Qemu and create counterpart in Xen hypervisor. The reasons are
>  1) Avoid round trips between Qemu and Xen hypervisor
>  2) Ease of integration with the rest part of the hypervisor(vIOAPIC,
> vMSI and so on).

Fair enough, though I'd be worried about increasing the attack surface
of the hypervisor.  For KVM, for example, IOMMU emulation requires using
the "split irqchip" feature to move the PIC and IOAPIC out of the kernel
and back to QEMU.

Also, I think this series is missing changes to support IOMMU
translation in the vIOMMU device model.

Paolo

> We don't have plan to enable Q35 for Xen now. There is no dependency
> between Q35 emulation and vIOMMU implementation from our test. As
> Stefano mentioned, Anthony worked on the Q35 support before. These two
> tasks can be done in parallel.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [Xen-devel] [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[Roger Pau Monné]

On Mon, Mar 20, 2017 at 12:38:41PM +0100, Paolo Bonzini wrote:
> 
> 
> On 20/03/2017 03:40, Lan Tianyu wrote:
> >>> Xen only supports emulated I440 and so we enable vIOMMU with emulated
> >>> I440 chipset. This works on Linux and Windows guest.
> >> Any plans to change this?  Why is Xen not able to use Q35 with Intel
> >> IOMMU, with only special hooks for interrupt remapping?
> >>
> >> Paolo
> >>
> > Hi Paolo:
> > Thanks for review. For Xen side, we won't reuse Intel IOMMU device model
> > in Qemu and create counterpart in Xen hypervisor. The reasons are
> >  1) Avoid round trips between Qemu and Xen hypervisor
> >  2) Ease of integration with the rest part of the hypervisor(vIOAPIC,
> > vMSI and so on).
> 
> Fair enough, though I'd be worried about increasing the attack surface
> of the hypervisor.  For KVM, for example, IOMMU emulation requires using
> the "split irqchip" feature to move the PIC and IOAPIC out of the kernel
> and back to QEMU.

Yes, that's right, we are increasing the surface of attack. But Xen also needs
it in order to support APIC IDs > 255 on PVH guests (that have a local APIC but
no QEMU).

Roger.

Re: [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[Roger Pau Monné]

On Mon, Mar 20, 2017 at 12:38:41PM +0100, Paolo Bonzini wrote:
> 
> 
> On 20/03/2017 03:40, Lan Tianyu wrote:
> >>> Xen only supports emulated I440 and so we enable vIOMMU with emulated
> >>> I440 chipset. This works on Linux and Windows guest.
> >> Any plans to change this?  Why is Xen not able to use Q35 with Intel
> >> IOMMU, with only special hooks for interrupt remapping?
> >>
> >> Paolo
> >>
> > Hi Paolo:
> > Thanks for review. For Xen side, we won't reuse Intel IOMMU device model
> > in Qemu and create counterpart in Xen hypervisor. The reasons are
> >  1) Avoid round trips between Qemu and Xen hypervisor
> >  2) Ease of integration with the rest part of the hypervisor(vIOAPIC,
> > vMSI and so on).
> 
> Fair enough, though I'd be worried about increasing the attack surface
> of the hypervisor.  For KVM, for example, IOMMU emulation requires using
> the "split irqchip" feature to move the PIC and IOAPIC out of the kernel
> and back to QEMU.

Yes, that's right, we are increasing the surface of attack. But Xen also needs
it in order to support APIC IDs > 255 on PVH guests (that have a local APIC but
no QEMU).

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [Xen-devel] [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[Paolo Bonzini]

On 20/03/2017 15:17, Roger Pau Monné wrote:
>>> Hi Paolo:
>>> Thanks for review. For Xen side, we won't reuse Intel IOMMU device model
>>> in Qemu and create counterpart in Xen hypervisor. The reasons are
>>>  1) Avoid round trips between Qemu and Xen hypervisor
>>>  2) Ease of integration with the rest part of the hypervisor(vIOAPIC,
>>> vMSI and so on).
>> Fair enough, though I'd be worried about increasing the attack surface
>> of the hypervisor.  For KVM, for example, IOMMU emulation requires using
>> the "split irqchip" feature to move the PIC and IOAPIC out of the kernel
>> and back to QEMU.
> Yes, that's right, we are increasing the surface of attack. But Xen also needs
> it in order to support APIC IDs > 255 on PVH guests (that have a local APIC but
> no QEMU).

Not necessarily, you only need x2APIC support for that in the local APIC
emulation.  The MSI hypercalls (similar to KVM's MSI ioctls) can be
extended to accept x2APIC VCPU ids.

Paolo

Re: [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[Paolo Bonzini]

On 20/03/2017 15:17, Roger Pau Monné wrote:
>>> Hi Paolo:
>>> Thanks for review. For Xen side, we won't reuse Intel IOMMU device model
>>> in Qemu and create counterpart in Xen hypervisor. The reasons are
>>>  1) Avoid round trips between Qemu and Xen hypervisor
>>>  2) Ease of integration with the rest part of the hypervisor(vIOAPIC,
>>> vMSI and so on).
>> Fair enough, though I'd be worried about increasing the attack surface
>> of the hypervisor.  For KVM, for example, IOMMU emulation requires using
>> the "split irqchip" feature to move the PIC and IOAPIC out of the kernel
>> and back to QEMU.
> Yes, that's right, we are increasing the surface of attack. But Xen also needs
> it in order to support APIC IDs > 255 on PVH guests (that have a local APIC but
> no QEMU).

Not necessarily, you only need x2APIC support for that in the local APIC
emulation.  The MSI hypercalls (similar to KVM's MSI ioctls) can be
extended to accept x2APIC VCPU ids.

Paolo

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[Lan Tianyu]

On 2017年03月20日 19:38, Paolo Bonzini wrote:
> Fair enough, though I'd be worried about increasing the attack surface
> of the hypervisor.  For KVM, for example, IOMMU emulation requires using
> the "split irqchip" feature to move the PIC and IOAPIC out of the kernel
> and back to QEMU.

Yes, just like Roger mentioned we also need to support no-qemu mode on
Xen and this is tradeoff result.

> 
> Also, I think this series is missing changes to support IOMMU
> translation in the vIOMMU device model.

Yes, this series just enabled vIOMMU's irq remapping function and we
need to pass virtual device's DMA request to Xen hypervisor for
translation when enable DMA translation.

-- 
Best regards
Tianyu Lan

Re: [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support

[Lan Tianyu]

On 2017年03月20日 19:38, Paolo Bonzini wrote:
> Fair enough, though I'd be worried about increasing the attack surface
> of the hypervisor.  For KVM, for example, IOMMU emulation requires using
> the "split irqchip" feature to move the PIC and IOAPIC out of the kernel
> and back to QEMU.

Yes, just like Roger mentioned we also need to support no-qemu mode on
Xen and this is tradeoff result.

> 
> Also, I think this series is missing changes to support IOMMU
> translation in the vIOMMU device model.

Yes, this series just enabled vIOMMU's irq remapping function and we
need to pass virtual device's DMA request to Xen hypervisor for
translation when enable DMA translation.

-- 
Best regards
Tianyu Lan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel