On 05.08.22 18:43, Rahul Singh wrote:
Hello Rahul
Thank you very much for that patch!
From: Oleksandr Andrushchenko <[email protected]>
I am not 100% sure regarding that. This is *completely* different patch
from what Oleksandr initially made here:
https://lore.kernel.org/xen-devel/[email protected]/
Copy below for the convenience:
+bool is_memory_hole(mfn_t start, mfn_t end)
+{
+ /* TODO: this needs to be properly implemented. */
+ return true;
+}
Patch looks good, just a couple of minor comments/nits.
is_memory_hole was implemented for x86 and not for ARM when introduced.
Replace is_memory_hole call to pci_check_bar as function should check
if device BAR is in defined memory range. Also, add an implementation
for ARM which is required for PCI passthrough.
On x86, pci_check_bar will call is_memory_hole which will check if BAR
is not overlapping with any memory region defined in the memory map.
On ARM, pci_check_bar will go through the host bridge ranges and check
if the BAR is in the range of defined ranges.
Signed-off-by: Rahul Singh <[email protected]>
---
xen/arch/arm/include/asm/pci.h | 12 ++++++++++
xen/arch/arm/pci/pci-host-common.c | 35 ++++++++++++++++++++++++++++++
xen/arch/x86/include/asm/pci.h | 10 +++++++++
xen/drivers/passthrough/pci.c | 8 +++----
4 files changed, 61 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 7c7449d64f..5c4ab2c4dc 100644
--- a/xen/arch/arm/include/asm/pci.h
+++ b/xen/arch/arm/include/asm/pci.h
@@ -91,6 +91,16 @@ struct pci_ecam_ops {
int (*init)(struct pci_config_window *);
};
+/*
+ * struct to hold pci device bar.
+ */
+struct pdev_bar
+{
+ mfn_t start;
+ mfn_t end;
+ bool is_valid;
+};
Nit: This is only used by pci-host-common.c, so I think it could be
declared there.
+
/* Default ECAM ops */
extern const struct pci_ecam_ops pci_generic_ecam_ops;
@@ -125,6 +135,8 @@ int pci_host_iterate_bridges_and_count(struct domain *d,
int pci_host_bridge_mappings(struct domain *d);
+bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end);
+
#else /*!CONFIG_HAS_PCI*/
struct arch_pci_dev { };
diff --git a/xen/arch/arm/pci/pci-host-common.c
b/xen/arch/arm/pci/pci-host-common.c
index fd8c0f837a..8ea1aaeece 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -363,6 +363,41 @@ int __init pci_host_bridge_mappings(struct domain *d)
return 0;
}
+static int is_bar_valid(const struct dt_device_node *dev,
+ u64 addr, u64 len, void *data)
+{
+ struct pdev_bar *bar_data = data;
+ unsigned long s = mfn_x(bar_data->start);
+ unsigned long e = mfn_x(bar_data->end);
+
+ if ( (s < e) && (s >= PFN_DOWN(addr)) && (e<= PFN_DOWN(addr + len - 1)) )
Nit: white space after 'e' is missed in the last part of the check
+ bar_data->is_valid = true;
+
+ return 0;
+}
+
+bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
+{
+ int ret;
+ struct dt_device_node *dt_node;
+ struct device *dev = (struct device *)pci_to_dev(pdev);
The cast is present here because of the const?
I would consider passing "const struct pci_dev *pdev" instead of "struct
device *dev" to pci_find_host_bridge_node() and dropping conversion
(pci<->dev) in both functions.
Something like below (not tested):
diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 5c4ab2c4dc..a17ef32252 100644
--- a/xen/arch/arm/include/asm/pci.h
+++ b/xen/arch/arm/include/asm/pci.h
@@ -116,7 +116,7 @@ bool pci_ecam_need_p2m_hwdom_mapping(struct domain *d,
struct pci_host_bridge *bridge,
uint64_t addr);
struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t
bus);
-struct dt_device_node *pci_find_host_bridge_node(struct device *dev);
+struct dt_device_node *pci_find_host_bridge_node(const struct pci_dev
*pdev);
int pci_get_host_bridge_segment(const struct dt_device_node *node,
uint16_t *segment);
diff --git a/xen/arch/arm/pci/pci-host-common.c
b/xen/arch/arm/pci/pci-host-common.c
index 8ea1aaeece..3a64a7350f 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -243,10 +243,9 @@ err_exit:
/*
* Get host bridge node given a device attached to it.
*/
-struct dt_device_node *pci_find_host_bridge_node(struct device *dev)
+struct dt_device_node *pci_find_host_bridge_node(const struct pci_dev
*pdev)
{
struct pci_host_bridge *bridge;
- struct pci_dev *pdev = dev_to_pci(dev);
bridge = pci_find_host_bridge(pdev->seg, pdev->bus);
if ( unlikely(!bridge) )
@@ -380,14 +379,13 @@ bool pci_check_bar(const struct pci_dev *pdev,
mfn_t start, mfn_t end)
{
int ret;
struct dt_device_node *dt_node;
- struct device *dev = (struct device *)pci_to_dev(pdev);
struct pdev_bar bar_data = {
.start = start,
.end = end,
.is_valid = false
};
- dt_node = pci_find_host_bridge_node(dev);
+ dt_node = pci_find_host_bridge_node(pdev);
ret = dt_for_each_range(dt_node, &is_bar_valid, &bar_data);
if ( ret < 0 )
+ struct pdev_bar bar_data = {
+ .start = start,
+ .end = end,
+ .is_valid = false
+ };
+
+ dt_node = pci_find_host_bridge_node(dev);
if ( !dt_node )
return false;
+
+ ret = dt_for_each_range(dt_node, &is_bar_valid, &bar_data);
+ if ( ret < 0 )
+ return ret;
s/return ret;/return false;
+
+ if ( !bar_data.is_valid )
+ return false;
+
+ return true;
+}
/*
* Local variables:
* mode: C
diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
index c8e1a9ecdb..f4a58c8acf 100644
--- a/xen/arch/x86/include/asm/pci.h
+++ b/xen/arch/x86/include/asm/pci.h
@@ -57,4 +57,14 @@ static always_inline bool is_pci_passthrough_enabled(void)
void arch_pci_init_pdev(struct pci_dev *pdev);
+static inline bool pci_check_bar(const struct pci_dev *pdev,
+ mfn_t start, mfn_t end)
+{
+ /*
+ * Check if BAR is not overlapping with any memory region defined
+ * in the memory map.
+ */
+ return is_memory_hole(start, end);
+}
Nit: I would use simple #define instead of static inline here
But I am not 100% sure that x86 maintainers would be happy.
+
#endif /* __X86_PCI_H__ */
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 29356d59a7..52453a05fe 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -304,8 +304,8 @@ static void check_pdev(const struct pci_dev *pdev)
if ( rc < 0 )
/* Unable to size, better leave memory decoding disabled. */
return;
- if ( size && !is_memory_hole(maddr_to_mfn(addr),
- maddr_to_mfn(addr + size - 1)) )
+ if ( size && !pci_check_bar(pdev, maddr_to_mfn(addr),
+ maddr_to_mfn(addr + size - 1)) )
{
/*
* Return without enabling memory decoding if BAR position is not
@@ -331,8 +331,8 @@ static void check_pdev(const struct pci_dev *pdev)
if ( rc < 0 )
return;
- if ( size && !is_memory_hole(maddr_to_mfn(addr),
- maddr_to_mfn(addr + size - 1)) )
+ if ( size && !pci_check_bar(pdev, maddr_to_mfn(addr),
+ maddr_to_mfn(addr + size - 1)) )
{
printk(warn, &pdev->sbdf, "ROM ", PFN_DOWN(addr),
PFN_DOWN(addr + size - 1));
--
Regards,
Oleksandr Tyshchenko
