Re: [Qemu-devel] [RFC PATCH V2 1/2] xen-pt: bind/unbind interrupt remapping format MSI

[Anthony PERARD]

On Thu, May 18, 2017 at 01:32:59AM -0400, Lan Tianyu wrote:
> From: Chao Gao 
> 
> 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 
> Signed-off-by: Lan Tianyu 
> ---
>  hw/xen/xen_pt_msi.c   | 50 
> ---
>  include/hw/i386/apic-msidef.h |  3 ++-
>  2 files changed, 39 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> index 62add06..5fab95e 100644
> --- a/hw/xen/xen_pt_msi.c
> +++ b/hw/xen/xen_pt_msi.c
> @@ -163,16 +163,24 @@ static int msi_msix_update(XenPCIPassthroughState *s,
>  int rc = 0;
>  uint64_t table_addr = 0;
>  
> -XEN_PT_LOG(d, "Updating MSI%s with pirq %d gvec %#x gflags %#x"
> -   " (entry: %#x)\n",
> -   is_msix ? "-X" : "", pirq, gvec, gflags, msix_entry);
> -
>  if (is_msix) {
>  table_addr = s->msix->mmio_base_addr;
>  }
>  
> -rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
> -  pirq, gflags, table_addr);
> +if (addr & MSI_ADDR_IF_MASK) {
> +XEN_PT_LOG(d, "Updating MSI%s with addr %#" PRIx64 "data %#x\n",

With a space before "data", I think it will be easier to read the debug
log.

> +   is_msix ? "-X": "", addr, data);
> +rc = xc_domain_update_msi_irq_remapping(xen_xc, xen_domid, pirq,
> + d->devfn, data, addr, 
> table_addr);

We are going to need a stub function for
xc_domain_update_msi_irq_remapping(), when Xen does not have support for
it, so QEMU can compile in any case. (same for unbind version.)

I think the stub can just return -ENOSYS. That going to require changes
in configure to detect newer xen version and the stub can be in
xen_common.h.

> +}
> +else {
> +XEN_PT_LOG(d, "Updating MSI%s with pirq %d gvec %#x gflags %#x"
> +   " (entry: %#x)\n",
> +   is_msix ? "-X" : "", pirq, gvec, gflags, msix_entry);
> +
> +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 +212,29 @@ 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_IF_MASK ) {
> +XEN_PT_LOG(d, "Unbinding of MSI%s . ( pirq: %d, data: %x, "
> +   "addr: %#" PRIx64 ")\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: %#" PRIx64 ")\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..2c450f9 100644
> --- a/include/hw/i386/apic-msidef.h
> +++ b/include/hw/i386/apic-msidef.h
> @@ -26,6 +26,7 @@
>  
>  #define MSI_ADDR_DEST_ID_SHIFT  12
>  #define MSI_ADDR_DEST_IDX_SHIFT 4
> -#define  MSI_ADDR_DEST_ID_MASK  0x000
> +#define  MSI_ADDR_DEST_ID_MASK  0x000fff00

The value of MSI_ADDR_DEST_ID_MASK is changed here. I think the patch
should be:
+#define  MSI_ADDR_DEST_ID_MASK  0x0000


> +#define  MSI_ADDR_IF_MASK   0x0010
>  
>  #endif /* HW_APIC_MSIDEF_H */

Thanks,

-- 
Anthony PERARD

[Qemu-devel] [RFC PATCH V2 1/2] xen-pt: bind/unbind interrupt remapping format MSI

[Lan Tianyu]

From: Chao Gao 

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 
Signed-off-by: Lan Tianyu 
---
 hw/xen/xen_pt_msi.c   | 50 ---
 include/hw/i386/apic-msidef.h |  3 ++-
 2 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index 62add06..5fab95e 100644
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -163,16 +163,24 @@ static int msi_msix_update(XenPCIPassthroughState *s,
 int rc = 0;
 uint64_t table_addr = 0;
 
-XEN_PT_LOG(d, "Updating MSI%s with pirq %d gvec %#x gflags %#x"
-   " (entry: %#x)\n",
-   is_msix ? "-X" : "", pirq, gvec, gflags, msix_entry);
-
 if (is_msix) {
 table_addr = s->msix->mmio_base_addr;
 }
 
-rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
-  pirq, gflags, table_addr);
+if (addr & MSI_ADDR_IF_MASK) {
+XEN_PT_LOG(d, "Updating MSI%s with addr %#" PRIx64 "data %#x\n",
+   is_msix ? "-X": "", addr, data);
+rc = xc_domain_update_msi_irq_remapping(xen_xc, xen_domid, pirq,
+ d->devfn, data, addr, table_addr);
+}
+else {
+XEN_PT_LOG(d, "Updating MSI%s with pirq %d gvec %#x gflags %#x"
+   " (entry: %#x)\n",
+   is_msix ? "-X" : "", pirq, gvec, gflags, msix_entry);
+
+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 +212,29 @@ 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_IF_MASK ) {
+XEN_PT_LOG(d, "Unbinding of MSI%s . ( pirq: %d, data: %x, "
+   "addr: %#" PRIx64 ")\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: %#" PRIx64 ")\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..2c450f9 100644
--- a/include/hw/i386/apic-msidef.h
+++ b/include/hw/i386/apic-msidef.h
@@ -26,6 +26,7 @@
 
 #define MSI_ADDR_DEST_ID_SHIFT  12
 #define MSI_ADDR_DEST_IDX_SHIFT 4
-#define  MSI_ADDR_DEST_ID_MASK  0x000
+#define  MSI_ADDR_DEST_ID_MASK  0x000fff00
+#define  MSI_ADDR_IF_MASK   0x0010
 
 #endif /* HW_APIC_MSIDEF_H */
-- 
1.8.3.1