[Qemu-devel] [RFC PATCH V2 0/2] Qemu: Add Xen vIOMMU interrupt remapping function.

[Qemu-devel] [RFC PATCH V2 0/2] Qemu: Add Xen vIOMMU interrupt remapping function.

[Lan Tianyu]

Change since V1:
       1) Move create/destroy vIOMMU and query vIOMMU capabilities to tool stack.
       2) Fix some code style issue.

This patchset is to deal with MSI interrupt remapping request when guest
updates MSI registers.

Repo:
https://github.com/lantianyu/qemu/tree/xen_viommu_rfc_v2

Chao Gao (2):
  xen-pt: bind/unbind interrupt remapping format MSI
  msi: Handle remappable format interrupt request

 hw/pci/msi.c                  |  5 +++--
 hw/pci/msix.c                 |  4 +++-
 hw/xen/xen_pt_msi.c           | 52 +++++++++++++++++++++++++++++++------------
 include/hw/i386/apic-msidef.h |  3 ++-
 include/hw/xen/xen.h          |  2 +-
 xen-hvm-stub.c                |  2 +-
 xen-hvm.c                     |  7 +++++-
 7 files changed, 54 insertions(+), 21 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [RFC PATCH V2 1/2] 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           | 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          0x00ffff0
+#define  MSI_ADDR_DEST_ID_MASK          0x000fff00
+#define  MSI_ADDR_IF_MASK               0x00000010
 
 #endif /* HW_APIC_MSIDEF_H */
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH V2 2/2] msi: Handle remappable format interrupt request

[Lan Tianyu]

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

According to VT-d spec Interrupt Remapping and Interrupt Posting ->
Interrupt Remapping -> Interrupt Request Formats On Intel 64
Platforms, fields of MSI data register have changed. This patch
avoids wrongly regarding a remappable format interrupt request as
an interrupt binded with an event channel.

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..199cb47 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_long(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 bb54e8b..efe2982 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -82,9 +82,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 5fab95e..45a9e9f 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, addr)) {
         *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 09c2ce5..af759bc 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 5043beb..db29121 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 & MSI_ADDR_IF_MASK)
+        return 0;
     /* If vector is 0, the msi is remapped into a pirq, passed as
      * dest_id.
      */
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH V2 0/2] Qemu: Add Xen vIOMMU interrupt remapping function.

[no-reply]

Hi,

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

Subject: [Qemu-devel] [RFC PATCH V2 0/2] Qemu: Add Xen vIOMMU interrupt remapping function.
Type: series
Message-id: 1495085580-10631-1-git-send-email-tianyu.lan@intel.com

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

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

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
Switched to a new branch 'test'
cbe4736 msi: Handle remappable format interrupt request
65e2601 xen-pt: bind/unbind interrupt remapping format MSI

=== OUTPUT BEGIN ===
Checking PATCH 1/2: xen-pt: bind/unbind interrupt remapping format MSI...
ERROR: spaces required around that ':' (ctx:VxW)
#35: FILE: hw/xen/xen_pt_msi.c:172:
+                   is_msix ? "-X": "", addr, data);
                                  ^

ERROR: else should follow close brace '}'
#39: FILE: hw/xen/xen_pt_msi.c:176:
+    }
+    else {

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

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

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

total: 4 errors, 1 warnings, 74 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 2/2: msi: Handle remappable format interrupt request...
ERROR: braces {} are necessary for all arms of this statement
#30: FILE: hw/i386/xen/xen-hvm.c:154:
+    if (msi_addr_lo & MSI_ADDR_IF_MASK)
[...]

total: 1 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 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 <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           | 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          0x00ffff0
> +#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          0x000ffff0


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

Thanks,

-- 
Anthony PERARD

Re: [Qemu-devel] [RFC PATCH V2 2/2] msi: Handle remappable format interrupt request

[Anthony PERARD]

On Thu, May 18, 2017 at 01:33:00AM -0400, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@intel.com>
> 
> According to VT-d spec Interrupt Remapping and Interrupt Posting ->
> Interrupt Remapping -> Interrupt Request Formats On Intel 64
> Platforms, fields of MSI data register have changed. This patch
> avoids wrongly regarding a remappable format interrupt request as
> an interrupt binded with an event channel.
> 
> 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..199cb47 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_long(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 bb54e8b..efe2982 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -82,9 +82,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 5fab95e..45a9e9f 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, addr)) {
>          *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 09c2ce5..af759bc 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);

Maybe inverting the arguments would be better, so the arguments would be
the address first, then the data, like I think it is often the case.
What do you think?

>  
>  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 5043beb..db29121 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.

What do you think of: "If the MSI address is configured in remappable
format, the MSI will not be remapped into a pirq." ?

> +     */
> +    if (msi_addr_lo & MSI_ADDR_IF_MASK)
> +        return 0;
>      /* If vector is 0, the msi is remapped into a pirq, passed as
>       * dest_id.
>       */

Thanks,

-- 
Anthony PERARD

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

[Jan Beulich]

>>> On 19.05.17 at 13:16, <anthony.perard@citrix.com> wrote:
> On Thu, May 18, 2017 at 01:32:59AM -0400, Lan Tianyu wrote:
>> --- 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          0x00ffff0
>> +#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          0x000ffff0

Judging from other sources, rather the other way around - the
mask needs to have further bits removed (should be 0x000ff000
afaict). Xen sources confirm this, and while Linux has the value
you suggest, that contradicts

#define MSI_ADDR_DEST_ID_SHIFT		12
#define  MSI_ADDR_DEST_ID(dest)		(((dest) << MSI_ADDR_DEST_ID_SHIFT) & \
					 MSI_ADDR_DEST_ID_MASK)

as well as

#define MSI_ADDR_EXT_DEST_ID(dest)	((dest) & 0xffffff00)

chopping off just the low 8 bits.

Jan

Re: [Qemu-devel] [RFC PATCH V2 2/2] msi: Handle remappable format interrupt request

[Lan Tianyu]

Hi Anthony:
	Thanks for your review.

On 2017年05月19日 19:57, Anthony PERARD wrote:
> On Thu, May 18, 2017 at 01:33:00AM -0400, Lan Tianyu wrote:
>> From: Chao Gao <chao.gao@intel.com>
>>
>> According to VT-d spec Interrupt Remapping and Interrupt Posting ->
>> Interrupt Remapping -> Interrupt Request Formats On Intel 64
>> Platforms, fields of MSI data register have changed. This patch
>> avoids wrongly regarding a remappable format interrupt request as
>> an interrupt binded with an event channel.
>>
>> 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..199cb47 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_long(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 bb54e8b..efe2982 100644
>> --- a/hw/pci/msix.c
>> +++ b/hw/pci/msix.c
>> @@ -82,9 +82,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 5fab95e..45a9e9f 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, addr)) {
>>          *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 09c2ce5..af759bc 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);
> 
> Maybe inverting the arguments would be better, so the arguments would be
> the address first, then the data, like I think it is often the case.
> What do you think?


That make sense. Will update.

> 
>>  
>>  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 5043beb..db29121 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.
> 
> What do you think of: "If the MSI address is configured in remappable
> format, the MSI will not be remapped into a pirq." ?

Will update.

> 
>> +     */
>> +    if (msi_addr_lo & MSI_ADDR_IF_MASK)
>> +        return 0;
>>      /* If vector is 0, the msi is remapped into a pirq, passed as
>>       * dest_id.
>>       */
> 
> Thanks,
> 


-- 
Best regards
Tianyu Lan

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

[Lan Tianyu]

On 2017年05月19日 20:04, Jan Beulich wrote:
>>>> On 19.05.17 at 13:16, <anthony.perard@citrix.com> wrote:
>> On Thu, May 18, 2017 at 01:32:59AM -0400, Lan Tianyu wrote:
>>> --- 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          0x00ffff0
>>> +#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          0x000ffff0
> Judging from other sources, rather the other way around - the
> mask needs to have further bits removed (should be 0x000ff000
> afaict). Xen sources confirm this, and while Linux has the value
> you suggest, that contradicts
Agree. Defining the mask as "0x000ff000" makes more sense.
Just check Qemu source code. Only apic_send_msi() and msi_dest_id() use
the mask
to get dest apic id. They mask MSI address field with 
MSI_ADDR_DEST_ID_MASK and
then right-shift 12bit. The low 12bit won't be used.

Anthony, does this make sense?

-- 
Best regards
Tianyu Lan

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

[Anthony PERARD]

On Tue, May 23, 2017 at 08:16:25PM +0800, Lan Tianyu wrote:
> On 2017年05月19日 20:04, Jan Beulich wrote:
> >>>> On 19.05.17 at 13:16, <anthony.perard@citrix.com> wrote:
> >> On Thu, May 18, 2017 at 01:32:59AM -0400, Lan Tianyu wrote:
> >>> --- 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          0x00ffff0
> >>> +#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          0x000ffff0
> > Judging from other sources, rather the other way around - the
> > mask needs to have further bits removed (should be 0x000ff000
> > afaict). Xen sources confirm this, and while Linux has the value
> > you suggest, that contradicts
> Agree. Defining the mask as "0x000ff000" makes more sense.
> Just check Qemu source code. Only apic_send_msi() and msi_dest_id() use
> the mask
> to get dest apic id. They mask MSI address field with 
> MSI_ADDR_DEST_ID_MASK and
> then right-shift 12bit. The low 12bit won't be used.
> 
> Anthony, does this make sense?

Yes, it does.
The change to MSI_ADDR_DEST_ID_MASK should probably go in its own patch.

-- 
Anthony PERARD

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

[Lan Tianyu]

On 2017年05月24日 01:06, Anthony PERARD wrote:
> On Tue, May 23, 2017 at 08:16:25PM +0800, Lan Tianyu wrote:
>> On 2017年05月19日 20:04, Jan Beulich wrote:
>>>>>> On 19.05.17 at 13:16, <anthony.perard@citrix.com> wrote:
>>>> On Thu, May 18, 2017 at 01:32:59AM -0400, Lan Tianyu wrote:
>>>>> --- 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          0x00ffff0
>>>>> +#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          0x000ffff0
>>> Judging from other sources, rather the other way around - the
>>> mask needs to have further bits removed (should be 0x000ff000
>>> afaict). Xen sources confirm this, and while Linux has the value
>>> you suggest, that contradicts
>> Agree. Defining the mask as "0x000ff000" makes more sense.
>> Just check Qemu source code. Only apic_send_msi() and msi_dest_id() use
>> the mask
>> to get dest apic id. They mask MSI address field with 
>> MSI_ADDR_DEST_ID_MASK and
>> then right-shift 12bit. The low 12bit won't be used.
>>
>> Anthony, does this make sense?
> Yes, it does.
> The change to MSI_ADDR_DEST_ID_MASK should probably go in its own patch.
>
OK. Will update.


-- 
Best regards
Tianyu Lan