[snip]
>> Sorry I didn't follow your explanation.
>>
>> My suggestion is to remove the #ifdef CONFIG_HAS_PCI completely from
>> map_range_to_domain. At the beginning of map_range_to_domain, there is
>> already this line:
>>
>> bool need_mapping = !dt_device_for_passthrough(dev);
>>
>> We can change it into:
>>
>> bool need_mapping = !dt_device_for_passthrough(dev) &&
>> !mr_data->skip_mapping;
>>
>>
>> Then, in map_device_children we can set mr_data->skip_mapping to true
>> for PCI devices.
> This is the key. I am fine with this, but it just means we move the
>
> check to the outside of this function which looks good. Will do
>
>> There is already a pci check there:
>>
>> if ( dt_device_type_is_equal(dev, "pci") )
>>
>> so it should be easy to do. What am I missing?
>>
>>
>
I did some experiments. If we move the check to map_device_children
it is not enough because part of the ranges is still mapped at handle_device
level:
handle_device:
(XEN) --- /axi/pcie@fd0e0000 need_mapping 1 addr fd0e0000
(XEN) --- /axi/pcie@fd0e0000 need_mapping 1 addr fd480000
(XEN) --- /axi/pcie@fd0e0000 need_mapping 1 addr 8000000000
map_device_children:
(XEN) Mapping children of /axi/pcie@fd0e0000 to guest skip 1
(XEN) --- /axi/pcie@fd0e0000 need_mapping 0 addr e0000000
(XEN) --- /axi/pcie@fd0e0000 need_mapping 0 addr 600000000
pci_host_bridge_mappings:
(XEN) --- /axi/pcie@fd0e0000 need_mapping 1 addr fd0e0000
(XEN) --- /axi/pcie@fd0e0000 need_mapping 1 addr fd480000
So, I did more intrusive change:
@@ -1540,6 +1534,12 @@ static int __init handle_device(struct domain *d, struct
dt_device_node *dev,
int res;
u64 addr, size;
bool need_mapping = !dt_device_for_passthrough(dev);
+ struct map_range_data mr_data = {
+ .d = d,
+ .p2mt = p2mt,
+ .skip_mapping = is_pci_passthrough_enabled() &&
+ (device_get_class(dev) == DEVICE_PCI)
+ };
With this I see that now mappings are done correctly:
handle_device:
(XEN) --- /axi/pcie@fd0e0000 need_mapping 0 addr fd0e0000
(XEN) --- /axi/pcie@fd0e0000 need_mapping 0 addr fd480000
(XEN) --- /axi/pcie@fd0e0000 need_mapping 0 addr 8000000000
map_device_children:
(XEN) Mapping children of /axi/pcie@fd0e0000 to guest skip 1
(XEN) --- /axi/pcie@fd0e0000 need_mapping 0 addr e0000000
(XEN) --- /axi/pcie@fd0e0000 need_mapping 0 addr 600000000
pci_host_bridge_mappings:
(XEN) --- /axi/pcie@fd0e0000 need_mapping 1 addr fd0e0000
(XEN) --- /axi/pcie@fd0e0000 need_mapping 1 addr fd480000
So, handle_device seems to be the right place. While at it I have also
optimized the way we setup struct map_range_data mr_data in both
handle_device and map_device_children: I removed structure initialization
from within the relevant loop and also pass mr_data to map_device_children,
so it doesn't need to create its own copy of the same and perform yet
another computation for .skip_mapping: it does need to not only know
that dev is a PCI device (this is done by the dt_device_type_is_equal(dev,
"pci")
check, but also account on is_pci_passthrough_enabled().
Thus, the change will be more intrusive, but I hope will simplify things.
I am attaching the fixup patch for just in case you want more details.
Thank you,
Oleksandr
From 07d6523be2535293d3e34ffd1c8508a0812a4cd8 Mon Sep 17 00:00:00 2001
From: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
Date: Tue, 28 Sep 2021 13:24:42 +0300
Subject: [PATCH] Fixes: 4480fb1a5c83 ("xen/arm: Do not map PCI ECAM and MMIO
space to Domain-0's p2m")
Since v2:
- removed check in map_range_to_domain for PCI_DEV
and moved it to handle_device, so the code is
simpler
- s/map_pci_bridge/skip_mapping
- extended comment in pci_host_bridge_mappings
- minor code restructure in construct_dom0
- s/.need_p2m_mapping/.need_p2m_hwdom_mapping and related
callbacks
- unsigned int i; in pci_host_bridge_mappings
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
---
xen/arch/arm/domain_build.c | 43 +++++++++++-------------------
xen/arch/arm/pci/ecam.c | 8 +++---
xen/arch/arm/pci/pci-host-common.c | 15 ++++++-----
xen/arch/arm/pci/pci-host-zynqmp.c | 2 +-
xen/include/asm-arm/pci.h | 12 ++++-----
xen/include/asm-arm/setup.h | 2 +-
6 files changed, 36 insertions(+), 46 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index e72c1b881cae..17f3db6a1f48 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1386,7 +1386,8 @@ int __init map_range_to_domain(const struct dt_device_node *dev,
{
struct map_range_data *mr_data = data;
struct domain *d = mr_data->d;
- bool need_mapping = !dt_device_for_passthrough(dev);
+ bool need_mapping = !dt_device_for_passthrough(dev) &&
+ !mr_data->skip_mapping;
int res;
/*
@@ -1409,13 +1410,6 @@ int __init map_range_to_domain(const struct dt_device_node *dev,
}
}
-#ifdef CONFIG_HAS_PCI
- if ( is_pci_passthrough_enabled() &&
- (device_get_class(dev) == DEVICE_PCI) &&
- !mr_data->map_pci_bridge )
- need_mapping = false;
-#endif
-
if ( need_mapping )
{
res = map_regions_p2mt(d,
@@ -1445,27 +1439,21 @@ int __init map_range_to_domain(const struct dt_device_node *dev,
* then we may need to perform additional mappings in order to make
* the child resources available to domain 0.
*/
-static int __init map_device_children(struct domain *d,
- const struct dt_device_node *dev,
- p2m_type_t p2mt)
+static int __init map_device_children(const struct dt_device_node *dev,
+ struct map_range_data *mr_data)
{
- struct map_range_data mr_data = {
- .d = d,
- .p2mt = p2mt,
- .map_pci_bridge = false
- };
- int ret;
-
if ( dt_device_type_is_equal(dev, "pci") )
{
+ int ret;
+
dt_dprintk("Mapping children of %s to guest\n",
dt_node_full_name(dev));
- ret = dt_for_each_irq_map(dev, &map_dt_irq_to_domain, d);
+ ret = dt_for_each_irq_map(dev, &map_dt_irq_to_domain, mr_data->d);
if ( ret < 0 )
return ret;
- ret = dt_for_each_range(dev, &map_range_to_domain, &mr_data);
+ ret = dt_for_each_range(dev, &map_range_to_domain, mr_data);
if ( ret < 0 )
return ret;
}
@@ -1546,6 +1534,12 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
int res;
u64 addr, size;
bool need_mapping = !dt_device_for_passthrough(dev);
+ struct map_range_data mr_data = {
+ .d = d,
+ .p2mt = p2mt,
+ .skip_mapping = is_pci_passthrough_enabled() &&
+ (device_get_class(dev) == DEVICE_PCI)
+ };
naddr = dt_number_of_address(dev);
@@ -1585,11 +1579,6 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
/* Give permission and map MMIOs */
for ( i = 0; i < naddr; i++ )
{
- struct map_range_data mr_data = {
- .d = d,
- .p2mt = p2mt,
- .map_pci_bridge = false
- };
res = dt_device_get_address(dev, i, &addr, &size);
if ( res )
{
@@ -1603,7 +1592,7 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
return res;
}
- res = map_device_children(d, dev, p2mt);
+ res = map_device_children(dev, &mr_data);
if ( res )
return res;
@@ -2763,9 +2752,9 @@ static int __init construct_dom0(struct domain *d)
if ( acpi_disabled )
{
rc = prepare_dtb_hwdom(d, &kinfo);
-#ifdef CONFIG_HAS_PCI
if ( rc < 0 )
return rc;
+#ifdef CONFIG_HAS_PCI
rc = pci_host_bridge_mappings(d, p2m_mmio_direct_c);
#endif
}
diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
index eae177f2cbc2..4c54998d1b4b 100644
--- a/xen/arch/arm/pci/ecam.c
+++ b/xen/arch/arm/pci/ecam.c
@@ -39,9 +39,9 @@ void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
return base + (PCI_DEVFN2(sbdf) << devfn_shift) + where;
}
-bool pci_ecam_need_p2m_mapping(struct domain *d,
- struct pci_host_bridge *bridge,
- uint64_t addr)
+bool pci_ecam_need_p2m_hwdom_mapping(struct domain *d,
+ struct pci_host_bridge *bridge,
+ uint64_t addr)
{
struct pci_config_window *cfg = bridge->cfg;
@@ -59,7 +59,7 @@ const struct pci_ecam_ops pci_generic_ecam_ops = {
.map_bus = pci_ecam_map_bus,
.read = pci_generic_config_read,
.write = pci_generic_config_write,
- .need_p2m_mapping = pci_ecam_need_p2m_mapping,
+ .need_p2m_hwdom_mapping = pci_ecam_need_p2m_hwdom_mapping,
}
};
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index f350826ea26b..f993deba8712 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -335,21 +335,21 @@ int __init pci_host_bridge_mappings(struct domain *d, p2m_type_t p2mt)
struct map_range_data mr_data = {
.d = d,
.p2mt = p2mt,
- .map_pci_bridge = true
+ .skip_mapping = false
};
/*
* For each PCI host bridge we need to only map those ranges
* which are used by Domain-0 to properly initialize the bridge,
* e.g. we do not want to map ECAM configuration space which lives in
- * "reg" or "assigned-addresses" device tree property.
- * Neither we want to map any of the MMIO ranges found in the "ranges"
- * device tree property.
+ * "reg" or "assigned-addresses" device tree property, but we want to
+ * map other regions of the host bridge. The PCI aperture defined by
+ * the "ranges" device tree property should also be skipped.
*/
list_for_each_entry( bridge, &pci_host_bridges, node )
{
const struct dt_device_node *dev = bridge->dt_node;
- int i;
+ unsigned int i;
for ( i = 0; i < dt_number_of_address(dev); i++ )
{
@@ -359,12 +359,13 @@ int __init pci_host_bridge_mappings(struct domain *d, p2m_type_t p2mt)
err = dt_device_get_address(dev, i, &addr, &size);
if ( err )
{
- printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
+ printk(XENLOG_ERR
+ "Unable to retrieve address range index=%u for %s\n",
i, dt_node_full_name(dev));
return err;
}
- if ( bridge->ops->need_p2m_mapping(d, bridge, addr) )
+ if ( bridge->ops->need_p2m_hwdom_mapping(d, bridge, addr) )
{
err = map_range_to_domain(dev, addr, size, &mr_data);
if ( err )
diff --git a/xen/arch/arm/pci/pci-host-zynqmp.c b/xen/arch/arm/pci/pci-host-zynqmp.c
index adbe3627871f..aef5d6af7ef5 100644
--- a/xen/arch/arm/pci/pci-host-zynqmp.c
+++ b/xen/arch/arm/pci/pci-host-zynqmp.c
@@ -33,7 +33,7 @@ const struct pci_ecam_ops nwl_pcie_ops = {
.map_bus = pci_ecam_map_bus,
.read = pci_generic_config_read,
.write = pci_generic_config_write,
- .need_p2m_mapping = pci_ecam_need_p2m_mapping,
+ .need_p2m_hwdom_mapping = pci_ecam_need_p2m_hwdom_mapping,
}
};
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index ef986bd5994f..3c222acc46be 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -79,9 +79,9 @@ struct pci_ops {
uint32_t reg, uint32_t len, uint32_t *value);
int (*write)(struct pci_host_bridge *bridge, uint32_t sbdf,
uint32_t reg, uint32_t len, uint32_t value);
- bool (*need_p2m_mapping)(struct domain *d,
- struct pci_host_bridge *bridge,
- uint64_t addr);
+ bool (*need_p2m_hwdom_mapping)(struct domain *d,
+ struct pci_host_bridge *bridge,
+ uint64_t addr);
};
/*
@@ -105,9 +105,9 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
uint32_t reg, uint32_t len, uint32_t value);
void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
uint32_t sbdf, uint32_t where);
-bool pci_ecam_need_p2m_mapping(struct domain *d,
- struct pci_host_bridge *bridge,
- uint64_t addr);
+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);
int pci_get_host_bridge_segment(const struct dt_device_node *node,
uint16_t *segment);
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 21863dd2bc58..5b30135fda38 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -84,7 +84,7 @@ struct map_range_data
struct domain *d;
p2m_type_t p2mt;
/* Set if mappings for PCI host bridges must not be skipped. */
- bool map_pci_bridge;
+ bool skip_mapping;
};
extern struct bootinfo bootinfo;
--
2.25.1
