[PATCH v2 0/3] xen/vpci: msix: Make VPCI MSI-X code non-x86 specific

[PATCH v2 0/3] xen/vpci: msix: Make VPCI MSI-X code non-x86 specific

[Rahul Singh]

This patch series is v2 of preparatory work to make VPCI MSI-X code non-x86
specific.

Rahul Singh (3):
  xen/vpci: msix: move x86 specific code to x86 file
  xen/vpci: msix: change return value of vpci_msix_{read,write}
  xen/vpci: msix: move read/write call to MSI-X PBA entry to arch file

 xen/arch/x86/hvm/vmsi.c             | 155 +++++++++++++++++++++++++++
 xen/arch/x86/include/asm/msi.h      |  28 -----
 xen/arch/x86/msi.c                  |   2 +-
 xen/drivers/passthrough/amd/iommu.h |   1 +
 xen/drivers/vpci/msi.c              |   3 +-
 xen/drivers/vpci/msix.c             | 158 +++-------------------------
 xen/include/xen/msi.h               |  28 +++++
 xen/include/xen/vpci.h              |  19 ++++
 8 files changed, 222 insertions(+), 172 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Rahul Singh]

vpci/msix.c file will be used for arm architecture when vpci msix
support will be added to ARM, but there is x86 specific code in this
file.

Move x86 specific code to the x86/hvm/vmsi.c file to make sure common
code will be used for other architecture.

No functional change intended.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
Changes since v1:
 - Split return value of vpci_msix_{read,write} to separate patch.
 - Fix the {read,write}{l,q} call in common code to move to arch specific file. 
---
 xen/arch/x86/hvm/vmsi.c             | 102 ++++++++++++++++++++++++++++
 xen/arch/x86/include/asm/msi.h      |  28 --------
 xen/arch/x86/msi.c                  |   2 +-
 xen/drivers/passthrough/amd/iommu.h |   1 +
 xen/drivers/vpci/msi.c              |   3 +-
 xen/drivers/vpci/msix.c             | 101 +++------------------------
 xen/include/xen/msi.h               |  28 ++++++++
 xen/include/xen/vpci.h              |  13 ++++
 8 files changed, 154 insertions(+), 124 deletions(-)

diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 13e2a190b4..17426f238c 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
 
     return 0;
 }
+
+int vpci_make_msix_hole(const struct pci_dev *pdev)
+{
+    struct domain *d = pdev->domain;
+    unsigned int i;
+
+    if ( !pdev->vpci->msix )
+        return 0;
+
+    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
+    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
+    {
+        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
+        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
+                                     vmsix_table_size(pdev->vpci, i) - 1);
+
+        for ( ; start <= end; start++ )
+        {
+            p2m_type_t t;
+            mfn_t mfn = get_gfn_query(d, start, &t);
+
+            switch ( t )
+            {
+            case p2m_mmio_dm:
+            case p2m_invalid:
+                break;
+            case p2m_mmio_direct:
+                if ( mfn_x(mfn) == start )
+                {
+                    clear_identity_p2m_entry(d, start);
+                    break;
+                }
+                /* fallthrough. */
+            default:
+                put_gfn(d, start);
+                gprintk(XENLOG_WARNING,
+                        "%pp: existing mapping (mfn: %" PRI_mfn
+                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
+                        &pdev->sbdf, mfn_x(mfn), t, start);
+                return -EEXIST;
+            }
+            put_gfn(d, start);
+        }
+    }
+
+    return 0;
+}
+
+struct vpci_msix *vpci_msix_find(const struct domain *d, unsigned long addr)
+{
+    struct vpci_msix *msix;
+
+    list_for_each_entry ( msix, &d->arch.hvm.msix_tables, next )
+    {
+        const struct vpci_bar *bars = msix->pdev->vpci->header.bars;
+        unsigned int i;
+
+        for ( i = 0; i < ARRAY_SIZE(msix->tables); i++ )
+            if ( bars[msix->tables[i] & PCI_MSIX_BIRMASK].enabled &&
+                 VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, i) )
+                return msix;
+    }
+
+    return NULL;
+}
+
+static int x86_msix_accept(struct vcpu *v, unsigned long addr)
+{
+    return !!vpci_msix_find(v->domain, addr);
+}
+
+static int x86_msix_write(struct vcpu *v, unsigned long addr, unsigned int len,
+                          unsigned long data)
+{
+    const struct domain *d = v->domain;
+    struct vpci_msix *msix = vpci_msix_find(d, addr);
+
+    return vpci_msix_write(msix, addr, len, data);
+}
+
+static int x86_msix_read(struct vcpu *v, unsigned long addr, unsigned int len,
+                         unsigned long *data)
+{
+    const struct domain *d = v->domain;
+    struct vpci_msix *msix = vpci_msix_find(d, addr);
+
+    return vpci_msix_read(msix, addr, len, data);
+}
+
+static const struct hvm_mmio_ops vpci_msix_table_ops = {
+    .check = x86_msix_accept,
+    .read = x86_msix_read,
+    .write = x86_msix_write,
+};
+
+void vpci_msix_arch_register(struct vpci_msix *msix, struct domain *d)
+{
+    if ( list_empty(&d->arch.hvm.msix_tables) )
+        register_mmio_handler(d, &vpci_msix_table_ops);
+
+    list_add(&msix->next, &d->arch.hvm.msix_tables);
+}
 #endif /* CONFIG_HAS_VPCI */
diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
index e228b0f3f3..0a7912e9be 100644
--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -148,34 +148,6 @@ int msi_free_irq(struct msi_desc *entry);
  */
 #define NR_HP_RESERVED_VECTORS 	20
 
-#define msi_control_reg(base)		(base + PCI_MSI_FLAGS)
-#define msi_lower_address_reg(base)	(base + PCI_MSI_ADDRESS_LO)
-#define msi_upper_address_reg(base)	(base + PCI_MSI_ADDRESS_HI)
-#define msi_data_reg(base, is64bit)	\
-	( (is64bit == 1) ? base+PCI_MSI_DATA_64 : base+PCI_MSI_DATA_32 )
-#define msi_mask_bits_reg(base, is64bit) \
-	( (is64bit == 1) ? base+PCI_MSI_MASK_BIT : base+PCI_MSI_MASK_BIT-4)
-#define msi_pending_bits_reg(base, is64bit) \
-	((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
-#define msi_disable(control)		control &= ~PCI_MSI_FLAGS_ENABLE
-#define multi_msi_capable(control) \
-	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
-#define multi_msi_enable(control, num) \
-	control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
-#define is_64bit_address(control)	(!!(control & PCI_MSI_FLAGS_64BIT))
-#define is_mask_bit_support(control)	(!!(control & PCI_MSI_FLAGS_MASKBIT))
-#define msi_enable(control, num) multi_msi_enable(control, num); \
-	control |= PCI_MSI_FLAGS_ENABLE
-
-#define msix_control_reg(base)		(base + PCI_MSIX_FLAGS)
-#define msix_table_offset_reg(base)	(base + PCI_MSIX_TABLE)
-#define msix_pba_offset_reg(base)	(base + PCI_MSIX_PBA)
-#define msix_enable(control)	 	control |= PCI_MSIX_FLAGS_ENABLE
-#define msix_disable(control)	 	control &= ~PCI_MSIX_FLAGS_ENABLE
-#define msix_table_size(control) 	((control & PCI_MSIX_FLAGS_QSIZE)+1)
-#define msix_unmask(address)	 	(address & ~PCI_MSIX_VECTOR_BITMASK)
-#define msix_mask(address)		(address | PCI_MSIX_VECTOR_BITMASK)
-
 /*
  * MSI Defined Data Structures
  */
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 5febc0ea4b..62fd7351dd 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -20,10 +20,10 @@
 #include <xen/iocap.h>
 #include <xen/keyhandler.h>
 #include <xen/pfn.h>
+#include <xen/msi.h>
 #include <asm/io.h>
 #include <asm/smp.h>
 #include <asm/desc.h>
-#include <asm/msi.h>
 #include <asm/fixmap.h>
 #include <asm/p2m.h>
 #include <mach_apic.h>
diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h
index 93243424e8..f007a0c083 100644
--- a/xen/drivers/passthrough/amd/iommu.h
+++ b/xen/drivers/passthrough/amd/iommu.h
@@ -26,6 +26,7 @@
 #include <xen/tasklet.h>
 #include <xen/sched.h>
 #include <xen/domain_page.h>
+#include <xen/msi.h>
 
 #include <asm/msi.h>
 #include <asm/apicdef.h>
diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
index 5757a7aed2..8fc82a9b8d 100644
--- a/xen/drivers/vpci/msi.c
+++ b/xen/drivers/vpci/msi.c
@@ -16,12 +16,11 @@
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <xen/msi.h>
 #include <xen/sched.h>
 #include <xen/softirq.h>
 #include <xen/vpci.h>
 
-#include <asm/msi.h>
-
 static uint32_t control_read(const struct pci_dev *pdev, unsigned int reg,
                              void *data)
 {
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 846f1b8d70..d89396a3b4 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -17,16 +17,12 @@
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <xen/msi.h>
 #include <xen/sched.h>
 #include <xen/vpci.h>
 
-#include <asm/msi.h>
 #include <asm/p2m.h>
 
-#define VMSIX_ADDR_IN_RANGE(addr, vpci, nr)                               \
-    ((addr) >= vmsix_table_addr(vpci, nr) &&                              \
-     (addr) < vmsix_table_addr(vpci, nr) + vmsix_table_size(vpci, nr))
-
 static uint32_t control_read(const struct pci_dev *pdev, unsigned int reg,
                              void *data)
 {
@@ -138,29 +134,6 @@ static void control_write(const struct pci_dev *pdev, unsigned int reg,
         pci_conf_write16(pdev->sbdf, reg, val);
 }
 
-static struct vpci_msix *msix_find(const struct domain *d, unsigned long addr)
-{
-    struct vpci_msix *msix;
-
-    list_for_each_entry ( msix, &d->arch.hvm.msix_tables, next )
-    {
-        const struct vpci_bar *bars = msix->pdev->vpci->header.bars;
-        unsigned int i;
-
-        for ( i = 0; i < ARRAY_SIZE(msix->tables); i++ )
-            if ( bars[msix->tables[i] & PCI_MSIX_BIRMASK].enabled &&
-                 VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, i) )
-                return msix;
-    }
-
-    return NULL;
-}
-
-static int msix_accept(struct vcpu *v, unsigned long addr)
-{
-    return !!msix_find(v->domain, addr);
-}
-
 static bool access_allowed(const struct pci_dev *pdev, unsigned long addr,
                            unsigned int len)
 {
@@ -182,11 +155,9 @@ static struct vpci_msix_entry *get_entry(struct vpci_msix *msix,
     return &msix->entries[(addr - start) / PCI_MSIX_ENTRY_SIZE];
 }
 
-static int msix_read(struct vcpu *v, unsigned long addr, unsigned int len,
-                     unsigned long *data)
+int vpci_msix_read(struct vpci_msix *msix, unsigned long addr,
+                   unsigned int len, unsigned long *data)
 {
-    const struct domain *d = v->domain;
-    struct vpci_msix *msix = msix_find(d, addr);
     const struct vpci_msix_entry *entry;
     unsigned int offset;
 
@@ -259,11 +230,10 @@ static int msix_read(struct vcpu *v, unsigned long addr, unsigned int len,
     return X86EMUL_OKAY;
 }
 
-static int msix_write(struct vcpu *v, unsigned long addr, unsigned int len,
-                      unsigned long data)
+int vpci_msix_write(struct vpci_msix *msix, unsigned long addr,
+                    unsigned int len, unsigned long data)
 {
-    const struct domain *d = v->domain;
-    struct vpci_msix *msix = msix_find(d, addr);
+    const struct domain *d = msix->pdev->domain;
     struct vpci_msix_entry *entry;
     unsigned int offset;
 
@@ -375,59 +345,6 @@ static int msix_write(struct vcpu *v, unsigned long addr, unsigned int len,
     return X86EMUL_OKAY;
 }
 
-static const struct hvm_mmio_ops vpci_msix_table_ops = {
-    .check = msix_accept,
-    .read = msix_read,
-    .write = msix_write,
-};
-
-int vpci_make_msix_hole(const struct pci_dev *pdev)
-{
-    struct domain *d = pdev->domain;
-    unsigned int i;
-
-    if ( !pdev->vpci->msix )
-        return 0;
-
-    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
-    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
-    {
-        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
-        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
-                                     vmsix_table_size(pdev->vpci, i) - 1);
-
-        for ( ; start <= end; start++ )
-        {
-            p2m_type_t t;
-            mfn_t mfn = get_gfn_query(d, start, &t);
-
-            switch ( t )
-            {
-            case p2m_mmio_dm:
-            case p2m_invalid:
-                break;
-            case p2m_mmio_direct:
-                if ( mfn_x(mfn) == start )
-                {
-                    clear_identity_p2m_entry(d, start);
-                    break;
-                }
-                /* fallthrough. */
-            default:
-                put_gfn(d, start);
-                gprintk(XENLOG_WARNING,
-                        "%pp: existing mapping (mfn: %" PRI_mfn
-                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
-                        &pdev->sbdf, mfn_x(mfn), t, start);
-                return -EEXIST;
-            }
-            put_gfn(d, start);
-        }
-    }
-
-    return 0;
-}
-
 static int init_msix(struct pci_dev *pdev)
 {
     struct domain *d = pdev->domain;
@@ -472,11 +389,9 @@ static int init_msix(struct pci_dev *pdev)
         vpci_msix_arch_init_entry(&msix->entries[i]);
     }
 
-    if ( list_empty(&d->arch.hvm.msix_tables) )
-        register_mmio_handler(d, &vpci_msix_table_ops);
-
     pdev->vpci->msix = msix;
-    list_add(&msix->next, &d->arch.hvm.msix_tables);
+
+    vpci_msix_arch_register(msix, d);
 
     return 0;
 }
diff --git a/xen/include/xen/msi.h b/xen/include/xen/msi.h
index c903d0050c..c5c8e65feb 100644
--- a/xen/include/xen/msi.h
+++ b/xen/include/xen/msi.h
@@ -3,6 +3,34 @@
 
 #include <xen/pci.h>
 
+#define msi_control_reg(base)       (base + PCI_MSI_FLAGS)
+#define msi_lower_address_reg(base) (base + PCI_MSI_ADDRESS_LO)
+#define msi_upper_address_reg(base) (base + PCI_MSI_ADDRESS_HI)
+#define msi_data_reg(base, is64bit) \
+	( (is64bit) ? (base) + PCI_MSI_DATA_64 : (base) + PCI_MSI_DATA_32 )
+#define msi_mask_bits_reg(base, is64bit) \
+	( (is64bit) ? (base) + PCI_MSI_MASK_BIT : (base) + PCI_MSI_MASK_BIT - 4)
+#define msi_pending_bits_reg(base, is64bit) \
+	( (is64bit) ? (base) + PCI_MSI_MASK_BIT + 4 : (base) + PCI_MSI_MASK_BIT)
+#define msi_disable(control)        control &= ~PCI_MSI_FLAGS_ENABLE
+#define multi_msi_capable(control) \
+	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
+#define multi_msi_enable(control, num) \
+	control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
+#define is_64bit_address(control)   (!!(control & PCI_MSI_FLAGS_64BIT))
+#define is_mask_bit_support(control)    (!!(control & PCI_MSI_FLAGS_MASKBIT))
+#define msi_enable(control, num) multi_msi_enable(control, num); \
+	control |= PCI_MSI_FLAGS_ENABLE
+
+#define msix_control_reg(base)      (base + PCI_MSIX_FLAGS)
+#define msix_table_offset_reg(base) (base + PCI_MSIX_TABLE)
+#define msix_pba_offset_reg(base)   (base + PCI_MSIX_PBA)
+#define msix_enable(control)        control |= PCI_MSIX_FLAGS_ENABLE
+#define msix_disable(control)       control &= ~PCI_MSIX_FLAGS_ENABLE
+#define msix_table_size(control)    ((control & PCI_MSIX_FLAGS_QSIZE)+1)
+#define msix_unmask(address)        (address & ~PCI_MSIX_VECTOR_BITMASK)
+#define msix_mask(address)          (address | PCI_MSIX_VECTOR_BITMASK)
+
 #ifdef CONFIG_HAS_PCI_MSI
 
 #include <asm/msi.h>
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index e8ac1eb395..0381a2c911 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -148,6 +148,11 @@ struct vpci_vcpu {
 };
 
 #ifdef __XEN__
+
+#define VMSIX_ADDR_IN_RANGE(addr, vpci, nr)                               \
+    ((addr) >= vmsix_table_addr(vpci, nr) &&                              \
+     (addr) < vmsix_table_addr(vpci, nr) + vmsix_table_size(vpci, nr))
+
 void vpci_dump_msi(void);
 
 /* Make sure there's a hole in the p2m for the MSIX mmio areas. */
@@ -218,6 +223,14 @@ bool vpci_ecam_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len,
 bool vpci_ecam_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len,
                     unsigned long *data);
 
+void vpci_msix_arch_register(struct vpci_msix *msix, struct domain *d);
+
+int vpci_msix_write(struct vpci_msix *msix, unsigned long addr,
+                    unsigned int len, unsigned long data);
+
+int vpci_msix_read(struct vpci_msix *msix, unsigned long addr,
+                   unsigned int len, unsigned long *data);
+
 #endif /* __XEN__ */
 
 #else /* !CONFIG_HAS_VPCI */
-- 
2.25.1

Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Jan Beulich]

On 15.02.2022 16:25, Rahul Singh wrote:
> vpci/msix.c file will be used for arm architecture when vpci msix
> support will be added to ARM, but there is x86 specific code in this
> file.
> 
> Move x86 specific code to the x86/hvm/vmsi.c file to make sure common
> code will be used for other architecture.

Could you provide some criteria by which code is considered x86-specific
(or not)? For example ...

> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
>  
>      return 0;
>  }
> +
> +int vpci_make_msix_hole(const struct pci_dev *pdev)
> +{
> +    struct domain *d = pdev->domain;
> +    unsigned int i;
> +
> +    if ( !pdev->vpci->msix )
> +        return 0;
> +
> +    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
> +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
> +    {
> +        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
> +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
> +                                     vmsix_table_size(pdev->vpci, i) - 1);
> +
> +        for ( ; start <= end; start++ )
> +        {
> +            p2m_type_t t;
> +            mfn_t mfn = get_gfn_query(d, start, &t);
> +
> +            switch ( t )
> +            {
> +            case p2m_mmio_dm:
> +            case p2m_invalid:
> +                break;
> +            case p2m_mmio_direct:
> +                if ( mfn_x(mfn) == start )
> +                {
> +                    clear_identity_p2m_entry(d, start);
> +                    break;
> +                }
> +                /* fallthrough. */
> +            default:
> +                put_gfn(d, start);
> +                gprintk(XENLOG_WARNING,
> +                        "%pp: existing mapping (mfn: %" PRI_mfn
> +                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
> +                        &pdev->sbdf, mfn_x(mfn), t, start);
> +                return -EEXIST;
> +            }
> +            put_gfn(d, start);
> +        }
> +    }
> +
> +    return 0;
> +}

... nothing in this function looks to be x86-specific, except maybe
functions like clear_identity_p2m_entry() may not currently be available
on Arm. But this doesn't make the code x86-specific.

> +struct vpci_msix *vpci_msix_find(const struct domain *d, unsigned long addr)
> +{
> +    struct vpci_msix *msix;
> +
> +    list_for_each_entry ( msix, &d->arch.hvm.msix_tables, next )
> +    {
> +        const struct vpci_bar *bars = msix->pdev->vpci->header.bars;
> +        unsigned int i;
> +
> +        for ( i = 0; i < ARRAY_SIZE(msix->tables); i++ )
> +            if ( bars[msix->tables[i] & PCI_MSIX_BIRMASK].enabled &&
> +                 VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, i) )
> +                return msix;
> +    }
> +
> +    return NULL;
> +}

Or take this one - I don't see anything x86-specific in here. The use
of d->arch.hvm merely points out that there may be a field which now
needs generalizing.

> +static int x86_msix_accept(struct vcpu *v, unsigned long addr)
> +{
> +    return !!vpci_msix_find(v->domain, addr);
> +}
> +
> +static int x86_msix_write(struct vcpu *v, unsigned long addr, unsigned int len,
> +                          unsigned long data)
> +{
> +    const struct domain *d = v->domain;
> +    struct vpci_msix *msix = vpci_msix_find(d, addr);
> +
> +    return vpci_msix_write(msix, addr, len, data);
> +}
> +
> +static int x86_msix_read(struct vcpu *v, unsigned long addr, unsigned int len,
> +                         unsigned long *data)
> +{
> +    const struct domain *d = v->domain;
> +    struct vpci_msix *msix = vpci_msix_find(d, addr);
> +
> +    return vpci_msix_read(msix, addr, len, data);
> +}
> +
> +static const struct hvm_mmio_ops vpci_msix_table_ops = {
> +    .check = x86_msix_accept,
> +    .read = x86_msix_read,
> +    .write = x86_msix_write,
> +};

I don't see the need to add x86_ prefixes to static functions while
you're moving them. Provided any of this is really x86-specific in
the first place.

> +void vpci_msix_arch_register(struct vpci_msix *msix, struct domain *d)
> +{
> +    if ( list_empty(&d->arch.hvm.msix_tables) )
> +        register_mmio_handler(d, &vpci_msix_table_ops);

This is perhaps the only thing which I could see would better live
in arch-specific code.

> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -20,10 +20,10 @@
>  #include <xen/iocap.h>
>  #include <xen/keyhandler.h>
>  #include <xen/pfn.h>
> +#include <xen/msi.h>
>  #include <asm/io.h>
>  #include <asm/smp.h>
>  #include <asm/desc.h>
> -#include <asm/msi.h>

Just like you do here and elsewhere, ...

> --- a/xen/drivers/passthrough/amd/iommu.h
> +++ b/xen/drivers/passthrough/amd/iommu.h
> @@ -26,6 +26,7 @@
>  #include <xen/tasklet.h>
>  #include <xen/sched.h>
>  #include <xen/domain_page.h>
> +#include <xen/msi.h>
>  
>  #include <asm/msi.h>

... I think you want to remove this now redundant #include as well.

> --- a/xen/include/xen/msi.h
> +++ b/xen/include/xen/msi.h
> @@ -3,6 +3,34 @@
>  
>  #include <xen/pci.h>
>  
> +#define msi_control_reg(base)       (base + PCI_MSI_FLAGS)
> +#define msi_lower_address_reg(base) (base + PCI_MSI_ADDRESS_LO)
> +#define msi_upper_address_reg(base) (base + PCI_MSI_ADDRESS_HI)
> +#define msi_data_reg(base, is64bit) \
> +	( (is64bit) ? (base) + PCI_MSI_DATA_64 : (base) + PCI_MSI_DATA_32 )
> +#define msi_mask_bits_reg(base, is64bit) \
> +	( (is64bit) ? (base) + PCI_MSI_MASK_BIT : (base) + PCI_MSI_MASK_BIT - 4)
> +#define msi_pending_bits_reg(base, is64bit) \
> +	( (is64bit) ? (base) + PCI_MSI_MASK_BIT + 4 : (base) + PCI_MSI_MASK_BIT)
> +#define msi_disable(control)        control &= ~PCI_MSI_FLAGS_ENABLE
> +#define multi_msi_capable(control) \
> +	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
> +#define multi_msi_enable(control, num) \
> +	control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
> +#define is_64bit_address(control)   (!!(control & PCI_MSI_FLAGS_64BIT))
> +#define is_mask_bit_support(control)    (!!(control & PCI_MSI_FLAGS_MASKBIT))
> +#define msi_enable(control, num) multi_msi_enable(control, num); \
> +	control |= PCI_MSI_FLAGS_ENABLE
> +
> +#define msix_control_reg(base)      (base + PCI_MSIX_FLAGS)
> +#define msix_table_offset_reg(base) (base + PCI_MSIX_TABLE)
> +#define msix_pba_offset_reg(base)   (base + PCI_MSIX_PBA)
> +#define msix_enable(control)        control |= PCI_MSIX_FLAGS_ENABLE
> +#define msix_disable(control)       control &= ~PCI_MSIX_FLAGS_ENABLE
> +#define msix_table_size(control)    ((control & PCI_MSIX_FLAGS_QSIZE)+1)
> +#define msix_unmask(address)        (address & ~PCI_MSIX_VECTOR_BITMASK)
> +#define msix_mask(address)          (address | PCI_MSIX_VECTOR_BITMASK)

Why did you put these not ...

>  #ifdef CONFIG_HAS_PCI_MSI

.. below here?

Also the movement of these is quite the opposite of what the title
says. IOW this likely wants to be a separate change.

Jan

Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Rahul Singh]

H Jan,

> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 15.02.2022 16:25, Rahul Singh wrote:
>> vpci/msix.c file will be used for arm architecture when vpci msix
>> support will be added to ARM, but there is x86 specific code in this
>> file.
>> 
>> Move x86 specific code to the x86/hvm/vmsi.c file to make sure common
>> code will be used for other architecture.
> 
> Could you provide some criteria by which code is considered x86-specific
> (or not)? For example ...

Code moved to x86 file is based on criteria that either the code will be unusable or will be different 
for ARM when MSIX  support will be introduce for ARM.
> 
>> --- a/xen/arch/x86/hvm/vmsi.c
>> +++ b/xen/arch/x86/hvm/vmsi.c
>> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
>> 
>>     return 0;
>> }
>> +
>> +int vpci_make_msix_hole(const struct pci_dev *pdev)
>> +{
>> +    struct domain *d = pdev->domain;
>> +    unsigned int i;
>> +
>> +    if ( !pdev->vpci->msix )
>> +        return 0;
>> +
>> +    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
>> +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
>> +    {
>> +        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
>> +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
>> +                                     vmsix_table_size(pdev->vpci, i) - 1);
>> +
>> +        for ( ; start <= end; start++ )
>> +        {
>> +            p2m_type_t t;
>> +            mfn_t mfn = get_gfn_query(d, start, &t);
>> +
>> +            switch ( t )
>> +            {
>> +            case p2m_mmio_dm:
>> +            case p2m_invalid:
>> +                break;
>> +            case p2m_mmio_direct:
>> +                if ( mfn_x(mfn) == start )
>> +                {
>> +                    clear_identity_p2m_entry(d, start);
>> +                    break;
>> +                }
>> +                /* fallthrough. */
>> +            default:
>> +                put_gfn(d, start);
>> +                gprintk(XENLOG_WARNING,
>> +                        "%pp: existing mapping (mfn: %" PRI_mfn
>> +                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
>> +                        &pdev->sbdf, mfn_x(mfn), t, start);
>> +                return -EEXIST;
>> +            }
>> +            put_gfn(d, start);
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
> 
> ... nothing in this function looks to be x86-specific, except maybe
> functions like clear_identity_p2m_entry() may not currently be available
> on Arm. But this doesn't make the code x86-specific.

I will maybe be wrong but what I understand from the code is that for x86 
if there is no p2m entries setup for the region, accesses to them will be trapped 
into the hypervisor and can be handled by specific MMIO handler.

But for ARM when we are registering the MMIO handler we have to provide 
the GPA also for the MMIO handler. 

For ARM arch vpci_make_msix_hole() will be used to register the MMIO handler 
for the MSIX MMIO region.

int vpci_make_msix_hole(const struct pci_dev *pdev)
{
    struct vpci_msix *msix = pdev->vpci->msix;
    paddr_t addr,size;

   for ( int i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ )
   {                                                                           
       addr = vmsix_table_addr(pdev->vpci, i);               
       size = vmsix_table_size(pdev->vpci, i) - 1;                                                                         
       register_mmio_handler(pdev->domain, &vpci_msi_mmio_handler,             
                                              addr, size, NULL);                                
    }                                                                                                                 
    return 0;                                                                   
}

Therefore in this case there is difference how ARM handle this case.
 
> 
>> +struct vpci_msix *vpci_msix_find(const struct domain *d, unsigned long addr)
>> +{
>> +    struct vpci_msix *msix;
>> +
>> +    list_for_each_entry ( msix, &d->arch.hvm.msix_tables, next )
>> +    {
>> +        const struct vpci_bar *bars = msix->pdev->vpci->header.bars;
>> +        unsigned int i;
>> +
>> +        for ( i = 0; i < ARRAY_SIZE(msix->tables); i++ )
>> +            if ( bars[msix->tables[i] & PCI_MSIX_BIRMASK].enabled &&
>> +                 VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, i) )
>> +                return msix;
>> +    }
>> +
>> +    return NULL;
>> +}
> 
> Or take this one - I don't see anything x86-specific in here. The use
> of d->arch.hvm merely points out that there may be a field which now
> needs generalizing.

Yes, you are right here I can avoid this change if I will introduce 
"struct list_head msix_tables"  in "d->arch.hvm" for ARM also. 

> 
>> +static int x86_msix_accept(struct vcpu *v, unsigned long addr)
>> +{
>> +    return !!vpci_msix_find(v->domain, addr);
>> +}
>> +
>> +static int x86_msix_write(struct vcpu *v, unsigned long addr, unsigned int len,
>> +                          unsigned long data)
>> +{
>> +    const struct domain *d = v->domain;
>> +    struct vpci_msix *msix = vpci_msix_find(d, addr);
>> +
>> +    return vpci_msix_write(msix, addr, len, data);
>> +}
>> +
>> +static int x86_msix_read(struct vcpu *v, unsigned long addr, unsigned int len,
>> +                         unsigned long *data)
>> +{
>> +    const struct domain *d = v->domain;
>> +    struct vpci_msix *msix = vpci_msix_find(d, addr);
>> +
>> +    return vpci_msix_read(msix, addr, len, data);
>> +}
>> +
>> +static const struct hvm_mmio_ops vpci_msix_table_ops = {
>> +    .check = x86_msix_accept,
>> +    .read = x86_msix_read,
>> +    .write = x86_msix_write,
>> +};
> 
> I don't see the need to add x86_ prefixes to static functions while
> you're moving them. Provided any of this is really x86-specific in
> the first place.
> 

Ok. Let me remove the x86_ prefixes in next version.  MMIO handler functions declaration is different 
for ARM and X86 therefore I need to move this code to arch specific file.

>> +void vpci_msix_arch_register(struct vpci_msix *msix, struct domain *d)
>> +{
>> +    if ( list_empty(&d->arch.hvm.msix_tables) )
>> +        register_mmio_handler(d, &vpci_msix_table_ops);
> 
> This is perhaps the only thing which I could see would better live
> in arch-specific code.
> 
>> --- a/xen/arch/x86/msi.c
>> +++ b/xen/arch/x86/msi.c
>> @@ -20,10 +20,10 @@
>> #include <xen/iocap.h>
>> #include <xen/keyhandler.h>
>> #include <xen/pfn.h>
>> +#include <xen/msi.h>
>> #include <asm/io.h>
>> #include <asm/smp.h>
>> #include <asm/desc.h>
>> -#include <asm/msi.h>
> 
> Just like you do here and elsewhere, ...
> 
>> --- a/xen/drivers/passthrough/amd/iommu.h
>> +++ b/xen/drivers/passthrough/amd/iommu.h
>> @@ -26,6 +26,7 @@
>> #include <xen/tasklet.h>
>> #include <xen/sched.h>
>> #include <xen/domain_page.h>
>> +#include <xen/msi.h>
>> 
>> #include <asm/msi.h>
> 
> ... I think you want to remove this now redundant #include as well.

Ok.
> 
>> --- a/xen/include/xen/msi.h
>> +++ b/xen/include/xen/msi.h
>> @@ -3,6 +3,34 @@
>> 
>> #include <xen/pci.h>
>> 
>> +#define msi_control_reg(base)       (base + PCI_MSI_FLAGS)
>> +#define msi_lower_address_reg(base) (base + PCI_MSI_ADDRESS_LO)
>> +#define msi_upper_address_reg(base) (base + PCI_MSI_ADDRESS_HI)
>> +#define msi_data_reg(base, is64bit) \
>> +	( (is64bit) ? (base) + PCI_MSI_DATA_64 : (base) + PCI_MSI_DATA_32 )
>> +#define msi_mask_bits_reg(base, is64bit) \
>> +	( (is64bit) ? (base) + PCI_MSI_MASK_BIT : (base) + PCI_MSI_MASK_BIT - 4)
>> +#define msi_pending_bits_reg(base, is64bit) \
>> +	( (is64bit) ? (base) + PCI_MSI_MASK_BIT + 4 : (base) + PCI_MSI_MASK_BIT)
>> +#define msi_disable(control)        control &= ~PCI_MSI_FLAGS_ENABLE
>> +#define multi_msi_capable(control) \
>> +	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
>> +#define multi_msi_enable(control, num) \
>> +	control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
>> +#define is_64bit_address(control)   (!!(control & PCI_MSI_FLAGS_64BIT))
>> +#define is_mask_bit_support(control)    (!!(control & PCI_MSI_FLAGS_MASKBIT))
>> +#define msi_enable(control, num) multi_msi_enable(control, num); \
>> +	control |= PCI_MSI_FLAGS_ENABLE
>> +
>> +#define msix_control_reg(base)      (base + PCI_MSIX_FLAGS)
>> +#define msix_table_offset_reg(base) (base + PCI_MSIX_TABLE)
>> +#define msix_pba_offset_reg(base)   (base + PCI_MSIX_PBA)
>> +#define msix_enable(control)        control |= PCI_MSIX_FLAGS_ENABLE
>> +#define msix_disable(control)       control &= ~PCI_MSIX_FLAGS_ENABLE
>> +#define msix_table_size(control)    ((control & PCI_MSIX_FLAGS_QSIZE)+1)
>> +#define msix_unmask(address)        (address & ~PCI_MSIX_VECTOR_BITMASK)
>> +#define msix_mask(address)          (address | PCI_MSIX_VECTOR_BITMASK)
> 
> Why did you put these not ...
> 
>> #ifdef CONFIG_HAS_PCI_MSI
> 
> .. below here?

I will fix this in next version.

> 
> Also the movement of these is quite the opposite of what the title
> says. IOW this likely wants to be a separate change.

Ok. Let me move this change in separate patch in next series.

Regards,
Rahul
> 
> Jan
> 

Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Jan Beulich]

On 01.03.2022 14:34, Rahul Singh wrote:
>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 15.02.2022 16:25, Rahul Singh wrote:
>>> vpci/msix.c file will be used for arm architecture when vpci msix
>>> support will be added to ARM, but there is x86 specific code in this
>>> file.
>>>
>>> Move x86 specific code to the x86/hvm/vmsi.c file to make sure common
>>> code will be used for other architecture.
>>
>> Could you provide some criteria by which code is considered x86-specific
>> (or not)? For example ...
> 
> Code moved to x86 file is based on criteria that either the code will be unusable or will be different 
> for ARM when MSIX  support will be introduce for ARM.

That's a very abstract statement, which you can't really derive any
judgement from. It'll be necessary to see in how far the code is
indeed different. After all PCI, MSI, and MSI-X are largely arch-
agnostic.

>>> --- a/xen/arch/x86/hvm/vmsi.c
>>> +++ b/xen/arch/x86/hvm/vmsi.c
>>> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
>>>
>>>     return 0;
>>> }
>>> +
>>> +int vpci_make_msix_hole(const struct pci_dev *pdev)
>>> +{
>>> +    struct domain *d = pdev->domain;
>>> +    unsigned int i;
>>> +
>>> +    if ( !pdev->vpci->msix )
>>> +        return 0;
>>> +
>>> +    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
>>> +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
>>> +    {
>>> +        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
>>> +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
>>> +                                     vmsix_table_size(pdev->vpci, i) - 1);
>>> +
>>> +        for ( ; start <= end; start++ )
>>> +        {
>>> +            p2m_type_t t;
>>> +            mfn_t mfn = get_gfn_query(d, start, &t);
>>> +
>>> +            switch ( t )
>>> +            {
>>> +            case p2m_mmio_dm:
>>> +            case p2m_invalid:
>>> +                break;
>>> +            case p2m_mmio_direct:
>>> +                if ( mfn_x(mfn) == start )
>>> +                {
>>> +                    clear_identity_p2m_entry(d, start);
>>> +                    break;
>>> +                }
>>> +                /* fallthrough. */
>>> +            default:
>>> +                put_gfn(d, start);
>>> +                gprintk(XENLOG_WARNING,
>>> +                        "%pp: existing mapping (mfn: %" PRI_mfn
>>> +                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
>>> +                        &pdev->sbdf, mfn_x(mfn), t, start);
>>> +                return -EEXIST;
>>> +            }
>>> +            put_gfn(d, start);
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>
>> ... nothing in this function looks to be x86-specific, except maybe
>> functions like clear_identity_p2m_entry() may not currently be available
>> on Arm. But this doesn't make the code x86-specific.
> 
> I will maybe be wrong but what I understand from the code is that for x86 
> if there is no p2m entries setup for the region, accesses to them will be trapped 
> into the hypervisor and can be handled by specific MMIO handler.
> 
> But for ARM when we are registering the MMIO handler we have to provide 
> the GPA also for the MMIO handler. 

Question is: Is this just an effect resulting from different implementation,
or an inherent requirement? In the former case, harmonizing things may be an
alternative option.

> For ARM arch vpci_make_msix_hole() will be used to register the MMIO handler 
> for the MSIX MMIO region.
> 
> int vpci_make_msix_hole(const struct pci_dev *pdev)
> {
>     struct vpci_msix *msix = pdev->vpci->msix;
>     paddr_t addr,size;
> 
>    for ( int i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ )
>    {                                                                           
>        addr = vmsix_table_addr(pdev->vpci, i);               
>        size = vmsix_table_size(pdev->vpci, i) - 1;                                                                         
>        register_mmio_handler(pdev->domain, &vpci_msi_mmio_handler,             
>                                               addr, size, NULL);                                
>     }                                                                                                                 
>     return 0;                                                                   
> }
> 
> Therefore in this case there is difference how ARM handle this case.
>  
>>
>>> +struct vpci_msix *vpci_msix_find(const struct domain *d, unsigned long addr)
>>> +{
>>> +    struct vpci_msix *msix;
>>> +
>>> +    list_for_each_entry ( msix, &d->arch.hvm.msix_tables, next )
>>> +    {
>>> +        const struct vpci_bar *bars = msix->pdev->vpci->header.bars;
>>> +        unsigned int i;
>>> +
>>> +        for ( i = 0; i < ARRAY_SIZE(msix->tables); i++ )
>>> +            if ( bars[msix->tables[i] & PCI_MSIX_BIRMASK].enabled &&
>>> +                 VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, i) )
>>> +                return msix;
>>> +    }
>>> +
>>> +    return NULL;
>>> +}
>>
>> Or take this one - I don't see anything x86-specific in here. The use
>> of d->arch.hvm merely points out that there may be a field which now
>> needs generalizing.
> 
> Yes, you are right here I can avoid this change if I will introduce 
> "struct list_head msix_tables"  in "d->arch.hvm" for ARM also. 

Wait - if you pass in the guest address at registration time, you
shouldn't have a need for a "find" function.

Jan

Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Rahul Singh]

Hi Jan,

> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 01.03.2022 14:34, Rahul Singh wrote:
>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> On 15.02.2022 16:25, Rahul Singh wrote:
>>>> vpci/msix.c file will be used for arm architecture when vpci msix
>>>> support will be added to ARM, but there is x86 specific code in this
>>>> file.
>>>> 
>>>> Move x86 specific code to the x86/hvm/vmsi.c file to make sure common
>>>> code will be used for other architecture.
>>> 
>>> Could you provide some criteria by which code is considered x86-specific
>>> (or not)? For example ...
>> 
>> Code moved to x86 file is based on criteria that either the code will be unusable or will be different 
>> for ARM when MSIX  support will be introduce for ARM.
> 
> That's a very abstract statement, which you can't really derive any
> judgement from. It'll be necessary to see in how far the code is
> indeed different. After all PCI, MSI, and MSI-X are largely arch-
> agnostic.
> 
>>>> --- a/xen/arch/x86/hvm/vmsi.c
>>>> +++ b/xen/arch/x86/hvm/vmsi.c
>>>> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
>>>> 
>>>>    return 0;
>>>> }
>>>> +
>>>> +int vpci_make_msix_hole(const struct pci_dev *pdev)
>>>> +{
>>>> +    struct domain *d = pdev->domain;
>>>> +    unsigned int i;
>>>> +
>>>> +    if ( !pdev->vpci->msix )
>>>> +        return 0;
>>>> +
>>>> +    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
>>>> +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
>>>> +    {
>>>> +        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
>>>> +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
>>>> +                                     vmsix_table_size(pdev->vpci, i) - 1);
>>>> +
>>>> +        for ( ; start <= end; start++ )
>>>> +        {
>>>> +            p2m_type_t t;
>>>> +            mfn_t mfn = get_gfn_query(d, start, &t);
>>>> +
>>>> +            switch ( t )
>>>> +            {
>>>> +            case p2m_mmio_dm:
>>>> +            case p2m_invalid:
>>>> +                break;
>>>> +            case p2m_mmio_direct:
>>>> +                if ( mfn_x(mfn) == start )
>>>> +                {
>>>> +                    clear_identity_p2m_entry(d, start);
>>>> +                    break;
>>>> +                }
>>>> +                /* fallthrough. */
>>>> +            default:
>>>> +                put_gfn(d, start);
>>>> +                gprintk(XENLOG_WARNING,
>>>> +                        "%pp: existing mapping (mfn: %" PRI_mfn
>>>> +                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
>>>> +                        &pdev->sbdf, mfn_x(mfn), t, start);
>>>> +                return -EEXIST;
>>>> +            }
>>>> +            put_gfn(d, start);
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>> 
>>> ... nothing in this function looks to be x86-specific, except maybe
>>> functions like clear_identity_p2m_entry() may not currently be available
>>> on Arm. But this doesn't make the code x86-specific.
>> 
>> I will maybe be wrong but what I understand from the code is that for x86 
>> if there is no p2m entries setup for the region, accesses to them will be trapped 
>> into the hypervisor and can be handled by specific MMIO handler.
>> 
>> But for ARM when we are registering the MMIO handler we have to provide 
>> the GPA also for the MMIO handler. 
> 
> Question is: Is this just an effect resulting from different implementation,
> or an inherent requirement? In the former case, harmonizing things may be an
> alternative option.

This is an inherent requirement to provide a GPA when registering the MMIO handler.

For x86 msix mmio handlers is registered in init_msix(..) function as there is no requirement
on x86 to provide GPA when registering the handler. Later point of time when BARs are configured
and memory decoding bit is enabled vpci_make_msix_hole() will clear the identity mapping for msix
base table address so that access to msix tables will be trapped.

On ARM we need to provide GPA to register the mmio handler and MSIX table base
address is not valid when init_msix() is called as BAR will be configured later point in time.
Therefore on ARM mmio handler will be registered in function vpci_make_msix_hole() when
memory decoding bit is enabled.

> 
>> For ARM arch vpci_make_msix_hole() will be used to register the MMIO handler 
>> for the MSIX MMIO region.
>> 
>> int vpci_make_msix_hole(const struct pci_dev *pdev)
>> {
>>    struct vpci_msix *msix = pdev->vpci->msix;
>>    paddr_t addr,size;
>> 
>>   for ( int i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ )
>>   {                                                                           
>>       addr = vmsix_table_addr(pdev->vpci, i);               
>>       size = vmsix_table_size(pdev->vpci, i) - 1;                                                                         
>>       register_mmio_handler(pdev->domain, &vpci_msi_mmio_handler,             
>>                                              addr, size, NULL);                                
>>    }                                                                                                                 
>>    return 0;                                                                   
>> }
>> 
>> Therefore in this case there is difference how ARM handle this case.
>> 
>>> 
>>>> +struct vpci_msix *vpci_msix_find(const struct domain *d, unsigned long addr)
>>>> +{
>>>> +    struct vpci_msix *msix;
>>>> +
>>>> +    list_for_each_entry ( msix, &d->arch.hvm.msix_tables, next )
>>>> +    {
>>>> +        const struct vpci_bar *bars = msix->pdev->vpci->header.bars;
>>>> +        unsigned int i;
>>>> +
>>>> +        for ( i = 0; i < ARRAY_SIZE(msix->tables); i++ )
>>>> +            if ( bars[msix->tables[i] & PCI_MSIX_BIRMASK].enabled &&
>>>> +                 VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, i) )
>>>> +                return msix;
>>>> +    }
>>>> +
>>>> +    return NULL;
>>>> +}
>>> 
>>> Or take this one - I don't see anything x86-specific in here. The use
>>> of d->arch.hvm merely points out that there may be a field which now
>>> needs generalizing.
>> 
>> Yes, you are right here I can avoid this change if I will introduce 
>> "struct list_head msix_tables"  in "d->arch.hvm" for ARM also. 
> 
> Wait - if you pass in the guest address at registration time, you
> shouldn't have a need for a "find" function.

Yes you are right we don’t need to call msix_find() on ARM. In that case there is
need to move msix_find() function to x86 file as I did in v1.

Regards,
Rahul
> 
> Jan
> 

Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Jan Beulich]

On 03.03.2022 17:31, Rahul Singh wrote:
>> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeulich@suse.com> wrote:
>> On 01.03.2022 14:34, Rahul Singh wrote:
>>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 15.02.2022 16:25, Rahul Singh wrote:
>>>>> --- a/xen/arch/x86/hvm/vmsi.c
>>>>> +++ b/xen/arch/x86/hvm/vmsi.c
>>>>> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
>>>>>
>>>>>    return 0;
>>>>> }
>>>>> +
>>>>> +int vpci_make_msix_hole(const struct pci_dev *pdev)
>>>>> +{
>>>>> +    struct domain *d = pdev->domain;
>>>>> +    unsigned int i;
>>>>> +
>>>>> +    if ( !pdev->vpci->msix )
>>>>> +        return 0;
>>>>> +
>>>>> +    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
>>>>> +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
>>>>> +    {
>>>>> +        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
>>>>> +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
>>>>> +                                     vmsix_table_size(pdev->vpci, i) - 1);
>>>>> +
>>>>> +        for ( ; start <= end; start++ )
>>>>> +        {
>>>>> +            p2m_type_t t;
>>>>> +            mfn_t mfn = get_gfn_query(d, start, &t);
>>>>> +
>>>>> +            switch ( t )
>>>>> +            {
>>>>> +            case p2m_mmio_dm:
>>>>> +            case p2m_invalid:
>>>>> +                break;
>>>>> +            case p2m_mmio_direct:
>>>>> +                if ( mfn_x(mfn) == start )
>>>>> +                {
>>>>> +                    clear_identity_p2m_entry(d, start);
>>>>> +                    break;
>>>>> +                }
>>>>> +                /* fallthrough. */
>>>>> +            default:
>>>>> +                put_gfn(d, start);
>>>>> +                gprintk(XENLOG_WARNING,
>>>>> +                        "%pp: existing mapping (mfn: %" PRI_mfn
>>>>> +                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
>>>>> +                        &pdev->sbdf, mfn_x(mfn), t, start);
>>>>> +                return -EEXIST;
>>>>> +            }
>>>>> +            put_gfn(d, start);
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>
>>>> ... nothing in this function looks to be x86-specific, except maybe
>>>> functions like clear_identity_p2m_entry() may not currently be available
>>>> on Arm. But this doesn't make the code x86-specific.
>>>
>>> I will maybe be wrong but what I understand from the code is that for x86 
>>> if there is no p2m entries setup for the region, accesses to them will be trapped 
>>> into the hypervisor and can be handled by specific MMIO handler.
>>>
>>> But for ARM when we are registering the MMIO handler we have to provide 
>>> the GPA also for the MMIO handler. 
>>
>> Question is: Is this just an effect resulting from different implementation,
>> or an inherent requirement? In the former case, harmonizing things may be an
>> alternative option.
> 
> This is an inherent requirement to provide a GPA when registering the MMIO handler.

So you first say yes to my "inherent" question, but then ...

> For x86 msix mmio handlers is registered in init_msix(..) function as there is no requirement
> on x86 to provide GPA when registering the handler. Later point of time when BARs are configured
> and memory decoding bit is enabled vpci_make_msix_hole() will clear the identity mapping for msix
> base table address so that access to msix tables will be trapped.
> 
> On ARM we need to provide GPA to register the mmio handler and MSIX table base
> address is not valid when init_msix() is called as BAR will be configured later point in time.
> Therefore on ARM mmio handler will be registered in function vpci_make_msix_hole() when
> memory decoding bit is enabled.

... you explain it's an implementation detail. I'm inclined to
suggest that x86 also pass the GPA where possible. Handler lookup
really would benefit from not needing to iterate over all registered
handlers, until one claims the access. The optimization part of this
of course doesn't need to be done right here, but harmonizing
register_mmio_handler() between both architectures would seem to be
a reasonable prereq step.

I'm adding Paul to Cc in case he wants to comment, as this would
touch his territory on the x86 side.

Jan

Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Rahul Singh]

Hi Jan,

> On 4 Mar 2022, at 7:23 am, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 03.03.2022 17:31, Rahul Singh wrote:
>>> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 01.03.2022 14:34, Rahul Singh wrote:
>>>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>>>> On 15.02.2022 16:25, Rahul Singh wrote:
>>>>>> --- a/xen/arch/x86/hvm/vmsi.c
>>>>>> +++ b/xen/arch/x86/hvm/vmsi.c
>>>>>> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
>>>>>> 
>>>>>>   return 0;
>>>>>> }
>>>>>> +
>>>>>> +int vpci_make_msix_hole(const struct pci_dev *pdev)
>>>>>> +{
>>>>>> +    struct domain *d = pdev->domain;
>>>>>> +    unsigned int i;
>>>>>> +
>>>>>> +    if ( !pdev->vpci->msix )
>>>>>> +        return 0;
>>>>>> +
>>>>>> +    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
>>>>>> +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
>>>>>> +    {
>>>>>> +        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
>>>>>> +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
>>>>>> +                                     vmsix_table_size(pdev->vpci, i) - 1);
>>>>>> +
>>>>>> +        for ( ; start <= end; start++ )
>>>>>> +        {
>>>>>> +            p2m_type_t t;
>>>>>> +            mfn_t mfn = get_gfn_query(d, start, &t);
>>>>>> +
>>>>>> +            switch ( t )
>>>>>> +            {
>>>>>> +            case p2m_mmio_dm:
>>>>>> +            case p2m_invalid:
>>>>>> +                break;
>>>>>> +            case p2m_mmio_direct:
>>>>>> +                if ( mfn_x(mfn) == start )
>>>>>> +                {
>>>>>> +                    clear_identity_p2m_entry(d, start);
>>>>>> +                    break;
>>>>>> +                }
>>>>>> +                /* fallthrough. */
>>>>>> +            default:
>>>>>> +                put_gfn(d, start);
>>>>>> +                gprintk(XENLOG_WARNING,
>>>>>> +                        "%pp: existing mapping (mfn: %" PRI_mfn
>>>>>> +                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
>>>>>> +                        &pdev->sbdf, mfn_x(mfn), t, start);
>>>>>> +                return -EEXIST;
>>>>>> +            }
>>>>>> +            put_gfn(d, start);
>>>>>> +        }
>>>>>> +    }
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>> 
>>>>> ... nothing in this function looks to be x86-specific, except maybe
>>>>> functions like clear_identity_p2m_entry() may not currently be available
>>>>> on Arm. But this doesn't make the code x86-specific.
>>>> 
>>>> I will maybe be wrong but what I understand from the code is that for x86 
>>>> if there is no p2m entries setup for the region, accesses to them will be trapped 
>>>> into the hypervisor and can be handled by specific MMIO handler.
>>>> 
>>>> But for ARM when we are registering the MMIO handler we have to provide 
>>>> the GPA also for the MMIO handler. 
>>> 
>>> Question is: Is this just an effect resulting from different implementation,
>>> or an inherent requirement? In the former case, harmonizing things may be an
>>> alternative option.
>> 
>> This is an inherent requirement to provide a GPA when registering the MMIO handler.
> 
> So you first say yes to my "inherent" question, but then ...
> 
>> For x86 msix mmio handlers is registered in init_msix(..) function as there is no requirement
>> on x86 to provide GPA when registering the handler. Later point of time when BARs are configured
>> and memory decoding bit is enabled vpci_make_msix_hole() will clear the identity mapping for msix
>> base table address so that access to msix tables will be trapped.
>> 
>> On ARM we need to provide GPA to register the mmio handler and MSIX table base
>> address is not valid when init_msix() is called as BAR will be configured later point in time.
>> Therefore on ARM mmio handler will be registered in function vpci_make_msix_hole() when
>> memory decoding bit is enabled.
> 
> ... you explain it's an implementation detail. I'm inclined to
> suggest that x86 also pass the GPA where possible. Handler lookup
> really would benefit from not needing to iterate over all registered
> handlers, until one claims the access. The optimization part of this
> of course doesn't need to be done right here, but harmonizing
> register_mmio_handler() between both architectures would seem to be
> a reasonable prereq step.

I agree with you that if we modify the register_mmio_handler() for x86 to pass GPA
we can have the common code for x86 and ARM and also we can optimize the MMIO
trap handling for x86.

What I understand from the code is that modifying the register_mmio_handler() for
x86 to pass GPA requires a lot of effort and testing.

Unfortunately, I have another high priority task that I have to complete I don’t have time
to optimise the register_mmio_handler() for x86 at this time.

If you are ok if we can make vpci_make_msix_hole() function arch-specific something like
vpci_msix_arch_check_mmio() and get this patch merged.

Regards,
Rahul
> 
> I'm adding Paul to Cc in case he wants to comment, as this would
> touch his territory on the x86 side.
> 
> Jan
> 

Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Roger Pau Monné]

On Wed, Mar 09, 2022 at 10:08:12AM +0000, Rahul Singh wrote:
> Hi Jan,
> 
> > On 4 Mar 2022, at 7:23 am, Jan Beulich <jbeulich@suse.com> wrote:
> > 
> > On 03.03.2022 17:31, Rahul Singh wrote:
> >>> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeulich@suse.com> wrote:
> >>> On 01.03.2022 14:34, Rahul Singh wrote:
> >>>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@suse.com> wrote:
> >>>>> On 15.02.2022 16:25, Rahul Singh wrote:
> >>>>>> --- a/xen/arch/x86/hvm/vmsi.c
> >>>>>> +++ b/xen/arch/x86/hvm/vmsi.c
> >>>>>> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
> >>>>>> 
> >>>>>>   return 0;
> >>>>>> }
> >>>>>> +
> >>>>>> +int vpci_make_msix_hole(const struct pci_dev *pdev)
> >>>>>> +{
> >>>>>> +    struct domain *d = pdev->domain;
> >>>>>> +    unsigned int i;
> >>>>>> +
> >>>>>> +    if ( !pdev->vpci->msix )
> >>>>>> +        return 0;
> >>>>>> +
> >>>>>> +    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
> >>>>>> +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
> >>>>>> +    {
> >>>>>> +        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
> >>>>>> +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
> >>>>>> +                                     vmsix_table_size(pdev->vpci, i) - 1);
> >>>>>> +
> >>>>>> +        for ( ; start <= end; start++ )
> >>>>>> +        {
> >>>>>> +            p2m_type_t t;
> >>>>>> +            mfn_t mfn = get_gfn_query(d, start, &t);
> >>>>>> +
> >>>>>> +            switch ( t )
> >>>>>> +            {
> >>>>>> +            case p2m_mmio_dm:
> >>>>>> +            case p2m_invalid:
> >>>>>> +                break;
> >>>>>> +            case p2m_mmio_direct:
> >>>>>> +                if ( mfn_x(mfn) == start )
> >>>>>> +                {
> >>>>>> +                    clear_identity_p2m_entry(d, start);
> >>>>>> +                    break;
> >>>>>> +                }
> >>>>>> +                /* fallthrough. */
> >>>>>> +            default:
> >>>>>> +                put_gfn(d, start);
> >>>>>> +                gprintk(XENLOG_WARNING,
> >>>>>> +                        "%pp: existing mapping (mfn: %" PRI_mfn
> >>>>>> +                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
> >>>>>> +                        &pdev->sbdf, mfn_x(mfn), t, start);
> >>>>>> +                return -EEXIST;
> >>>>>> +            }
> >>>>>> +            put_gfn(d, start);
> >>>>>> +        }
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    return 0;
> >>>>>> +}
> >>>>> 
> >>>>> ... nothing in this function looks to be x86-specific, except maybe
> >>>>> functions like clear_identity_p2m_entry() may not currently be available
> >>>>> on Arm. But this doesn't make the code x86-specific.
> >>>> 
> >>>> I will maybe be wrong but what I understand from the code is that for x86 
> >>>> if there is no p2m entries setup for the region, accesses to them will be trapped 
> >>>> into the hypervisor and can be handled by specific MMIO handler.
> >>>> 
> >>>> But for ARM when we are registering the MMIO handler we have to provide 
> >>>> the GPA also for the MMIO handler. 

Right, but you still need those regions to not be mapped on the second
stage translation, or else no trap will be triggered and thus the
handlers won't run?

Regardless of whether the way to register the handlers is different on
Arm and x86, you still need to assure that the MSI-X related tables
are not mapped on the guest second stage translation, or else you are
just allowing guest access to the native ones.

So you do need this function on Arm in order to prevent hardware MSI-X
tables being accessed by the guest. Or are you suggesting it's
intended for Arm guest to access the native MSI-X tables?

Thanks, Roger.

Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Rahul Singh]

Hi Roger,

> On 9 Mar 2022, at 10:16 am, Roger Pau Monné <roger.pau@citrix.com> wrote:
> 
> On Wed, Mar 09, 2022 at 10:08:12AM +0000, Rahul Singh wrote:
>> Hi Jan,
>> 
>>> On 4 Mar 2022, at 7:23 am, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> On 03.03.2022 17:31, Rahul Singh wrote:
>>>>> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>>>> On 01.03.2022 14:34, Rahul Singh wrote:
>>>>>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>> On 15.02.2022 16:25, Rahul Singh wrote:
>>>>>>>> --- a/xen/arch/x86/hvm/vmsi.c
>>>>>>>> +++ b/xen/arch/x86/hvm/vmsi.c
>>>>>>>> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
>>>>>>>> 
>>>>>>>>  return 0;
>>>>>>>> }
>>>>>>>> +
>>>>>>>> +int vpci_make_msix_hole(const struct pci_dev *pdev)
>>>>>>>> +{
>>>>>>>> +    struct domain *d = pdev->domain;
>>>>>>>> +    unsigned int i;
>>>>>>>> +
>>>>>>>> +    if ( !pdev->vpci->msix )
>>>>>>>> +        return 0;
>>>>>>>> +
>>>>>>>> +    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
>>>>>>>> +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
>>>>>>>> +    {
>>>>>>>> +        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
>>>>>>>> +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
>>>>>>>> +                                     vmsix_table_size(pdev->vpci, i) - 1);
>>>>>>>> +
>>>>>>>> +        for ( ; start <= end; start++ )
>>>>>>>> +        {
>>>>>>>> +            p2m_type_t t;
>>>>>>>> +            mfn_t mfn = get_gfn_query(d, start, &t);
>>>>>>>> +
>>>>>>>> +            switch ( t )
>>>>>>>> +            {
>>>>>>>> +            case p2m_mmio_dm:
>>>>>>>> +            case p2m_invalid:
>>>>>>>> +                break;
>>>>>>>> +            case p2m_mmio_direct:
>>>>>>>> +                if ( mfn_x(mfn) == start )
>>>>>>>> +                {
>>>>>>>> +                    clear_identity_p2m_entry(d, start);
>>>>>>>> +                    break;
>>>>>>>> +                }
>>>>>>>> +                /* fallthrough. */
>>>>>>>> +            default:
>>>>>>>> +                put_gfn(d, start);
>>>>>>>> +                gprintk(XENLOG_WARNING,
>>>>>>>> +                        "%pp: existing mapping (mfn: %" PRI_mfn
>>>>>>>> +                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
>>>>>>>> +                        &pdev->sbdf, mfn_x(mfn), t, start);
>>>>>>>> +                return -EEXIST;
>>>>>>>> +            }
>>>>>>>> +            put_gfn(d, start);
>>>>>>>> +        }
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    return 0;
>>>>>>>> +}
>>>>>>> 
>>>>>>> ... nothing in this function looks to be x86-specific, except maybe
>>>>>>> functions like clear_identity_p2m_entry() may not currently be available
>>>>>>> on Arm. But this doesn't make the code x86-specific.
>>>>>> 
>>>>>> I will maybe be wrong but what I understand from the code is that for x86 
>>>>>> if there is no p2m entries setup for the region, accesses to them will be trapped 
>>>>>> into the hypervisor and can be handled by specific MMIO handler.
>>>>>> 
>>>>>> But for ARM when we are registering the MMIO handler we have to provide 
>>>>>> the GPA also for the MMIO handler. 
> 
> Right, but you still need those regions to not be mapped on the second
> stage translation, or else no trap will be triggered and thus the
> handlers won't run?
> 
> Regardless of whether the way to register the handlers is different on
> Arm and x86, you still need to assure that the MSI-X related tables
> are not mapped on the guest second stage translation, or else you are
> just allowing guest access to the native ones.

What I understand from the VPCI code we are not mapping the MSI-X related tables/BAR
to Stage-2 translation therefore no need to remove the mapping.

http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/drivers/vpci/header.c;h=a1c928a0d26f5cefd98727ee37f5dc43ceba80a6;hb=HEAD#l248

> 
> So you do need this function on Arm in order to prevent hardware MSI-X
> tables being accessed by the guest. Or are you suggesting it's
> intended for Arm guest to access the native MSI-X tables?

On ARM also access to the MSI-X tables will be trapped and physical MSI-X table
will be updated accordingly.

Regards,
Rahul
> 
> Thanks, Roger.

Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Roger Pau Monné]

On Wed, Mar 09, 2022 at 10:47:06AM +0000, Rahul Singh wrote:
> Hi Roger,
> 
> > On 9 Mar 2022, at 10:16 am, Roger Pau Monné <roger.pau@citrix.com> wrote:
> > 
> > On Wed, Mar 09, 2022 at 10:08:12AM +0000, Rahul Singh wrote:
> >> Hi Jan,
> >> 
> >>> On 4 Mar 2022, at 7:23 am, Jan Beulich <jbeulich@suse.com> wrote:
> >>> 
> >>> On 03.03.2022 17:31, Rahul Singh wrote:
> >>>>> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeulich@suse.com> wrote:
> >>>>> On 01.03.2022 14:34, Rahul Singh wrote:
> >>>>>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>> On 15.02.2022 16:25, Rahul Singh wrote:
> >>>>>>>> --- a/xen/arch/x86/hvm/vmsi.c
> >>>>>>>> +++ b/xen/arch/x86/hvm/vmsi.c
> >>>>>>>> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
> >>>>>>>> 
> >>>>>>>>  return 0;
> >>>>>>>> }
> >>>>>>>> +
> >>>>>>>> +int vpci_make_msix_hole(const struct pci_dev *pdev)
> >>>>>>>> +{
> >>>>>>>> +    struct domain *d = pdev->domain;
> >>>>>>>> +    unsigned int i;
> >>>>>>>> +
> >>>>>>>> +    if ( !pdev->vpci->msix )
> >>>>>>>> +        return 0;
> >>>>>>>> +
> >>>>>>>> +    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
> >>>>>>>> +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
> >>>>>>>> +    {
> >>>>>>>> +        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
> >>>>>>>> +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
> >>>>>>>> +                                     vmsix_table_size(pdev->vpci, i) - 1);
> >>>>>>>> +
> >>>>>>>> +        for ( ; start <= end; start++ )
> >>>>>>>> +        {
> >>>>>>>> +            p2m_type_t t;
> >>>>>>>> +            mfn_t mfn = get_gfn_query(d, start, &t);
> >>>>>>>> +
> >>>>>>>> +            switch ( t )
> >>>>>>>> +            {
> >>>>>>>> +            case p2m_mmio_dm:
> >>>>>>>> +            case p2m_invalid:
> >>>>>>>> +                break;
> >>>>>>>> +            case p2m_mmio_direct:
> >>>>>>>> +                if ( mfn_x(mfn) == start )
> >>>>>>>> +                {
> >>>>>>>> +                    clear_identity_p2m_entry(d, start);
> >>>>>>>> +                    break;
> >>>>>>>> +                }
> >>>>>>>> +                /* fallthrough. */
> >>>>>>>> +            default:
> >>>>>>>> +                put_gfn(d, start);
> >>>>>>>> +                gprintk(XENLOG_WARNING,
> >>>>>>>> +                        "%pp: existing mapping (mfn: %" PRI_mfn
> >>>>>>>> +                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
> >>>>>>>> +                        &pdev->sbdf, mfn_x(mfn), t, start);
> >>>>>>>> +                return -EEXIST;
> >>>>>>>> +            }
> >>>>>>>> +            put_gfn(d, start);
> >>>>>>>> +        }
> >>>>>>>> +    }
> >>>>>>>> +
> >>>>>>>> +    return 0;
> >>>>>>>> +}
> >>>>>>> 
> >>>>>>> ... nothing in this function looks to be x86-specific, except maybe
> >>>>>>> functions like clear_identity_p2m_entry() may not currently be available
> >>>>>>> on Arm. But this doesn't make the code x86-specific.
> >>>>>> 
> >>>>>> I will maybe be wrong but what I understand from the code is that for x86 
> >>>>>> if there is no p2m entries setup for the region, accesses to them will be trapped 
> >>>>>> into the hypervisor and can be handled by specific MMIO handler.
> >>>>>> 
> >>>>>> But for ARM when we are registering the MMIO handler we have to provide 
> >>>>>> the GPA also for the MMIO handler. 
> > 
> > Right, but you still need those regions to not be mapped on the second
> > stage translation, or else no trap will be triggered and thus the
> > handlers won't run?
> > 
> > Regardless of whether the way to register the handlers is different on
> > Arm and x86, you still need to assure that the MSI-X related tables
> > are not mapped on the guest second stage translation, or else you are
> > just allowing guest access to the native ones.
> 
> What I understand from the VPCI code we are not mapping the MSI-X related tables/BAR
> to Stage-2 translation therefore no need to remove the mapping.
> 
> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/drivers/vpci/header.c;h=a1c928a0d26f5cefd98727ee37f5dc43ceba80a6;hb=HEAD#l248

Right, sorry, was slightly confused. So this is indeed only needed if
Arm does some kind of pre-mapping of non-RAM regions. For example an
x86 PVH dom0 will add the regions marked as 'reserved' to the second
stage translation, and we need vpci_make_msix_hole in order to punch
holes there if those pre-mapped regions happen to overlap with any
MSI-X table.

If there aren't any non-RAM regions mapped on Arm for it's hardware
domain by default then I guess it's safe to make this arch-specific.

Thanks, Roger.

Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Jan Beulich]

On 09.03.2022 11:08, Rahul Singh wrote:
> Hi Jan,
> 
>> On 4 Mar 2022, at 7:23 am, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 03.03.2022 17:31, Rahul Singh wrote:
>>>> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 01.03.2022 14:34, Rahul Singh wrote:
>>>>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>>>>> On 15.02.2022 16:25, Rahul Singh wrote:
>>>>>>> --- a/xen/arch/x86/hvm/vmsi.c
>>>>>>> +++ b/xen/arch/x86/hvm/vmsi.c
>>>>>>> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
>>>>>>>
>>>>>>>   return 0;
>>>>>>> }
>>>>>>> +
>>>>>>> +int vpci_make_msix_hole(const struct pci_dev *pdev)
>>>>>>> +{
>>>>>>> +    struct domain *d = pdev->domain;
>>>>>>> +    unsigned int i;
>>>>>>> +
>>>>>>> +    if ( !pdev->vpci->msix )
>>>>>>> +        return 0;
>>>>>>> +
>>>>>>> +    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
>>>>>>> +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
>>>>>>> +    {
>>>>>>> +        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
>>>>>>> +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
>>>>>>> +                                     vmsix_table_size(pdev->vpci, i) - 1);
>>>>>>> +
>>>>>>> +        for ( ; start <= end; start++ )
>>>>>>> +        {
>>>>>>> +            p2m_type_t t;
>>>>>>> +            mfn_t mfn = get_gfn_query(d, start, &t);
>>>>>>> +
>>>>>>> +            switch ( t )
>>>>>>> +            {
>>>>>>> +            case p2m_mmio_dm:
>>>>>>> +            case p2m_invalid:
>>>>>>> +                break;
>>>>>>> +            case p2m_mmio_direct:
>>>>>>> +                if ( mfn_x(mfn) == start )
>>>>>>> +                {
>>>>>>> +                    clear_identity_p2m_entry(d, start);
>>>>>>> +                    break;
>>>>>>> +                }
>>>>>>> +                /* fallthrough. */
>>>>>>> +            default:
>>>>>>> +                put_gfn(d, start);
>>>>>>> +                gprintk(XENLOG_WARNING,
>>>>>>> +                        "%pp: existing mapping (mfn: %" PRI_mfn
>>>>>>> +                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
>>>>>>> +                        &pdev->sbdf, mfn_x(mfn), t, start);
>>>>>>> +                return -EEXIST;
>>>>>>> +            }
>>>>>>> +            put_gfn(d, start);
>>>>>>> +        }
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>>
>>>>>> ... nothing in this function looks to be x86-specific, except maybe
>>>>>> functions like clear_identity_p2m_entry() may not currently be available
>>>>>> on Arm. But this doesn't make the code x86-specific.
>>>>>
>>>>> I will maybe be wrong but what I understand from the code is that for x86 
>>>>> if there is no p2m entries setup for the region, accesses to them will be trapped 
>>>>> into the hypervisor and can be handled by specific MMIO handler.
>>>>>
>>>>> But for ARM when we are registering the MMIO handler we have to provide 
>>>>> the GPA also for the MMIO handler. 
>>>>
>>>> Question is: Is this just an effect resulting from different implementation,
>>>> or an inherent requirement? In the former case, harmonizing things may be an
>>>> alternative option.
>>>
>>> This is an inherent requirement to provide a GPA when registering the MMIO handler.
>>
>> So you first say yes to my "inherent" question, but then ...
>>
>>> For x86 msix mmio handlers is registered in init_msix(..) function as there is no requirement
>>> on x86 to provide GPA when registering the handler. Later point of time when BARs are configured
>>> and memory decoding bit is enabled vpci_make_msix_hole() will clear the identity mapping for msix
>>> base table address so that access to msix tables will be trapped.
>>>
>>> On ARM we need to provide GPA to register the mmio handler and MSIX table base
>>> address is not valid when init_msix() is called as BAR will be configured later point in time.
>>> Therefore on ARM mmio handler will be registered in function vpci_make_msix_hole() when
>>> memory decoding bit is enabled.
>>
>> ... you explain it's an implementation detail. I'm inclined to
>> suggest that x86 also pass the GPA where possible. Handler lookup
>> really would benefit from not needing to iterate over all registered
>> handlers, until one claims the access. The optimization part of this
>> of course doesn't need to be done right here, but harmonizing
>> register_mmio_handler() between both architectures would seem to be
>> a reasonable prereq step.
> 
> I agree with you that if we modify the register_mmio_handler() for x86 to pass GPA
> we can have the common code for x86 and ARM and also we can optimize the MMIO
> trap handling for x86.
> 
> What I understand from the code is that modifying the register_mmio_handler() for
> x86 to pass GPA requires a lot of effort and testing.
> 
> Unfortunately, I have another high priority task that I have to complete I don’t have time
> to optimise the register_mmio_handler() for x86 at this time.

Actually making use of the parameter is nothing I would expect you to
do. But is just adding the extra parameter similarly out of scope for
you?

Jan

Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Rahul Singh]

Hi Jan,

> On 9 Mar 2022, at 10:17 am, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 09.03.2022 11:08, Rahul Singh wrote:
>> Hi Jan,
>> 
>>> On 4 Mar 2022, at 7:23 am, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> On 03.03.2022 17:31, Rahul Singh wrote:
>>>>> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>>>> On 01.03.2022 14:34, Rahul Singh wrote:
>>>>>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>> On 15.02.2022 16:25, Rahul Singh wrote:
>>>>>>>> --- a/xen/arch/x86/hvm/vmsi.c
>>>>>>>> +++ b/xen/arch/x86/hvm/vmsi.c
>>>>>>>> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
>>>>>>>> 
>>>>>>>>  return 0;
>>>>>>>> }
>>>>>>>> +
>>>>>>>> +int vpci_make_msix_hole(const struct pci_dev *pdev)
>>>>>>>> +{
>>>>>>>> +    struct domain *d = pdev->domain;
>>>>>>>> +    unsigned int i;
>>>>>>>> +
>>>>>>>> +    if ( !pdev->vpci->msix )
>>>>>>>> +        return 0;
>>>>>>>> +
>>>>>>>> +    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
>>>>>>>> +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
>>>>>>>> +    {
>>>>>>>> +        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
>>>>>>>> +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
>>>>>>>> +                                     vmsix_table_size(pdev->vpci, i) - 1);
>>>>>>>> +
>>>>>>>> +        for ( ; start <= end; start++ )
>>>>>>>> +        {
>>>>>>>> +            p2m_type_t t;
>>>>>>>> +            mfn_t mfn = get_gfn_query(d, start, &t);
>>>>>>>> +
>>>>>>>> +            switch ( t )
>>>>>>>> +            {
>>>>>>>> +            case p2m_mmio_dm:
>>>>>>>> +            case p2m_invalid:
>>>>>>>> +                break;
>>>>>>>> +            case p2m_mmio_direct:
>>>>>>>> +                if ( mfn_x(mfn) == start )
>>>>>>>> +                {
>>>>>>>> +                    clear_identity_p2m_entry(d, start);
>>>>>>>> +                    break;
>>>>>>>> +                }
>>>>>>>> +                /* fallthrough. */
>>>>>>>> +            default:
>>>>>>>> +                put_gfn(d, start);
>>>>>>>> +                gprintk(XENLOG_WARNING,
>>>>>>>> +                        "%pp: existing mapping (mfn: %" PRI_mfn
>>>>>>>> +                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
>>>>>>>> +                        &pdev->sbdf, mfn_x(mfn), t, start);
>>>>>>>> +                return -EEXIST;
>>>>>>>> +            }
>>>>>>>> +            put_gfn(d, start);
>>>>>>>> +        }
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    return 0;
>>>>>>>> +}
>>>>>>> 
>>>>>>> ... nothing in this function looks to be x86-specific, except maybe
>>>>>>> functions like clear_identity_p2m_entry() may not currently be available
>>>>>>> on Arm. But this doesn't make the code x86-specific.
>>>>>> 
>>>>>> I will maybe be wrong but what I understand from the code is that for x86 
>>>>>> if there is no p2m entries setup for the region, accesses to them will be trapped 
>>>>>> into the hypervisor and can be handled by specific MMIO handler.
>>>>>> 
>>>>>> But for ARM when we are registering the MMIO handler we have to provide 
>>>>>> the GPA also for the MMIO handler. 
>>>>> 
>>>>> Question is: Is this just an effect resulting from different implementation,
>>>>> or an inherent requirement? In the former case, harmonizing things may be an
>>>>> alternative option.
>>>> 
>>>> This is an inherent requirement to provide a GPA when registering the MMIO handler.
>>> 
>>> So you first say yes to my "inherent" question, but then ...
>>> 
>>>> For x86 msix mmio handlers is registered in init_msix(..) function as there is no requirement
>>>> on x86 to provide GPA when registering the handler. Later point of time when BARs are configured
>>>> and memory decoding bit is enabled vpci_make_msix_hole() will clear the identity mapping for msix
>>>> base table address so that access to msix tables will be trapped.
>>>> 
>>>> On ARM we need to provide GPA to register the mmio handler and MSIX table base
>>>> address is not valid when init_msix() is called as BAR will be configured later point in time.
>>>> Therefore on ARM mmio handler will be registered in function vpci_make_msix_hole() when
>>>> memory decoding bit is enabled.
>>> 
>>> ... you explain it's an implementation detail. I'm inclined to
>>> suggest that x86 also pass the GPA where possible. Handler lookup
>>> really would benefit from not needing to iterate over all registered
>>> handlers, until one claims the access. The optimization part of this
>>> of course doesn't need to be done right here, but harmonizing
>>> register_mmio_handler() between both architectures would seem to be
>>> a reasonable prereq step.
>> 
>> I agree with you that if we modify the register_mmio_handler() for x86 to pass GPA
>> we can have the common code for x86 and ARM and also we can optimize the MMIO
>> trap handling for x86.
>> 
>> What I understand from the code is that modifying the register_mmio_handler() for
>> x86 to pass GPA requires a lot of effort and testing.
>> 
>> Unfortunately, I have another high priority task that I have to complete I don’t have time
>> to optimise the register_mmio_handler() for x86 at this time.
> 
> Actually making use of the parameter is nothing I would expect you to
> do. But is just adding the extra parameter similarly out of scope for
> you?
> 

If I understand correctly you are asking to make register_mmio_handler() declaration
same for x86 and ARM. You are not asking to modify the x86 MMIO handler logic to
use GPA to find the handler?

As Roger also mentioned that vpci_make_msix_hole() is required only for x86 to clear
the identity mapping. If we make the vpci_make_msix_hole() arch-specific there is no
need to modify the parameter for register_mmio_handler(), as for x86 and ARM
register_mmio_handler() will be called in different places.

For x86 register_mmio_handler() will be called in init_msix() whereas for ARM
register_mmio_handler() will be called  in vpci_make_msix_hole(). In this case we
have to move the call to register_mmio_handler() also to arch-specific files. If we move
the register_mmio_handler()  to arch specific file there is no need to make the
function common.

Regards,
Rahul 

> Jan

Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Jan Beulich]

On 09.03.2022 16:50, Rahul Singh wrote:
>> On 9 Mar 2022, at 10:17 am, Jan Beulich <jbeulich@suse.com> wrote:
>> On 09.03.2022 11:08, Rahul Singh wrote:
>>>> On 4 Mar 2022, at 7:23 am, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 03.03.2022 17:31, Rahul Singh wrote:
>>>>>> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>>>>> On 01.03.2022 14:34, Rahul Singh wrote:
>>>>>>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>> On 15.02.2022 16:25, Rahul Singh wrote:
>>>>>>>>> --- a/xen/arch/x86/hvm/vmsi.c
>>>>>>>>> +++ b/xen/arch/x86/hvm/vmsi.c
>>>>>>>>> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
>>>>>>>>>
>>>>>>>>>  return 0;
>>>>>>>>> }
>>>>>>>>> +
>>>>>>>>> +int vpci_make_msix_hole(const struct pci_dev *pdev)
>>>>>>>>> +{
>>>>>>>>> +    struct domain *d = pdev->domain;
>>>>>>>>> +    unsigned int i;
>>>>>>>>> +
>>>>>>>>> +    if ( !pdev->vpci->msix )
>>>>>>>>> +        return 0;
>>>>>>>>> +
>>>>>>>>> +    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
>>>>>>>>> +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
>>>>>>>>> +    {
>>>>>>>>> +        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
>>>>>>>>> +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
>>>>>>>>> +                                     vmsix_table_size(pdev->vpci, i) - 1);
>>>>>>>>> +
>>>>>>>>> +        for ( ; start <= end; start++ )
>>>>>>>>> +        {
>>>>>>>>> +            p2m_type_t t;
>>>>>>>>> +            mfn_t mfn = get_gfn_query(d, start, &t);
>>>>>>>>> +
>>>>>>>>> +            switch ( t )
>>>>>>>>> +            {
>>>>>>>>> +            case p2m_mmio_dm:
>>>>>>>>> +            case p2m_invalid:
>>>>>>>>> +                break;
>>>>>>>>> +            case p2m_mmio_direct:
>>>>>>>>> +                if ( mfn_x(mfn) == start )
>>>>>>>>> +                {
>>>>>>>>> +                    clear_identity_p2m_entry(d, start);
>>>>>>>>> +                    break;
>>>>>>>>> +                }
>>>>>>>>> +                /* fallthrough. */
>>>>>>>>> +            default:
>>>>>>>>> +                put_gfn(d, start);
>>>>>>>>> +                gprintk(XENLOG_WARNING,
>>>>>>>>> +                        "%pp: existing mapping (mfn: %" PRI_mfn
>>>>>>>>> +                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
>>>>>>>>> +                        &pdev->sbdf, mfn_x(mfn), t, start);
>>>>>>>>> +                return -EEXIST;
>>>>>>>>> +            }
>>>>>>>>> +            put_gfn(d, start);
>>>>>>>>> +        }
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    return 0;
>>>>>>>>> +}
>>>>>>>>
>>>>>>>> ... nothing in this function looks to be x86-specific, except maybe
>>>>>>>> functions like clear_identity_p2m_entry() may not currently be available
>>>>>>>> on Arm. But this doesn't make the code x86-specific.
>>>>>>>
>>>>>>> I will maybe be wrong but what I understand from the code is that for x86 
>>>>>>> if there is no p2m entries setup for the region, accesses to them will be trapped 
>>>>>>> into the hypervisor and can be handled by specific MMIO handler.
>>>>>>>
>>>>>>> But for ARM when we are registering the MMIO handler we have to provide 
>>>>>>> the GPA also for the MMIO handler. 
>>>>>>
>>>>>> Question is: Is this just an effect resulting from different implementation,
>>>>>> or an inherent requirement? In the former case, harmonizing things may be an
>>>>>> alternative option.
>>>>>
>>>>> This is an inherent requirement to provide a GPA when registering the MMIO handler.
>>>>
>>>> So you first say yes to my "inherent" question, but then ...
>>>>
>>>>> For x86 msix mmio handlers is registered in init_msix(..) function as there is no requirement
>>>>> on x86 to provide GPA when registering the handler. Later point of time when BARs are configured
>>>>> and memory decoding bit is enabled vpci_make_msix_hole() will clear the identity mapping for msix
>>>>> base table address so that access to msix tables will be trapped.
>>>>>
>>>>> On ARM we need to provide GPA to register the mmio handler and MSIX table base
>>>>> address is not valid when init_msix() is called as BAR will be configured later point in time.
>>>>> Therefore on ARM mmio handler will be registered in function vpci_make_msix_hole() when
>>>>> memory decoding bit is enabled.
>>>>
>>>> ... you explain it's an implementation detail. I'm inclined to
>>>> suggest that x86 also pass the GPA where possible. Handler lookup
>>>> really would benefit from not needing to iterate over all registered
>>>> handlers, until one claims the access. The optimization part of this
>>>> of course doesn't need to be done right here, but harmonizing
>>>> register_mmio_handler() between both architectures would seem to be
>>>> a reasonable prereq step.
>>>
>>> I agree with you that if we modify the register_mmio_handler() for x86 to pass GPA
>>> we can have the common code for x86 and ARM and also we can optimize the MMIO
>>> trap handling for x86.
>>>
>>> What I understand from the code is that modifying the register_mmio_handler() for
>>> x86 to pass GPA requires a lot of effort and testing.
>>>
>>> Unfortunately, I have another high priority task that I have to complete I don’t have time
>>> to optimise the register_mmio_handler() for x86 at this time.
>>
>> Actually making use of the parameter is nothing I would expect you to
>> do. But is just adding the extra parameter similarly out of scope for
>> you?
>>
> 
> If I understand correctly you are asking to make register_mmio_handler() declaration
> same for x86 and ARM. You are not asking to modify the x86 MMIO handler logic to
> use GPA to find the handler?

Yes, but ...

> As Roger also mentioned that vpci_make_msix_hole() is required only for x86 to clear
> the identity mapping. If we make the vpci_make_msix_hole() arch-specific there is no
> need to modify the parameter for register_mmio_handler(), as for x86 and ARM
> register_mmio_handler() will be called in different places.

... with Roger agreeing with this plan, that other alternative is
likely dead now. Provided other stuff which isn't obviously arch-
specific remains in common code.

Jan

Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Roger Pau Monné]

On Wed, Mar 09, 2022 at 03:50:12PM +0000, Rahul Singh wrote:
> Hi Jan,
> 
> > On 9 Mar 2022, at 10:17 am, Jan Beulich <jbeulich@suse.com> wrote:
> > 
> > On 09.03.2022 11:08, Rahul Singh wrote:
> >> Hi Jan,
> >> 
> >>> On 4 Mar 2022, at 7:23 am, Jan Beulich <jbeulich@suse.com> wrote:
> >>> 
> >>> On 03.03.2022 17:31, Rahul Singh wrote:
> >>>>> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeulich@suse.com> wrote:
> >>>>> On 01.03.2022 14:34, Rahul Singh wrote:
> >>>>>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>> On 15.02.2022 16:25, Rahul Singh wrote:
> >>>>>>>> --- a/xen/arch/x86/hvm/vmsi.c
> >>>>>>>> +++ b/xen/arch/x86/hvm/vmsi.c
> >>>>>>>> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
> >>>>>>>> 
> >>>>>>>>  return 0;
> >>>>>>>> }
> >>>>>>>> +
> >>>>>>>> +int vpci_make_msix_hole(const struct pci_dev *pdev)
> >>>>>>>> +{
> >>>>>>>> +    struct domain *d = pdev->domain;
> >>>>>>>> +    unsigned int i;
> >>>>>>>> +
> >>>>>>>> +    if ( !pdev->vpci->msix )
> >>>>>>>> +        return 0;
> >>>>>>>> +
> >>>>>>>> +    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
> >>>>>>>> +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
> >>>>>>>> +    {
> >>>>>>>> +        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
> >>>>>>>> +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
> >>>>>>>> +                                     vmsix_table_size(pdev->vpci, i) - 1);
> >>>>>>>> +
> >>>>>>>> +        for ( ; start <= end; start++ )
> >>>>>>>> +        {
> >>>>>>>> +            p2m_type_t t;
> >>>>>>>> +            mfn_t mfn = get_gfn_query(d, start, &t);
> >>>>>>>> +
> >>>>>>>> +            switch ( t )
> >>>>>>>> +            {
> >>>>>>>> +            case p2m_mmio_dm:
> >>>>>>>> +            case p2m_invalid:
> >>>>>>>> +                break;
> >>>>>>>> +            case p2m_mmio_direct:
> >>>>>>>> +                if ( mfn_x(mfn) == start )
> >>>>>>>> +                {
> >>>>>>>> +                    clear_identity_p2m_entry(d, start);
> >>>>>>>> +                    break;
> >>>>>>>> +                }
> >>>>>>>> +                /* fallthrough. */
> >>>>>>>> +            default:
> >>>>>>>> +                put_gfn(d, start);
> >>>>>>>> +                gprintk(XENLOG_WARNING,
> >>>>>>>> +                        "%pp: existing mapping (mfn: %" PRI_mfn
> >>>>>>>> +                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
> >>>>>>>> +                        &pdev->sbdf, mfn_x(mfn), t, start);
> >>>>>>>> +                return -EEXIST;
> >>>>>>>> +            }
> >>>>>>>> +            put_gfn(d, start);
> >>>>>>>> +        }
> >>>>>>>> +    }
> >>>>>>>> +
> >>>>>>>> +    return 0;
> >>>>>>>> +}
> >>>>>>> 
> >>>>>>> ... nothing in this function looks to be x86-specific, except maybe
> >>>>>>> functions like clear_identity_p2m_entry() may not currently be available
> >>>>>>> on Arm. But this doesn't make the code x86-specific.
> >>>>>> 
> >>>>>> I will maybe be wrong but what I understand from the code is that for x86 
> >>>>>> if there is no p2m entries setup for the region, accesses to them will be trapped 
> >>>>>> into the hypervisor and can be handled by specific MMIO handler.
> >>>>>> 
> >>>>>> But for ARM when we are registering the MMIO handler we have to provide 
> >>>>>> the GPA also for the MMIO handler. 
> >>>>> 
> >>>>> Question is: Is this just an effect resulting from different implementation,
> >>>>> or an inherent requirement? In the former case, harmonizing things may be an
> >>>>> alternative option.
> >>>> 
> >>>> This is an inherent requirement to provide a GPA when registering the MMIO handler.
> >>> 
> >>> So you first say yes to my "inherent" question, but then ...
> >>> 
> >>>> For x86 msix mmio handlers is registered in init_msix(..) function as there is no requirement
> >>>> on x86 to provide GPA when registering the handler. Later point of time when BARs are configured
> >>>> and memory decoding bit is enabled vpci_make_msix_hole() will clear the identity mapping for msix
> >>>> base table address so that access to msix tables will be trapped.
> >>>> 
> >>>> On ARM we need to provide GPA to register the mmio handler and MSIX table base
> >>>> address is not valid when init_msix() is called as BAR will be configured later point in time.
> >>>> Therefore on ARM mmio handler will be registered in function vpci_make_msix_hole() when
> >>>> memory decoding bit is enabled.
> >>> 
> >>> ... you explain it's an implementation detail. I'm inclined to
> >>> suggest that x86 also pass the GPA where possible. Handler lookup
> >>> really would benefit from not needing to iterate over all registered
> >>> handlers, until one claims the access. The optimization part of this
> >>> of course doesn't need to be done right here, but harmonizing
> >>> register_mmio_handler() between both architectures would seem to be
> >>> a reasonable prereq step.
> >> 
> >> I agree with you that if we modify the register_mmio_handler() for x86 to pass GPA
> >> we can have the common code for x86 and ARM and also we can optimize the MMIO
> >> trap handling for x86.
> >> 
> >> What I understand from the code is that modifying the register_mmio_handler() for
> >> x86 to pass GPA requires a lot of effort and testing.
> >> 
> >> Unfortunately, I have another high priority task that I have to complete I don’t have time
> >> to optimise the register_mmio_handler() for x86 at this time.
> > 
> > Actually making use of the parameter is nothing I would expect you to
> > do. But is just adding the extra parameter similarly out of scope for
> > you?
> > 
> 
> If I understand correctly you are asking to make register_mmio_handler() declaration
> same for x86 and ARM. You are not asking to modify the x86 MMIO handler logic to
> use GPA to find the handler?
> 
> As Roger also mentioned that vpci_make_msix_hole() is required only for x86 to clear
> the identity mapping. If we make the vpci_make_msix_hole() arch-specific there is no
> need to modify the parameter for register_mmio_handler(), as for x86 and ARM
> register_mmio_handler() will be called in different places.
> 
> For x86 register_mmio_handler() will be called in init_msix() whereas for ARM
> register_mmio_handler() will be called  in vpci_make_msix_hole(). In this case we
> have to move the call to register_mmio_handler() also to arch-specific files. If we move
> the register_mmio_handler()  to arch specific file there is no need to make the
> function common.

So then for Arm you will need something akin to
unregister_mmio_handler so the handler can be removed when memory
decoding is disabled?

Or else you would keep adding new handlers every time the guest
enables memory decoding for the device without having removed the
stale ones?

Thanks, Roger.

Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Rahul Singh]

Hello Roger,

> On 9 Mar 2022, at 4:06 pm, Roger Pau Monné <roger.pau@citrix.com> wrote:
> 
> On Wed, Mar 09, 2022 at 03:50:12PM +0000, Rahul Singh wrote:
>> Hi Jan,
>> 
>>> On 9 Mar 2022, at 10:17 am, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> On 09.03.2022 11:08, Rahul Singh wrote:
>>>> Hi Jan,
>>>> 
>>>>> On 4 Mar 2022, at 7:23 am, Jan Beulich <jbeulich@suse.com> wrote:
>>>>> 
>>>>> On 03.03.2022 17:31, Rahul Singh wrote:
>>>>>>> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>> On 01.03.2022 14:34, Rahul Singh wrote:
>>>>>>>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>> On 15.02.2022 16:25, Rahul Singh wrote:
>>>>>>>>>> --- a/xen/arch/x86/hvm/vmsi.c
>>>>>>>>>> +++ b/xen/arch/x86/hvm/vmsi.c
>>>>>>>>>> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
>>>>>>>>>> 
>>>>>>>>>> return 0;
>>>>>>>>>> }
>>>>>>>>>> +
>>>>>>>>>> +int vpci_make_msix_hole(const struct pci_dev *pdev)
>>>>>>>>>> +{
>>>>>>>>>> +    struct domain *d = pdev->domain;
>>>>>>>>>> +    unsigned int i;
>>>>>>>>>> +
>>>>>>>>>> +    if ( !pdev->vpci->msix )
>>>>>>>>>> +        return 0;
>>>>>>>>>> +
>>>>>>>>>> +    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
>>>>>>>>>> +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
>>>>>>>>>> +    {
>>>>>>>>>> +        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
>>>>>>>>>> +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
>>>>>>>>>> +                                     vmsix_table_size(pdev->vpci, i) - 1);
>>>>>>>>>> +
>>>>>>>>>> +        for ( ; start <= end; start++ )
>>>>>>>>>> +        {
>>>>>>>>>> +            p2m_type_t t;
>>>>>>>>>> +            mfn_t mfn = get_gfn_query(d, start, &t);
>>>>>>>>>> +
>>>>>>>>>> +            switch ( t )
>>>>>>>>>> +            {
>>>>>>>>>> +            case p2m_mmio_dm:
>>>>>>>>>> +            case p2m_invalid:
>>>>>>>>>> +                break;
>>>>>>>>>> +            case p2m_mmio_direct:
>>>>>>>>>> +                if ( mfn_x(mfn) == start )
>>>>>>>>>> +                {
>>>>>>>>>> +                    clear_identity_p2m_entry(d, start);
>>>>>>>>>> +                    break;
>>>>>>>>>> +                }
>>>>>>>>>> +                /* fallthrough. */
>>>>>>>>>> +            default:
>>>>>>>>>> +                put_gfn(d, start);
>>>>>>>>>> +                gprintk(XENLOG_WARNING,
>>>>>>>>>> +                        "%pp: existing mapping (mfn: %" PRI_mfn
>>>>>>>>>> +                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
>>>>>>>>>> +                        &pdev->sbdf, mfn_x(mfn), t, start);
>>>>>>>>>> +                return -EEXIST;
>>>>>>>>>> +            }
>>>>>>>>>> +            put_gfn(d, start);
>>>>>>>>>> +        }
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>> +    return 0;
>>>>>>>>>> +}
>>>>>>>>> 
>>>>>>>>> ... nothing in this function looks to be x86-specific, except maybe
>>>>>>>>> functions like clear_identity_p2m_entry() may not currently be available
>>>>>>>>> on Arm. But this doesn't make the code x86-specific.
>>>>>>>> 
>>>>>>>> I will maybe be wrong but what I understand from the code is that for x86 
>>>>>>>> if there is no p2m entries setup for the region, accesses to them will be trapped 
>>>>>>>> into the hypervisor and can be handled by specific MMIO handler.
>>>>>>>> 
>>>>>>>> But for ARM when we are registering the MMIO handler we have to provide 
>>>>>>>> the GPA also for the MMIO handler. 
>>>>>>> 
>>>>>>> Question is: Is this just an effect resulting from different implementation,
>>>>>>> or an inherent requirement? In the former case, harmonizing things may be an
>>>>>>> alternative option.
>>>>>> 
>>>>>> This is an inherent requirement to provide a GPA when registering the MMIO handler.
>>>>> 
>>>>> So you first say yes to my "inherent" question, but then ...
>>>>> 
>>>>>> For x86 msix mmio handlers is registered in init_msix(..) function as there is no requirement
>>>>>> on x86 to provide GPA when registering the handler. Later point of time when BARs are configured
>>>>>> and memory decoding bit is enabled vpci_make_msix_hole() will clear the identity mapping for msix
>>>>>> base table address so that access to msix tables will be trapped.
>>>>>> 
>>>>>> On ARM we need to provide GPA to register the mmio handler and MSIX table base
>>>>>> address is not valid when init_msix() is called as BAR will be configured later point in time.
>>>>>> Therefore on ARM mmio handler will be registered in function vpci_make_msix_hole() when
>>>>>> memory decoding bit is enabled.
>>>>> 
>>>>> ... you explain it's an implementation detail. I'm inclined to
>>>>> suggest that x86 also pass the GPA where possible. Handler lookup
>>>>> really would benefit from not needing to iterate over all registered
>>>>> handlers, until one claims the access. The optimization part of this
>>>>> of course doesn't need to be done right here, but harmonizing
>>>>> register_mmio_handler() between both architectures would seem to be
>>>>> a reasonable prereq step.
>>>> 
>>>> I agree with you that if we modify the register_mmio_handler() for x86 to pass GPA
>>>> we can have the common code for x86 and ARM and also we can optimize the MMIO
>>>> trap handling for x86.
>>>> 
>>>> What I understand from the code is that modifying the register_mmio_handler() for
>>>> x86 to pass GPA requires a lot of effort and testing.
>>>> 
>>>> Unfortunately, I have another high priority task that I have to complete I don’t have time
>>>> to optimise the register_mmio_handler() for x86 at this time.
>>> 
>>> Actually making use of the parameter is nothing I would expect you to
>>> do. But is just adding the extra parameter similarly out of scope for
>>> you?
>>> 
>> 
>> If I understand correctly you are asking to make register_mmio_handler() declaration
>> same for x86 and ARM. You are not asking to modify the x86 MMIO handler logic to
>> use GPA to find the handler?
>> 
>> As Roger also mentioned that vpci_make_msix_hole() is required only for x86 to clear
>> the identity mapping. If we make the vpci_make_msix_hole() arch-specific there is no
>> need to modify the parameter for register_mmio_handler(), as for x86 and ARM
>> register_mmio_handler() will be called in different places.
>> 
>> For x86 register_mmio_handler() will be called in init_msix() whereas for ARM
>> register_mmio_handler() will be called  in vpci_make_msix_hole(). In this case we
>> have to move the call to register_mmio_handler() also to arch-specific files. If we move
>> the register_mmio_handler()  to arch specific file there is no need to make the
>> function common.
> 
> So then for Arm you will need something akin to
> unregister_mmio_handler so the handler can be removed when memory
> decoding is disabled?
> 
> Or else you would keep adding new handlers every time the guest
> enables memory decoding for the device without having removed the
> stale ones?

Yes, when I will send the patches for ARM I will take care of this not to register the handler 
again if the memory decoding bit is changed. Before registering the handler will check 
if the handler is already for GPA if it is already registered no need to register.

Regards,
Rahul
> 
> Thanks, Roger.

Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Roger Pau Monné]

On Thu, Mar 10, 2022 at 11:48:15AM +0000, Rahul Singh wrote:
> Hello Roger,
> 
> > On 9 Mar 2022, at 4:06 pm, Roger Pau Monné <roger.pau@citrix.com> wrote:
> > 
> > On Wed, Mar 09, 2022 at 03:50:12PM +0000, Rahul Singh wrote:
> >> Hi Jan,
> >> 
> >>> On 9 Mar 2022, at 10:17 am, Jan Beulich <jbeulich@suse.com> wrote:
> >>> 
> >>> On 09.03.2022 11:08, Rahul Singh wrote:
> >>>> Hi Jan,
> >>>> 
> >>>>> On 4 Mar 2022, at 7:23 am, Jan Beulich <jbeulich@suse.com> wrote:
> >>>>> 
> >>>>> On 03.03.2022 17:31, Rahul Singh wrote:
> >>>>>>> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>> On 01.03.2022 14:34, Rahul Singh wrote:
> >>>>>>>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>> On 15.02.2022 16:25, Rahul Singh wrote:
> >>>>>>>>>> --- a/xen/arch/x86/hvm/vmsi.c
> >>>>>>>>>> +++ b/xen/arch/x86/hvm/vmsi.c
> >>>>>>>>>> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
> >>>>>>>>>> 
> >>>>>>>>>> return 0;
> >>>>>>>>>> }
> >>>>>>>>>> +
> >>>>>>>>>> +int vpci_make_msix_hole(const struct pci_dev *pdev)
> >>>>>>>>>> +{
> >>>>>>>>>> +    struct domain *d = pdev->domain;
> >>>>>>>>>> +    unsigned int i;
> >>>>>>>>>> +
> >>>>>>>>>> +    if ( !pdev->vpci->msix )
> >>>>>>>>>> +        return 0;
> >>>>>>>>>> +
> >>>>>>>>>> +    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
> >>>>>>>>>> +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
> >>>>>>>>>> +    {
> >>>>>>>>>> +        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
> >>>>>>>>>> +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
> >>>>>>>>>> +                                     vmsix_table_size(pdev->vpci, i) - 1);
> >>>>>>>>>> +
> >>>>>>>>>> +        for ( ; start <= end; start++ )
> >>>>>>>>>> +        {
> >>>>>>>>>> +            p2m_type_t t;
> >>>>>>>>>> +            mfn_t mfn = get_gfn_query(d, start, &t);
> >>>>>>>>>> +
> >>>>>>>>>> +            switch ( t )
> >>>>>>>>>> +            {
> >>>>>>>>>> +            case p2m_mmio_dm:
> >>>>>>>>>> +            case p2m_invalid:
> >>>>>>>>>> +                break;
> >>>>>>>>>> +            case p2m_mmio_direct:
> >>>>>>>>>> +                if ( mfn_x(mfn) == start )
> >>>>>>>>>> +                {
> >>>>>>>>>> +                    clear_identity_p2m_entry(d, start);
> >>>>>>>>>> +                    break;
> >>>>>>>>>> +                }
> >>>>>>>>>> +                /* fallthrough. */
> >>>>>>>>>> +            default:
> >>>>>>>>>> +                put_gfn(d, start);
> >>>>>>>>>> +                gprintk(XENLOG_WARNING,
> >>>>>>>>>> +                        "%pp: existing mapping (mfn: %" PRI_mfn
> >>>>>>>>>> +                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
> >>>>>>>>>> +                        &pdev->sbdf, mfn_x(mfn), t, start);
> >>>>>>>>>> +                return -EEXIST;
> >>>>>>>>>> +            }
> >>>>>>>>>> +            put_gfn(d, start);
> >>>>>>>>>> +        }
> >>>>>>>>>> +    }
> >>>>>>>>>> +
> >>>>>>>>>> +    return 0;
> >>>>>>>>>> +}
> >>>>>>>>> 
> >>>>>>>>> ... nothing in this function looks to be x86-specific, except maybe
> >>>>>>>>> functions like clear_identity_p2m_entry() may not currently be available
> >>>>>>>>> on Arm. But this doesn't make the code x86-specific.
> >>>>>>>> 
> >>>>>>>> I will maybe be wrong but what I understand from the code is that for x86 
> >>>>>>>> if there is no p2m entries setup for the region, accesses to them will be trapped 
> >>>>>>>> into the hypervisor and can be handled by specific MMIO handler.
> >>>>>>>> 
> >>>>>>>> But for ARM when we are registering the MMIO handler we have to provide 
> >>>>>>>> the GPA also for the MMIO handler. 
> >>>>>>> 
> >>>>>>> Question is: Is this just an effect resulting from different implementation,
> >>>>>>> or an inherent requirement? In the former case, harmonizing things may be an
> >>>>>>> alternative option.
> >>>>>> 
> >>>>>> This is an inherent requirement to provide a GPA when registering the MMIO handler.
> >>>>> 
> >>>>> So you first say yes to my "inherent" question, but then ...
> >>>>> 
> >>>>>> For x86 msix mmio handlers is registered in init_msix(..) function as there is no requirement
> >>>>>> on x86 to provide GPA when registering the handler. Later point of time when BARs are configured
> >>>>>> and memory decoding bit is enabled vpci_make_msix_hole() will clear the identity mapping for msix
> >>>>>> base table address so that access to msix tables will be trapped.
> >>>>>> 
> >>>>>> On ARM we need to provide GPA to register the mmio handler and MSIX table base
> >>>>>> address is not valid when init_msix() is called as BAR will be configured later point in time.
> >>>>>> Therefore on ARM mmio handler will be registered in function vpci_make_msix_hole() when
> >>>>>> memory decoding bit is enabled.
> >>>>> 
> >>>>> ... you explain it's an implementation detail. I'm inclined to
> >>>>> suggest that x86 also pass the GPA where possible. Handler lookup
> >>>>> really would benefit from not needing to iterate over all registered
> >>>>> handlers, until one claims the access. The optimization part of this
> >>>>> of course doesn't need to be done right here, but harmonizing
> >>>>> register_mmio_handler() between both architectures would seem to be
> >>>>> a reasonable prereq step.
> >>>> 
> >>>> I agree with you that if we modify the register_mmio_handler() for x86 to pass GPA
> >>>> we can have the common code for x86 and ARM and also we can optimize the MMIO
> >>>> trap handling for x86.
> >>>> 
> >>>> What I understand from the code is that modifying the register_mmio_handler() for
> >>>> x86 to pass GPA requires a lot of effort and testing.
> >>>> 
> >>>> Unfortunately, I have another high priority task that I have to complete I don’t have time
> >>>> to optimise the register_mmio_handler() for x86 at this time.
> >>> 
> >>> Actually making use of the parameter is nothing I would expect you to
> >>> do. But is just adding the extra parameter similarly out of scope for
> >>> you?
> >>> 
> >> 
> >> If I understand correctly you are asking to make register_mmio_handler() declaration
> >> same for x86 and ARM. You are not asking to modify the x86 MMIO handler logic to
> >> use GPA to find the handler?
> >> 
> >> As Roger also mentioned that vpci_make_msix_hole() is required only for x86 to clear
> >> the identity mapping. If we make the vpci_make_msix_hole() arch-specific there is no
> >> need to modify the parameter for register_mmio_handler(), as for x86 and ARM
> >> register_mmio_handler() will be called in different places.
> >> 
> >> For x86 register_mmio_handler() will be called in init_msix() whereas for ARM
> >> register_mmio_handler() will be called  in vpci_make_msix_hole(). In this case we
> >> have to move the call to register_mmio_handler() also to arch-specific files. If we move
> >> the register_mmio_handler()  to arch specific file there is no need to make the
> >> function common.
> > 
> > So then for Arm you will need something akin to
> > unregister_mmio_handler so the handler can be removed when memory
> > decoding is disabled?
> > 
> > Or else you would keep adding new handlers every time the guest
> > enables memory decoding for the device without having removed the
> > stale ones?
> 
> Yes, when I will send the patches for ARM I will take care of this not to register the handler 
> again if the memory decoding bit is changed. Before registering the handler will check 
> if the handler is already for GPA if it is already registered no need to register.

I think it might be helpful to post the Arm bits together with the
moving of the x86 ones. It's way easier to see why you need to make
certain things arch-specific if you also provide the Arm
implementation at the same time. Or else it's just code movement that
might need to be redone when Arm support is introduced if we deem that
certain parts could be unified.

Thanks, Roger.

[PATCH v2 2/3] xen/vpci: msix: change return value of vpci_msix_{read,write}

[Rahul Singh]

Return value is different for the MMIO handler on ARM and x86
architecture.

To make the code common for both architectures change the return value
of vpci_msix_{read, write} to bool. Architecture-specific return value
will be handled in arch code.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
Changes since v1:
 - Added in this version
---
 xen/arch/x86/hvm/vmsi.c | 10 ++++++++--
 xen/drivers/vpci/msix.c | 24 ++++++++++++------------
 xen/include/xen/vpci.h  |  8 ++++----
 3 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 17426f238c..761ce674d7 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -1002,7 +1002,10 @@ static int x86_msix_write(struct vcpu *v, unsigned long addr, unsigned int len,
     const struct domain *d = v->domain;
     struct vpci_msix *msix = vpci_msix_find(d, addr);
 
-    return vpci_msix_write(msix, addr, len, data);
+    if( !vpci_msix_write(msix, addr, len, data) )
+        return X86EMUL_RETRY;
+
+    return X86EMUL_OKAY;
 }
 
 static int x86_msix_read(struct vcpu *v, unsigned long addr, unsigned int len,
@@ -1011,7 +1014,10 @@ static int x86_msix_read(struct vcpu *v, unsigned long addr, unsigned int len,
     const struct domain *d = v->domain;
     struct vpci_msix *msix = vpci_msix_find(d, addr);
 
-    return vpci_msix_read(msix, addr, len, data);
+    if ( !vpci_msix_read(msix, addr, len, data) )
+        return X86EMUL_RETRY;
+
+    return X86EMUL_OKAY;
 }
 
 static const struct hvm_mmio_ops vpci_msix_table_ops = {
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index d89396a3b4..5b315757ef 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -155,8 +155,8 @@ static struct vpci_msix_entry *get_entry(struct vpci_msix *msix,
     return &msix->entries[(addr - start) / PCI_MSIX_ENTRY_SIZE];
 }
 
-int vpci_msix_read(struct vpci_msix *msix, unsigned long addr,
-                   unsigned int len, unsigned long *data)
+bool vpci_msix_read(struct vpci_msix *msix, unsigned long addr,
+                    unsigned int len, unsigned long *data)
 {
     const struct vpci_msix_entry *entry;
     unsigned int offset;
@@ -164,10 +164,10 @@ int vpci_msix_read(struct vpci_msix *msix, unsigned long addr,
     *data = ~0ul;
 
     if ( !msix )
-        return X86EMUL_RETRY;
+        return false;
 
     if ( !access_allowed(msix->pdev, addr, len) )
-        return X86EMUL_OKAY;
+        return true;
 
     if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
     {
@@ -193,7 +193,7 @@ int vpci_msix_read(struct vpci_msix *msix, unsigned long addr,
             break;
         }
 
-        return X86EMUL_OKAY;
+        return true;
     }
 
     spin_lock(&msix->pdev->vpci->lock);
@@ -227,21 +227,21 @@ int vpci_msix_read(struct vpci_msix *msix, unsigned long addr,
     }
     spin_unlock(&msix->pdev->vpci->lock);
 
-    return X86EMUL_OKAY;
+    return true;
 }
 
-int vpci_msix_write(struct vpci_msix *msix, unsigned long addr,
-                    unsigned int len, unsigned long data)
+bool vpci_msix_write(struct vpci_msix *msix, unsigned long addr,
+                     unsigned int len, unsigned long data)
 {
     const struct domain *d = msix->pdev->domain;
     struct vpci_msix_entry *entry;
     unsigned int offset;
 
     if ( !msix )
-        return X86EMUL_RETRY;
+        return false;
 
     if ( !access_allowed(msix->pdev, addr, len) )
-        return X86EMUL_OKAY;
+        return true;
 
     if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
     {
@@ -264,7 +264,7 @@ int vpci_msix_write(struct vpci_msix *msix, unsigned long addr,
             }
         }
 
-        return X86EMUL_OKAY;
+        return true;
     }
 
     spin_lock(&msix->pdev->vpci->lock);
@@ -342,7 +342,7 @@ int vpci_msix_write(struct vpci_msix *msix, unsigned long addr,
     }
     spin_unlock(&msix->pdev->vpci->lock);
 
-    return X86EMUL_OKAY;
+    return true;
 }
 
 static int init_msix(struct pci_dev *pdev)
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 0381a2c911..1c36845abf 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -225,11 +225,11 @@ bool vpci_ecam_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len,
 
 void vpci_msix_arch_register(struct vpci_msix *msix, struct domain *d);
 
-int vpci_msix_write(struct vpci_msix *msix, unsigned long addr,
-                    unsigned int len, unsigned long data);
+bool vpci_msix_write(struct vpci_msix *msix, unsigned long addr,
+                     unsigned int len, unsigned long data);
 
-int vpci_msix_read(struct vpci_msix *msix, unsigned long addr,
-                   unsigned int len, unsigned long *data);
+bool vpci_msix_read(struct vpci_msix *msix, unsigned long addr,
+                    unsigned int len, unsigned long *data);
 
 #endif /* __XEN__ */
 
-- 
2.25.1

[PATCH v2 3/3] xen/vpci: msix: move read/write call to MSI-X PBA entry to arch file

[Rahul Singh]

{read,write}{l,q} function argument is different for ARM and x86.
ARM {read,wrie}(l,q} function argument is pointer whereas X86
{read,wrie}(l,q} function argument is address itself.

{read,write}{l,q} is only used in common file to access the MSI-X PBA
structure. To avoid impacting other x86 code and to make the code common
move the read/write call to MSI-X PBA to arch specific file.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
Changes since v1:
 - Added in this version
---
 xen/arch/x86/hvm/vmsi.c | 47 +++++++++++++++++++++++++++++++++++++++++
 xen/drivers/vpci/msix.c | 43 ++-----------------------------------
 xen/include/xen/vpci.h  |  6 ++++++
 3 files changed, 55 insertions(+), 41 deletions(-)

diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 761ce674d7..f124a1d07d 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -1033,4 +1033,51 @@ void vpci_msix_arch_register(struct vpci_msix *msix, struct domain *d)
 
     list_add(&msix->next, &d->arch.hvm.msix_tables);
 }
+
+bool vpci_msix_arch_pba_read(unsigned long addr, unsigned int len,
+                             unsigned long *data)
+{
+    /*
+     * Access to PBA.
+     *
+     * TODO: note that this relies on having the PBA identity mapped to the
+     * guest address space. If this changes the address will need to be
+     * translated.
+     */
+    switch ( len )
+    {
+    case 4:
+        *data = readl(addr);
+        break;
+
+    case 8:
+        *data = readq(addr);
+        break;
+
+    default:
+        ASSERT_UNREACHABLE();
+        break;
+    }
+
+    return true;
+}
+
+void vpci_msix_arch_pba_write(unsigned long addr, unsigned int len,
+                              unsigned long data)
+{
+    switch ( len )
+    {
+    case 4:
+        writel(data, addr);
+        break;
+
+    case 8:
+        writeq(data, addr);
+        break;
+
+    default:
+        ASSERT_UNREACHABLE();
+        break;
+    }
+}
 #endif /* CONFIG_HAS_VPCI */
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 5b315757ef..b6720f1a1a 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -170,31 +170,7 @@ bool vpci_msix_read(struct vpci_msix *msix, unsigned long addr,
         return true;
 
     if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
-    {
-        /*
-         * Access to PBA.
-         *
-         * TODO: note that this relies on having the PBA identity mapped to the
-         * guest address space. If this changes the address will need to be
-         * translated.
-         */
-        switch ( len )
-        {
-        case 4:
-            *data = readl(addr);
-            break;
-
-        case 8:
-            *data = readq(addr);
-            break;
-
-        default:
-            ASSERT_UNREACHABLE();
-            break;
-        }
-
-        return true;
-    }
+        return vpci_msix_arch_pba_read(addr, len, data);
 
     spin_lock(&msix->pdev->vpci->lock);
     entry = get_entry(msix, addr);
@@ -247,22 +223,7 @@ bool vpci_msix_write(struct vpci_msix *msix, unsigned long addr,
     {
         /* Ignore writes to PBA for DomUs, it's behavior is undefined. */
         if ( is_hardware_domain(d) )
-        {
-            switch ( len )
-            {
-            case 4:
-                writel(data, addr);
-                break;
-
-            case 8:
-                writeq(data, addr);
-                break;
-
-            default:
-                ASSERT_UNREACHABLE();
-                break;
-            }
-        }
+            vpci_msix_arch_pba_write(addr, len, data);
 
         return true;
     }
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 1c36845abf..a61daf9d53 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -231,6 +231,12 @@ bool vpci_msix_write(struct vpci_msix *msix, unsigned long addr,
 bool vpci_msix_read(struct vpci_msix *msix, unsigned long addr,
                     unsigned int len, unsigned long *data);
 
+bool vpci_msix_arch_pba_read(unsigned long addr, unsigned int len,
+                             unsigned long *data);
+
+void vpci_msix_arch_pba_write(unsigned long addr, unsigned int len,
+                              unsigned long data);
+
 #endif /* __XEN__ */
 
 #else /* !CONFIG_HAS_VPCI */
-- 
2.25.1

Re: [PATCH v2 3/3] xen/vpci: msix: move read/write call to MSI-X PBA entry to arch file

[Jan Beulich]

On 15.02.2022 16:25, Rahul Singh wrote:
> {read,write}{l,q} function argument is different for ARM and x86.
> ARM {read,wrie}(l,q} function argument is pointer whereas X86
> {read,wrie}(l,q} function argument is address itself.

I'm afraid I don't follow: x86 has

#define readl(x) (*(volatile uint32_t *)(x))
#define readq(x) (*(volatile uint64_t *)(x))

That's no different from Arm64:

#define readl(c)                ({ u32 __v = readl_relaxed(c); __iormb(); __v; })

#define readl_relaxed(c)        ({ u32 __v = le32_to_cpu((__force __le32)__raw_readl(c)); __v; })

static inline u32 __raw_readl(const volatile void __iomem *addr)

The difference is whether the address is expressed as a pointer, or
_may_ also be expressed as unsigned long. IOW the x86 variant is
perfectly fine to be passed e.g. a void * (preferably qualified
appropriately). The conversion from unsigned long to a pointer type
is actually expressed ...

> @@ -170,31 +170,7 @@ bool vpci_msix_read(struct vpci_msix *msix, unsigned long addr,
>          return true;
>  
>      if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
> -    {
> -        /*
> -         * Access to PBA.
> -         *
> -         * TODO: note that this relies on having the PBA identity mapped to the
> -         * guest address space. If this changes the address will need to be
> -         * translated.
> -         */
> -        switch ( len )
> -        {
> -        case 4:
> -            *data = readl(addr);
> -            break;
> -
> -        case 8:
> -            *data = readq(addr);
> -            break;
> -
> -        default:
> -            ASSERT_UNREACHABLE();
> -            break;
> -        }

... in the comment ahead of this switch() (and the assumption is likely
wrong for DomU).

But then, Roger: What "identity mapped" is meant here? Surely not GVA ->
GPA, but rather GPA -> HPA? The address here is a guest physical one,
but read{l,q}() act on (host) virtual addresses. This would have been
easier to notice as wrong if read{l,q}() weren't allowing unsigned long
arguments to be passed to them.

Jan

Re: [PATCH v2 3/3] xen/vpci: msix: move read/write call to MSI-X PBA entry to arch file

[Roger Pau Monné]

On Thu, Feb 24, 2022 at 04:18:31PM +0100, Jan Beulich wrote:
> On 15.02.2022 16:25, Rahul Singh wrote:
> > @@ -170,31 +170,7 @@ bool vpci_msix_read(struct vpci_msix *msix, unsigned long addr,
> >          return true;
> >  
> >      if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
> > -    {
> > -        /*
> > -         * Access to PBA.
> > -         *
> > -         * TODO: note that this relies on having the PBA identity mapped to the
> > -         * guest address space. If this changes the address will need to be
> > -         * translated.
> > -         */
> > -        switch ( len )
> > -        {
> > -        case 4:
> > -            *data = readl(addr);
> > -            break;
> > -
> > -        case 8:
> > -            *data = readq(addr);
> > -            break;
> > -
> > -        default:
> > -            ASSERT_UNREACHABLE();
> > -            break;
> > -        }
> 
> ... in the comment ahead of this switch() (and the assumption is likely
> wrong for DomU).
> 
> But then, Roger: What "identity mapped" is meant here? Surely not GVA ->
> GPA, but rather GPA -> HPA? The address here is a guest physical one,
> but read{l,q}() act on (host) virtual addresses. This would have been
> easier to notice as wrong if read{l,q}() weren't allowing unsigned long
> arguments to be passed to them.

Urg, indeed, thanks for noticing. I will send a patch to fix this
right now.

Roger.

Re: [PATCH v2 3/3] xen/vpci: msix: move read/write call to MSI-X PBA entry to arch file

[Roger Pau Monné]

On Tue, Feb 15, 2022 at 03:25:18PM +0000, Rahul Singh wrote:
> {read,write}{l,q} function argument is different for ARM and x86.
> ARM {read,wrie}(l,q} function argument is pointer whereas X86
> {read,wrie}(l,q} function argument is address itself.
> 
> {read,write}{l,q} is only used in common file to access the MSI-X PBA
> structure. To avoid impacting other x86 code and to make the code common
> move the read/write call to MSI-X PBA to arch specific file.

I think we agreed where going to unify {read,write}{l,q} so they could
be used in arch-agnostic code?

Thanks, Roger.

Re: [PATCH v2 3/3] xen/vpci: msix: move read/write call to MSI-X PBA entry to arch file

[Rahul Singh]

Hi Roger,

> On 25 Feb 2022, at 8:20 am, Roger Pau Monné <roger.pau@citrix.com> wrote:
> 
> On Tue, Feb 15, 2022 at 03:25:18PM +0000, Rahul Singh wrote:
>> {read,write}{l,q} function argument is different for ARM and x86.
>> ARM {read,wrie}(l,q} function argument is pointer whereas X86
>> {read,wrie}(l,q} function argument is address itself.
>> 
>> {read,write}{l,q} is only used in common file to access the MSI-X PBA
>> structure. To avoid impacting other x86 code and to make the code common
>> move the read/write call to MSI-X PBA to arch specific file.
> 
> I think we agreed where going to unify {read,write}{l,q} so they could
> be used in arch-agnostic code?

We agreed to modify the vPCI MSIx code to use a pointer, but that not 
helped me to make code arch-agnostic. I decided to move the PBA handling code to an 
arch-specific file to make the code usable.

Thanks for the series  "vpci/msix: fix PBA acceses” series that will help to use the 
code for ARM arch also without any modification to {read,write}{l,q} .

Regards,
Rahul
> 
> Thanks, Roger.
>