xen/arch/x86/msi.c | 64 +++++++++++++++++++++++++++++++++++ xen/drivers/passthrough/pci.c | 51 +++++++--------------------- xen/drivers/pci/Kconfig | 4 +++ xen/drivers/vpci/Makefile | 3 +- xen/drivers/vpci/header.c | 19 +++-------- xen/drivers/vpci/msix.c | 25 ++++++++++++++ xen/drivers/vpci/vpci.c | 3 +- xen/include/asm-arm/msi.h | 44 ++++++++++++++++++++++++ xen/include/asm-x86/msi.h | 4 +++ xen/include/xen/pci.h | 11 +++--- xen/include/xen/vpci.h | 24 ++++++++++++- xen/xsm/flask/hooks.c | 8 ++--- 12 files changed, 195 insertions(+), 65 deletions(-) create mode 100644 xen/include/asm-arm/msi.h
MSI related code in the "passthrough/pci.c” file is not useful for ARM
when MSI interrupts are injected via GICv3 ITS.
Therefore introducing the new flag CONFIG_PCI_MSI_INTERCEPT to gate the
MSI code for ARM in common code. Also move the arch-specific code to an
arch-specific directory and implement the stub version for the unused
code for ARM to avoid compilation error when HAS_PCI is enabled for ARM.
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
Changes since v1:
- Rename CONFIG_HAS_PCI_MSI to CONFIG_PCI_MSI_INTERCEPT
- Implement stub version of the unused function for ARM.
- Move unneeded function to x86 file.
---
xen/arch/x86/msi.c | 64 +++++++++++++++++++++++++++++++++++
xen/drivers/passthrough/pci.c | 51 +++++++---------------------
xen/drivers/pci/Kconfig | 4 +++
xen/drivers/vpci/Makefile | 3 +-
xen/drivers/vpci/header.c | 19 +++--------
xen/drivers/vpci/msix.c | 25 ++++++++++++++
xen/drivers/vpci/vpci.c | 3 +-
xen/include/asm-arm/msi.h | 44 ++++++++++++++++++++++++
xen/include/asm-x86/msi.h | 4 +++
xen/include/xen/pci.h | 11 +++---
xen/include/xen/vpci.h | 24 ++++++++++++-
xen/xsm/flask/hooks.c | 8 ++---
12 files changed, 195 insertions(+), 65 deletions(-)
create mode 100644 xen/include/asm-arm/msi.h
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 5febc0ea4b..a6356f4a63 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -1411,6 +1411,70 @@ void __init early_msi_init(void)
return;
}
+int alloc_pci_msi(struct pci_dev *pdev)
+{
+ unsigned int pos;
+
+ INIT_LIST_HEAD(&pdev->msi_list);
+
+ pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
+ PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSI);
+ if ( pos )
+ {
+ uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
+
+ pdev->msi_maxvec = multi_msi_capable(ctrl);
+ }
+
+ pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
+ PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSIX);
+ if ( pos )
+ {
+ struct arch_msix *msix = xzalloc(struct arch_msix);
+ uint16_t ctrl;
+
+ if ( !msix )
+ return -ENOMEM;
+
+ spin_lock_init(&msix->table_lock);
+
+ ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
+ msix->nr_entries = msix_table_size(ctrl);
+
+ pdev->msix = msix;
+ }
+
+ return 0;
+}
+
+void free_pci_msi(struct pci_dev *pdev)
+{
+ xfree(pdev->msix);
+}
+
+int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev)
+{
+ int rc;
+
+ if ( pdev->msix )
+ {
+ rc = pci_reset_msix_state(pdev);
+ if ( rc )
+ return rc;
+ msixtbl_init(d);
+ }
+
+ return 0;
+}
+
+void dump_pci_msi(struct pci_dev *pdev)
+{
+ struct msi_desc *msi;
+
+ list_for_each_entry ( msi, &pdev->msi_list, list )
+ printk("%d ", msi->irq);
+}
+
static void dump_msi(unsigned char key)
{
unsigned int irq;
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 705137f8be..1b473502a1 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -314,6 +314,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
{
struct pci_dev *pdev;
unsigned int pos;
+ int rc;
list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
if ( pdev->bus == bus && pdev->devfn == devfn )
@@ -327,35 +328,12 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
*((u8*) &pdev->bus) = bus;
*((u8*) &pdev->devfn) = devfn;
pdev->domain = NULL;
- INIT_LIST_HEAD(&pdev->msi_list);
-
- pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
- PCI_CAP_ID_MSI);
- if ( pos )
- {
- uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
-
- pdev->msi_maxvec = multi_msi_capable(ctrl);
- }
- pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
- PCI_CAP_ID_MSIX);
- if ( pos )
+ rc = alloc_pci_msi(pdev);
+ if ( rc )
{
- struct arch_msix *msix = xzalloc(struct arch_msix);
- uint16_t ctrl;
-
- if ( !msix )
- {
- xfree(pdev);
- return NULL;
- }
- spin_lock_init(&msix->table_lock);
-
- ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
- msix->nr_entries = msix_table_size(ctrl);
-
- pdev->msix = msix;
+ xfree(pdev);
+ return NULL;
}
list_add(&pdev->alldevs_list, &pseg->alldevs_list);
@@ -449,7 +427,7 @@ static void free_pdev(struct pci_seg *pseg, struct pci_dev *pdev)
}
list_del(&pdev->alldevs_list);
- xfree(pdev->msix);
+ free_pci_msi(pdev);
xfree(pdev);
}
@@ -1271,7 +1249,6 @@ bool_t pcie_aer_get_firmware_first(const struct pci_dev *pdev)
static int _dump_pci_devices(struct pci_seg *pseg, void *arg)
{
struct pci_dev *pdev;
- struct msi_desc *msi;
printk("==== segment %04x ====\n", pseg->nr);
@@ -1280,8 +1257,7 @@ static int _dump_pci_devices(struct pci_seg *pseg, void *arg)
printk("%pp - %pd - node %-3d - MSIs < ",
&pdev->sbdf, pdev->domain,
(pdev->node != NUMA_NO_NODE) ? pdev->node : -1);
- list_for_each_entry ( msi, &pdev->msi_list, list )
- printk("%d ", msi->irq);
+ dump_pci_msi(pdev);
printk(">\n");
}
@@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
}
__initcall(setup_dump_pcidevs);
+
+#ifdef CONFIG_PCI_MSI_INTERCEPT
int iommu_update_ire_from_msi(
struct msi_desc *msi_desc, struct msi_msg *msg)
{
return iommu_intremap
? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
}
+#endif
static int iommu_add_device(struct pci_dev *pdev)
{
@@ -1429,13 +1408,9 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
ASSERT(pdev && (pdev->domain == hardware_domain ||
pdev->domain == dom_io));
- if ( pdev->msix )
- {
- rc = pci_reset_msix_state(pdev);
- if ( rc )
- goto done;
- msixtbl_init(d);
- }
+ rc = pci_assign_msix_init(d, pdev);
+ if ( rc )
+ goto done;
pdev->fault.count = 0;
diff --git a/xen/drivers/pci/Kconfig b/xen/drivers/pci/Kconfig
index 7da03fa13b..7ab92bde6e 100644
--- a/xen/drivers/pci/Kconfig
+++ b/xen/drivers/pci/Kconfig
@@ -1,3 +1,7 @@
config HAS_PCI
bool
+
+config PCI_MSI_INTERCEPT
+ def_bool y
+ depends on X86 && HAS_PCI
diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile
index 55d1bdfda0..f91fa71a40 100644
--- a/xen/drivers/vpci/Makefile
+++ b/xen/drivers/vpci/Makefile
@@ -1 +1,2 @@
-obj-y += vpci.o header.o msi.o msix.o
+obj-y += vpci.o header.o
+obj-$(CONFIG_PCI_MSI_INTERCEPT) += msi.o msix.o
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index ba9a036202..a7695a0e2a 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -206,7 +206,6 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
struct vpci_header *header = &pdev->vpci->header;
struct rangeset *mem = rangeset_new(NULL, NULL, 0);
struct pci_dev *tmp, *dev = NULL;
- const struct vpci_msix *msix = pdev->vpci->msix;
unsigned int i;
int rc;
@@ -244,21 +243,11 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
}
/* Remove any MSIX regions if present. */
- for ( i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ )
+ rc = remove_msix_regions(pdev,mem);
+ if ( rc )
{
- 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);
-
- rc = rangeset_remove_range(mem, start, end);
- if ( rc )
- {
- printk(XENLOG_G_WARNING
- "Failed to remove MSIX table [%lx, %lx]: %d\n",
- start, end, rc);
- rangeset_destroy(mem);
- return rc;
- }
+ rangeset_destroy(mem);
+ return rc;
}
/*
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 846f1b8d70..ca0a9b5665 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -428,6 +428,31 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
return 0;
}
+int remove_msix_regions(const struct pci_dev *pdev, struct rangeset *mem)
+{
+ const struct vpci_msix *msix = pdev->vpci->msix;
+ unsigned int i;
+ int rc;
+
+ for ( i = 0; msix && i < ARRAY_SIZE(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);
+
+ rc = rangeset_remove_range(mem, start, end);
+ if ( rc )
+ {
+ printk(XENLOG_G_WARNING
+ "Failed to remove MSIX table [%lx, %lx]: %d\n",
+ start, end, rc);
+ return rc;
+ }
+ }
+
+ return 0;
+}
+
static int init_msix(struct pci_dev *pdev)
{
struct domain *d = pdev->domain;
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index cbd1bac7fc..ebe40b0538 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -48,8 +48,7 @@ void vpci_remove_device(struct pci_dev *pdev)
xfree(r);
}
spin_unlock(&pdev->vpci->lock);
- xfree(pdev->vpci->msix);
- xfree(pdev->vpci->msi);
+ free_vpci_msi(pdev);
xfree(pdev->vpci);
pdev->vpci = NULL;
}
diff --git a/xen/include/asm-arm/msi.h b/xen/include/asm-arm/msi.h
new file mode 100644
index 0000000000..c54193310e
--- /dev/null
+++ b/xen/include/asm-arm/msi.h
@@ -0,0 +1,44 @@
+/*
+ * Copyright (C) 2021 Arm Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __ASM_MSI_H_
+#define __ASM_MSI_H_
+
+static inline int alloc_pci_msi(struct pci_dev *pdev)
+{
+ return 0;
+}
+
+static inline int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev)
+{
+ return 0;
+}
+
+static inline void dump_pci_msi(struct pci_dev *pdev) {}
+static inline void free_pci_msi(struct pci_dev *pdev) {}
+static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
+
+#endif /* __ASM_MSI_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h
index e228b0f3f3..7ef0161b67 100644
--- a/xen/include/asm-x86/msi.h
+++ b/xen/include/asm-x86/msi.h
@@ -252,5 +252,9 @@ void unmask_msi_irq(struct irq_desc *);
void guest_mask_msi_irq(struct irq_desc *, bool mask);
void ack_nonmaskable_msi_irq(struct irq_desc *);
void set_msi_affinity(struct irq_desc *, const cpumask_t *);
+int alloc_pci_msi(struct pci_dev *pdev);
+void free_pci_msi(struct pci_dev *pdev);
+void dump_pci_msi(struct pci_dev *pdev);
+int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev);
#endif /* __ASM_MSI_H */
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 8e3d4d9454..f5b57270be 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -79,10 +79,6 @@ struct pci_dev {
struct list_head alldevs_list;
struct list_head domain_list;
- struct list_head msi_list;
-
- struct arch_msix *msix;
-
struct domain *domain;
const union {
@@ -94,7 +90,14 @@ struct pci_dev {
pci_sbdf_t sbdf;
};
+#ifdef CONFIG_PCI_MSI_INTERCEPT
+ struct list_head msi_list;
+
+ struct arch_msix *msix;
+
uint8_t msi_maxvec;
+#endif
+
uint8_t phantom_stride;
nodeid_t node; /* NUMA node */
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 9f5b5d52e1..a6b12717b1 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -91,6 +91,7 @@ struct vpci {
/* FIXME: currently there's no support for SR-IOV. */
} header;
+#ifdef CONFIG_PCI_MSI_INTERCEPT
/* MSI data. */
struct vpci_msi {
/* Address. */
@@ -136,6 +137,7 @@ struct vpci {
struct vpci_arch_msix_entry arch;
} entries[];
} *msix;
+#endif /* CONFIG_PCI_MSI_INTERCEPT */
#endif
};
@@ -148,6 +150,7 @@ struct vpci_vcpu {
};
#ifdef __XEN__
+#ifdef CONFIG_PCI_MSI_INTERCEPT
void vpci_dump_msi(void);
/* Make sure there's a hole in the p2m for the MSIX mmio areas. */
@@ -174,7 +177,7 @@ int __must_check vpci_msix_arch_disable_entry(struct vpci_msix_entry *entry,
const struct pci_dev *pdev);
void vpci_msix_arch_init_entry(struct vpci_msix_entry *entry);
int vpci_msix_arch_print(const struct vpci_msix *msix);
-
+int remove_msix_regions(const struct pci_dev *pdev, struct rangeset *mem);
/*
* Helper functions to fetch MSIX related data. They are used by both the
* emulated MSIX code and the BAR handlers.
@@ -208,6 +211,25 @@ static inline unsigned int vmsix_entry_nr(const struct vpci_msix *msix,
{
return entry - msix->entries;
}
+
+static inline void free_vpci_msi(const struct pci_dev *pdev)
+{
+ xfree(pdev->vpci->msix);
+ xfree(pdev->vpci->msi);
+}
+#else /* !CONFIG_PCI_MSI_INTERCEPT */
+static inline int vpci_make_msix_hole(const struct pci_dev *pdev)
+{
+ return 0;
+}
+
+static inline int remove_msix_regions(const struct pci_dev *pdev,
+ struct rangeset *mem)
+{
+ return 0;
+}
+static inline void free_vpci_msi(const struct pci_dev *pdev) {}
+#endif /* CONFIG_PCI_MSI_INTERCEPT */
#endif /* __XEN__ */
#else /* !CONFIG_HAS_VPCI */
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 3b7313b949..1d6c994444 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -21,7 +21,7 @@
#include <xen/guest_access.h>
#include <xen/xenoprof.h>
#include <xen/iommu.h>
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_PCI_MSI_INTERCEPT
#include <asm/msi.h>
#endif
#include <public/xen.h>
@@ -114,7 +114,7 @@ static int get_irq_sid(int irq, u32 *sid, struct avc_audit_data *ad)
}
return security_irq_sid(irq, sid);
}
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_PCI_MSI_INTERCEPT
{
struct irq_desc *desc = irq_to_desc(irq);
if ( desc->msi_desc && desc->msi_desc->dev ) {
@@ -868,7 +868,7 @@ static int flask_map_domain_pirq (struct domain *d)
static int flask_map_domain_msi (struct domain *d, int irq, const void *data,
u32 *sid, struct avc_audit_data *ad)
{
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_PCI_MSI_INTERCEPT
const struct msi_info *msi = data;
u32 machine_bdf = (msi->seg << 16) | (msi->bus << 8) | msi->devfn;
@@ -934,7 +934,7 @@ static int flask_unmap_domain_pirq (struct domain *d)
static int flask_unmap_domain_msi (struct domain *d, int irq, const void *data,
u32 *sid, struct avc_audit_data *ad)
{
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_PCI_MSI_INTERCEPT
const struct pci_dev *pdev = data;
u32 machine_bdf = (pdev->seg << 16) | (pdev->bus << 8) | pdev->devfn;
--
2.17.1
On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
> MSI related code in the "passthrough/pci.c” file is not useful for ARM
> when MSI interrupts are injected via GICv3 ITS.
>
> Therefore introducing the new flag CONFIG_PCI_MSI_INTERCEPT to gate the
> MSI code for ARM in common code. Also move the arch-specific code to an
> arch-specific directory and implement the stub version for the unused
> code for ARM to avoid compilation error when HAS_PCI is enabled for ARM.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
> Changes since v1:
> - Rename CONFIG_HAS_PCI_MSI to CONFIG_PCI_MSI_INTERCEPT
> - Implement stub version of the unused function for ARM.
> - Move unneeded function to x86 file.
> ---
> xen/arch/x86/msi.c | 64 +++++++++++++++++++++++++++++++++++
> xen/drivers/passthrough/pci.c | 51 +++++++---------------------
> xen/drivers/pci/Kconfig | 4 +++
> xen/drivers/vpci/Makefile | 3 +-
> xen/drivers/vpci/header.c | 19 +++--------
> xen/drivers/vpci/msix.c | 25 ++++++++++++++
> xen/drivers/vpci/vpci.c | 3 +-
> xen/include/asm-arm/msi.h | 44 ++++++++++++++++++++++++
> xen/include/asm-x86/msi.h | 4 +++
> xen/include/xen/pci.h | 11 +++---
> xen/include/xen/vpci.h | 24 ++++++++++++-
> xen/xsm/flask/hooks.c | 8 ++---
> 12 files changed, 195 insertions(+), 65 deletions(-)
> create mode 100644 xen/include/asm-arm/msi.h
>
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index 5febc0ea4b..a6356f4a63 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -1411,6 +1411,70 @@ void __init early_msi_init(void)
> return;
> }
>
> +int alloc_pci_msi(struct pci_dev *pdev)
I would rather name it pci_msi_init...
> +{
> + unsigned int pos;
> +
> + INIT_LIST_HEAD(&pdev->msi_list);
> +
> + pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> + PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSI);
> + if ( pos )
> + {
> + uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
> +
> + pdev->msi_maxvec = multi_msi_capable(ctrl);
> + }
> +
> + pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> + PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSIX);
> + if ( pos )
> + {
> + struct arch_msix *msix = xzalloc(struct arch_msix);
> + uint16_t ctrl;
> +
> + if ( !msix )
> + return -ENOMEM;
> +
> + spin_lock_init(&msix->table_lock);
> +
> + ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
> + msix->nr_entries = msix_table_size(ctrl);
> +
> + pdev->msix = msix;
> + }
> +
> + return 0;
> +}
> +
> +void free_pci_msi(struct pci_dev *pdev)
...and pci_msi_free.
> +{
> + xfree(pdev->msix);
> +}
> +
> +int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev)
This kind of a confusing name - what about pci_msix_assign?
> +{
> + int rc;
> +
> + if ( pdev->msix )
> + {
> + rc = pci_reset_msix_state(pdev);
> + if ( rc )
> + return rc;
> + msixtbl_init(d);
> + }
> +
> + return 0;
> +}
> +
> +void dump_pci_msi(struct pci_dev *pdev)
> +{
> + struct msi_desc *msi;
> +
> + list_for_each_entry ( msi, &pdev->msi_list, list )
> + printk("%d ", msi->irq);
> +}
I wonder, those those function really want to be in a x86 specific
file? There's nothing x86 specific about them AFAICT.
Would it make sense to create a separate msi-intercept.c file with
those that gets included when CONFIG_PCI_MSI_INTERCEPT?
> static void dump_msi(unsigned char key)
> {
> unsigned int irq;
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 705137f8be..1b473502a1 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -314,6 +314,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
> {
> struct pci_dev *pdev;
> unsigned int pos;
> + int rc;
>
> list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> if ( pdev->bus == bus && pdev->devfn == devfn )
> @@ -327,35 +328,12 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
> *((u8*) &pdev->bus) = bus;
> *((u8*) &pdev->devfn) = devfn;
> pdev->domain = NULL;
> - INIT_LIST_HEAD(&pdev->msi_list);
> -
> - pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
> - PCI_CAP_ID_MSI);
> - if ( pos )
> - {
> - uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
> -
> - pdev->msi_maxvec = multi_msi_capable(ctrl);
> - }
>
> - pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
> - PCI_CAP_ID_MSIX);
> - if ( pos )
> + rc = alloc_pci_msi(pdev);
> + if ( rc )
> {
> - struct arch_msix *msix = xzalloc(struct arch_msix);
> - uint16_t ctrl;
> -
> - if ( !msix )
> - {
> - xfree(pdev);
> - return NULL;
> - }
> - spin_lock_init(&msix->table_lock);
> -
> - ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
> - msix->nr_entries = msix_table_size(ctrl);
> -
> - pdev->msix = msix;
> + xfree(pdev);
> + return NULL;
> }
>
> list_add(&pdev->alldevs_list, &pseg->alldevs_list);
> @@ -449,7 +427,7 @@ static void free_pdev(struct pci_seg *pseg, struct pci_dev *pdev)
> }
>
> list_del(&pdev->alldevs_list);
> - xfree(pdev->msix);
> + free_pci_msi(pdev);
> xfree(pdev);
> }
>
> @@ -1271,7 +1249,6 @@ bool_t pcie_aer_get_firmware_first(const struct pci_dev *pdev)
> static int _dump_pci_devices(struct pci_seg *pseg, void *arg)
> {
> struct pci_dev *pdev;
> - struct msi_desc *msi;
>
> printk("==== segment %04x ====\n", pseg->nr);
>
> @@ -1280,8 +1257,7 @@ static int _dump_pci_devices(struct pci_seg *pseg, void *arg)
> printk("%pp - %pd - node %-3d - MSIs < ",
> &pdev->sbdf, pdev->domain,
> (pdev->node != NUMA_NO_NODE) ? pdev->node : -1);
> - list_for_each_entry ( msi, &pdev->msi_list, list )
> - printk("%d ", msi->irq);
> + dump_pci_msi(pdev);
> printk(">\n");
> }
>
> @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
> }
> __initcall(setup_dump_pcidevs);
>
> +
> +#ifdef CONFIG_PCI_MSI_INTERCEPT
> int iommu_update_ire_from_msi(
> struct msi_desc *msi_desc, struct msi_msg *msg)
> {
> return iommu_intremap
> ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
> }
> +#endif
This is not exactly related to MSI intercepts, the IOMMU interrupt
remapping table will also be used for interrupt entries for devices
being used by Xen directly, where no intercept is required.
And then you also want to gate the hook from iommu_ops itself with
CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.
>
> static int iommu_add_device(struct pci_dev *pdev)
> {
> @@ -1429,13 +1408,9 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
> ASSERT(pdev && (pdev->domain == hardware_domain ||
> pdev->domain == dom_io));
>
> - if ( pdev->msix )
> - {
> - rc = pci_reset_msix_state(pdev);
> - if ( rc )
> - goto done;
> - msixtbl_init(d);
> - }
> + rc = pci_assign_msix_init(d, pdev);
> + if ( rc )
> + goto done;
>
> pdev->fault.count = 0;
>
> diff --git a/xen/drivers/pci/Kconfig b/xen/drivers/pci/Kconfig
> index 7da03fa13b..7ab92bde6e 100644
> --- a/xen/drivers/pci/Kconfig
> +++ b/xen/drivers/pci/Kconfig
> @@ -1,3 +1,7 @@
>
> config HAS_PCI
> bool
> +
> +config PCI_MSI_INTERCEPT
> + def_bool y
> + depends on X86 && HAS_PCI
> diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile
> index 55d1bdfda0..f91fa71a40 100644
> --- a/xen/drivers/vpci/Makefile
> +++ b/xen/drivers/vpci/Makefile
> @@ -1 +1,2 @@
> -obj-y += vpci.o header.o msi.o msix.o
> +obj-y += vpci.o header.o
> +obj-$(CONFIG_PCI_MSI_INTERCEPT) += msi.o msix.o
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index ba9a036202..a7695a0e2a 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -206,7 +206,6 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
> struct vpci_header *header = &pdev->vpci->header;
> struct rangeset *mem = rangeset_new(NULL, NULL, 0);
> struct pci_dev *tmp, *dev = NULL;
> - const struct vpci_msix *msix = pdev->vpci->msix;
> unsigned int i;
> int rc;
>
> @@ -244,21 +243,11 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
> }
>
> /* Remove any MSIX regions if present. */
> - for ( i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ )
> + rc = remove_msix_regions(pdev,mem);
Coding style: missing space between parameters.
> + if ( rc )
> {
> - 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);
> -
> - rc = rangeset_remove_range(mem, start, end);
> - if ( rc )
> - {
> - printk(XENLOG_G_WARNING
> - "Failed to remove MSIX table [%lx, %lx]: %d\n",
> - start, end, rc);
> - rangeset_destroy(mem);
> - return rc;
> - }
> + rangeset_destroy(mem);
> + return rc;
> }
>
> /*
> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> index 846f1b8d70..ca0a9b5665 100644
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -428,6 +428,31 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
> return 0;
> }
>
> +int remove_msix_regions(const struct pci_dev *pdev, struct rangeset *mem)
This needs to start with a vpci_ prefix, since it's a global function.
What about vpci_msix_remove_regions.
> +{
> + const struct vpci_msix *msix = pdev->vpci->msix;
> + unsigned int i;
> + int rc;
> +
> + for ( i = 0; msix && i < ARRAY_SIZE(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);
> +
> + rc = rangeset_remove_range(mem, start, end);
> + if ( rc )
> + {
> + printk(XENLOG_G_WARNING
> + "Failed to remove MSIX table [%lx, %lx]: %d\n",
> + start, end, rc);
> + return rc;
> + }
> + }
> +
> + return 0;
> +}
> +
> static int init_msix(struct pci_dev *pdev)
> {
> struct domain *d = pdev->domain;
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index cbd1bac7fc..ebe40b0538 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -48,8 +48,7 @@ void vpci_remove_device(struct pci_dev *pdev)
> xfree(r);
> }
> spin_unlock(&pdev->vpci->lock);
> - xfree(pdev->vpci->msix);
> - xfree(pdev->vpci->msi);
> + free_vpci_msi(pdev);
> xfree(pdev->vpci);
> pdev->vpci = NULL;
> }
> diff --git a/xen/include/asm-arm/msi.h b/xen/include/asm-arm/msi.h
> new file mode 100644
> index 0000000000..c54193310e
> --- /dev/null
> +++ b/xen/include/asm-arm/msi.h
> @@ -0,0 +1,44 @@
> +/*
> + * Copyright (C) 2021 Arm Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __ASM_MSI_H_
> +#define __ASM_MSI_H_
> +
> +static inline int alloc_pci_msi(struct pci_dev *pdev)
> +{
> + return 0;
> +}
> +
> +static inline int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev)
> +{
> + return 0;
> +}
> +
> +static inline void dump_pci_msi(struct pci_dev *pdev) {}
> +static inline void free_pci_msi(struct pci_dev *pdev) {}
> +static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
> +
> +#endif /* __ASM_MSI_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
Should you instead have a non-arch specific file with those dummy
helpers that get used when !CONFIG_PCI_MSI_INTERCEPT?
> diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h
> index e228b0f3f3..7ef0161b67 100644
> --- a/xen/include/asm-x86/msi.h
> +++ b/xen/include/asm-x86/msi.h
> @@ -252,5 +252,9 @@ void unmask_msi_irq(struct irq_desc *);
> void guest_mask_msi_irq(struct irq_desc *, bool mask);
> void ack_nonmaskable_msi_irq(struct irq_desc *);
> void set_msi_affinity(struct irq_desc *, const cpumask_t *);
> +int alloc_pci_msi(struct pci_dev *pdev);
> +void free_pci_msi(struct pci_dev *pdev);
> +void dump_pci_msi(struct pci_dev *pdev);
> +int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev);
>
> #endif /* __ASM_MSI_H */
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index 8e3d4d9454..f5b57270be 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -79,10 +79,6 @@ struct pci_dev {
> struct list_head alldevs_list;
> struct list_head domain_list;
>
> - struct list_head msi_list;
> -
> - struct arch_msix *msix;
> -
> struct domain *domain;
>
> const union {
> @@ -94,7 +90,14 @@ struct pci_dev {
> pci_sbdf_t sbdf;
> };
>
> +#ifdef CONFIG_PCI_MSI_INTERCEPT
> + struct list_head msi_list;
> +
> + struct arch_msix *msix;
> +
> uint8_t msi_maxvec;
> +#endif
> +
> uint8_t phantom_stride;
>
> nodeid_t node; /* NUMA node */
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index 9f5b5d52e1..a6b12717b1 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -91,6 +91,7 @@ struct vpci {
> /* FIXME: currently there's no support for SR-IOV. */
> } header;
>
> +#ifdef CONFIG_PCI_MSI_INTERCEPT
> /* MSI data. */
> struct vpci_msi {
> /* Address. */
> @@ -136,6 +137,7 @@ struct vpci {
> struct vpci_arch_msix_entry arch;
> } entries[];
> } *msix;
> +#endif /* CONFIG_PCI_MSI_INTERCEPT */
Note that here you just remove two pointers from the struct, not that
I'm opposed to it, but it's not that much space that's saved anyway.
Ie: it might also be fine to just leave them as NULL unconditionally
on Arm.
> #endif
> };
>
> @@ -148,6 +150,7 @@ struct vpci_vcpu {
> };
>
> #ifdef __XEN__
> +#ifdef CONFIG_PCI_MSI_INTERCEPT
> void vpci_dump_msi(void);
>
> /* Make sure there's a hole in the p2m for the MSIX mmio areas. */
> @@ -174,7 +177,7 @@ int __must_check vpci_msix_arch_disable_entry(struct vpci_msix_entry *entry,
> const struct pci_dev *pdev);
> void vpci_msix_arch_init_entry(struct vpci_msix_entry *entry);
> int vpci_msix_arch_print(const struct vpci_msix *msix);
> -
> +int remove_msix_regions(const struct pci_dev *pdev, struct rangeset *mem);
> /*
> * Helper functions to fetch MSIX related data. They are used by both the
> * emulated MSIX code and the BAR handlers.
> @@ -208,6 +211,25 @@ static inline unsigned int vmsix_entry_nr(const struct vpci_msix *msix,
> {
> return entry - msix->entries;
> }
> +
> +static inline void free_vpci_msi(const struct pci_dev *pdev)
vpci_msi_free please.
Thanks, Roger.
On 12.04.2021 12:49, Roger Pau Monné wrote:
> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
>> --- a/xen/arch/x86/msi.c
>> +++ b/xen/arch/x86/msi.c
>> @@ -1411,6 +1411,70 @@ void __init early_msi_init(void)
>> return;
>> }
>>
>> +int alloc_pci_msi(struct pci_dev *pdev)
>
> I would rather name it pci_msi_init...
Or maybe pdev_msi_init(), as pci_msi_init() looks more like a one-
time, global init function?
>> +void free_pci_msi(struct pci_dev *pdev)
>
> ...and pci_msi_free.
The counterpart of "init" really would be "deinit", IOW I'd like to
ask for either alloc/free or init/deinit.
>> +{
>> + xfree(pdev->msix);
Could this maybe become XFREE() at this occasion?
>> +int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev)
>
> This kind of a confusing name - what about pci_msix_assign?
>
>> +{
>> + int rc;
>> +
>> + if ( pdev->msix )
Wouldn't this check better live in the sole caller?
>> +void dump_pci_msi(struct pci_dev *pdev)
pdev_dump_msi() or some such?
Also - const here and ...
>> +{
>> + struct msi_desc *msi;
... here please, while you already move this code.
>> + list_for_each_entry ( msi, &pdev->msi_list, list )
>> + printk("%d ", msi->irq);
>> +}
>
> I wonder, those those function really want to be in a x86 specific
> file? There's nothing x86 specific about them AFAICT.
>
> Would it make sense to create a separate msi-intercept.c file with
> those that gets included when CONFIG_PCI_MSI_INTERCEPT?
+1
>> @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
>> }
>> __initcall(setup_dump_pcidevs);
>>
>> +
>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
No double blank lines please.
>> int iommu_update_ire_from_msi(
>> struct msi_desc *msi_desc, struct msi_msg *msg)
>> {
>> return iommu_intremap
>> ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
>> }
>> +#endif
>
> This is not exactly related to MSI intercepts, the IOMMU interrupt
> remapping table will also be used for interrupt entries for devices
> being used by Xen directly, where no intercept is required.
>
> And then you also want to gate the hook from iommu_ops itself with
> CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.
I think the two aspects of MSI should be kept separate.
>> --- a/xen/drivers/pci/Kconfig
>> +++ b/xen/drivers/pci/Kconfig
>> @@ -1,3 +1,7 @@
>>
>> config HAS_PCI
>> bool
>> +
>> +config PCI_MSI_INTERCEPT
>> + def_bool y
>> + depends on X86 && HAS_PCI
Depending on HAS_PCI is fine (albeit not strictly needed afaict),
but this shouldn't have a default (like HAS_PCI doesn't) and
instead be selected by x86.
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -48,8 +48,7 @@ void vpci_remove_device(struct pci_dev *pdev)
>> xfree(r);
>> }
>> spin_unlock(&pdev->vpci->lock);
>> - xfree(pdev->vpci->msix);
>> - xfree(pdev->vpci->msi);
>> + free_vpci_msi(pdev);
I don't think the function needs to be passed a pdev, and ...
>> xfree(pdev->vpci);
>> pdev->vpci = NULL;
... it would only be consistent with this if pdev->vpci was passed
instead.
>> --- /dev/null
>> +++ b/xen/include/asm-arm/msi.h
>> @@ -0,0 +1,44 @@
>> +/*
>> + * Copyright (C) 2021 Arm Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef __ASM_MSI_H_
>> +#define __ASM_MSI_H_
>> +
>> +static inline int alloc_pci_msi(struct pci_dev *pdev)
>> +{
>> + return 0;
>> +}
>> +
>> +static inline int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev)
>> +{
>> + return 0;
>> +}
>> +
>> +static inline void dump_pci_msi(struct pci_dev *pdev) {}
>> +static inline void free_pci_msi(struct pci_dev *pdev) {}
>> +static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
>> +
>> +#endif /* __ASM_MSI_H */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>
> Should you instead have a non-arch specific file with those dummy
> helpers that get used when !CONFIG_PCI_MSI_INTERCEPT?
+1
>> --- a/xen/include/asm-x86/msi.h
>> +++ b/xen/include/asm-x86/msi.h
>> @@ -252,5 +252,9 @@ void unmask_msi_irq(struct irq_desc *);
>> void guest_mask_msi_irq(struct irq_desc *, bool mask);
>> void ack_nonmaskable_msi_irq(struct irq_desc *);
>> void set_msi_affinity(struct irq_desc *, const cpumask_t *);
>> +int alloc_pci_msi(struct pci_dev *pdev);
>> +void free_pci_msi(struct pci_dev *pdev);
>> +void dump_pci_msi(struct pci_dev *pdev);
>> +int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev);
These should then live in the other "half" of that header.
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -79,10 +79,6 @@ struct pci_dev {
>> struct list_head alldevs_list;
>> struct list_head domain_list;
>>
>> - struct list_head msi_list;
>> -
>> - struct arch_msix *msix;
>> -
>> struct domain *domain;
>>
>> const union {
>> @@ -94,7 +90,14 @@ struct pci_dev {
>> pci_sbdf_t sbdf;
>> };
>>
>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>> + struct list_head msi_list;
>> +
>> + struct arch_msix *msix;
>> +
>> uint8_t msi_maxvec;
>> +#endif
>> +
>> uint8_t phantom_stride;
Like Roger also said for struct vpci, it's not clear this is worth
it. And while you may have paid attention (and there simply is no
better arrangement) I'd also like to point out that such field
movement should be done while keeping padding hole sizes in mind.
>> @@ -174,7 +177,7 @@ int __must_check vpci_msix_arch_disable_entry(struct vpci_msix_entry *entry,
>> const struct pci_dev *pdev);
>> void vpci_msix_arch_init_entry(struct vpci_msix_entry *entry);
>> int vpci_msix_arch_print(const struct vpci_msix *msix);
>> -
>> +int remove_msix_regions(const struct pci_dev *pdev, struct rangeset *mem);
>> /*
>> * Helper functions to fetch MSIX related data. They are used by both the
>> * emulated MSIX code and the BAR handlers.
Please don't remove blank lines like this one, unless you actually
see a reason.
Jan
Hi Jan,
> On 12 Apr 2021, at 12:28 pm, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 12.04.2021 12:49, Roger Pau Monné wrote:
>> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
>>> --- a/xen/arch/x86/msi.c
>>> +++ b/xen/arch/x86/msi.c
>>> @@ -1411,6 +1411,70 @@ void __init early_msi_init(void)
>>> return;
>>> }
>>>
>>> +int alloc_pci_msi(struct pci_dev *pdev)
>>
>> I would rather name it pci_msi_init...
>
> Or maybe pdev_msi_init(), as pci_msi_init() looks more like a one-
> time, global init function?
>
Ok. I will use pdev_msi_init().
>>> +void free_pci_msi(struct pci_dev *pdev)
>>
>> ...and pci_msi_free.
>
> The counterpart of "init" really would be "deinit", IOW I'd like to
> ask for either alloc/free or init/deinit.
Ok. I will use pdev_msi_deinit().
>
>>> +{
>>> + xfree(pdev->msix);
>
> Could this maybe become XFREE() at this occasion?
Ok.
>
>>> +int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev)
>>
>> This kind of a confusing name - what about pci_msix_assign?
>>
>>> +{
>>> + int rc;
>>> +
>>> + if ( pdev->msix )
>
> Wouldn't this check better live in the sole caller?
Ok.
>
>
>>> +void dump_pci_msi(struct pci_dev *pdev)
>
> pdev_dump_msi() or some such?
>
> Also - const here and ...
Ok.
>
>>> +{
>>> + struct msi_desc *msi;
>
> ... here please, while you already move this code.
Ok.
>
>>> + list_for_each_entry ( msi, &pdev->msi_list, list )
>>> + printk("%d ", msi->irq);
>>> +}
>>
>> I wonder, those those function really want to be in a x86 specific
>> file? There's nothing x86 specific about them AFAICT.
>>
>> Would it make sense to create a separate msi-intercept.c file with
>> those that gets included when CONFIG_PCI_MSI_INTERCEPT?
>
> +1
>
>>> @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
>>> }
>>> __initcall(setup_dump_pcidevs);
>>>
>>> +
>>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>
> No double blank lines please.
Ok.
>
>>> int iommu_update_ire_from_msi(
>>> struct msi_desc *msi_desc, struct msi_msg *msg)
>>> {
>>> return iommu_intremap
>>> ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
>>> }
>>> +#endif
>>
>> This is not exactly related to MSI intercepts, the IOMMU interrupt
>> remapping table will also be used for interrupt entries for devices
>> being used by Xen directly, where no intercept is required.
>>
>> And then you also want to gate the hook from iommu_ops itself with
>> CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.
>
> I think the two aspects of MSI should be kept separate.
Ok. I will split the patch in two patches.
>
>>> --- a/xen/drivers/pci/Kconfig
>>> +++ b/xen/drivers/pci/Kconfig
>>> @@ -1,3 +1,7 @@
>>>
>>> config HAS_PCI
>>> bool
>>> +
>>> +config PCI_MSI_INTERCEPT
>>> + def_bool y
>>> + depends on X86 && HAS_PCI
>
> Depending on HAS_PCI is fine (albeit not strictly needed afaict),
> but this shouldn't have a default (like HAS_PCI doesn't) and
> instead be selected by x86.
Ok.
>
>>> --- a/xen/drivers/vpci/vpci.c
>>> +++ b/xen/drivers/vpci/vpci.c
>>> @@ -48,8 +48,7 @@ void vpci_remove_device(struct pci_dev *pdev)
>>> xfree(r);
>>> }
>>> spin_unlock(&pdev->vpci->lock);
>>> - xfree(pdev->vpci->msix);
>>> - xfree(pdev->vpci->msi);
>>> + free_vpci_msi(pdev);
>
> I don't think the function needs to be passed a pdev, and ...
Ok.
>>> xfree(pdev->vpci);
>>> pdev->vpci = NULL;
>
> ... it would only be consistent with this if pdev->vpci was passed
> instead.
OK.
>
>>> --- /dev/null
>>> +++ b/xen/include/asm-arm/msi.h
>>> @@ -0,0 +1,44 @@
>>> +/*
>>> + * Copyright (C) 2021 Arm Ltd.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#ifndef __ASM_MSI_H_
>>> +#define __ASM_MSI_H_
>>> +
>>> +static inline int alloc_pci_msi(struct pci_dev *pdev)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +static inline int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +static inline void dump_pci_msi(struct pci_dev *pdev) {}
>>> +static inline void free_pci_msi(struct pci_dev *pdev) {}
>>> +static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
>>> +
>>> +#endif /* __ASM_MSI_H */
>>> +
>>> +/*
>>> + * Local variables:
>>> + * mode: C
>>> + * c-file-style: "BSD"
>>> + * c-basic-offset: 4
>>> + * tab-width: 4
>>> + * indent-tabs-mode: nil
>>> + * End:
>>> + */
>>
>> Should you instead have a non-arch specific file with those dummy
>> helpers that get used when !CONFIG_PCI_MSI_INTERCEPT?
>
> +1
>
>>> --- a/xen/include/asm-x86/msi.h
>>> +++ b/xen/include/asm-x86/msi.h
>>> @@ -252,5 +252,9 @@ void unmask_msi_irq(struct irq_desc *);
>>> void guest_mask_msi_irq(struct irq_desc *, bool mask);
>>> void ack_nonmaskable_msi_irq(struct irq_desc *);
>>> void set_msi_affinity(struct irq_desc *, const cpumask_t *);
>>> +int alloc_pci_msi(struct pci_dev *pdev);
>>> +void free_pci_msi(struct pci_dev *pdev);
>>> +void dump_pci_msi(struct pci_dev *pdev);
>>> +int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev);
>
> These should then live in the other "half" of that header.
Ok Yes noted
>
>>> --- a/xen/include/xen/pci.h
>>> +++ b/xen/include/xen/pci.h
>>> @@ -79,10 +79,6 @@ struct pci_dev {
>>> struct list_head alldevs_list;
>>> struct list_head domain_list;
>>>
>>> - struct list_head msi_list;
>>> -
>>> - struct arch_msix *msix;
>>> -
>>> struct domain *domain;
>>>
>>> const union {
>>> @@ -94,7 +90,14 @@ struct pci_dev {
>>> pci_sbdf_t sbdf;
>>> };
>>>
>>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>>> + struct list_head msi_list;
>>> +
>>> + struct arch_msix *msix;
>>> +
>>> uint8_t msi_maxvec;
>>> +#endif
>>> +
>>> uint8_t phantom_stride;
>
> Like Roger also said for struct vpci, it's not clear this is worth
> it. And while you may have paid attention (and there simply is no
> better arrangement) I'd also like to point out that such field
> movement should be done while keeping padding hole sizes in mind.
Ok.
>
>>> @@ -174,7 +177,7 @@ int __must_check vpci_msix_arch_disable_entry(struct vpci_msix_entry *entry,
>>> const struct pci_dev *pdev);
>>> void vpci_msix_arch_init_entry(struct vpci_msix_entry *entry);
>>> int vpci_msix_arch_print(const struct vpci_msix *msix);
>>> -
>>> +int remove_msix_regions(const struct pci_dev *pdev, struct rangeset *mem);
>>> /*
>>> * Helper functions to fetch MSIX related data. They are used by both the
>>> * emulated MSIX code and the BAR handlers.
>
> Please don't remove blank lines like this one, unless you actually
> see a reason.
>
Ok.
Regards,
Rahul
> Jan
Hi Roger,
> On 12 Apr 2021, at 11:49 am, Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
>> MSI related code in the "passthrough/pci.c” file is not useful for ARM
>> when MSI interrupts are injected via GICv3 ITS.
>>
>> Therefore introducing the new flag CONFIG_PCI_MSI_INTERCEPT to gate the
>> MSI code for ARM in common code. Also move the arch-specific code to an
>> arch-specific directory and implement the stub version for the unused
>> code for ARM to avoid compilation error when HAS_PCI is enabled for ARM.
>>
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> Changes since v1:
>> - Rename CONFIG_HAS_PCI_MSI to CONFIG_PCI_MSI_INTERCEPT
>> - Implement stub version of the unused function for ARM.
>> - Move unneeded function to x86 file.
>> ---
>> xen/arch/x86/msi.c | 64 +++++++++++++++++++++++++++++++++++
>> xen/drivers/passthrough/pci.c | 51 +++++++---------------------
>> xen/drivers/pci/Kconfig | 4 +++
>> xen/drivers/vpci/Makefile | 3 +-
>> xen/drivers/vpci/header.c | 19 +++--------
>> xen/drivers/vpci/msix.c | 25 ++++++++++++++
>> xen/drivers/vpci/vpci.c | 3 +-
>> xen/include/asm-arm/msi.h | 44 ++++++++++++++++++++++++
>> xen/include/asm-x86/msi.h | 4 +++
>> xen/include/xen/pci.h | 11 +++---
>> xen/include/xen/vpci.h | 24 ++++++++++++-
>> xen/xsm/flask/hooks.c | 8 ++---
>> 12 files changed, 195 insertions(+), 65 deletions(-)
>> create mode 100644 xen/include/asm-arm/msi.h
>>
>> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
>> index 5febc0ea4b..a6356f4a63 100644
>> --- a/xen/arch/x86/msi.c
>> +++ b/xen/arch/x86/msi.c
>> @@ -1411,6 +1411,70 @@ void __init early_msi_init(void)
>> return;
>> }
>>
>> +int alloc_pci_msi(struct pci_dev *pdev)
>
> I would rather name it pci_msi_init...
>
>> +{
>> + unsigned int pos;
>> +
>> + INIT_LIST_HEAD(&pdev->msi_list);
>> +
>> + pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
>> + PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSI);
>> + if ( pos )
>> + {
>> + uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
>> +
>> + pdev->msi_maxvec = multi_msi_capable(ctrl);
>> + }
>> +
>> + pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
>> + PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSIX);
>> + if ( pos )
>> + {
>> + struct arch_msix *msix = xzalloc(struct arch_msix);
>> + uint16_t ctrl;
>> +
>> + if ( !msix )
>> + return -ENOMEM;
>> +
>> + spin_lock_init(&msix->table_lock);
>> +
>> + ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
>> + msix->nr_entries = msix_table_size(ctrl);
>> +
>> + pdev->msix = msix;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +void free_pci_msi(struct pci_dev *pdev)
>
> ...and pci_msi_free.
Ok. I will change the name in next version.
>
>> +{
>> + xfree(pdev->msix);
>> +}
>> +
>> +int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev)
>
> This kind of a confusing name - what about pci_msix_assign?
Ok.
>
>> +{
>> + int rc;
>> +
>> + if ( pdev->msix )
>> + {
>> + rc = pci_reset_msix_state(pdev);
>> + if ( rc )
>> + return rc;
>> + msixtbl_init(d);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +void dump_pci_msi(struct pci_dev *pdev)
>> +{
>> + struct msi_desc *msi;
>> +
>> + list_for_each_entry ( msi, &pdev->msi_list, list )
>> + printk("%d ", msi->irq);
>> +}
>
> I wonder, those those function really want to be in a x86 specific
> file? There's nothing x86 specific about them AFAICT.
>
> Would it make sense to create a separate msi-intercept.c file with
> those that gets included when CONFIG_PCI_MSI_INTERCEPT?
Yes that will be fine to create the separate file msi-intercept.c.
>
>
>> static void dump_msi(unsigned char key)
>> {
>> unsigned int irq;
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 705137f8be..1b473502a1 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -314,6 +314,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>> {
>> struct pci_dev *pdev;
>> unsigned int pos;
>> + int rc;
>>
>> list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>> if ( pdev->bus == bus && pdev->devfn == devfn )
>> @@ -327,35 +328,12 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>> *((u8*) &pdev->bus) = bus;
>> *((u8*) &pdev->devfn) = devfn;
>> pdev->domain = NULL;
>> - INIT_LIST_HEAD(&pdev->msi_list);
>> -
>> - pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
>> - PCI_CAP_ID_MSI);
>> - if ( pos )
>> - {
>> - uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
>> -
>> - pdev->msi_maxvec = multi_msi_capable(ctrl);
>> - }
>>
>> - pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
>> - PCI_CAP_ID_MSIX);
>> - if ( pos )
>> + rc = alloc_pci_msi(pdev);
>> + if ( rc )
>> {
>> - struct arch_msix *msix = xzalloc(struct arch_msix);
>> - uint16_t ctrl;
>> -
>> - if ( !msix )
>> - {
>> - xfree(pdev);
>> - return NULL;
>> - }
>> - spin_lock_init(&msix->table_lock);
>> -
>> - ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
>> - msix->nr_entries = msix_table_size(ctrl);
>> -
>> - pdev->msix = msix;
>> + xfree(pdev);
>> + return NULL;
>> }
>>
>> list_add(&pdev->alldevs_list, &pseg->alldevs_list);
>> @@ -449,7 +427,7 @@ static void free_pdev(struct pci_seg *pseg, struct pci_dev *pdev)
>> }
>>
>> list_del(&pdev->alldevs_list);
>> - xfree(pdev->msix);
>> + free_pci_msi(pdev);
>> xfree(pdev);
>> }
>>
>> @@ -1271,7 +1249,6 @@ bool_t pcie_aer_get_firmware_first(const struct pci_dev *pdev)
>> static int _dump_pci_devices(struct pci_seg *pseg, void *arg)
>> {
>> struct pci_dev *pdev;
>> - struct msi_desc *msi;
>>
>> printk("==== segment %04x ====\n", pseg->nr);
>>
>> @@ -1280,8 +1257,7 @@ static int _dump_pci_devices(struct pci_seg *pseg, void *arg)
>> printk("%pp - %pd - node %-3d - MSIs < ",
>> &pdev->sbdf, pdev->domain,
>> (pdev->node != NUMA_NO_NODE) ? pdev->node : -1);
>> - list_for_each_entry ( msi, &pdev->msi_list, list )
>> - printk("%d ", msi->irq);
>> + dump_pci_msi(pdev);
>> printk(">\n");
>> }
>>
>> @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
>> }
>> __initcall(setup_dump_pcidevs);
>>
>> +
>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>> int iommu_update_ire_from_msi(
>> struct msi_desc *msi_desc, struct msi_msg *msg)
>> {
>> return iommu_intremap
>> ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
>> }
>> +#endif
>
> This is not exactly related to MSI intercepts, the IOMMU interrupt
> remapping table will also be used for interrupt entries for devices
> being used by Xen directly, where no intercept is required.
>
> And then you also want to gate the hook from iommu_ops itself with
> CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.
Ok. Let me check this and come back to you.
>
>>
>> static int iommu_add_device(struct pci_dev *pdev)
>> {
>> @@ -1429,13 +1408,9 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>> ASSERT(pdev && (pdev->domain == hardware_domain ||
>> pdev->domain == dom_io));
>>
>> - if ( pdev->msix )
>> - {
>> - rc = pci_reset_msix_state(pdev);
>> - if ( rc )
>> - goto done;
>> - msixtbl_init(d);
>> - }
>> + rc = pci_assign_msix_init(d, pdev);
>> + if ( rc )
>> + goto done;
>>
>> pdev->fault.count = 0;
>>
>> diff --git a/xen/drivers/pci/Kconfig b/xen/drivers/pci/Kconfig
>> index 7da03fa13b..7ab92bde6e 100644
>> --- a/xen/drivers/pci/Kconfig
>> +++ b/xen/drivers/pci/Kconfig
>> @@ -1,3 +1,7 @@
>>
>> config HAS_PCI
>> bool
>> +
>> +config PCI_MSI_INTERCEPT
>> + def_bool y
>> + depends on X86 && HAS_PCI
>> diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile
>> index 55d1bdfda0..f91fa71a40 100644
>> --- a/xen/drivers/vpci/Makefile
>> +++ b/xen/drivers/vpci/Makefile
>> @@ -1 +1,2 @@
>> -obj-y += vpci.o header.o msi.o msix.o
>> +obj-y += vpci.o header.o
>> +obj-$(CONFIG_PCI_MSI_INTERCEPT) += msi.o msix.o
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index ba9a036202..a7695a0e2a 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -206,7 +206,6 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>> struct vpci_header *header = &pdev->vpci->header;
>> struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>> struct pci_dev *tmp, *dev = NULL;
>> - const struct vpci_msix *msix = pdev->vpci->msix;
>> unsigned int i;
>> int rc;
>>
>> @@ -244,21 +243,11 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>> }
>>
>> /* Remove any MSIX regions if present. */
>> - for ( i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ )
>> + rc = remove_msix_regions(pdev,mem);
>
> Coding style: missing space between parameters.
Ack.
>
>> + if ( rc )
>> {
>> - 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);
>> -
>> - rc = rangeset_remove_range(mem, start, end);
>> - if ( rc )
>> - {
>> - printk(XENLOG_G_WARNING
>> - "Failed to remove MSIX table [%lx, %lx]: %d\n",
>> - start, end, rc);
>> - rangeset_destroy(mem);
>> - return rc;
>> - }
>> + rangeset_destroy(mem);
>> + return rc;
>> }
>>
>> /*
>> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
>> index 846f1b8d70..ca0a9b5665 100644
>> --- a/xen/drivers/vpci/msix.c
>> +++ b/xen/drivers/vpci/msix.c
>> @@ -428,6 +428,31 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
>> return 0;
>> }
>>
>> +int remove_msix_regions(const struct pci_dev *pdev, struct rangeset *mem)
>
> This needs to start with a vpci_ prefix, since it's a global function.
> What about vpci_msix_remove_regions.
Ok.
>
>> +{
>> + const struct vpci_msix *msix = pdev->vpci->msix;
>> + unsigned int i;
>> + int rc;
>> +
>> + for ( i = 0; msix && i < ARRAY_SIZE(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);
>> +
>> + rc = rangeset_remove_range(mem, start, end);
>> + if ( rc )
>> + {
>> + printk(XENLOG_G_WARNING
>> + "Failed to remove MSIX table [%lx, %lx]: %d\n",
>> + start, end, rc);
>> + return rc;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int init_msix(struct pci_dev *pdev)
>> {
>> struct domain *d = pdev->domain;
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index cbd1bac7fc..ebe40b0538 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -48,8 +48,7 @@ void vpci_remove_device(struct pci_dev *pdev)
>> xfree(r);
>> }
>> spin_unlock(&pdev->vpci->lock);
>> - xfree(pdev->vpci->msix);
>> - xfree(pdev->vpci->msi);
>> + free_vpci_msi(pdev);
>> xfree(pdev->vpci);
>> pdev->vpci = NULL;
>> }
>> diff --git a/xen/include/asm-arm/msi.h b/xen/include/asm-arm/msi.h
>> new file mode 100644
>> index 0000000000..c54193310e
>> --- /dev/null
>> +++ b/xen/include/asm-arm/msi.h
>> @@ -0,0 +1,44 @@
>> +/*
>> + * Copyright (C) 2021 Arm Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef __ASM_MSI_H_
>> +#define __ASM_MSI_H_
>> +
>> +static inline int alloc_pci_msi(struct pci_dev *pdev)
>> +{
>> + return 0;
>> +}
>> +
>> +static inline int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev)
>> +{
>> + return 0;
>> +}
>> +
>> +static inline void dump_pci_msi(struct pci_dev *pdev) {}
>> +static inline void free_pci_msi(struct pci_dev *pdev) {}
>> +static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
>> +
>> +#endif /* __ASM_MSI_H */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>
> Should you instead have a non-arch specific file with those dummy
> helpers that get used when !CONFIG_PCI_MSI_INTERCEPT?
Ok.
>
>> diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h
>> index e228b0f3f3..7ef0161b67 100644
>> --- a/xen/include/asm-x86/msi.h
>> +++ b/xen/include/asm-x86/msi.h
>> @@ -252,5 +252,9 @@ void unmask_msi_irq(struct irq_desc *);
>> void guest_mask_msi_irq(struct irq_desc *, bool mask);
>> void ack_nonmaskable_msi_irq(struct irq_desc *);
>> void set_msi_affinity(struct irq_desc *, const cpumask_t *);
>> +int alloc_pci_msi(struct pci_dev *pdev);
>> +void free_pci_msi(struct pci_dev *pdev);
>> +void dump_pci_msi(struct pci_dev *pdev);
>> +int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev);
>>
>> #endif /* __ASM_MSI_H */
>> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
>> index 8e3d4d9454..f5b57270be 100644
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -79,10 +79,6 @@ struct pci_dev {
>> struct list_head alldevs_list;
>> struct list_head domain_list;
>>
>> - struct list_head msi_list;
>> -
>> - struct arch_msix *msix;
>> -
>> struct domain *domain;
>>
>> const union {
>> @@ -94,7 +90,14 @@ struct pci_dev {
>> pci_sbdf_t sbdf;
>> };
>>
>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>> + struct list_head msi_list;
>> +
>> + struct arch_msix *msix;
>> +
>> uint8_t msi_maxvec;
>> +#endif
>> +
>> uint8_t phantom_stride;
>>
>> nodeid_t node; /* NUMA node */
>> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
>> index 9f5b5d52e1..a6b12717b1 100644
>> --- a/xen/include/xen/vpci.h
>> +++ b/xen/include/xen/vpci.h
>> @@ -91,6 +91,7 @@ struct vpci {
>> /* FIXME: currently there's no support for SR-IOV. */
>> } header;
>>
>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>> /* MSI data. */
>> struct vpci_msi {
>> /* Address. */
>> @@ -136,6 +137,7 @@ struct vpci {
>> struct vpci_arch_msix_entry arch;
>> } entries[];
>> } *msix;
>> +#endif /* CONFIG_PCI_MSI_INTERCEPT */
>
> Note that here you just remove two pointers from the struct, not that
> I'm opposed to it, but it's not that much space that's saved anyway.
> Ie: it might also be fine to just leave them as NULL unconditionally
> on Arm.
Ok.
>
>> #endif
>> };
>>
>> @@ -148,6 +150,7 @@ struct vpci_vcpu {
>> };
>>
>> #ifdef __XEN__
>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>> void vpci_dump_msi(void);
>>
>> /* Make sure there's a hole in the p2m for the MSIX mmio areas. */
>> @@ -174,7 +177,7 @@ int __must_check vpci_msix_arch_disable_entry(struct vpci_msix_entry *entry,
>> const struct pci_dev *pdev);
>> void vpci_msix_arch_init_entry(struct vpci_msix_entry *entry);
>> int vpci_msix_arch_print(const struct vpci_msix *msix);
>> -
>> +int remove_msix_regions(const struct pci_dev *pdev, struct rangeset *mem);
>> /*
>> * Helper functions to fetch MSIX related data. They are used by both the
>> * emulated MSIX code and the BAR handlers.
>> @@ -208,6 +211,25 @@ static inline unsigned int vmsix_entry_nr(const struct vpci_msix *msix,
>> {
>> return entry - msix->entries;
>> }
>> +
>> +static inline void free_vpci_msi(const struct pci_dev *pdev)
>
> vpci_msi_free please.
Ok.
I will modify the code as per your comments and will send next version of the patch.
Regards,
Rahul
>
> Thanks, Roger.
Hi Roger,
On 12/04/2021 11:49, Roger Pau Monné wrote:
> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
>> MSI related code in the "passthrough/pci.c” file is not useful for ARM
>> when MSI interrupts are injected via GICv3 ITS.
>>
>> Therefore introducing the new flag CONFIG_PCI_MSI_INTERCEPT to gate the
>> MSI code for ARM in common code. Also move the arch-specific code to an
>> arch-specific directory and implement the stub version for the unused
>> code for ARM to avoid compilation error when HAS_PCI is enabled for ARM.
>>
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> Changes since v1:
>> - Rename CONFIG_HAS_PCI_MSI to CONFIG_PCI_MSI_INTERCEPT
>> - Implement stub version of the unused function for ARM.
>> - Move unneeded function to x86 file.
>> ---
>> xen/arch/x86/msi.c | 64 +++++++++++++++++++++++++++++++++++
>> xen/drivers/passthrough/pci.c | 51 +++++++---------------------
>> xen/drivers/pci/Kconfig | 4 +++
>> xen/drivers/vpci/Makefile | 3 +-
>> xen/drivers/vpci/header.c | 19 +++--------
>> xen/drivers/vpci/msix.c | 25 ++++++++++++++
>> xen/drivers/vpci/vpci.c | 3 +-
>> xen/include/asm-arm/msi.h | 44 ++++++++++++++++++++++++
>> xen/include/asm-x86/msi.h | 4 +++
>> xen/include/xen/pci.h | 11 +++---
>> xen/include/xen/vpci.h | 24 ++++++++++++-
>> xen/xsm/flask/hooks.c | 8 ++---
>> 12 files changed, 195 insertions(+), 65 deletions(-)
>> create mode 100644 xen/include/asm-arm/msi.h
>>
>> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
>> index 5febc0ea4b..a6356f4a63 100644
>> --- a/xen/arch/x86/msi.c
>> +++ b/xen/arch/x86/msi.c
>> @@ -1411,6 +1411,70 @@ void __init early_msi_init(void)
>> return;
>> }
>>
>> +int alloc_pci_msi(struct pci_dev *pdev)
>
> I would rather name it pci_msi_init...
>
>> +{
>> + unsigned int pos;
>> +
>> + INIT_LIST_HEAD(&pdev->msi_list);
>> +
>> + pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
>> + PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSI);
>> + if ( pos )
>> + {
>> + uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
>> +
>> + pdev->msi_maxvec = multi_msi_capable(ctrl);
>> + }
>> +
>> + pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
>> + PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSIX);
>> + if ( pos )
>> + {
>> + struct arch_msix *msix = xzalloc(struct arch_msix);
>> + uint16_t ctrl;
>> +
>> + if ( !msix )
>> + return -ENOMEM;
>> +
>> + spin_lock_init(&msix->table_lock);
>> +
>> + ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
>> + msix->nr_entries = msix_table_size(ctrl);
>> +
>> + pdev->msix = msix;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +void free_pci_msi(struct pci_dev *pdev)
>
> ...and pci_msi_free.
>
>> +{
>> + xfree(pdev->msix);
>> +}
>> +
>> +int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev)
>
> This kind of a confusing name - what about pci_msix_assign?
>
>> +{
>> + int rc;
>> +
>> + if ( pdev->msix )
>> + {
>> + rc = pci_reset_msix_state(pdev);
>> + if ( rc )
>> + return rc;
>> + msixtbl_init(d);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +void dump_pci_msi(struct pci_dev *pdev)
>> +{
>> + struct msi_desc *msi;
>> +
>> + list_for_each_entry ( msi, &pdev->msi_list, list )
>> + printk("%d ", msi->irq);
>> +}
>
> I wonder, those those function really want to be in a x86 specific
> file? There's nothing x86 specific about them AFAICT.
>
> Would it make sense to create a separate msi-intercept.c file with
> those that gets included when CONFIG_PCI_MSI_INTERCEPT?
>
>> static void dump_msi(unsigned char key)
>> {
>> unsigned int irq;
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 705137f8be..1b473502a1 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -314,6 +314,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>> {
>> struct pci_dev *pdev;
>> unsigned int pos;
>> + int rc;
>>
>> list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>> if ( pdev->bus == bus && pdev->devfn == devfn )
>> @@ -327,35 +328,12 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>> *((u8*) &pdev->bus) = bus;
>> *((u8*) &pdev->devfn) = devfn;
>> pdev->domain = NULL;
>> - INIT_LIST_HEAD(&pdev->msi_list);
>> -
>> - pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
>> - PCI_CAP_ID_MSI);
>> - if ( pos )
>> - {
>> - uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
>> -
>> - pdev->msi_maxvec = multi_msi_capable(ctrl);
>> - }
>>
>> - pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
>> - PCI_CAP_ID_MSIX);
>> - if ( pos )
>> + rc = alloc_pci_msi(pdev);
>> + if ( rc )
>> {
>> - struct arch_msix *msix = xzalloc(struct arch_msix);
>> - uint16_t ctrl;
>> -
>> - if ( !msix )
>> - {
>> - xfree(pdev);
>> - return NULL;
>> - }
>> - spin_lock_init(&msix->table_lock);
>> -
>> - ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
>> - msix->nr_entries = msix_table_size(ctrl);
>> -
>> - pdev->msix = msix;
>> + xfree(pdev);
>> + return NULL;
>> }
>>
>> list_add(&pdev->alldevs_list, &pseg->alldevs_list);
>> @@ -449,7 +427,7 @@ static void free_pdev(struct pci_seg *pseg, struct pci_dev *pdev)
>> }
>>
>> list_del(&pdev->alldevs_list);
>> - xfree(pdev->msix);
>> + free_pci_msi(pdev);
>> xfree(pdev);
>> }
>>
>> @@ -1271,7 +1249,6 @@ bool_t pcie_aer_get_firmware_first(const struct pci_dev *pdev)
>> static int _dump_pci_devices(struct pci_seg *pseg, void *arg)
>> {
>> struct pci_dev *pdev;
>> - struct msi_desc *msi;
>>
>> printk("==== segment %04x ====\n", pseg->nr);
>>
>> @@ -1280,8 +1257,7 @@ static int _dump_pci_devices(struct pci_seg *pseg, void *arg)
>> printk("%pp - %pd - node %-3d - MSIs < ",
>> &pdev->sbdf, pdev->domain,
>> (pdev->node != NUMA_NO_NODE) ? pdev->node : -1);
>> - list_for_each_entry ( msi, &pdev->msi_list, list )
>> - printk("%d ", msi->irq);
>> + dump_pci_msi(pdev);
>> printk(">\n");
>> }
>>
>> @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
>> }
>> __initcall(setup_dump_pcidevs);
>>
>> +
>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>> int iommu_update_ire_from_msi(
>> struct msi_desc *msi_desc, struct msi_msg *msg)
>> {
>> return iommu_intremap
>> ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
>> }
>> +#endif
>
> This is not exactly related to MSI intercepts, the IOMMU interrupt
> remapping table will also be used for interrupt entries for devices
> being used by Xen directly, where no intercept is required.
On Arm, this is not tie to the IOMMU. Instead, this handled is a
separate MSI controller (such as the ITS).
>
> And then you also want to gate the hook from iommu_ops itself with
> CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.
All the callers are in the x86 code. So how about moving the function in
an x86 specific file?
>> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
>> index 9f5b5d52e1..a6b12717b1 100644
>> --- a/xen/include/xen/vpci.h
>> +++ b/xen/include/xen/vpci.h
>> @@ -91,6 +91,7 @@ struct vpci {
>> /* FIXME: currently there's no support for SR-IOV. */
>> } header;
>>
>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>> /* MSI data. */
>> struct vpci_msi {
>> /* Address. */
>> @@ -136,6 +137,7 @@ struct vpci {
>> struct vpci_arch_msix_entry arch;
>> } entries[];
>> } *msix;
>> +#endif /* CONFIG_PCI_MSI_INTERCEPT */
>
> Note that here you just remove two pointers from the struct, not that
> I'm opposed to it, but it's not that much space that's saved anyway.
> Ie: it might also be fine to just leave them as NULL unconditionally
> on Arm.
Can the two pointers be NULL on x86? If not, then I would prefer if they
disappear on Arm so there is less chance to make any mistake (such as
unconditionally accessing the pointer in common code).
Cheers,
--
Julien Grall
On 13.04.2021 19:12, Julien Grall wrote:
> On 12/04/2021 11:49, Roger Pau Monné wrote:
>> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
>>> --- a/xen/include/xen/vpci.h
>>> +++ b/xen/include/xen/vpci.h
>>> @@ -91,6 +91,7 @@ struct vpci {
>>> /* FIXME: currently there's no support for SR-IOV. */
>>> } header;
>>>
>>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>>> /* MSI data. */
>>> struct vpci_msi {
>>> /* Address. */
>>> @@ -136,6 +137,7 @@ struct vpci {
>>> struct vpci_arch_msix_entry arch;
>>> } entries[];
>>> } *msix;
>>> +#endif /* CONFIG_PCI_MSI_INTERCEPT */
>>
>> Note that here you just remove two pointers from the struct, not that
>> I'm opposed to it, but it's not that much space that's saved anyway.
>> Ie: it might also be fine to just leave them as NULL unconditionally
>> on Arm.
>
> Can the two pointers be NULL on x86? If not, then I would prefer if they
> disappear on Arm so there is less chance to make any mistake (such as
> unconditionally accessing the pointer in common code).
Alternative proposal: How about making it effectively impossible to
de-reference the pointer on Arm by leaving the field there, but having
the struct definition available on non-Arm only?
Jan
On Wed, Apr 14, 2021 at 09:08:53AM +0200, Jan Beulich wrote:
> On 13.04.2021 19:12, Julien Grall wrote:
> > On 12/04/2021 11:49, Roger Pau Monné wrote:
> >> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
> >>> --- a/xen/include/xen/vpci.h
> >>> +++ b/xen/include/xen/vpci.h
> >>> @@ -91,6 +91,7 @@ struct vpci {
> >>> /* FIXME: currently there's no support for SR-IOV. */
> >>> } header;
> >>>
> >>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
> >>> /* MSI data. */
> >>> struct vpci_msi {
> >>> /* Address. */
> >>> @@ -136,6 +137,7 @@ struct vpci {
> >>> struct vpci_arch_msix_entry arch;
> >>> } entries[];
> >>> } *msix;
> >>> +#endif /* CONFIG_PCI_MSI_INTERCEPT */
> >>
> >> Note that here you just remove two pointers from the struct, not that
> >> I'm opposed to it, but it's not that much space that's saved anyway.
> >> Ie: it might also be fine to just leave them as NULL unconditionally
> >> on Arm.
> >
> > Can the two pointers be NULL on x86? If not, then I would prefer if they
> > disappear on Arm so there is less chance to make any mistake (such as
> > unconditionally accessing the pointer in common code).
>
> Alternative proposal: How about making it effectively impossible to
> de-reference the pointer on Arm by leaving the field there, but having
> the struct definition available on non-Arm only?
We could place the struct definitions somewhere else protected by
CONFIG_PCI_MSI_INTERCEPT, but I'm not sure that would be much
different than the current proposal, and overall I think I prefer this
approach then, as we keep the definition and the usage closer
together.
Maybe we could slightly modify the current layout so that
the field is always present, but the struct definition is made
conditional to CONFIG_PCI_MSI_INTERCEPT?
Thanks, Roger.
On 14.04.2021 10:28, Roger Pau Monné wrote:
> On Wed, Apr 14, 2021 at 09:08:53AM +0200, Jan Beulich wrote:
>> On 13.04.2021 19:12, Julien Grall wrote:
>>> On 12/04/2021 11:49, Roger Pau Monné wrote:
>>>> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
>>>>> --- a/xen/include/xen/vpci.h
>>>>> +++ b/xen/include/xen/vpci.h
>>>>> @@ -91,6 +91,7 @@ struct vpci {
>>>>> /* FIXME: currently there's no support for SR-IOV. */
>>>>> } header;
>>>>>
>>>>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>>>>> /* MSI data. */
>>>>> struct vpci_msi {
>>>>> /* Address. */
>>>>> @@ -136,6 +137,7 @@ struct vpci {
>>>>> struct vpci_arch_msix_entry arch;
>>>>> } entries[];
>>>>> } *msix;
>>>>> +#endif /* CONFIG_PCI_MSI_INTERCEPT */
>>>>
>>>> Note that here you just remove two pointers from the struct, not that
>>>> I'm opposed to it, but it's not that much space that's saved anyway.
>>>> Ie: it might also be fine to just leave them as NULL unconditionally
>>>> on Arm.
>>>
>>> Can the two pointers be NULL on x86? If not, then I would prefer if they
>>> disappear on Arm so there is less chance to make any mistake (such as
>>> unconditionally accessing the pointer in common code).
>>
>> Alternative proposal: How about making it effectively impossible to
>> de-reference the pointer on Arm by leaving the field there, but having
>> the struct definition available on non-Arm only?
>
> We could place the struct definitions somewhere else protected by
> CONFIG_PCI_MSI_INTERCEPT, but I'm not sure that would be much
> different than the current proposal, and overall I think I prefer this
> approach then, as we keep the definition and the usage closer
> together.
>
> Maybe we could slightly modify the current layout so that
> the field is always present, but the struct definition is made
> conditional to CONFIG_PCI_MSI_INTERCEPT?
You mean like this
/* MSI data. */
struct vpci_msi {
#ifdef CONFIG_PCI_MSI_INTERCEPT
/* Address. */
...
struct vpci_arch_msix_entry arch;
} entries[];
#endif /* CONFIG_PCI_MSI_INTERCEPT */
} *msix;
? I could live with it, but this would have the compiler not
refuse e.g. sizeof(struct vpci_msi) or instantiation of the
struct as a (local) variable, unlike my proposal.
Jan
Hi Jan,
On 14/04/2021 08:08, Jan Beulich wrote:
> On 13.04.2021 19:12, Julien Grall wrote:
>> On 12/04/2021 11:49, Roger Pau Monné wrote:
>>> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
>>>> --- a/xen/include/xen/vpci.h
>>>> +++ b/xen/include/xen/vpci.h
>>>> @@ -91,6 +91,7 @@ struct vpci {
>>>> /* FIXME: currently there's no support for SR-IOV. */
>>>> } header;
>>>>
>>>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>>>> /* MSI data. */
>>>> struct vpci_msi {
>>>> /* Address. */
>>>> @@ -136,6 +137,7 @@ struct vpci {
>>>> struct vpci_arch_msix_entry arch;
>>>> } entries[];
>>>> } *msix;
>>>> +#endif /* CONFIG_PCI_MSI_INTERCEPT */
>>>
>>> Note that here you just remove two pointers from the struct, not that
>>> I'm opposed to it, but it's not that much space that's saved anyway.
>>> Ie: it might also be fine to just leave them as NULL unconditionally
>>> on Arm.
>>
>> Can the two pointers be NULL on x86? If not, then I would prefer if they
>> disappear on Arm so there is less chance to make any mistake (such as
>> unconditionally accessing the pointer in common code).
>
> Alternative proposal: How about making it effectively impossible to
> de-reference the pointer on Arm by leaving the field there, but having
> the struct definition available on non-Arm only?
Meh, it making more difficult for the reader because it has to go
through one more hop to find out why the structure is not defined.
IOW, I still prefer Rahul's version.
Cheers,
--
Julien Grall
On Tue, Apr 13, 2021 at 06:12:10PM +0100, Julien Grall wrote:
> Hi Roger,
>
> On 12/04/2021 11:49, Roger Pau Monné wrote:
> > On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
> > > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> > > index 705137f8be..1b473502a1 100644
> > > --- a/xen/drivers/passthrough/pci.c
> > > +++ b/xen/drivers/passthrough/pci.c
> > > @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
> > > }
> > > __initcall(setup_dump_pcidevs);
> > > +
> > > +#ifdef CONFIG_PCI_MSI_INTERCEPT
> > > int iommu_update_ire_from_msi(
> > > struct msi_desc *msi_desc, struct msi_msg *msg)
> > > {
> > > return iommu_intremap
> > > ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
> > > }
> > > +#endif
> >
> > This is not exactly related to MSI intercepts, the IOMMU interrupt
> > remapping table will also be used for interrupt entries for devices
> > being used by Xen directly, where no intercept is required.
>
> On Arm, this is not tie to the IOMMU. Instead, this handled is a separate
> MSI controller (such as the ITS).
>
> >
> > And then you also want to gate the hook from iommu_ops itself with
> > CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.
>
>
> All the callers are in the x86 code. So how about moving the function in an
> x86 specific file?
Yes, that seems fine. Just place it in asm-x86/iommu.h. I wonder why
update_ire_from_msi wasn't moved when the rest of the x86 specific
functions where moved there. Was there an aim to use that in other
arches?
The hook in iommu_ops also need to be moved inside the x86 region.
Please do this iommu change in a separate patch.
> > > diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> > > index 9f5b5d52e1..a6b12717b1 100644
> > > --- a/xen/include/xen/vpci.h
> > > +++ b/xen/include/xen/vpci.h
> > > @@ -91,6 +91,7 @@ struct vpci {
> > > /* FIXME: currently there's no support for SR-IOV. */
> > > } header;
> > > +#ifdef CONFIG_PCI_MSI_INTERCEPT
> > > /* MSI data. */
> > > struct vpci_msi {
> > > /* Address. */
> > > @@ -136,6 +137,7 @@ struct vpci {
> > > struct vpci_arch_msix_entry arch;
> > > } entries[];
> > > } *msix;
> > > +#endif /* CONFIG_PCI_MSI_INTERCEPT */
> >
> > Note that here you just remove two pointers from the struct, not that
> > I'm opposed to it, but it's not that much space that's saved anyway.
> > Ie: it might also be fine to just leave them as NULL unconditionally
> > on Arm.
>
> Can the two pointers be NULL on x86?
Yes, they can be NULL on x86.
> If not, then I would prefer if they
> disappear on Arm so there is less chance to make any mistake (such as
> unconditionally accessing the pointer in common code).
Any access to them needs to be protected anyway, or else we would be
in trouble. I don't think Xen ever accesses them based on the PCI
capabilities of the device.
Thanks, Roger.
Hi Roger,
On 14/04/2021 09:05, Roger Pau Monné wrote:
> On Tue, Apr 13, 2021 at 06:12:10PM +0100, Julien Grall wrote:
>> Hi Roger,
>>
>> On 12/04/2021 11:49, Roger Pau Monné wrote:
>>> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
>>>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>>>> index 705137f8be..1b473502a1 100644
>>>> --- a/xen/drivers/passthrough/pci.c
>>>> +++ b/xen/drivers/passthrough/pci.c
>>>> @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
>>>> }
>>>> __initcall(setup_dump_pcidevs);
>>>> +
>>>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>>>> int iommu_update_ire_from_msi(
>>>> struct msi_desc *msi_desc, struct msi_msg *msg)
>>>> {
>>>> return iommu_intremap
>>>> ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
>>>> }
>>>> +#endif
>>>
>>> This is not exactly related to MSI intercepts, the IOMMU interrupt
>>> remapping table will also be used for interrupt entries for devices
>>> being used by Xen directly, where no intercept is required.
>>
>> On Arm, this is not tie to the IOMMU. Instead, this handled is a separate
>> MSI controller (such as the ITS).
>>
>>>
>>> And then you also want to gate the hook from iommu_ops itself with
>>> CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.
>>
>>
>> All the callers are in the x86 code. So how about moving the function in an
>> x86 specific file?
>
> Yes, that seems fine. Just place it in asm-x86/iommu.h. I wonder why
> update_ire_from_msi wasn't moved when the rest of the x86 specific
> functions where moved there.
I am guessing it is because the function was protected by CONFIG_HAS_PCI
rather than CONFIG_X86. So it was deferred until another arch enables
CONFIG_HAS_PCI (it is easier to know what code should be moved).
> Was there an aim to use that in other
> arches?
In the future we may need to use MSIs in Xen (IIRC some SMMUs only
support MSI interrupt). But I think the naming would be misleading as
the IOMMU will not be used for the remapping.
So most likely, we would want a more generic name (maybe
"arch_register_msi()"). This could call iommu_update_ire_from_msi() on
x86 and the ITS on Arm.
I don't know how RISCv and PowerPC remap the interrupt. But if they are
using the IOMMU then we could provide a generic helper protected by
CONFIG_HAS_IOMMU_INTERRUPT_REMAP (or a similar name).
>
> The hook in iommu_ops also need to be moved inside the x86 region.
> Please do this iommu change in a separate patch.
+1
Cheers,
--
Julien Grall
On Wed, Apr 14, 2021 at 09:49:37AM +0100, Julien Grall wrote:
> Hi Roger,
>
> On 14/04/2021 09:05, Roger Pau Monné wrote:
> > On Tue, Apr 13, 2021 at 06:12:10PM +0100, Julien Grall wrote:
> > > Hi Roger,
> > >
> > > On 12/04/2021 11:49, Roger Pau Monné wrote:
> > > > On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
> > > > > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> > > > > index 705137f8be..1b473502a1 100644
> > > > > --- a/xen/drivers/passthrough/pci.c
> > > > > +++ b/xen/drivers/passthrough/pci.c
> > > > > @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
> > > > > }
> > > > > __initcall(setup_dump_pcidevs);
> > > > > +
> > > > > +#ifdef CONFIG_PCI_MSI_INTERCEPT
> > > > > int iommu_update_ire_from_msi(
> > > > > struct msi_desc *msi_desc, struct msi_msg *msg)
> > > > > {
> > > > > return iommu_intremap
> > > > > ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
> > > > > }
> > > > > +#endif
> > > >
> > > > This is not exactly related to MSI intercepts, the IOMMU interrupt
> > > > remapping table will also be used for interrupt entries for devices
> > > > being used by Xen directly, where no intercept is required.
> > >
> > > On Arm, this is not tie to the IOMMU. Instead, this handled is a separate
> > > MSI controller (such as the ITS).
> > >
> > > >
> > > > And then you also want to gate the hook from iommu_ops itself with
> > > > CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.
> > >
> > >
> > > All the callers are in the x86 code. So how about moving the function in an
> > > x86 specific file?
> >
> > Yes, that seems fine. Just place it in asm-x86/iommu.h. I wonder why
> > update_ire_from_msi wasn't moved when the rest of the x86 specific
> > functions where moved there.
>
> I am guessing it is because the function was protected by CONFIG_HAS_PCI
> rather than CONFIG_X86. So it was deferred until another arch enables
> CONFIG_HAS_PCI (it is easier to know what code should be moved).
>
> > Was there an aim to use that in other
> > arches?
>
> In the future we may need to use MSIs in Xen (IIRC some SMMUs only support
> MSI interrupt).
Then I think some of the bits that are moved from
xen/drivers/passthrough/pci.c (alloc_pci_msi, free_pci_msi and
dump_pci_msi) need to be protected by a Kconfig option different than
CONFIG_PCI_MSI_INTERCEPT, as those are not related to MSI interception,
but to MSI handling in general (ie: Xen devices using MSI need
those). The same with the msi_list and msix fields of pci_dev, those
are not only used for MSI interception.
Or at least might be worth mentioning that the functions will be
needed for Xen to be able to use MSI interrupts itself, even if not
for interception purposes.
Thanks, Roger.
Hi Roger,
On 15/04/2021 14:26, Roger Pau Monné wrote:
> On Wed, Apr 14, 2021 at 09:49:37AM +0100, Julien Grall wrote:
>> Hi Roger,
>>
>> On 14/04/2021 09:05, Roger Pau Monné wrote:
>>> On Tue, Apr 13, 2021 at 06:12:10PM +0100, Julien Grall wrote:
>>>> Hi Roger,
>>>>
>>>> On 12/04/2021 11:49, Roger Pau Monné wrote:
>>>>> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
>>>>>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>>>>>> index 705137f8be..1b473502a1 100644
>>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>>> @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
>>>>>> }
>>>>>> __initcall(setup_dump_pcidevs);
>>>>>> +
>>>>>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>>>>>> int iommu_update_ire_from_msi(
>>>>>> struct msi_desc *msi_desc, struct msi_msg *msg)
>>>>>> {
>>>>>> return iommu_intremap
>>>>>> ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
>>>>>> }
>>>>>> +#endif
>>>>>
>>>>> This is not exactly related to MSI intercepts, the IOMMU interrupt
>>>>> remapping table will also be used for interrupt entries for devices
>>>>> being used by Xen directly, where no intercept is required.
>>>>
>>>> On Arm, this is not tie to the IOMMU. Instead, this handled is a separate
>>>> MSI controller (such as the ITS).
>>>>
>>>>>
>>>>> And then you also want to gate the hook from iommu_ops itself with
>>>>> CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.
>>>>
>>>>
>>>> All the callers are in the x86 code. So how about moving the function in an
>>>> x86 specific file?
>>>
>>> Yes, that seems fine. Just place it in asm-x86/iommu.h. I wonder why
>>> update_ire_from_msi wasn't moved when the rest of the x86 specific
>>> functions where moved there.
>>
>> I am guessing it is because the function was protected by CONFIG_HAS_PCI
>> rather than CONFIG_X86. So it was deferred until another arch enables
>> CONFIG_HAS_PCI (it is easier to know what code should be moved).
>>
>>> Was there an aim to use that in other
>>> arches?
>>
>> In the future we may need to use MSIs in Xen (IIRC some SMMUs only support
>> MSI interrupt).
>
> Then I think some of the bits that are moved from
> xen/drivers/passthrough/pci.c (alloc_pci_msi, free_pci_msi and
> dump_pci_msi) need to be protected by a Kconfig option different than
> CONFIG_PCI_MSI_INTERCEPT, as those are not related to MSI interception,
> but to MSI handling in general (ie: Xen devices using MSI need
> those).
Well... x86-specific devices yes. We don't know in what form MSI will be
added on for other arch-specific devices.
The same with the msi_list and msix fields of pci_dev, those
> are not only used for MSI interception.
>
> Or at least might be worth mentioning that the functions will be
> needed for Xen to be able to use MSI interrupts itself, even if not
> for interception purposes.
My preference would be to comment in the commit message (although, there
is no promise they will ever get re-used). We can then modify the #ifdef
once we introduce any use.
Cheers,
--
Julien Grall
Hi All,
> On 15 Apr 2021, at 2:31 pm, Julien Grall <julien@xen.org> wrote:
>
> Hi Roger,
>
> On 15/04/2021 14:26, Roger Pau Monné wrote:
>> On Wed, Apr 14, 2021 at 09:49:37AM +0100, Julien Grall wrote:
>>> Hi Roger,
>>>
>>> On 14/04/2021 09:05, Roger Pau Monné wrote:
>>>> On Tue, Apr 13, 2021 at 06:12:10PM +0100, Julien Grall wrote:
>>>>> Hi Roger,
>>>>>
>>>>> On 12/04/2021 11:49, Roger Pau Monné wrote:
>>>>>> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
>>>>>>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>>>>>>> index 705137f8be..1b473502a1 100644
>>>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>>>> @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
>>>>>>> }
>>>>>>> __initcall(setup_dump_pcidevs);
>>>>>>> +
>>>>>>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>>>>>>> int iommu_update_ire_from_msi(
>>>>>>> struct msi_desc *msi_desc, struct msi_msg *msg)
>>>>>>> {
>>>>>>> return iommu_intremap
>>>>>>> ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
>>>>>>> }
>>>>>>> +#endif
>>>>>>
>>>>>> This is not exactly related to MSI intercepts, the IOMMU interrupt
>>>>>> remapping table will also be used for interrupt entries for devices
>>>>>> being used by Xen directly, where no intercept is required.
>>>>>
>>>>> On Arm, this is not tie to the IOMMU. Instead, this handled is a separate
>>>>> MSI controller (such as the ITS).
>>>>>
>>>>>>
>>>>>> And then you also want to gate the hook from iommu_ops itself with
>>>>>> CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.
>>>>>
>>>>>
>>>>> All the callers are in the x86 code. So how about moving the function in an
>>>>> x86 specific file?
>>>>
>>>> Yes, that seems fine. Just place it in asm-x86/iommu.h. I wonder why
>>>> update_ire_from_msi wasn't moved when the rest of the x86 specific
>>>> functions where moved there.
>>>
>>> I am guessing it is because the function was protected by CONFIG_HAS_PCI
>>> rather than CONFIG_X86. So it was deferred until another arch enables
>>> CONFIG_HAS_PCI (it is easier to know what code should be moved).
>>>
>>>> Was there an aim to use that in other
>>>> arches?
>>>
>>> In the future we may need to use MSIs in Xen (IIRC some SMMUs only support
>>> MSI interrupt).
>> Then I think some of the bits that are moved from
>> xen/drivers/passthrough/pci.c (alloc_pci_msi, free_pci_msi and
>> dump_pci_msi) need to be protected by a Kconfig option different than
>> CONFIG_PCI_MSI_INTERCEPT, as those are not related to MSI interception,
>> but to MSI handling in general (ie: Xen devices using MSI need
>> those).
>
> Well... x86-specific devices yes. We don't know in what form MSI will be added on for other arch-specific devices.
>
> The same with the msi_list and msix fields of pci_dev, those
>> are not only used for MSI interception.
>> Or at least might be worth mentioning that the functions will be
>> needed for Xen to be able to use MSI interrupts itself, even if not
>> for interception purposes.
>
> My preference would be to comment in the commit message (although, there is no promise they will ever get re-used). We can then modify the #ifdef once we introduce any use.
>
Thanks you everyone for reviewing the code. I will summarise what I have understood from all the comments
and what I will be doing for the next version of the patch. Please let me know your view on this.
1. Create a separate non-arch specific file "msi-intercept.c" for the below newly introduced function and
compile that file if CONFIG_PCI_MSI_INTERCEPT is enabled.CONFIG_PCI_MSI_INTERCEPT will be
enabled for x86 by default. Also Mention in the commit message that these function will be needed for Xen to
support MSI interrupt within XEN.
pdev_msi_initi(..)
pdev_msi_deiniti(..)
pdev_dump_msi(..),
pdev_msix_assign(..)
2. Create separate patch for iommu_update_ire_from_msi() related code. There are two suggestion please help me which one to choose.
- Move the iommu_update_ire_from_msi() function to asm-x86/iommu.h and also move the hook from iommu_ops under CONFIG_X86.
- Implement a more generic function "arch_register_msi()"). This could call iommu_update_ire_from_msi() on x86 and the
ITS related code once implemented on Arm. Introduce the new Kconfig CONFIG_HAS_IOMMU_INTERRUPT_REMAP for this option.
3. As per the discussion I will gate the struct vpci_msi {..} and struct vpci_msix{..} using CONFIG_PCI_MSI_INTERCEPT so that
they will not will be accessible on ARM.
Regards,
Rahul
> Cheers,
>
> --
> Julien Grall
On Mon, Apr 19, 2021 at 07:16:52AM +0000, Rahul Singh wrote: > Thanks you everyone for reviewing the code. I will summarise what I have understood from all the comments > and what I will be doing for the next version of the patch. Please let me know your view on this. > > 1. Create a separate non-arch specific file "msi-intercept.c" for the below newly introduced function and > compile that file if CONFIG_PCI_MSI_INTERCEPT is enabled.CONFIG_PCI_MSI_INTERCEPT will be > enabled for x86 by default. Also Mention in the commit message that these function will be needed for Xen to > support MSI interrupt within XEN. > > pdev_msi_initi(..) > pdev_msi_deiniti(..) I would drop the last 'i' from both function names above, as we use init/deinit in the rest of the code base. > pdev_dump_msi(..), > pdev_msix_assign(..) > > 2. Create separate patch for iommu_update_ire_from_msi() related code. There are two suggestion please help me which one to choose. > > - Move the iommu_update_ire_from_msi() function to asm-x86/iommu.h and also move the hook from iommu_ops under CONFIG_X86. I would go for this one. > > - Implement a more generic function "arch_register_msi()"). This could call iommu_update_ire_from_msi() on x86 and the > ITS related code once implemented on Arm. Introduce the new Kconfig CONFIG_HAS_IOMMU_INTERRUPT_REMAP for this option. I think it's best to introduce this hook when you actually have to implement the Arm version of it. Thanks, Roger.
On 19.04.2021 10:40, Roger Pau Monné wrote: > On Mon, Apr 19, 2021 at 07:16:52AM +0000, Rahul Singh wrote: >> Thanks you everyone for reviewing the code. I will summarise what I have understood from all the comments >> and what I will be doing for the next version of the patch. Please let me know your view on this. >> >> 1. Create a separate non-arch specific file "msi-intercept.c" for the below newly introduced function and >> compile that file if CONFIG_PCI_MSI_INTERCEPT is enabled.CONFIG_PCI_MSI_INTERCEPT will be >> enabled for x86 by default. Everything up to here wants to be separate from ... >> Also Mention in the commit message that these function will be needed for Xen to >> support MSI interrupt within XEN. >> >> pdev_msi_initi(..) >> pdev_msi_deiniti(..) ... this (if all of these functions really are needed beyond the purpose of intercepting MSI accesses). > I would drop the last 'i' from both function names above, as we use > init/deinit in the rest of the code base. +1 >> pdev_dump_msi(..), >> pdev_msix_assign(..) >> >> 2. Create separate patch for iommu_update_ire_from_msi() related code. There are two suggestion please help me which one to choose. >> >> - Move the iommu_update_ire_from_msi() function to asm-x86/iommu.h and also move the hook from iommu_ops under CONFIG_X86. > > I would go for this one. Strictly speaking this isn't x86-specific and hence shouldn't move there. It merely depends on whether full MSI support is wanted by an arch. I'd therefore guard the declaration by an #ifdef (if needed at all - have a declaration without implementation isn't really that problematic). For the definition question is going to be whether you introduce another new file for the pdev_*() functions above. If not, #ifdef may again be better than moving to an x86-specific file. >> - Implement a more generic function "arch_register_msi()"). This could call iommu_update_ire_from_msi() on x86 and the >> ITS related code once implemented on Arm. Introduce the new Kconfig CONFIG_HAS_IOMMU_INTERRUPT_REMAP for this option. > > I think it's best to introduce this hook when you actually have to > implement the Arm version of it. Plus arch_register_msi() sounds like a one-time operation, whereas (iirc) iommu_update_ire_from_msi() may get called many times during the lifetime of a single MSI interrupt. Jan
On 19/04/2021 12:16, Jan Beulich wrote: > On 19.04.2021 10:40, Roger Pau Monné wrote: >> On Mon, Apr 19, 2021 at 07:16:52AM +0000, Rahul Singh wrote: >>> Thanks you everyone for reviewing the code. I will summarise what I have understood from all the comments >>> and what I will be doing for the next version of the patch. Please let me know your view on this. >>> >>> 1. Create a separate non-arch specific file "msi-intercept.c" for the below newly introduced function and >>> compile that file if CONFIG_PCI_MSI_INTERCEPT is enabled.CONFIG_PCI_MSI_INTERCEPT will be >>> enabled for x86 by default. > > Everything up to here wants to be separate from ... > >>> Also Mention in the commit message that these function will be needed for Xen to >>> support MSI interrupt within XEN. >>> >>> pdev_msi_initi(..) >>> pdev_msi_deiniti(..) > > ... this (if all of these functions really are needed beyond the > purpose of intercepting MSI accesses). > >> I would drop the last 'i' from both function names above, as we use >> init/deinit in the rest of the code base. > > +1 > >>> pdev_dump_msi(..), >>> pdev_msix_assign(..) >>> >>> 2. Create separate patch for iommu_update_ire_from_msi() related code. There are two suggestion please help me which one to choose. >>> >>> - Move the iommu_update_ire_from_msi() function to asm-x86/iommu.h and also move the hook from iommu_ops under CONFIG_X86. >> >> I would go for this one. > > Strictly speaking this isn't x86-specific and hence shouldn't move there. > It merely depends on whether full MSI support is wanted by an arch. As I pointed out before, Arm doesn't use the IOMMU to setup the MSIs. So the naming and using an IOMMU callback is definitely wrong for Arm. > I'd > therefore guard the declaration by an #ifdef (if needed at all - have a > declaration without implementation isn't really that problematic). For > the definition question is going to be whether you introduce another new > file for the pdev_*() functions above. If not, #ifdef may again be better > than moving to an x86-specific file. AFAIK, this helper is only called by x86 specific code and it will not be used as-is by Arm. I can't tell for other arch (e.g RISCv, PowerPC). However... we can take the decision to move the code back to common back when it is necessary. For the time being, I think move this code in x86 is a lot better than #ifdef or keep the code in common code. Cheers, -- Julien Grall
On 19.04.2021 13:54, Julien Grall wrote: > For the time being, I think move this code in x86 is a lot better than > #ifdef or keep the code in common code. Well, I would perhaps agree if it ended up being #ifdef CONFIG_X86. I would perhaps not agree if there was a new CONFIG_* which other (future) arch-es could select if desired. Jan
Hi Jan > On 19 Apr 2021, at 1:33 pm, Jan Beulich <jbeulich@suse.com> wrote: > > On 19.04.2021 13:54, Julien Grall wrote: >> For the time being, I think move this code in x86 is a lot better than >> #ifdef or keep the code in common code. > > Well, I would perhaps agree if it ended up being #ifdef CONFIG_X86. > I would perhaps not agree if there was a new CONFIG_* which other > (future) arch-es could select if desired. I agree with Julien moving the code to x86 file as currently it is referenced only in x86 code and as of now we are not sure how other architecture will implement the Interrupt remapping (via IOMMU or any other means). Let me know if you are ok with moving the code to x86. Regards, Rahul > > Jan
On 20.04.2021 15:45, Rahul Singh wrote: >> On 19 Apr 2021, at 1:33 pm, Jan Beulich <jbeulich@suse.com> wrote: >> On 19.04.2021 13:54, Julien Grall wrote: >>> For the time being, I think move this code in x86 is a lot better than >>> #ifdef or keep the code in common code. >> >> Well, I would perhaps agree if it ended up being #ifdef CONFIG_X86. >> I would perhaps not agree if there was a new CONFIG_* which other >> (future) arch-es could select if desired. > > I agree with Julien moving the code to x86 file as currently it is referenced only in x86 code > and as of now we are not sure how other architecture will implement the Interrupt remapping > (via IOMMU or any other means). > > Let me know if you are ok with moving the code to x86. I can't answer this with "yes" or "no" without knowing what the alternative would be. As said, if the alternative is CONFIG_X86 #ifdef-ary, then yes. If a separate CONFIG_* gets introduced (and selected by X86), then a separate file (getting built only when that new setting is y) would seem better to me. Jan
Hi Jan,
> On 20 Apr 2021, at 4:36 pm, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 20.04.2021 15:45, Rahul Singh wrote:
>>> On 19 Apr 2021, at 1:33 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 19.04.2021 13:54, Julien Grall wrote:
>>>> For the time being, I think move this code in x86 is a lot better than
>>>> #ifdef or keep the code in common code.
>>>
>>> Well, I would perhaps agree if it ended up being #ifdef CONFIG_X86.
>>> I would perhaps not agree if there was a new CONFIG_* which other
>>> (future) arch-es could select if desired.
>>
>> I agree with Julien moving the code to x86 file as currently it is referenced only in x86 code
>> and as of now we are not sure how other architecture will implement the Interrupt remapping
>> (via IOMMU or any other means).
>>
>> Let me know if you are ok with moving the code to x86.
>
> I can't answer this with "yes" or "no" without knowing what the alternative
> would be. As said, if the alternative is CONFIG_X86 #ifdef-ary, then yes.
> If a separate CONFIG_* gets introduced (and selected by X86), then a
> separate file (getting built only when that new setting is y) would seem
> better to me.
I just made a quick patch. Please let me know if below patch is ok. I move the definition to "passthrough/x86/iommu.c” file.
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 705137f8be..199ce08612 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1303,13 +1303,6 @@ static int __init setup_dump_pcidevs(void)
}
__initcall(setup_dump_pcidevs);
-int iommu_update_ire_from_msi(
- struct msi_desc *msi_desc, struct msi_msg *msg)
-{
- return iommu_intremap
- ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
-}
-
static int iommu_add_device(struct pci_dev *pdev)
{
const struct domain_iommu *hd;
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index b90bb31bfe..cf51dec564 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -340,6 +340,13 @@ bool arch_iommu_use_permitted(const struct domain *d)
likely(!p2m_get_hostp2m(d)->global_logdirty));
}
+int iommu_update_ire_from_msi(
+ struct msi_desc *msi_desc, struct msi_msg *msg)
+{
+ return iommu_intremap
+ ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index ea0cd0f1a2..bd42d87b72 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -243,7 +243,6 @@ struct iommu_ops {
u8 devfn, device_t *dev);
#ifdef CONFIG_HAS_PCI
int (*get_device_group_id)(u16 seg, u8 bus, u8 devfn);
- int (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg *msg);
#endif /* HAS_PCI */
void (*teardown)(struct domain *d);
@@ -272,6 +271,7 @@ struct iommu_ops {
int (*adjust_irq_affinities)(void);
void (*sync_cache)(const void *addr, unsigned int size);
void (*clear_root_pgtable)(struct domain *d);
+ int (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg *msg);
#endif /* CONFIG_X86 */
int __must_check (*suspend)(void)
Regards,
Rahul
>
> Jan
On 21.04.2021 10:07, Rahul Singh wrote: > Hi Jan, > >> On 20 Apr 2021, at 4:36 pm, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 20.04.2021 15:45, Rahul Singh wrote: >>>> On 19 Apr 2021, at 1:33 pm, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 19.04.2021 13:54, Julien Grall wrote: >>>>> For the time being, I think move this code in x86 is a lot better than >>>>> #ifdef or keep the code in common code. >>>> >>>> Well, I would perhaps agree if it ended up being #ifdef CONFIG_X86. >>>> I would perhaps not agree if there was a new CONFIG_* which other >>>> (future) arch-es could select if desired. >>> >>> I agree with Julien moving the code to x86 file as currently it is referenced only in x86 code >>> and as of now we are not sure how other architecture will implement the Interrupt remapping >>> (via IOMMU or any other means). >>> >>> Let me know if you are ok with moving the code to x86. >> >> I can't answer this with "yes" or "no" without knowing what the alternative >> would be. As said, if the alternative is CONFIG_X86 #ifdef-ary, then yes. >> If a separate CONFIG_* gets introduced (and selected by X86), then a >> separate file (getting built only when that new setting is y) would seem >> better to me. > > I just made a quick patch. Please let me know if below patch is ok. I move the definition to "passthrough/x86/iommu.c” file. This patch on its own looks okay, but assumes you've already taken the decision that no proper new CONFIG_* would want introducing. That decision, however, touches (aiui) more than just this one hook. Jan
© 2016 - 2024 Red Hat, Inc.