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
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
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
>
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
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
>
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
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
>
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.
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.
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.
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
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
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.
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.
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.
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
© 2016 - 2024 Red Hat, Inc.