[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v5 05/11] mm: move modify_identity_mmio to global file and drop __init
Hi Roger,
On 14/08/17 15:28, Roger Pau Monne wrote:
And also allow it to do non-identity mappings by adding a new
parameter.
This function will be needed in order to map the BARs from PCI devices
into the Dom0 p2m (and is also used by the x86 Dom0 builder). While
there fix the function to use gfn_t and mfn_t instead of unsigned long
for memory addresses. >
Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
---
Cc: Jan Beulich <jbeulich@xxxxxxxx>
Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
Changes since v4:
- Guard the function with CONFIG_HAS_PCI only.
- s/non-trival/non-negligible in the comment.
- Change XENLOG_G_WARNING to XENLOG_WARNING like the original
function.
Changes since v3:
- Remove the dummy modify_identity_mmio helper in dom0_build.c
- Try to make the comment in modify MMIO less scary.
- Clarify commit message.
- Only build the function for x86 or if there's PCI support.
Changes since v2:
- Use mfn_t and gfn_t.
- Remove stray newline.
---
xen/arch/x86/hvm/dom0_build.c | 30 ++----------------------------
xen/common/memory.c | 40 ++++++++++++++++++++++++++++++++++++++++
xen/include/xen/p2m-common.h | 3 +++
3 files changed, 45 insertions(+), 28 deletions(-)
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 04a8682d33..c65eb8503f 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -61,32 +61,6 @@ static struct acpi_madt_interrupt_override __initdata
*intsrcovr;
static unsigned int __initdata acpi_nmi_sources;
static struct acpi_madt_nmi_source __initdata *nmisrc;
-static int __init modify_identity_mmio(struct domain *d, unsigned long pfn,
- unsigned long nr_pages, const bool map)
-{
- int rc;
-
- for ( ; ; )
- {
- rc = (map ? map_mmio_regions : unmap_mmio_regions)
- (d, _gfn(pfn), nr_pages, _mfn(pfn));
- if ( rc == 0 )
- break;
- if ( rc < 0 )
- {
- printk(XENLOG_WARNING
- "Failed to identity %smap [%#lx,%#lx) for d%d: %d\n",
- map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, rc);
- break;
- }
- nr_pages -= rc;
- pfn += rc;
- process_pending_softirqs();
- }
-
- return rc;
-}
-
/* Populate a HVM memory range using the biggest possible order. */
static int __init pvh_populate_memory_range(struct domain *d,
unsigned long start,
@@ -397,7 +371,7 @@ static int __init pvh_setup_p2m(struct domain *d)
* Memory below 1MB is identity mapped.
* NB: this only makes sense when booted from legacy BIOS.
*/
- rc = modify_identity_mmio(d, 0, MB1_PAGES, true);
+ rc = modify_mmio(d, _gfn(0), _mfn(0), MB1_PAGES, true);
if ( rc )
{
printk("Failed to identity map low 1MB: %d\n", rc);
@@ -964,7 +938,7 @@ static int __init pvh_setup_acpi(struct domain *d, paddr_t
start_info)
nr_pages = PFN_UP((d->arch.e820[i].addr & ~PAGE_MASK) +
d->arch.e820[i].size);
- rc = modify_identity_mmio(d, pfn, nr_pages, true);
+ rc = modify_mmio(d, _gfn(pfn), _mfn(pfn), nr_pages, true);
if ( rc )
{
printk("Failed to map ACPI region [%#lx, %#lx) into Dom0 memory
map\n",
diff --git a/xen/common/memory.c b/xen/common/memory.c
index b2066db07e..86824edb09 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1465,6 +1465,46 @@ int prepare_ring_for_helper(
return 0;
}
+#if defined(CONFIG_HAS_PCI)
+int modify_mmio(struct domain *d, gfn_t gfn, mfn_t mfn, unsigned long nr_pages,
+ bool map)
+{
I don't think this is correct to move this function in common code
without making sure that all arch have *map_mmio_regions supporting
preemption.
This is actually not the case on ARM and IHMO should be fixed before
getting this function common. Otherwise you will expose a security issue
the day vCPI will get supported on ARM.
+ int rc;
+
+ /*
+ * ATM this function should only be used by the hardware domain
+ * because it doesn't support preemption/continuation, and as such
+ * can take a non-negligible amount of time. Note that it periodically
+ * calls process_pending_softirqs in order to avoid stalling the system.
+ */
+ ASSERT(is_hardware_domain(d));
+
+ for ( ; ; )
+ {
+ rc = (map ? map_mmio_regions : unmap_mmio_regions)
As mentioned in an earlier version, on ARM map_mmio_regions will map the
MMIO with very strict attribute (no-unaligned access,
non-gatherable,...). This will not be correct for some BARs. So I think
we should provide the attribute type in parameter of modify_mmio to know
the memory attribute.
+ (d, gfn, nr_pages, mfn);
+ if ( rc == 0 )
+ break;
+ if ( rc < 0 )
+ {
+ printk(XENLOG_WARNING
+ "Failed to %smap [%" PRI_gfn ", %" PRI_gfn ") -> "
+ "[%" PRI_mfn ", %" PRI_mfn ") for d%d: %d\n",
+ map ? "" : "un", gfn_x(gfn), gfn_x(gfn_add(gfn, nr_pages)),
+ mfn_x(mfn), mfn_x(mfn_add(mfn, nr_pages)), d->domain_id,
+ rc);
+ break;
+ }
+ nr_pages -= rc;
+ mfn = mfn_add(mfn, rc);
+ gfn = gfn_add(gfn, rc);
+ process_pending_softirqs();
+ }
+
+ return rc;
+}
+#endif
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
index 2b5696cf33..c2f9015ad8 100644
--- a/xen/include/xen/p2m-common.h
+++ b/xen/include/xen/p2m-common.h
@@ -20,4 +20,7 @@ int unmap_mmio_regions(struct domain *d,
unsigned long nr,
mfn_t mfn);
+int modify_mmio(struct domain *d, gfn_t gfn, mfn_t mfn, unsigned long nr_pages,
+ const bool map);
+
#endif /* _XEN_P2M_COMMON_H */
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
