* [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM
@ 2014-05-25 10:51 Arianna Avanzini
2014-05-25 10:51 ` [PATCH v8 01/14] arch/arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
` (14 more replies)
0 siblings, 15 replies; 50+ messages in thread
From: Arianna Avanzini @ 2014-05-25 10:51 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
andrew.cooper3, JBeulich, avanzini.arianna, viktor.kleinik
Hello,
this is a new version - the eighth - of my implementation proposal for the
memory_mapping DOMCTL for the ARM architecture. As usual, this cover letter just
lists the most relevant changes performed on the patchset, while a more detailed
description can be found in the description and changelog of each patch; also,
some more information about the patch series' origin can be found in the last
full cover letter ([1]).
First of all, the commit descriptions have been reworded to get rid of tentative
phrases that were pointless in that context, as suggested by Dario Faggioli.
Patch 0002 has been modified to revert some uncorrect changes due to my
misunderstanding of previous comments, as pointed out by Julien Grall. Also,
according to his suggestions, an error message has been added to let a failure
encountered during an unmap be less obscure. The error path has been improved
to include unmapping previously-created page-table mappings, unlocking the
p2m_lock and flushing the TLBs in case some mappings have been successfully
removed.
Patch 0004 has been changed to address issues pointed out by Julien Grall and
Ian Campbell. Now the correct DIV_ROUND_UP() macro is used instead of the
ternary operator, and the map_one_mmio() function, defined for the xgene-storm
platform, accepts pfns as arguments instead of paddr_t. Also, the function
apply_p2m_changes() is now called from map_mmio_regions() with the correct
count.
A new patch 0005 has been added to let the map_mmio_regions() function for ARM
unmap partially-mapped I/O-memory regions if mapping a page fails, as suggested
by Julien Grall. The approach which has been used includes having the function
apply_p2m_changes() return, in the case of an error, the number of pages that
have been successfully mapped, therefore allowing the caller to undo what has
been done, if necessary. The definition of unmap_mmio_regions() for ARM has been
moved to this patch and now uses the correct count as indicated by Julien Grall.
The title and description of patch 0006 have been changed to (hopefully) better
fit the patch content, as requested by Jan Beulich.
The patch factoring out map and unmap operations from the x86 version of the
memory_mapping DOMCTL has been split into two, as suggested by Jan Beulich, to
make the review easier: patch 0007 now just cleans up existing code, by letting
mfn + nr_mfns be computed only once and by changing the name of the tmp_rc local
variable to rc; patch 0009 instead actually factors out the operations.
A new patch 0008 now adds an empty stub for the ARM architecture of the MTRR-
related function memory_type_changed(), currently available only for x86, and
makes its definition common to both x86 and ARM. This was requested by Jan
Beulich and Ian Campbell.
The call to PERROR() in patch 0012 has been fixed, as it showed an error message
whose meaning was the opposite of the correct one, as pointed out by Jan
Beulich.
The patch which implemented a separation of the functions of the memory_mapping
and iomem_permission DOMCTLs has now been split into two, as suggested by Jan
Beulich: patch 0013 now contains only libxl-related preparatory changes, while
patch 0014 actually implements the separation. As for patch 0014, changes
performed on the iomem_permission DOMCTL handling code have been dropped, while
the permission checks performed by memory_mapping have been changed to be more
conservative and check both the calling domain and the grantee domain's
permissions.
As of now, patch 0014 performs the separation only for I/O memory and not for
I/O ports (as was instead suggested by Jan Beulich) because, as far as I know,
Julien Grall is working on the I/O ports patch series, and I'm afraid I might be
conflicting with his work. I am however surely willing to add a patch for that
if he is OK with it.
The code has again been tested on a cubieboard2, with Linux v3.15-rc3 as a dom0
and ERIKA Enterprise ([2]) as a domU. The x86 bits have been tested on an x86_64
machine, as I forgot to say in the last versions of the patchset and Jan Beulich
pointed out; in particular, this last version, has been tested with Linux
3.15.0-rc5 as both dom0 and domU.
Any feedback about this new version of the patchset is more than welcome,
Arianna
[1] http://lists.xen.org/archives/html/xen-devel/2014-03/msg01724.html
[2] http://erika.tuxfamily.org/drupal/
Arianna Avanzini (14):
arch/arm: domain build: let dom0 access I/O memory of mapped devices
arch/arm: add consistency check to REMOVE p2m changes
arch/arm: let map_mmio_regions() take pfn as parameters
arch/arm: let map_mmio_regions() use start and count
arch/arm: unmap partially-mapped I/O-memory regions
arch/x86: warn if to-be-removed mapping does not exist
arch/x86: cleanup memory_mapping DOMCTL
xen/common: move memory_type_changed() function to common code
xen/x86: factor out map and unmap from the memory_mapping DOMCTL
xen/common: move the memory_mapping DOMCTL hypercall to common code
tools/libxl: parse optional start gfn from the iomem config option
tools/libxl: handle the iomem parameter with the memory_mapping hcall
tools/libxl: explicitly grant access to needed I/O-memory ranges
xen/common: do not implicitly permit access to mapped I/O memory
docs/man/xl.cfg.pod.5 | 18 +++++--
tools/libxc/xc_domain.c | 12 +++++
tools/libxl/libxl.h | 10 ++++
tools/libxl/libxl_create.c | 34 +++++++++++++
tools/libxl/libxl_internal.h | 1 +
tools/libxl/libxl_pci.c | 26 ++++------
tools/libxl/libxl_types.idl | 4 ++
tools/libxl/xl_cmdimpl.c | 17 ++++---
xen/arch/arm/Makefile | 1 +
xen/arch/arm/domain_build.c | 18 +++++--
xen/arch/arm/gic.c | 15 +++---
xen/arch/arm/mtrr.c | 6 +++
xen/arch/arm/p2m.c | 99 ++++++++++++++++++++++++++++--------
xen/arch/arm/platforms/exynos5.c | 9 ++--
xen/arch/arm/platforms/omap5.c | 17 +++----
xen/arch/arm/platforms/xgene-storm.c | 16 +++---
xen/arch/x86/domctl.c | 76 ---------------------------
xen/arch/x86/mm/p2m.c | 53 +++++++++++++++++--
xen/common/domctl.c | 55 +++++++++++++++++++-
xen/include/asm-arm/mm.h | 2 +
xen/include/asm-arm/mtrr.h | 6 +++
xen/include/asm-arm/p2m.h | 11 ++--
xen/include/asm-x86/mtrr.h | 2 +-
xen/include/asm-x86/p2m.h | 3 +-
xen/include/xen/mtrr.h | 8 +++
xen/include/xen/p2m-common.h | 16 ++++++
26 files changed, 365 insertions(+), 170 deletions(-)
create mode 100644 xen/arch/arm/mtrr.c
create mode 100644 xen/include/asm-arm/mtrr.h
create mode 100644 xen/include/xen/mtrr.h
create mode 100644 xen/include/xen/p2m-common.h
--
1.9.2
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v8 01/14] arch/arm: domain build: let dom0 access I/O memory of mapped devices
2014-05-25 10:51 [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
@ 2014-05-25 10:51 ` Arianna Avanzini
2014-06-10 15:04 ` Ian Campbell
2014-05-25 10:51 ` [PATCH v8 02/14] arch/arm: add consistency check to REMOVE p2m changes Arianna Avanzini
` (13 subsequent siblings)
14 siblings, 1 reply; 50+ messages in thread
From: Arianna Avanzini @ 2014-05-25 10:51 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
andrew.cooper3, JBeulich, avanzini.arianna, viktor.kleinik
Currently, dom0 is allowed access to the I/O memory ranges used
to access devices exposed to it, but it doesn't have those
ranges in its iomem_caps. This commit implements the correct
bookkeeping in the generic function which actually maps a
device's I/O memory to the domain, adding the ranges to the
domain's iomem_caps.
NOTE: This commit suffers from the following limitations;
. with this patch, I/O memory ranges pertaining disabled
devices are not mapped;
. the "iomem" option could be used to map memory ranges that
are not described in the device tree.
In both these cases, this patch does not allow the domain
the privileges needed to map the needed I/O memory ranges
afterwards.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Acked-by: Ian Campbell <Ian.Campbell@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@citrix.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
v8:
- Remove tentative phrases from commit description.
v7:
- Add limitations to commit description.
v4:
- Hopefully improve commit description.
v3:
- Print the domain id in the error message instead of always
printing "dom0".
- Fix commit description.
---
xen/arch/arm/domain_build.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c424793..465cdcb 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -11,6 +11,7 @@
#include <xen/device_tree.h>
#include <xen/libfdt/libfdt.h>
#include <xen/guest_access.h>
+#include <xen/iocap.h>
#include <asm/device.h>
#include <asm/setup.h>
#include <asm/platform.h>
@@ -761,6 +762,16 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
DPRINT("addr %u = 0x%"PRIx64" - 0x%"PRIx64"\n",
i, addr, addr + size - 1);
+ res = iomem_permit_access(d, paddr_to_pfn(addr & PAGE_MASK),
+ paddr_to_pfn(PAGE_ALIGN(addr + size - 1)));
+ if ( res )
+ {
+ printk(XENLOG_ERR "Unable to permit to dom%d access to"
+ " 0x%"PRIx64" - 0x%"PRIx64"\n",
+ d->domain_id,
+ addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
+ return res;
+ }
res = map_mmio_regions(d, addr & PAGE_MASK,
PAGE_ALIGN(addr + size) - 1,
addr & PAGE_MASK);
--
1.9.2
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v8 02/14] arch/arm: add consistency check to REMOVE p2m changes
2014-05-25 10:51 [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
2014-05-25 10:51 ` [PATCH v8 01/14] arch/arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
@ 2014-05-25 10:51 ` Arianna Avanzini
2014-05-25 15:50 ` Julien Grall
2014-06-05 13:50 ` Ian Campbell
2014-05-25 10:51 ` [PATCH v8 03/14] arch/arm: let map_mmio_regions() take pfn as parameters Arianna Avanzini
` (12 subsequent siblings)
14 siblings, 2 replies; 50+ messages in thread
From: Arianna Avanzini @ 2014-05-25 10:51 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
andrew.cooper3, JBeulich, avanzini.arianna, viktor.kleinik
Currently, the REMOVE case of the switch in apply_p2m_changes()
does not perform any consistency check on the mapping to be removed.
More in detail, the code does not check if the guest address to be
unmapped is actually mapped to the machine address given as a
parameter.
This commit adds the above-described consistency check to the REMOVE
path of apply_p2m_changes(). This is instrumental to one of the
following commits, which implements the possibility to trigger the
removal of p2m ranges via the memory_mapping DOMCTL for ARM.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
v8:
- Re-add erroneously-removed increments to the maddr variable.
- When failing to remove a mapping, add previously-mapped PT entry,
unlock the p2m_lock and flush TLBs if necessary.
- Emit an error message when failing to remove a mapping.
- Remove tentative phrases from commit description.
v7:
- Silently ignore the fact that, when removing a mapping, the specified
gfn is not mapped at all.
- Remove spurious spacing change.
v6:
- Don't update "count" on REMOVE as it is only used inside the
RELINQUISH case of the switch in apply_p2m_changes().
- Return with an error if removal of a page fails instead of just
skipping the page.
v5:
- Do not use a temporary variable to hold the machine address:
use the "maddr" function parameter itself.
- Increment the machine address also when first and second level
mappings are not valid.
- Get the actual machine frame number mapped to the guest frame
number given as parameter to the function directly in the
REMOVE case of the switch construct, as it might not be valid
in other cases and its value might be uncorrectly used in the
future.
- Remove useless and/or harmful ASSERT; check however if the
mapping is valid and skip the page if it is not.
v4:
- Remove useless and slow lookup and use already-available
data from pte instead.
- Correctly increment the local variable used to keep the
machine address whose mapping is currently being removed.
- Return with an error upon finding a mismatch between the
actual machine address mapped to the guest address and
the machine address passed as parameter, instead of just
skipping the page.
---
xen/arch/arm/p2m.c | 50 ++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 40 insertions(+), 10 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index b85143b..a9a5826 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -331,6 +331,7 @@ static int apply_p2m_changes(struct domain *d,
if ( !populate )
{
addr = (addr + FIRST_SIZE) & FIRST_MASK;
+ maddr = (maddr + FIRST_SIZE) & FIRST_MASK;
continue;
}
@@ -357,6 +358,7 @@ static int apply_p2m_changes(struct domain *d,
if ( !populate )
{
addr = (addr + SECOND_SIZE) & SECOND_MASK;
+ maddr = (maddr + SECOND_SIZE) & SECOND_MASK;
continue;
}
@@ -418,12 +420,38 @@ static int apply_p2m_changes(struct domain *d,
{
pte = mfn_to_p2m_entry(maddr >> PAGE_SHIFT, mattr, t);
write_pte(&third[third_table_offset(addr)], pte);
- maddr += PAGE_SIZE;
}
break;
- case RELINQUISH:
case REMOVE:
{
+ unsigned long mfn = pte.p2m.base;
+
+ /*
+ * Ensure that the guest address given as argument to
+ * this function is actually mapped to the specified
+ * machine address. maddr here is the machine address
+ * given to the function, while mfn is the machine
+ * frame number actually mapped to the guest address:
+ * check if the two correspond.
+ */
+ if ( !pte.p2m.valid || maddr != pfn_to_paddr(mfn) )
+ {
+ printk("p2m_remove: nonexistent mapping: "
+ "%"PRIx64" and %"PRIx64"\n",
+ pfn_to_paddr(mfn), maddr);
+ /*
+ * If we have successfully removed other mappings,
+ * overload flush local variable to store if we need
+ * to flush TLBs.
+ */
+ if (count) flush = 1; else flush = 0;
+ rc = -EINVAL;
+ goto out_flush;
+ }
+ }
+ /* fall through */
+ case RELINQUISH:
+ {
if ( !pte.p2m.valid )
{
count++;
@@ -462,28 +490,30 @@ static int apply_p2m_changes(struct domain *d,
/* Got the next page */
addr += PAGE_SIZE;
+ maddr += PAGE_SIZE;
}
- if ( flush )
+ if ( op == ALLOCATE || op == INSERT )
{
unsigned long sgfn = paddr_to_pfn(start_gpaddr);
unsigned long egfn = paddr_to_pfn(end_gpaddr);
- flush_tlb_domain(d);
- iommu_iotlb_flush(d, sgfn, egfn - sgfn);
+ p2m->max_mapped_gfn = MAX(p2m->max_mapped_gfn, egfn);
+ p2m->lowest_mapped_gfn = MIN(p2m->lowest_mapped_gfn, sgfn);
}
- if ( op == ALLOCATE || op == INSERT )
+ rc = 0;
+
+out_flush:
+ if ( flush )
{
unsigned long sgfn = paddr_to_pfn(start_gpaddr);
unsigned long egfn = paddr_to_pfn(end_gpaddr);
- p2m->max_mapped_gfn = MAX(p2m->max_mapped_gfn, egfn);
- p2m->lowest_mapped_gfn = MIN(p2m->lowest_mapped_gfn, sgfn);
+ flush_tlb_domain(d);
+ iommu_iotlb_flush(d, sgfn, egfn - sgfn);
}
- rc = 0;
-
out:
if (third) unmap_domain_page(third);
if (second) unmap_domain_page(second);
--
1.9.2
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v8 03/14] arch/arm: let map_mmio_regions() take pfn as parameters
2014-05-25 10:51 [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
2014-05-25 10:51 ` [PATCH v8 01/14] arch/arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
2014-05-25 10:51 ` [PATCH v8 02/14] arch/arm: add consistency check to REMOVE p2m changes Arianna Avanzini
@ 2014-05-25 10:51 ` Arianna Avanzini
2014-05-25 10:51 ` [PATCH v8 04/14] arch/arm: let map_mmio_regions() use start and count Arianna Avanzini
` (11 subsequent siblings)
14 siblings, 0 replies; 50+ messages in thread
From: Arianna Avanzini @ 2014-05-25 10:51 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
andrew.cooper3, JBeulich, avanzini.arianna, viktor.kleinik
Currently, the map_mmio_regions() function, defined for the ARM
architecture, has parameters with paddr_t type. This interface,
however, needs caller functions to correctly page-align addresses
given as parameters to map_mmio_regions(). This commit changes the
function's interface to accept page frame numbers as parameters.
This commit also modifies caller functions in an attempt to adapt
them to the new interface.
This commit is meant to produce the minimum indispensable needed
changes; these are also instrumental to making the interface of
map_mmio_regions() symmetric with the unmap_mmio_regions() function,
that will be introduced in one of the next commits of the series
and will feature a pfn-based interface.
NOTE: platform-specific code has not been tested.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Acked-by: Julien Grall <julien.grall@citrix.com>
Acked-by: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
v7:
- Remove useless decrements when using paddr_to_pfn_aligned().
v5:
- Add a macro for the paddr_to_pfn(PAGE_ALIGN(...)) pattern.
- Hopefully improve commit description.
---
xen/arch/arm/domain_build.c | 7 ++++---
xen/arch/arm/gic.c | 20 +++++++++++---------
xen/arch/arm/p2m.c | 13 ++++++++-----
xen/arch/arm/platforms/exynos5.c | 11 ++++++-----
xen/arch/arm/platforms/omap5.c | 21 ++++++++++++---------
xen/arch/arm/platforms/xgene-storm.c | 4 +++-
xen/include/asm-arm/mm.h | 2 ++
xen/include/asm-arm/p2m.h | 11 ++++++-----
8 files changed, 52 insertions(+), 37 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 465cdcb..6c289d0 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -772,9 +772,10 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
return res;
}
- res = map_mmio_regions(d, addr & PAGE_MASK,
- PAGE_ALIGN(addr + size) - 1,
- addr & PAGE_MASK);
+ res = map_mmio_regions(d,
+ paddr_to_pfn(addr & PAGE_MASK),
+ paddr_to_pfn_aligned(addr + size),
+ paddr_to_pfn(addr & PAGE_MASK));
if ( res )
{
printk(XENLOG_ERR "Unable to map 0x%"PRIx64
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 577d85b..42c1a9c 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -760,20 +760,22 @@ int gicv_setup(struct domain *d)
* The second page is always mapped at +4K irrespective of the
* GIC_64K_STRIDE quirk. The DTB passed to the guest reflects this.
*/
- ret = map_mmio_regions(d, d->arch.vgic.cbase,
- d->arch.vgic.cbase + PAGE_SIZE - 1,
- gic.vbase);
+ ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase),
+ paddr_to_pfn_aligned(d->arch.vgic.cbase + PAGE_SIZE),
+ paddr_to_pfn(gic.vbase));
if (ret)
return ret;
if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
- ret = map_mmio_regions(d, d->arch.vgic.cbase + PAGE_SIZE,
- d->arch.vgic.cbase + (2 * PAGE_SIZE) - 1,
- gic.vbase + PAGE_SIZE);
+ ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + PAGE_SIZE),
+ paddr_to_pfn_aligned(d->arch.vgic.cbase +
+ (2 * PAGE_SIZE)),
+ paddr_to_pfn(gic.vbase + PAGE_SIZE));
else
- ret = map_mmio_regions(d, d->arch.vgic.cbase + PAGE_SIZE,
- d->arch.vgic.cbase + (2 * PAGE_SIZE) - 1,
- gic.vbase + 16*PAGE_SIZE);
+ ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + PAGE_SIZE),
+ paddr_to_pfn_aligned(d->arch.vgic.cbase +
+ (2 * PAGE_SIZE)),
+ paddr_to_pfn(gic.vbase + 16*PAGE_SIZE));
return ret;
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index a9a5826..bde61b7 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -533,12 +533,15 @@ int p2m_populate_ram(struct domain *d,
}
int map_mmio_regions(struct domain *d,
- paddr_t start_gaddr,
- paddr_t end_gaddr,
- paddr_t maddr)
+ unsigned long start_gfn,
+ unsigned long end_gfn,
+ unsigned long mfn)
{
- return apply_p2m_changes(d, INSERT, start_gaddr, end_gaddr,
- maddr, MATTR_DEV, p2m_mmio_direct);
+ return apply_p2m_changes(d, INSERT,
+ pfn_to_paddr(start_gfn),
+ pfn_to_paddr(end_gfn),
+ pfn_to_paddr(mfn),
+ MATTR_DEV, p2m_mmio_direct);
}
int guest_physmap_add_entry(struct domain *d,
diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
index 65e584f..078b020 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -54,13 +54,14 @@ static int exynos5_init_time(void)
static int exynos5_specific_mapping(struct domain *d)
{
/* Map the chip ID */
- map_mmio_regions(d, EXYNOS5_PA_CHIPID, EXYNOS5_PA_CHIPID + PAGE_SIZE - 1,
- EXYNOS5_PA_CHIPID);
+ map_mmio_regions(d, paddr_to_pfn(EXYNOS5_PA_CHIPID),
+ paddr_to_pfn_aligned(EXYNOS5_PA_CHIPID + PAGE_SIZE),
+ paddr_to_pfn(EXYNOS5_PA_CHIPID));
/* Map the PWM region */
- map_mmio_regions(d, EXYNOS5_PA_TIMER,
- EXYNOS5_PA_TIMER + (PAGE_SIZE * 2) - 1,
- EXYNOS5_PA_TIMER);
+ map_mmio_regions(d, paddr_to_pfn(EXYNOS5_PA_TIMER),
+ paddr_to_pfn_aligned(EXYNOS5_PA_TIMER + (PAGE_SIZE * 2)),
+ paddr_to_pfn(EXYNOS5_PA_TIMER));
return 0;
}
diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c
index 76d4d9b..f51810e 100644
--- a/xen/arch/arm/platforms/omap5.c
+++ b/xen/arch/arm/platforms/omap5.c
@@ -102,21 +102,24 @@ static int omap5_init_time(void)
static int omap5_specific_mapping(struct domain *d)
{
/* Map the PRM module */
- map_mmio_regions(d, OMAP5_PRM_BASE, OMAP5_PRM_BASE + (PAGE_SIZE * 2) - 1,
- OMAP5_PRM_BASE);
+ map_mmio_regions(d, paddr_to_pfn(OMAP5_PRM_BASE),
+ paddr_to_pfn_aligned(OMAP5_PRM_BASE + (PAGE_SIZE * 2)),
+ paddr_to_pfn(OMAP5_PRM_BASE));
/* Map the PRM_MPU */
- map_mmio_regions(d, OMAP5_PRCM_MPU_BASE,
- OMAP5_PRCM_MPU_BASE + PAGE_SIZE - 1,
- OMAP5_PRCM_MPU_BASE);
+ map_mmio_regions(d, paddr_to_pfn(OMAP5_PRCM_MPU_BASE),
+ paddr_to_pfn_aligned(OMAP5_PRCM_MPU_BASE + PAGE_SIZE),
+ paddr_to_pfn(OMAP5_PRCM_MPU_BASE));
/* Map the Wakeup Gen */
- map_mmio_regions(d, OMAP5_WKUPGEN_BASE, OMAP5_WKUPGEN_BASE + PAGE_SIZE - 1,
- OMAP5_WKUPGEN_BASE);
+ map_mmio_regions(d, paddr_to_pfn(OMAP5_WKUPGEN_BASE),
+ paddr_to_pfn_aligned(OMAP5_WKUPGEN_BASE + PAGE_SIZE),
+ paddr_to_pfn(OMAP5_WKUPGEN_BASE));
/* Map the on-chip SRAM */
- map_mmio_regions(d, OMAP5_SRAM_PA, OMAP5_SRAM_PA + (PAGE_SIZE * 32) - 1,
- OMAP5_SRAM_PA);
+ map_mmio_regions(d, paddr_to_pfn(OMAP5_SRAM_PA),
+ paddr_to_pfn_aligned(OMAP5_SRAM_PA + (PAGE_SIZE * 32)),
+ paddr_to_pfn(OMAP5_SRAM_PA));
return 0;
}
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index 70aab73..6ee1444 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -47,7 +47,9 @@ static int map_one_mmio(struct domain *d, const char *what,
printk("Additional MMIO %"PRIpaddr"-%"PRIpaddr" (%s)\n",
start, end, what);
- ret = map_mmio_regions(d, start, end, start);
+ ret = map_mmio_regions(d, paddr_to_pfn(start),
+ paddr_to_pfn_aligned(end),
+ paddr_to_pfn(start));
if ( ret )
printk("Failed to map %s @ %"PRIpaddr" to dom%d\n",
what, start, d->domain_id);
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index b8d4e7d..97e0a7b 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -209,6 +209,8 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len)
#define paddr_to_pfn(pa) ((unsigned long)((pa) >> PAGE_SHIFT))
#define paddr_to_pdx(pa) pfn_to_pdx(paddr_to_pfn(pa))
+/* Page-align address and convert to frame number format */
+#define paddr_to_pfn_aligned(paddr) paddr_to_pfn(PAGE_ALIGN(paddr))
static inline paddr_t __virt_to_maddr(vaddr_t va)
{
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index bd71abe..c7dd6aa 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -84,11 +84,12 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn);
/* Setup p2m RAM mapping for domain d from start-end. */
int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end);
-/* Map MMIO regions in the p2m: start_gaddr and end_gaddr is the range
- * in the guest physical address space to map, starting from the machine
- * address maddr. */
-int map_mmio_regions(struct domain *d, paddr_t start_gaddr,
- paddr_t end_gaddr, paddr_t maddr);
+/* Map MMIO regions in the p2m: start_gfn and end_gfn is the range in the guest
+ * physical address space to map, starting from the machine frame number mfn. */
+int map_mmio_regions(struct domain *d,
+ unsigned long start_gfn,
+ unsigned long end_gfn,
+ unsigned long mfn);
int guest_physmap_add_entry(struct domain *d,
unsigned long gfn,
--
1.9.2
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v8 04/14] arch/arm: let map_mmio_regions() use start and count
2014-05-25 10:51 [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (2 preceding siblings ...)
2014-05-25 10:51 ` [PATCH v8 03/14] arch/arm: let map_mmio_regions() take pfn as parameters Arianna Avanzini
@ 2014-05-25 10:51 ` Arianna Avanzini
2014-05-25 15:56 ` Julien Grall
2014-05-25 10:51 ` [PATCH v8 05/14] arch/arm: unmap partially-mapped I/O-memory regions Arianna Avanzini
` (10 subsequent siblings)
14 siblings, 1 reply; 50+ messages in thread
From: Arianna Avanzini @ 2014-05-25 10:51 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
andrew.cooper3, JBeulich, avanzini.arianna, viktor.kleinik
Currently, the arguments given to the function map_mmio_regions() to
describe the memory range to be mapped are the start and end page frame
numbers of the range to be mapped. However, this could give rise to
issues due to the range being inclusive or exclusive of the end gfn
given as parameter. This commit changes the interface of the function
to accept the start gfn and the number of gfns to be mapped.
This commit also changes the interface of the function map_one_mmio(),
helper for the xgene-storm platform, which is a wrapper for the function
map_mmio_regions() and does not need its arguments to be paddr_t.
NOTE: platform-specific code has not been tested.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
v8:
- Fix incorrect count used by map_mmio_regions() when calling
apply_p2m_changes().
- Use pfns as arguments to map_one_mmio().
- Use the DIV_ROUND_UP() macro when appropriate.
---
xen/arch/arm/domain_build.c | 2 +-
xen/arch/arm/gic.c | 11 +++--------
xen/arch/arm/p2m.c | 4 ++--
xen/arch/arm/platforms/exynos5.c | 6 ++----
xen/arch/arm/platforms/omap5.c | 12 ++++--------
xen/arch/arm/platforms/xgene-storm.c | 18 ++++++++++--------
xen/include/asm-arm/p2m.h | 7 ++++---
7 files changed, 26 insertions(+), 34 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 6c289d0..87f0134 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -774,7 +774,7 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
}
res = map_mmio_regions(d,
paddr_to_pfn(addr & PAGE_MASK),
- paddr_to_pfn_aligned(addr + size),
+ DIV_ROUND_UP(size, PAGE_SIZE),
paddr_to_pfn(addr & PAGE_MASK));
if ( res )
{
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 42c1a9c..f4702b4 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -760,22 +760,17 @@ int gicv_setup(struct domain *d)
* The second page is always mapped at +4K irrespective of the
* GIC_64K_STRIDE quirk. The DTB passed to the guest reflects this.
*/
- ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase),
- paddr_to_pfn_aligned(d->arch.vgic.cbase + PAGE_SIZE),
+ ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase), 1,
paddr_to_pfn(gic.vbase));
if (ret)
return ret;
if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + PAGE_SIZE),
- paddr_to_pfn_aligned(d->arch.vgic.cbase +
- (2 * PAGE_SIZE)),
- paddr_to_pfn(gic.vbase + PAGE_SIZE));
+ 2, paddr_to_pfn(gic.vbase + PAGE_SIZE));
else
ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + PAGE_SIZE),
- paddr_to_pfn_aligned(d->arch.vgic.cbase +
- (2 * PAGE_SIZE)),
- paddr_to_pfn(gic.vbase + 16*PAGE_SIZE));
+ 2, paddr_to_pfn(gic.vbase + 16*PAGE_SIZE));
return ret;
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index bde61b7..2d8b78f 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -534,12 +534,12 @@ int p2m_populate_ram(struct domain *d,
int map_mmio_regions(struct domain *d,
unsigned long start_gfn,
- unsigned long end_gfn,
+ unsigned long nr_mfns,
unsigned long mfn)
{
return apply_p2m_changes(d, INSERT,
pfn_to_paddr(start_gfn),
- pfn_to_paddr(end_gfn),
+ pfn_to_paddr(start_gfn + nr_mfns),
pfn_to_paddr(mfn),
MATTR_DEV, p2m_mmio_direct);
}
diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
index 078b020..b65c2c2 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -54,13 +54,11 @@ static int exynos5_init_time(void)
static int exynos5_specific_mapping(struct domain *d)
{
/* Map the chip ID */
- map_mmio_regions(d, paddr_to_pfn(EXYNOS5_PA_CHIPID),
- paddr_to_pfn_aligned(EXYNOS5_PA_CHIPID + PAGE_SIZE),
+ map_mmio_regions(d, paddr_to_pfn(EXYNOS5_PA_CHIPID), 1,
paddr_to_pfn(EXYNOS5_PA_CHIPID));
/* Map the PWM region */
- map_mmio_regions(d, paddr_to_pfn(EXYNOS5_PA_TIMER),
- paddr_to_pfn_aligned(EXYNOS5_PA_TIMER + (PAGE_SIZE * 2)),
+ map_mmio_regions(d, paddr_to_pfn(EXYNOS5_PA_TIMER), 2,
paddr_to_pfn(EXYNOS5_PA_TIMER));
return 0;
diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c
index f51810e..a47c1cc 100644
--- a/xen/arch/arm/platforms/omap5.c
+++ b/xen/arch/arm/platforms/omap5.c
@@ -102,23 +102,19 @@ static int omap5_init_time(void)
static int omap5_specific_mapping(struct domain *d)
{
/* Map the PRM module */
- map_mmio_regions(d, paddr_to_pfn(OMAP5_PRM_BASE),
- paddr_to_pfn_aligned(OMAP5_PRM_BASE + (PAGE_SIZE * 2)),
+ map_mmio_regions(d, paddr_to_pfn(OMAP5_PRM_BASE), 2,
paddr_to_pfn(OMAP5_PRM_BASE));
/* Map the PRM_MPU */
- map_mmio_regions(d, paddr_to_pfn(OMAP5_PRCM_MPU_BASE),
- paddr_to_pfn_aligned(OMAP5_PRCM_MPU_BASE + PAGE_SIZE),
+ map_mmio_regions(d, paddr_to_pfn(OMAP5_PRCM_MPU_BASE), 1,
paddr_to_pfn(OMAP5_PRCM_MPU_BASE));
/* Map the Wakeup Gen */
- map_mmio_regions(d, paddr_to_pfn(OMAP5_WKUPGEN_BASE),
- paddr_to_pfn_aligned(OMAP5_WKUPGEN_BASE + PAGE_SIZE),
+ map_mmio_regions(d, paddr_to_pfn(OMAP5_WKUPGEN_BASE), 1,
paddr_to_pfn(OMAP5_WKUPGEN_BASE));
/* Map the on-chip SRAM */
- map_mmio_regions(d, paddr_to_pfn(OMAP5_SRAM_PA),
- paddr_to_pfn_aligned(OMAP5_SRAM_PA + (PAGE_SIZE * 32)),
+ map_mmio_regions(d, paddr_to_pfn(OMAP5_SRAM_PA), 32,
paddr_to_pfn(OMAP5_SRAM_PA));
return 0;
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index 6ee1444..a8e4487 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -41,15 +41,13 @@ static uint32_t xgene_storm_quirks(void)
}
static int map_one_mmio(struct domain *d, const char *what,
- paddr_t start, paddr_t end)
+ unsigned long start, unsigned long end)
{
int ret;
printk("Additional MMIO %"PRIpaddr"-%"PRIpaddr" (%s)\n",
start, end, what);
- ret = map_mmio_regions(d, paddr_to_pfn(start),
- paddr_to_pfn_aligned(end),
- paddr_to_pfn(start));
+ ret = map_mmio_regions(d, start, end, end - start + 1, start);
if ( ret )
printk("Failed to map %s @ %"PRIpaddr" to dom%d\n",
what, start, d->domain_id);
@@ -86,18 +84,22 @@ static int xgene_storm_specific_mapping(struct domain *d)
int ret;
/* Map the PCIe bus resources */
- ret = map_one_mmio(d, "PCI MEM REGION", 0xe000000000UL, 0xe010000000UL);
+ ret = map_one_mmio(d, "PCI MEM REGION", paddr_to_pfn(0xe000000000UL),
+ paddr_to_pfn(0xe010000000UL));
if ( ret )
goto err;
- ret = map_one_mmio(d, "PCI IO REGION", 0xe080000000UL, 0xe080010000UL);
+ ret = map_one_mmio(d, "PCI IO REGION", paddr_to_pfn(0xe080000000UL),
+ paddr_to_pfn(0xe080010000UL));
if ( ret )
goto err;
- ret = map_one_mmio(d, "PCI CFG REGION", 0xe0d0000000UL, 0xe0d0200000UL);
+ ret = map_one_mmio(d, "PCI CFG REGION", paddr_to_pfn(0xe0d0000000UL),
+ paddr_to_pfn(0xe0d0200000UL));
if ( ret )
goto err;
- ret = map_one_mmio(d, "PCI MSI REGION", 0xe010000000UL, 0xe010800000UL);
+ ret = map_one_mmio(d, "PCI MSI REGION", paddr_to_pfn(0xe010000000UL),
+ paddr_to_pfn(0xe010800000UL));
if ( ret )
goto err;
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index c7dd6aa..6d56daa 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -84,11 +84,12 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn);
/* Setup p2m RAM mapping for domain d from start-end. */
int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end);
-/* Map MMIO regions in the p2m: start_gfn and end_gfn is the range in the guest
- * physical address space to map, starting from the machine frame number mfn. */
+/* Map MMIO regions in the p2m: start_gfn and nr_mfns describe the range
+ * in the guest physical address space to map, starting from the machine
+ * frame number mfn. */
int map_mmio_regions(struct domain *d,
unsigned long start_gfn,
- unsigned long end_gfn,
+ unsigned long nr_mfns,
unsigned long mfn);
int guest_physmap_add_entry(struct domain *d,
--
1.9.2
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v8 05/14] arch/arm: unmap partially-mapped I/O-memory regions
2014-05-25 10:51 [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (3 preceding siblings ...)
2014-05-25 10:51 ` [PATCH v8 04/14] arch/arm: let map_mmio_regions() use start and count Arianna Avanzini
@ 2014-05-25 10:51 ` Arianna Avanzini
2014-05-25 16:04 ` Julien Grall
2014-05-25 10:51 ` [PATCH v8 06/14] arch/x86: warn if to-be-removed mapping does not exist Arianna Avanzini
` (9 subsequent siblings)
14 siblings, 1 reply; 50+ messages in thread
From: Arianna Avanzini @ 2014-05-25 10:51 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
andrew.cooper3, JBeulich, avanzini.arianna, viktor.kleinik
This commit changes the interface of apply_p2m_changes() to accept
optionally the pointer to a counter of successfully performed
mappings; such a counter is used only in case of INSERT operation.
If an error is encountered during the operation, and therefore the
mapping is only partially performed, such a counter is useful to
let the caller be able to undo what has just been done.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
v8:
- Use correct count in unmap_mmio_regions().
v6:
- Pass p2m_invalid as last parameter to unmap_mmio_regions() for ARM
as it is not (and currently must not be) used.
---
xen/arch/arm/p2m.c | 42 +++++++++++++++++++++++++++++++++---------
xen/include/asm-arm/p2m.h | 4 ++++
2 files changed, 37 insertions(+), 9 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 2d8b78f..cf01736 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -294,6 +294,7 @@ static int apply_p2m_changes(struct domain *d,
paddr_t start_gpaddr,
paddr_t end_gpaddr,
paddr_t maddr,
+ unsigned long *nr_inserted,
int mattr,
p2m_type_t t)
{
@@ -304,7 +305,7 @@ static int apply_p2m_changes(struct domain *d,
unsigned long cur_first_page = ~0,
cur_first_offset = ~0,
cur_second_offset = ~0;
- unsigned long count = 0;
+ unsigned long count = 0, inserted = 0;
unsigned int flush = 0;
bool_t populate = (op == INSERT || op == ALLOCATE);
lpae_t pte;
@@ -420,6 +421,7 @@ static int apply_p2m_changes(struct domain *d,
{
pte = mfn_to_p2m_entry(maddr >> PAGE_SHIFT, mattr, t);
write_pte(&third[third_table_offset(addr)], pte);
+ inserted++;
}
break;
case REMOVE:
@@ -519,6 +521,8 @@ out:
if (second) unmap_domain_page(second);
if (first) unmap_domain_page(first);
+ if ( nr_inserted != NULL ) *nr_inserted = inserted;
+
spin_unlock(&p2m->lock);
return rc;
@@ -529,7 +533,7 @@ int p2m_populate_ram(struct domain *d,
paddr_t end)
{
return apply_p2m_changes(d, ALLOCATE, start, end,
- 0, MATTR_MEM, p2m_ram_rw);
+ 0, NULL, MATTR_MEM, p2m_ram_rw);
}
int map_mmio_regions(struct domain *d,
@@ -537,11 +541,31 @@ int map_mmio_regions(struct domain *d,
unsigned long nr_mfns,
unsigned long mfn)
{
- return apply_p2m_changes(d, INSERT,
+ unsigned long nr_inserted;
+ int ret;
+
+ ret = apply_p2m_changes(d, INSERT,
+ pfn_to_paddr(start_gfn),
+ pfn_to_paddr(start_gfn + nr_mfns),
+ pfn_to_paddr(mfn),
+ &nr_inserted,
+ MATTR_DEV, p2m_mmio_direct);
+ if ( ret && nr_inserted != 0 )
+ unmap_mmio_regions(d, start_gfn, nr_inserted, mfn);
+
+ return ret;
+}
+
+int unmap_mmio_regions(struct domain *d,
+ unsigned long start_gfn,
+ unsigned long nr_mfns,
+ unsigned long mfn)
+{
+ return apply_p2m_changes(d, REMOVE,
pfn_to_paddr(start_gfn),
pfn_to_paddr(start_gfn + nr_mfns),
- pfn_to_paddr(mfn),
- MATTR_DEV, p2m_mmio_direct);
+ pfn_to_paddr(mfn), NULL,
+ MATTR_DEV, p2m_invalid);
}
int guest_physmap_add_entry(struct domain *d,
@@ -553,7 +577,7 @@ int guest_physmap_add_entry(struct domain *d,
return apply_p2m_changes(d, INSERT,
pfn_to_paddr(gpfn),
pfn_to_paddr(gpfn + (1 << page_order)),
- pfn_to_paddr(mfn), MATTR_MEM, t);
+ pfn_to_paddr(mfn), NULL, MATTR_MEM, t);
}
void guest_physmap_remove_page(struct domain *d,
@@ -563,7 +587,7 @@ void guest_physmap_remove_page(struct domain *d,
apply_p2m_changes(d, REMOVE,
pfn_to_paddr(gpfn),
pfn_to_paddr(gpfn + (1<<page_order)),
- pfn_to_paddr(mfn), MATTR_MEM, p2m_invalid);
+ pfn_to_paddr(mfn), NULL, MATTR_MEM, p2m_invalid);
}
int p2m_alloc_table(struct domain *d)
@@ -710,7 +734,7 @@ int relinquish_p2m_mapping(struct domain *d)
return apply_p2m_changes(d, RELINQUISH,
pfn_to_paddr(p2m->lowest_mapped_gfn),
pfn_to_paddr(p2m->max_mapped_gfn),
- pfn_to_paddr(INVALID_MFN),
+ pfn_to_paddr(INVALID_MFN), NULL,
MATTR_MEM, p2m_invalid);
}
@@ -725,7 +749,7 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn)
pfn_to_paddr(start_mfn),
pfn_to_paddr(end_mfn),
pfn_to_paddr(INVALID_MFN),
- MATTR_MEM, p2m_invalid);
+ NULL, MATTR_MEM, p2m_invalid);
}
unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn)
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 6d56daa..71665f3 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -91,6 +91,10 @@ int map_mmio_regions(struct domain *d,
unsigned long start_gfn,
unsigned long nr_mfns,
unsigned long mfn);
+int unmap_mmio_regions(struct domain *d,
+ unsigned long start_gfn,
+ unsigned long nr_mfns,
+ unsigned long mfn);
int guest_physmap_add_entry(struct domain *d,
unsigned long gfn,
--
1.9.2
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v8 06/14] arch/x86: warn if to-be-removed mapping does not exist
2014-05-25 10:51 [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (4 preceding siblings ...)
2014-05-25 10:51 ` [PATCH v8 05/14] arch/arm: unmap partially-mapped I/O-memory regions Arianna Avanzini
@ 2014-05-25 10:51 ` Arianna Avanzini
2014-06-05 14:06 ` Ian Campbell
2014-05-25 10:51 ` [PATCH v8 07/14] arch/x86: cleanup memory_mapping DOMCTL Arianna Avanzini
` (8 subsequent siblings)
14 siblings, 1 reply; 50+ messages in thread
From: Arianna Avanzini @ 2014-05-25 10:51 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
andrew.cooper3, JBeulich, avanzini.arianna, viktor.kleinik
Currently, when a memory mapping is removed with the memory_mapping
DOMCTL, no check is performed on the existence of such a mapping.
This commit adds such a consistency check to the code performing the
unmap, emitting a warning message if the check fails.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
v8:
- Fix commit description and subject to better fit the contents of
the patch.
- Remove tentative phrases from commit description.
v7:
- Do not fail with an error when attempting to remove a non-existent
mapping: just emit a warning.
---
xen/arch/x86/domctl.c | 4 ++--
xen/arch/x86/mm/p2m.c | 12 ++++++++----
xen/include/asm-x86/p2m.h | 2 +-
3 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index f8b0a79..2f171ff 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -679,7 +679,7 @@ long arch_do_domctl(
"memory_map:fail: dom%d gfn=%lx mfn=%lx ret:%ld\n",
d->domain_id, gfn + i, mfn + i, ret);
while ( i-- )
- clear_mmio_p2m_entry(d, gfn + i);
+ clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
if ( iomem_deny_access(d, mfn, mfn + nr_mfns - 1) &&
is_hardware_domain(current->domain) )
printk(XENLOG_ERR
@@ -699,7 +699,7 @@ long arch_do_domctl(
if ( paging_mode_translate(d) )
for ( i = 0; i < nr_mfns; i++ )
{
- ret = clear_mmio_p2m_entry(d, gfn + i);
+ ret = clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
if ( ret )
tmp_rc = ret;
}
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b50747a..90359bd 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -848,10 +848,10 @@ int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
}
/* Returns: 0 for success, -errno for failure */
-int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn)
+int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
{
int rc = -EINVAL;
- mfn_t mfn;
+ mfn_t actual_mfn;
p2m_access_t a;
p2m_type_t t;
struct p2m_domain *p2m = p2m_get_hostp2m(d);
@@ -860,15 +860,19 @@ int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn)
return -EIO;
gfn_lock(p2m, gfn, 0);
- mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL);
+ actual_mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL);
/* Do not use mfn_valid() here as it will usually fail for MMIO pages. */
- if ( (INVALID_MFN == mfn_x(mfn)) || (t != p2m_mmio_direct) )
+ if ( (INVALID_MFN == mfn_x(actual_mfn)) || (t != p2m_mmio_direct) )
{
gdprintk(XENLOG_ERR,
"gfn_to_mfn failed! gfn=%08lx type:%d\n", gfn, t);
goto out;
}
+ if ( mfn_x(mfn) != mfn_x(actual_mfn) )
+ gdprintk(XENLOG_WARNING,
+ "no mapping between mfn %08lx and gfn %08lx\n",
+ mfn_x(mfn), gfn);
rc = p2m_set_entry(p2m, gfn, _mfn(INVALID_MFN), PAGE_ORDER_4K, p2m_invalid,
p2m->default_access);
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 027f011..b1163b5 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -530,7 +530,7 @@ int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start,
/* Set mmio addresses in the p2m table (for pass-through) */
int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
-int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn);
+int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
/*
--
1.9.2
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v8 07/14] arch/x86: cleanup memory_mapping DOMCTL
2014-05-25 10:51 [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (5 preceding siblings ...)
2014-05-25 10:51 ` [PATCH v8 06/14] arch/x86: warn if to-be-removed mapping does not exist Arianna Avanzini
@ 2014-05-25 10:51 ` Arianna Avanzini
2014-05-26 9:57 ` Jan Beulich
2014-05-25 10:51 ` [PATCH v8 08/14] xen/common: move memory_type_changed() function to common code Arianna Avanzini
` (7 subsequent siblings)
14 siblings, 1 reply; 50+ messages in thread
From: Arianna Avanzini @ 2014-05-25 10:51 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
andrew.cooper3, JBeulich, avanzini.arianna, viktor.kleinik
This commit lets the end mfn be computed only once while handling a
XEN_DOMCTL_memory_mapping hypercall. Also, the name of the tmp_rc
local variable is changed to rc.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
xen/arch/x86/domctl.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 2f171ff..544418d 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -646,19 +646,20 @@ long arch_do_domctl(
unsigned long gfn = domctl->u.memory_mapping.first_gfn;
unsigned long mfn = domctl->u.memory_mapping.first_mfn;
unsigned long nr_mfns = domctl->u.memory_mapping.nr_mfns;
+ unsigned long mfn_end = mfn + nr_mfns - 1;
int add = domctl->u.memory_mapping.add_mapping;
ret = -EINVAL;
- if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
- ((mfn | (mfn + nr_mfns - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
+ if ( mfn_end < mfn || /* wrap? */
+ ((mfn | mfn_end) >> (paddr_bits - PAGE_SHIFT)) ||
(gfn + nr_mfns - 1) < gfn ) /* wrap? */
break;
ret = -EPERM;
- if ( !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
+ if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
break;
- ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, add);
+ ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
if ( ret )
break;
@@ -668,7 +669,7 @@ long arch_do_domctl(
"memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
d->domain_id, gfn, mfn, nr_mfns);
- ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1);
+ ret = iomem_permit_access(d, mfn, mfn_end);
if ( !ret && paging_mode_translate(d) )
{
for ( i = 0; !ret && i < nr_mfns; i++ )
@@ -680,17 +681,17 @@ long arch_do_domctl(
d->domain_id, gfn + i, mfn + i, ret);
while ( i-- )
clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
- if ( iomem_deny_access(d, mfn, mfn + nr_mfns - 1) &&
+ if ( iomem_deny_access(d, mfn, mfn_end) &&
is_hardware_domain(current->domain) )
printk(XENLOG_ERR
"memory_map: failed to deny dom%d access to [%lx,%lx]\n",
- d->domain_id, mfn, mfn + nr_mfns - 1);
+ d->domain_id, mfn, mfn_end);
}
}
}
else
{
- int tmp_rc = 0;
+ int rc = 0;
printk(XENLOG_G_INFO
"memory_map:remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
@@ -701,16 +702,16 @@ long arch_do_domctl(
{
ret = clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
if ( ret )
- tmp_rc = ret;
+ rc = ret;
}
- ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
+ ret = iomem_deny_access(d, mfn, mfn_end);
if ( !ret )
- ret = tmp_rc;
+ ret = rc;
if ( ret && is_hardware_domain(current->domain) )
printk(XENLOG_ERR
"memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
- ret, tmp_rc ? "removing" : "denying", d->domain_id,
- mfn, mfn + nr_mfns - 1);
+ ret, rc ? "removing" : "denying", d->domain_id,
+ mfn, mfn_end);
}
/* Do this unconditionally to cover errors on above failure paths. */
memory_type_changed(d);
--
1.9.2
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v8 08/14] xen/common: move memory_type_changed() function to common code
2014-05-25 10:51 [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (6 preceding siblings ...)
2014-05-25 10:51 ` [PATCH v8 07/14] arch/x86: cleanup memory_mapping DOMCTL Arianna Avanzini
@ 2014-05-25 10:51 ` Arianna Avanzini
2014-05-25 16:15 ` Julien Grall
2014-05-25 10:51 ` [PATCH v8 09/14] xen/x86: factor out map and unmap from the memory_mapping DOMCTL Arianna Avanzini
` (6 subsequent siblings)
14 siblings, 1 reply; 50+ messages in thread
From: Arianna Avanzini @ 2014-05-25 10:51 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
andrew.cooper3, JBeulich, avanzini.arianna, viktor.kleinik
Currently, MTRR-related code is not available for the ARM architecture.
Given that the memory_type_changed() function must be called also from
common code, its invocation is currently ifdef'd out to be only compiled
in on an x86 machine. This commit adds an empty stub for ARM, instead,
also moving the function's definition to common code.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
xen/arch/arm/Makefile | 1 +
xen/arch/arm/mtrr.c | 6 ++++++
xen/common/domctl.c | 3 +--
xen/include/asm-arm/mtrr.h | 6 ++++++
xen/include/asm-x86/mtrr.h | 2 +-
xen/include/xen/mtrr.h | 8 ++++++++
6 files changed, 23 insertions(+), 3 deletions(-)
create mode 100644 xen/arch/arm/mtrr.c
create mode 100644 xen/include/asm-arm/mtrr.h
create mode 100644 xen/include/xen/mtrr.h
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 63e0460..5428ccc 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -16,6 +16,7 @@ obj-y += irq.o
obj-y += kernel.o
obj-y += mm.o
obj-y += p2m.o
+obj-y += mtrr.o
obj-y += percpu.o
obj-y += guestcopy.o
obj-y += physdev.o
diff --git a/xen/arch/arm/mtrr.c b/xen/arch/arm/mtrr.c
new file mode 100644
index 0000000..e9d52f5
--- /dev/null
+++ b/xen/arch/arm/mtrr.c
@@ -0,0 +1,6 @@
+#include <xen/mtrr.h>
+
+/* XXX: to be implemented */
+void memory_type_changed(struct domain *d)
+{
+}
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 4774277..e666abf 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -24,6 +24,7 @@
#include <xen/bitmap.h>
#include <xen/paging.h>
#include <xen/hypercall.h>
+#include <xen/mtrr.h>
#include <asm/current.h>
#include <asm/irq.h>
#include <asm/page.h>
@@ -818,10 +819,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1);
else
ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
-#ifdef CONFIG_X86
if ( !ret )
memory_type_changed(d);
-#endif
}
break;
diff --git a/xen/include/asm-arm/mtrr.h b/xen/include/asm-arm/mtrr.h
new file mode 100644
index 0000000..3928497
--- /dev/null
+++ b/xen/include/asm-arm/mtrr.h
@@ -0,0 +1,6 @@
+#ifndef __ASM_ARM_MTRR_H__
+#define __ASM_ARM_MTRR_H__
+
+#include <xen/mtrr.h>
+
+#endif /* __ASM_ARM_MTRR_H__ */
diff --git a/xen/include/asm-x86/mtrr.h b/xen/include/asm-x86/mtrr.h
index 328ba04..c7d577e 100644
--- a/xen/include/asm-x86/mtrr.h
+++ b/xen/include/asm-x86/mtrr.h
@@ -2,6 +2,7 @@
#define __ASM_X86_MTRR_H__
#include <xen/config.h>
+#include <xen/mtrr.h>
#include <asm/mm.h>
/* These are the region types. They match the architectural specification. */
@@ -89,7 +90,6 @@ extern bool_t mtrr_fix_range_msr_set(struct domain *, struct mtrr_state *,
uint32_t row, uint64_t msr_content);
extern bool_t mtrr_def_type_msr_set(struct domain *, struct mtrr_state *,
uint64_t msr_content);
-extern void memory_type_changed(struct domain *);
extern bool_t pat_msr_set(uint64_t *pat, uint64_t msr);
bool_t is_var_mtrr_overlapped(struct mtrr_state *m);
diff --git a/xen/include/xen/mtrr.h b/xen/include/xen/mtrr.h
new file mode 100644
index 0000000..9d3f908
--- /dev/null
+++ b/xen/include/xen/mtrr.h
@@ -0,0 +1,8 @@
+#ifndef __MTRR_H__
+#define __MTRR_H__
+
+struct domain;
+
+extern void memory_type_changed(struct domain *);
+
+#endif /* __MTRR_H__ */
--
1.9.2
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v8 09/14] xen/x86: factor out map and unmap from the memory_mapping DOMCTL
2014-05-25 10:51 [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (7 preceding siblings ...)
2014-05-25 10:51 ` [PATCH v8 08/14] xen/common: move memory_type_changed() function to common code Arianna Avanzini
@ 2014-05-25 10:51 ` Arianna Avanzini
2014-05-26 10:04 ` Jan Beulich
2014-05-25 10:51 ` [PATCH v8 10/14] xen/common: move the memory_mapping DOMCTL hypercall to common code Arianna Avanzini
` (5 subsequent siblings)
14 siblings, 1 reply; 50+ messages in thread
From: Arianna Avanzini @ 2014-05-25 10:51 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
andrew.cooper3, JBeulich, avanzini.arianna, viktor.kleinik
This commit factors out from the XEN_DOMCTL_memory_mapping hypercall
implementation, currently available only for x86, the operations
related to memory ranges map and unmap. The code is factored out
into two {map|unmap}_mmio_regions() functions for x86, that will match
the corresponding pair of ARM functions when the DOMCTL will be moved
to common code in the following commit.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
v8:
- Latch the ret variable in {map|unmap}_mmio_regions() to the
first or last errors encountered during mapping or unmapping
operations.
- Separate cleanup-related changes from refactoring ones.
v7:
- Use unmap_mmio_regions() for the error recovery path in
map_mmio_regions().
- Fix wrong use of memory-handling functions.
- Use mfn_t as last parameter to {map|unmap}_mmio_regions().
- Don't split format strings.
- Remove spurious changes, such as those to already-existing format
strings that are not necessary.
v6:
- Fix uncorrect usage of [mg]fn_end which, in the initial checks,
was subtracted 1 twice.
- Remove useless comment about the handling of errors in the
remove path of the hypercall.
- Replace stray hard tab with spaces.
v5:
- Let the unmap_mmio_regions() function for x86 return a proper
error code upon failure.
- Restore correct handling of errors in the remove path of the
hypercall, assigning to the "ret" local variable the error
code returned by the unmap_mmio_regions() function only if
iomem_deny_access() didn't return with an error.
- Compute gfn_end - 1 and mfn_end - 1 only once in the DOMCTL.
- Use a local variable to keep the return value of the function
unmap_mmio_regions() instead of re-using the "add" variable.
- Add a comment to make hopefully clearer how error values of
functions are handled in the remove path of the DOMCTL.
v4:
- Fix type and signedness of local variables used as indexes in
map_mmio_regions() and unmap_mmio_regions() for x86.
- Clear p2m entries in map_mmio_regions() for x86 only if
set_mmio_p2m_entry() returned with an error.
- Make ranges inclusive of the end address in map_mmio_regions()
and unmap_mmio_regions() for x86.
- Turn hard tabs into spaces.
v3:
- Add map_mmio_regions() and unmap_mmio_regions() functions for x86;
- Use pfn as parameters to the unmap_mmio_regions() function.
- Compute gfn + nr_mfns and mfn + nr_mfns only once.
v2:
- Use the already-defined PADDR_BITS constant in the new DOMCTL.
- Use paddr_t as arguments to the map_mmio_regions() function.
- Page-align addresses given as arguments to map_mmio_regions() and
unmap_mmio_regions().
---
xen/arch/x86/domctl.c | 17 ++++-------------
xen/arch/x86/mm/p2m.c | 41 +++++++++++++++++++++++++++++++++++++++++
xen/include/asm-x86/p2m.h | 12 ++++++++++++
3 files changed, 57 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 544418d..f7cde12 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -670,17 +670,14 @@ long arch_do_domctl(
d->domain_id, gfn, mfn, nr_mfns);
ret = iomem_permit_access(d, mfn, mfn_end);
- if ( !ret && paging_mode_translate(d) )
+ if ( !ret )
{
- for ( i = 0; !ret && i < nr_mfns; i++ )
- ret = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
+ ret = map_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
if ( ret )
{
printk(XENLOG_G_WARNING
"memory_map:fail: dom%d gfn=%lx mfn=%lx ret:%ld\n",
- d->domain_id, gfn + i, mfn + i, ret);
- while ( i-- )
- clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
+ d->domain_id, gfn, mfn, ret);
if ( iomem_deny_access(d, mfn, mfn_end) &&
is_hardware_domain(current->domain) )
printk(XENLOG_ERR
@@ -697,13 +694,7 @@ long arch_do_domctl(
"memory_map:remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
d->domain_id, gfn, mfn, nr_mfns);
- if ( paging_mode_translate(d) )
- for ( i = 0; i < nr_mfns; i++ )
- {
- ret = clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
- if ( ret )
- rc = ret;
- }
+ rc = unmap_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
ret = iomem_deny_access(d, mfn, mfn_end);
if ( !ret )
ret = rc;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 90359bd..a3a316c 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1693,6 +1693,47 @@ unsigned long paging_gva_to_gfn(struct vcpu *v,
return hostmode->gva_to_gfn(v, hostp2m, va, pfec);
}
+int map_mmio_regions(struct domain *d,
+ unsigned long start_gfn,
+ unsigned long nr_mfns,
+ mfn_t mfn)
+{
+ int ret = 0;
+ unsigned long i;
+
+ if ( !paging_mode_translate(d) )
+ return 0;
+
+ for ( i = 0; !ret && i < nr_mfns; i++ )
+ {
+ ret = set_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn_x(mfn) + i));
+ if ( ret )
+ {
+ unmap_mmio_regions(d, start_gfn, start_gfn + i, mfn);
+ break;
+ }
+ }
+
+ return ret;
+}
+
+int unmap_mmio_regions(struct domain *d,
+ unsigned long start_gfn,
+ unsigned long nr_mfns,
+ mfn_t mfn)
+{
+ int ret = 0;
+ unsigned long i;
+
+ if ( !paging_mode_translate(d) )
+ return 0;
+
+ for ( i = 0; i < nr_mfns; i++ )
+ ret = clear_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn_x(mfn) + i));
+
+ return ret;
+}
+
/*** Audit ***/
#if P2M_AUDIT
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index b1163b5..5c6c131 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -32,6 +32,18 @@
#include <asm/mem_sharing.h>
#include <asm/page.h> /* for pagetable_t */
+/* Map MMIO regions in the p2m: start_gfn and nr_mfns describe the range
+ * in the guest physical address space to map, starting from the machine
+ * frame number mfn. */
+int map_mmio_regions(struct domain *d,
+ unsigned long start_gfn,
+ unsigned long nr_mfns,
+ mfn_t mfn);
+int unmap_mmio_regions(struct domain *d,
+ unsigned long start_gfn,
+ unsigned long nr_mfns,
+ mfn_t mfn);
+
extern bool_t opt_hap_1gb, opt_hap_2mb;
/*
--
1.9.2
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v8 10/14] xen/common: move the memory_mapping DOMCTL hypercall to common code
2014-05-25 10:51 [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (8 preceding siblings ...)
2014-05-25 10:51 ` [PATCH v8 09/14] xen/x86: factor out map and unmap from the memory_mapping DOMCTL Arianna Avanzini
@ 2014-05-25 10:51 ` Arianna Avanzini
2014-05-25 16:42 ` Julien Grall
2014-05-26 10:06 ` Jan Beulich
2014-05-25 10:51 ` [PATCH v8 11/14] tools/libxl: parse optional start gfn from the iomem config option Arianna Avanzini
` (4 subsequent siblings)
14 siblings, 2 replies; 50+ messages in thread
From: Arianna Avanzini @ 2014-05-25 10:51 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
andrew.cooper3, JBeulich, avanzini.arianna, viktor.kleinik
This commit moves to common code the implementation of the memory_mapping
DOMCTL, currently available only for the x86 architecture. It also adds
some definitions for data types and constants which are used in common
code and are not available for the ARM architecture.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
v7:
- Change the name of tmp_rc to rc. Also, do not uselessly initialize it.
- ifdef out the invocation of memory_type_changed() to be called only
if the architecture is x86 instead of adding an useless empty stub
for ARM.
v6:
- Add an empty definition of the memory_type_changed() function for ARM.
v5:
- Rename new header to p2m-common.h.
v4:
- Use a define for paddr_bits instead of a new variable.
- Define prototypes for common functions map_mmio_regions() and
unmap_mmio_regions() only once in a common header.
v3:
- Add a paddr_bits variable for ARM.
v2:
- Move code to xen/arm/domctl.c.
---
xen/arch/x86/domctl.c | 68 --------------------------------------------
xen/common/domctl.c | 68 ++++++++++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/p2m.h | 17 ++++-------
xen/include/asm-x86/p2m.h | 13 +--------
xen/include/xen/p2m-common.h | 16 +++++++++++
5 files changed, 91 insertions(+), 91 deletions(-)
create mode 100644 xen/include/xen/p2m-common.h
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index f7cde12..dc39592 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -641,74 +641,6 @@ long arch_do_domctl(
}
break;
- case XEN_DOMCTL_memory_mapping:
- {
- unsigned long gfn = domctl->u.memory_mapping.first_gfn;
- unsigned long mfn = domctl->u.memory_mapping.first_mfn;
- unsigned long nr_mfns = domctl->u.memory_mapping.nr_mfns;
- unsigned long mfn_end = mfn + nr_mfns - 1;
- int add = domctl->u.memory_mapping.add_mapping;
-
- ret = -EINVAL;
- if ( mfn_end < mfn || /* wrap? */
- ((mfn | mfn_end) >> (paddr_bits - PAGE_SHIFT)) ||
- (gfn + nr_mfns - 1) < gfn ) /* wrap? */
- break;
-
- ret = -EPERM;
- if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
- break;
-
- ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
- if ( ret )
- break;
-
- if ( add )
- {
- printk(XENLOG_G_INFO
- "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
- d->domain_id, gfn, mfn, nr_mfns);
-
- ret = iomem_permit_access(d, mfn, mfn_end);
- if ( !ret )
- {
- ret = map_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
- if ( ret )
- {
- printk(XENLOG_G_WARNING
- "memory_map:fail: dom%d gfn=%lx mfn=%lx ret:%ld\n",
- d->domain_id, gfn, mfn, ret);
- if ( iomem_deny_access(d, mfn, mfn_end) &&
- is_hardware_domain(current->domain) )
- printk(XENLOG_ERR
- "memory_map: failed to deny dom%d access to [%lx,%lx]\n",
- d->domain_id, mfn, mfn_end);
- }
- }
- }
- else
- {
- int rc = 0;
-
- printk(XENLOG_G_INFO
- "memory_map:remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
- d->domain_id, gfn, mfn, nr_mfns);
-
- rc = unmap_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
- ret = iomem_deny_access(d, mfn, mfn_end);
- if ( !ret )
- ret = rc;
- if ( ret && is_hardware_domain(current->domain) )
- printk(XENLOG_ERR
- "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
- ret, rc ? "removing" : "denying", d->domain_id,
- mfn, mfn_end);
- }
- /* Do this unconditionally to cover errors on above failure paths. */
- memory_type_changed(d);
- }
- break;
-
case XEN_DOMCTL_ioport_mapping:
{
struct hvm_iommu *hd;
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index e666abf..1b246d1 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -824,6 +824,74 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
}
break;
+ case XEN_DOMCTL_memory_mapping:
+ {
+ unsigned long gfn = op->u.memory_mapping.first_gfn;
+ unsigned long mfn = op->u.memory_mapping.first_mfn;
+ unsigned long nr_mfns = op->u.memory_mapping.nr_mfns;
+ unsigned long mfn_end = mfn + nr_mfns - 1;
+ int add = op->u.memory_mapping.add_mapping;
+
+ ret = -EINVAL;
+ if ( mfn_end < mfn || /* wrap? */
+ ((mfn | mfn_end) >> (paddr_bits - PAGE_SHIFT)) ||
+ (gfn + nr_mfns - 1) < gfn ) /* wrap? */
+ break;
+
+ ret = -EPERM;
+ if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
+ break;
+
+ ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
+ if ( ret )
+ break;
+
+ if ( add )
+ {
+ printk(XENLOG_G_INFO
+ "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
+ d->domain_id, gfn, mfn, nr_mfns);
+
+ ret = iomem_permit_access(d, mfn, mfn_end);
+ if ( !ret )
+ {
+ ret = map_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
+ if ( ret )
+ {
+ printk(XENLOG_G_WARNING
+ "memory_map:fail: dom%d gfn=%lx mfn=%lx ret:%ld\n",
+ d->domain_id, gfn, mfn, ret);
+ if ( iomem_deny_access(d, mfn, mfn_end) &&
+ is_hardware_domain(current->domain) )
+ printk(XENLOG_ERR
+ "memory_map: failed to deny dom%d access to [%lx,%lx]\n",
+ d->domain_id, mfn, mfn_end);
+ }
+ }
+ }
+ else
+ {
+ int rc = 0;
+
+ printk(XENLOG_G_INFO
+ "memory_map:remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
+ d->domain_id, gfn, mfn, nr_mfns);
+
+ rc = unmap_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
+ ret = iomem_deny_access(d, mfn, mfn_end);
+ if ( !ret )
+ ret = rc;
+ if ( ret && is_hardware_domain(current->domain) )
+ printk(XENLOG_ERR
+ "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
+ ret, rc ? "removing" : "denying", d->domain_id,
+ mfn, mfn_end);
+ }
+ /* Do this unconditionally to cover errors on above failure paths. */
+ memory_type_changed(d);
+ }
+ break;
+
case XEN_DOMCTL_settimeoffset:
{
domain_set_time_offset(d, op->u.settimeoffset.time_offset_seconds);
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 71665f3..5e135bf 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -3,6 +3,12 @@
#include <xen/mm.h>
+#define mfn_t unsigned long
+#include <xen/p2m-common.h>
+
+#define paddr_bits PADDR_BITS
+#define _mfn(pfn) (pfn)
+
struct domain;
/* Per-p2m-table state */
@@ -84,17 +90,6 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn);
/* Setup p2m RAM mapping for domain d from start-end. */
int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end);
-/* Map MMIO regions in the p2m: start_gfn and nr_mfns describe the range
- * in the guest physical address space to map, starting from the machine
- * frame number mfn. */
-int map_mmio_regions(struct domain *d,
- unsigned long start_gfn,
- unsigned long nr_mfns,
- unsigned long mfn);
-int unmap_mmio_regions(struct domain *d,
- unsigned long start_gfn,
- unsigned long nr_mfns,
- unsigned long mfn);
int guest_physmap_add_entry(struct domain *d,
unsigned long gfn,
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 5c6c131..2a71fbd 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -29,21 +29,10 @@
#include <xen/config.h>
#include <xen/paging.h>
+#include <xen/p2m-common.h>
#include <asm/mem_sharing.h>
#include <asm/page.h> /* for pagetable_t */
-/* Map MMIO regions in the p2m: start_gfn and nr_mfns describe the range
- * in the guest physical address space to map, starting from the machine
- * frame number mfn. */
-int map_mmio_regions(struct domain *d,
- unsigned long start_gfn,
- unsigned long nr_mfns,
- mfn_t mfn);
-int unmap_mmio_regions(struct domain *d,
- unsigned long start_gfn,
- unsigned long nr_mfns,
- mfn_t mfn);
-
extern bool_t opt_hap_1gb, opt_hap_2mb;
/*
diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
new file mode 100644
index 0000000..8760b9d
--- /dev/null
+++ b/xen/include/xen/p2m-common.h
@@ -0,0 +1,16 @@
+#ifndef _XEN_P2M_COMMON_H
+#define _XEN_P2M_COMMON_H
+
+/* Map MMIO regions in the p2m: start_gfn and nr_mfns describe the range
+ * in the guest physical address space to map, starting from the machine
+ * frame number mfn. */
+int map_mmio_regions(struct domain *d,
+ unsigned long start_gfn,
+ unsigned long nr_mfns,
+ mfn_t mfn);
+int unmap_mmio_regions(struct domain *d,
+ unsigned long start_gfn,
+ unsigned long nr_mfns,
+ mfn_t mfn);
+
+#endif /* _XEN_P2M_COMMON_H */
--
1.9.2
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v8 11/14] tools/libxl: parse optional start gfn from the iomem config option
2014-05-25 10:51 [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (9 preceding siblings ...)
2014-05-25 10:51 ` [PATCH v8 10/14] xen/common: move the memory_mapping DOMCTL hypercall to common code Arianna Avanzini
@ 2014-05-25 10:51 ` Arianna Avanzini
2014-05-25 10:51 ` [PATCH v8 12/14] tools/libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
` (3 subsequent siblings)
14 siblings, 0 replies; 50+ messages in thread
From: Arianna Avanzini @ 2014-05-25 10:51 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
andrew.cooper3, JBeulich, avanzini.arianna, viktor.kleinik
Currently, the "iomem" domU config option allows to specify a machine
address range to be mapped to the domU. However, there is no way to
specify the guest address range used for the mapping. This commit
extends the iomem option handling code to parse an additional, optional
parameter: this parameter, if given, specifies the start guest address
used for the mapping; if no start guest address is given, a 1:1 mapping
is performed as default.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Acked-by: Ian Campbell <Ian.Campbell@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@citrix.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
v7:
- Use LIBXL_INVALID_GFN when needed.
- Add to the manpage a note about the mapping defaulting to 1:1 when
the gfn parameter is not specified, and about the option being for
auto-translated guests.
- Remove spurious change.
v6:
- Document what happens if the new "gfn" parameter is omitted
while specifying the "iomem" option in the domain configuration.
- Add LIBXL_INVALID_GFN macro with value "~(uint64_t)0".
- Fix typo in the comment to the LIBXL_HAVE_BUILDINFO_IOMEM_START_GFN
macro.
v5:
- Initialize the gfn field of the iomem structure with the
value "(uint64_t)-1".
- Defer the assignment of the value of "start" to "gfn", if
"gfn" has been initialized to the default value, in libxl.
- Use a local variable to keep the return value of sscanf()
for better code readability.
v4:
- Add definition of a LIBXL_HAVE_BUILDINFO_IOMEM_START_GFN macro
to indicate the availability of the new option.
- Simplify the code parsing the new optional parameter by using
the switch construct on the return value of the sscanf() function
instead of calling the function twice.
- Add a short paragraph to the manpage about the use of the iomem
option to passthrough devices protected by an IOMMU.
- Add comments to the fields of the "iomem" structure.
---
docs/man/xl.cfg.pod.5 | 18 +++++++++++++-----
tools/libxl/libxl.h | 10 ++++++++++
tools/libxl/libxl_create.c | 6 ++++++
tools/libxl/libxl_internal.h | 1 +
tools/libxl/libxl_types.idl | 4 ++++
tools/libxl/xl_cmdimpl.c | 17 ++++++++++-------
6 files changed, 44 insertions(+), 12 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 0ca37bc..f8f9c8d 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -610,12 +610,20 @@ is given in hexadecimal and may either a span e.g. C<2f8-2ff>
It is recommended to use this option only for trusted VMs under
administrator control.
-=item B<iomem=[ "IOMEM_START,NUM_PAGES", "IOMEM_START,NUM_PAGES", ... ]>
+=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN]", "IOMEM_START,NUM_PAGES[@GFN]", ... ]>
-Allow guest to access specific hardware I/O memory pages. B<IOMEM_START>
-is a physical page number. B<NUM_PAGES> is the number
-of pages beginning with B<START_PAGE> to allow access. Both values
-must be given in hexadecimal.
+Allow auto-translated domains to access specific hardware I/O memory pages.
+
+B<IOMEM_START> is a physical page number. B<NUM_PAGES> is the number of pages
+beginning with B<START_PAGE> to allow access. B<GFN> specifies the guest frame
+number where the mapping will start in the domU's address space. If B<GFN> is
+not given, the mapping will be performed using B<IOMEM_START> as a start in the
+domU's address space, therefore performing an 1:1 mapping as default.
+All of these values must be given in hexadecimal.
+
+Note that the IOMMU won't be updated with the mappings specified with this
+option. This option therefore should not be used to passthrough any
+IOMMU-protected device.
It is recommended to use this option only for trusted VMs under
administrator control.
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index c7aa817..e4fd0c0 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -107,6 +107,16 @@
#define LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_ENABLE 1
/*
+ * LIBXL_HAVE_BUILDINFO_IOMEM_START_GFN indicates that it is possible
+ * to specify the start guest frame number used to map a range of I/O
+ * memory machine frame numbers via the 'gfn' field (of type uint64)
+ * of the 'iomem' structure. An array of iomem structures is embedded
+ * in libxl_domain_build_info and used to map the indicated memory
+ * ranges during domain build.
+ */
+#define LIBXL_HAVE_BUILDINFO_IOMEM_START_GFN 1
+
+/*
* libxl ABI compatibility
*
* The only guarantee which libxl makes regarding ABI compatibility
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index d015cf4..6aa630e 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -102,6 +102,8 @@ static int sched_params_valid(libxl__gc *gc,
int libxl__domain_build_info_setdefault(libxl__gc *gc,
libxl_domain_build_info *b_info)
{
+ int i;
+
if (b_info->type != LIBXL_DOMAIN_TYPE_HVM &&
b_info->type != LIBXL_DOMAIN_TYPE_PV)
return ERROR_INVAL;
@@ -212,6 +214,10 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
libxl_defbool_setdefault(&b_info->disable_migrate, false);
+ for (i = 0 ; i < b_info->num_iomem; i++)
+ if (b_info->iomem[i].gfn == LIBXL_INVALID_GFN)
+ b_info->iomem[i].gfn = b_info->iomem[i].start;
+
if (!b_info->event_channels)
b_info->event_channels = 1023;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c2b73c4..2dba844 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -91,6 +91,7 @@
#define LIBXL_PV_EXTRA_MEMORY 1024
#define LIBXL_HVM_EXTRA_MEMORY 2048
#define LIBXL_MIN_DOM0_MEM (128*1024)
+#define LIBXL_INVALID_GFN (~(uint64_t)0)
/* use 0 as the domid of the toolstack domain for now */
#define LIBXL_TOOLSTACK_DOMID 0
#define QEMU_SIGNATURE "DeviceModelRecord0002"
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 52f1aa9..d166a53 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -170,8 +170,12 @@ libxl_ioport_range = Struct("ioport_range", [
])
libxl_iomem_range = Struct("iomem_range", [
+ # start host frame number to be mapped to the guest
("start", uint64),
+ # number of frames to be mapped
("number", uint64),
+ # guest frame number used as a start for the mapping
+ ("gfn", uint64, {'init_val': "LIBXL_INVALID_GFN"}),
])
libxl_vga_interface_info = Struct("vga_interface_info", [
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5195914..4b14c04 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1200,6 +1200,7 @@ static void parse_config_data(const char *config_source,
}
if (!xlu_cfg_get_list(config, "iomem", &iomem, &num_iomem, 0)) {
+ int ret;
b_info->num_iomem = num_iomem;
b_info->iomem = calloc(num_iomem, sizeof(*b_info->iomem));
if (b_info->iomem == NULL) {
@@ -1213,13 +1214,15 @@ static void parse_config_data(const char *config_source,
"xl: Unable to get element %d in iomem list\n", i);
exit(1);
}
- if(sscanf(buf, "%" SCNx64",%" SCNx64,
- &b_info->iomem[i].start,
- &b_info->iomem[i].number)
- != 2) {
- fprintf(stderr,
- "xl: Invalid argument parsing iomem: %s\n", buf);
- exit(1);
+ libxl_iomem_range_init(&b_info->iomem[i]);
+ ret = sscanf(buf, "%" SCNx64",%" SCNx64"@%" SCNx64,
+ &b_info->iomem[i].start,
+ &b_info->iomem[i].number,
+ &b_info->iomem[i].gfn);
+ if (ret < 2) {
+ fprintf(stderr,
+ "xl: Invalid argument parsing iomem: %s\n", buf);
+ exit(1);
}
}
}
--
1.9.2
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v8 12/14] tools/libxl: handle the iomem parameter with the memory_mapping hcall
2014-05-25 10:51 [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (10 preceding siblings ...)
2014-05-25 10:51 ` [PATCH v8 11/14] tools/libxl: parse optional start gfn from the iomem config option Arianna Avanzini
@ 2014-05-25 10:51 ` Arianna Avanzini
2014-05-25 17:04 ` Julien Grall
2014-05-25 10:51 ` [PATCH v8 13/14] tools/libxl: explicitly grant access to needed I/O-memory ranges Arianna Avanzini
` (2 subsequent siblings)
14 siblings, 1 reply; 50+ messages in thread
From: Arianna Avanzini @ 2014-05-25 10:51 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
andrew.cooper3, JBeulich, avanzini.arianna, viktor.kleinik
Currently, the configuration-parsing code concerning the handling of the
iomem parameter only invokes the XEN_DOMCTL_iomem_permission hypercall.
This commit lets the XEN_DOMCTL_memory_mapping hypercall be invoked
after XEN_DOMCTL_iomem_permission when the iomem parameter is parsed
from a domU configuration file, so that the address range can be mapped
to the address space of the domU. The hypercall is invoked only in case
of domains using an auto-translated physmap.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Acked-by: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
v8:
- Fix wrong error message emitted when the memory_mapping DOMCTL is
called by a domain that is not auto-translated.
v7:
- Move the check for an auto-translated physmap to xc to avoid having
to expose the information to libxl.
v4:
- Let the hypercall be called only in case the guest uses an
auto-translated physmap.
v3:
- Add ifdefs to let the hypercall be called only by ARM or x86
HVM guests, with a whitelist approach.
- Remove the NOTE comment.
v2:
- Add a comment explaining outstanding issues.
---
tools/libxc/xc_domain.c | 12 ++++++++++++
tools/libxl/libxl_create.c | 11 +++++++++++
2 files changed, 23 insertions(+)
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 369c3f3..14f9880 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1650,6 +1650,18 @@ int xc_domain_memory_mapping(
uint32_t add_mapping)
{
DECLARE_DOMCTL;
+ xc_dominfo_t info;
+
+ if ( xc_domain_getinfo(xch, domid, 1, &info) != 1 )
+ {
+ PERROR("Could not get info for domain");
+ return -EINVAL;
+ }
+ if ( !xc_core_arch_auto_translated_physmap(&info) )
+ {
+ PERROR("memory_mapping only available for auto-translated domains");
+ return 0;
+ }
domctl.cmd = XEN_DOMCTL_memory_mapping;
domctl.domain = domid;
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 6aa630e..4de0fb2 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1113,6 +1113,17 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
"failed give dom%d access to iomem range %"PRIx64"-%"PRIx64,
domid, io->start, io->start + io->number - 1);
ret = ERROR_FAIL;
+ continue;
+ }
+ ret = xc_domain_memory_mapping(CTX->xch, domid,
+ io->gfn, io->start,
+ io->number, 1);
+ if (ret < 0) {
+ LOGE(ERROR,
+ "failed to map to dom%d iomem range %"PRIx64"-%"PRIx64
+ " to guest address %"PRIx64,
+ domid, io->start, io->start + io->number - 1, io->gfn);
+ ret = ERROR_FAIL;
}
}
--
1.9.2
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v8 13/14] tools/libxl: explicitly grant access to needed I/O-memory ranges
2014-05-25 10:51 [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (11 preceding siblings ...)
2014-05-25 10:51 ` [PATCH v8 12/14] tools/libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
@ 2014-05-25 10:51 ` Arianna Avanzini
2014-05-25 17:08 ` Julien Grall
2014-05-26 10:10 ` Jan Beulich
2014-05-25 10:51 ` [PATCH v8 14/14] xen/common: do not implicitly permit access to mapped I/O memory Arianna Avanzini
2014-07-01 10:45 ` [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Julien Grall
14 siblings, 2 replies; 50+ messages in thread
From: Arianna Avanzini @ 2014-05-25 10:51 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
andrew.cooper3, JBeulich, avanzini.arianna, viktor.kleinik
This commit changes the existing libxl code to be sure to grant access
permission to PCI-related I/O memory ranges, while setting up passthrough
of PCI devices specified in the domain's configuration, and to VGA-related
memory ranges, while setting up VGA passthrough (if gfx_passthru = 1 in
the domain's configuration).
As for the latter, the newly-added code does not replace any existing one,
but instead matches the calls to xc_domain_memory_mapping() performed by
QEMU on the path that is executed if gfx passthru is enabled and follows
the registration of a new VGA controller (in register_vga_regions(),
defined in hw/pt-graphics.c). In fact, VGA needs some extra memory
ranges to be mapped with respect to PCI; QEMU expects that access to those
memory ranges is implicitly granted when he calls the hypervisor with the
function xc_domain_memory_mapping(): this commit calls iomem_permission
for it when needed.
This commit is instrumental to the following one, which will separate
the functions of the iomem_permission and memory_mapping DOMCTLs, so
that requesting an I/O-memory range will not imply that access to such a
range is implicitly granted.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
v8:
- Explain better in the commit description VGA-related code additions.
- Fix v6 changelog which, being too generic, ended up to uncorrectly
state that one of the requests had been addressed.
- Remove tentative phrases from commit description.
v7:
- Let libxl not handle I/O ports and I/O memory differently when access
to a PCI device is allowed to a domain.
- Change the construct used by libxl during PCI-related initialization
from a switch to an if to better suit the new execution flow.
---
tools/libxl/libxl_create.c | 17 +++++++++++++++++
tools/libxl/libxl_pci.c | 26 ++++++++++----------------
2 files changed, 27 insertions(+), 16 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 4de0fb2..e544bbf 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1149,6 +1149,23 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
libxl__spawn_stub_dm(egc, &dcs->dmss);
else
libxl__spawn_local_dm(egc, &dcs->dmss.dm);
+
+ /*
+ * If VGA passthru is enabled by domain config, be sure that the
+ * domain can access VGA-related iomem regions.
+ */
+ if (d_config->b_info.u.hvm.gfx_passthru.val) {
+ uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT;
+ ret = xc_domain_iomem_permission(CTX->xch, domid,
+ vga_iomem_start, 0x20, 1);
+ if (ret < 0) {
+ LOGE(ERROR,
+ "failed to give dom%d access to iomem range "
+ "%"PRIx64"-%"PRIx64" for VGA passthru",
+ domid, vga_iomem_start, (vga_iomem_start + 0x20 - 1));
+ goto error_out;
+ }
+ }
return;
}
case LIBXL_DOMAIN_TYPE_PV:
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 44d0453..032e981 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -846,10 +846,13 @@ static int qemu_pci_add_xenstore(libxl__gc *gc, uint32_t domid,
static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
+ libxl_domain_type type = libxl__domain_type(gc, domid);
int rc, hvm = 0;
- switch (libxl__domain_type(gc, domid)) {
- case LIBXL_DOMAIN_TYPE_HVM:
+ if (type == LIBXL_DOMAIN_TYPE_INVALID)
+ return ERROR_FAIL;
+
+ if (type == LIBXL_DOMAIN_TYPE_HVM) {
hvm = 1;
if (libxl__wait_for_device_model_deprecated(gc, domid, "running",
NULL, NULL, NULL) < 0) {
@@ -867,8 +870,7 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, i
}
if ( rc )
return ERROR_FAIL;
- break;
- case LIBXL_DOMAIN_TYPE_PV:
+ }
{
char *sysfs_path = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/resource", pcidev->domain,
pcidev->bus, pcidev->dev, pcidev->func);
@@ -937,10 +939,6 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, i
return ERROR_FAIL;
}
}
- break;
- }
- case LIBXL_DOMAIN_TYPE_INVALID:
- return ERROR_FAIL;
}
out:
if (!libxl_is_stubdom(ctx, domid, NULL)) {
@@ -1166,6 +1164,7 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid,
{
libxl_ctx *ctx = libxl__gc_owner(gc);
libxl_device_pci *assigned;
+ libxl_domain_type type = libxl__domain_type(gc, domid);
int hvm = 0, rc, num;
int stubdomid = 0;
@@ -1181,8 +1180,7 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid,
}
rc = ERROR_FAIL;
- switch (libxl__domain_type(gc, domid)) {
- case LIBXL_DOMAIN_TYPE_HVM:
+ if (type == LIBXL_DOMAIN_TYPE_HVM) {
hvm = 1;
if (libxl__wait_for_device_model_deprecated(gc, domid, "running",
NULL, NULL, NULL) < 0)
@@ -1203,8 +1201,8 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid,
rc = ERROR_FAIL;
goto out_fail;
}
- break;
- case LIBXL_DOMAIN_TYPE_PV:
+ } else if (type != LIBXL_DOMAIN_TYPE_PV)
+ abort();
{
char *sysfs_path = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/resource", pcidev->domain,
pcidev->bus, pcidev->dev, pcidev->func);
@@ -1254,10 +1252,6 @@ skip1:
}
}
fclose(f);
- break;
- }
- default:
- abort();
}
out:
/* don't do multiple resets while some functions are still passed through */
--
1.9.2
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v8 14/14] xen/common: do not implicitly permit access to mapped I/O memory
2014-05-25 10:51 [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (12 preceding siblings ...)
2014-05-25 10:51 ` [PATCH v8 13/14] tools/libxl: explicitly grant access to needed I/O-memory ranges Arianna Avanzini
@ 2014-05-25 10:51 ` Arianna Avanzini
2014-07-01 10:45 ` [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Julien Grall
14 siblings, 0 replies; 50+ messages in thread
From: Arianna Avanzini @ 2014-05-25 10:51 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
andrew.cooper3, JBeulich, avanzini.arianna, viktor.kleinik
Currently, the XEN_DOMCTL_memory_mapping hypercall implicitly grants
to a domain access permission to the I/O memory areas mapped in its
guest address space. This conflicts with the presence of a specific
hypercall (XEN_DOMCTL_iomem_permission) used to grant such a permission
to a domain.
This commit separates the functions of the two hypercalls by having only
the latter be able to permit I/O memory access to a domain, and the former
just performing the mapping after a permissions check on both the granting
and the grantee domains.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
v8:
- Drop iomem_permission-related changes.
- Conservatively check both the granting and the grantee domains'
permissions in the memory_mapping DOMCTL.
- Remove tentative phrases from commit description.
v7:
- Let iomem_permission check if the calling domain is allowed to access
memory ranges to be mapped to a domain. Remove such a check from the
memory_mapping hypercall.
---
xen/common/domctl.c | 36 ++++++++++--------------------------
1 file changed, 10 insertions(+), 26 deletions(-)
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 1b246d1..bf38242 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -839,7 +839,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
break;
ret = -EPERM;
- if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
+ if ( !iomem_access_permitted(current->domain, mfn, mfn_end) ||
+ !iomem_access_permitted(d, mfn, mfn_end) )
break;
ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
@@ -852,40 +853,23 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
"memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
d->domain_id, gfn, mfn, nr_mfns);
- ret = iomem_permit_access(d, mfn, mfn_end);
- if ( !ret )
- {
- ret = map_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
- if ( ret )
- {
- printk(XENLOG_G_WARNING
- "memory_map:fail: dom%d gfn=%lx mfn=%lx ret:%ld\n",
- d->domain_id, gfn, mfn, ret);
- if ( iomem_deny_access(d, mfn, mfn_end) &&
- is_hardware_domain(current->domain) )
- printk(XENLOG_ERR
- "memory_map: failed to deny dom%d access to [%lx,%lx]\n",
- d->domain_id, mfn, mfn_end);
- }
- }
+ ret = map_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
+ if ( ret )
+ printk(XENLOG_G_WARNING
+ "memory_map:fail: dom%d gfn=%lx mfn=%lx ret:%ld\n",
+ d->domain_id, gfn, mfn, ret);
}
else
{
- int rc = 0;
-
printk(XENLOG_G_INFO
"memory_map:remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
d->domain_id, gfn, mfn, nr_mfns);
- rc = unmap_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
- ret = iomem_deny_access(d, mfn, mfn_end);
- if ( !ret )
- ret = rc;
+ ret = unmap_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
if ( ret && is_hardware_domain(current->domain) )
printk(XENLOG_ERR
- "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
- ret, rc ? "removing" : "denying", d->domain_id,
- mfn, mfn_end);
+ "memory_map: error %ld removing dom%d access to [%lx,%lx]\n",
+ ret, d->domain_id, mfn, mfn_end);
}
/* Do this unconditionally to cover errors on above failure paths. */
memory_type_changed(d);
--
1.9.2
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH v8 02/14] arch/arm: add consistency check to REMOVE p2m changes
2014-05-25 10:51 ` [PATCH v8 02/14] arch/arm: add consistency check to REMOVE p2m changes Arianna Avanzini
@ 2014-05-25 15:50 ` Julien Grall
2014-06-05 13:45 ` Ian Campbell
2014-06-05 13:50 ` Ian Campbell
1 sibling, 1 reply; 50+ messages in thread
From: Julien Grall @ 2014-05-25 15:50 UTC (permalink / raw)
To: Arianna Avanzini, xen-devel
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, Ian.Campbell, etrudeau, JBeulich,
andrew.cooper3, viktor.kleinik
Hi Arianna,
On 25/05/14 11:51, Arianna Avanzini wrote:
> + unsigned long mfn = pte.p2m.base;
> +
> + /*
> + * Ensure that the guest address given as argument to
> + * this function is actually mapped to the specified
> + * machine address. maddr here is the machine address
> + * given to the function, while mfn is the machine
> + * frame number actually mapped to the guest address:
> + * check if the two correspond.
> + */
> + if ( !pte.p2m.valid || maddr != pfn_to_paddr(mfn) )
> + {
> + printk("p2m_remove: nonexistent mapping: "
> + "%"PRIx64" and %"PRIx64"\n",
> + pfn_to_paddr(mfn), maddr);
> + /*
> + * If we have successfully removed other mappings,
> + * overload flush local variable to store if we need
> + * to flush TLBs.
> + */
> + if (count) flush = 1; else flush = 0;
Hrrm, why do you need this line? Flush is already correctly set
previously (see flush |= ... few lines above).
> + rc = -EINVAL;
> + goto out_flush;
As I said on a previous mail, you should continue to unmap even if we
fail to remove one mapping. Otherwise, we may let the guest access to
some MMIO region by mistake.
> + }
> + }
> + /* fall through */
> + case RELINQUISH:
> + {
> if ( !pte.p2m.valid )
> {
> count++;
> @@ -462,28 +490,30 @@ static int apply_p2m_changes(struct domain *d,
>
> /* Got the next page */
> addr += PAGE_SIZE;
> + maddr += PAGE_SIZE;
> }
>
> - if ( flush )
> + if ( op == ALLOCATE || op == INSERT )
> {
> unsigned long sgfn = paddr_to_pfn(start_gpaddr);
> unsigned long egfn = paddr_to_pfn(end_gpaddr);
>
> - flush_tlb_domain(d);
> - iommu_iotlb_flush(d, sgfn, egfn - sgfn);
> + p2m->max_mapped_gfn = MAX(p2m->max_mapped_gfn, egfn);
> + p2m->lowest_mapped_gfn = MIN(p2m->lowest_mapped_gfn, sgfn);
> }
>
> - if ( op == ALLOCATE || op == INSERT )
> + rc = 0;
> +
> +out_flush:
There is no need to create a new label. You can move the label "out"
here and avoid to invert the position of the 2 if. It doesn't harm to
update p2m->max_mapped_gfn and p2m->lowest_mapped_gfn and/or flush TLBs
if we fail.
It could also be safeguard for later :).
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 04/14] arch/arm: let map_mmio_regions() use start and count
2014-05-25 10:51 ` [PATCH v8 04/14] arch/arm: let map_mmio_regions() use start and count Arianna Avanzini
@ 2014-05-25 15:56 ` Julien Grall
2014-06-05 13:53 ` Ian Campbell
0 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2014-05-25 15:56 UTC (permalink / raw)
To: Arianna Avanzini, xen-devel
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, Ian.Campbell, etrudeau, JBeulich,
andrew.cooper3, viktor.kleinik
Hi Arianna,
On 25/05/14 11:51, Arianna Avanzini wrote:
> Currently, the arguments given to the function map_mmio_regions() to
> describe the memory range to be mapped are the start and end page frame
> numbers of the range to be mapped. However, this could give rise to
> issues due to the range being inclusive or exclusive of the end gfn
> given as parameter. This commit changes the interface of the function
> to accept the start gfn and the number of gfns to be mapped.
> This commit also changes the interface of the function map_one_mmio(),
> helper for the xgene-storm platform, which is a wrapper for the function
> map_mmio_regions() and does not need its arguments to be paddr_t.
>
> NOTE: platform-specific code has not been tested.
FYI, I'm able to run Xen with your serie on the Arndale (an exynos5
platform). So I believe there is not issue on this platform :).
Acked-by: Julien Grall <julien.grall@linaro.org>
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 05/14] arch/arm: unmap partially-mapped I/O-memory regions
2014-05-25 10:51 ` [PATCH v8 05/14] arch/arm: unmap partially-mapped I/O-memory regions Arianna Avanzini
@ 2014-05-25 16:04 ` Julien Grall
2014-06-05 14:03 ` Ian Campbell
0 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2014-05-25 16:04 UTC (permalink / raw)
To: Arianna Avanzini, xen-devel
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, Ian.Campbell, etrudeau, JBeulich,
andrew.cooper3, viktor.kleinik
Hi Arianna,
On 25/05/14 11:51, Arianna Avanzini wrote:
> This commit changes the interface of apply_p2m_changes() to accept
> optionally the pointer to a counter of successfully performed
> mappings; such a counter is used only in case of INSERT operation.
> If an error is encountered during the operation, and therefore the
> mapping is only partially performed, such a counter is useful to
> let the caller be able to undo what has just been done.
I don't think you need to introduce another argument to
apply_p2m_changes. You can directly call apply_p2m_changes with the
whole range.
> +int unmap_mmio_regions(struct domain *d,
> + unsigned long start_gfn,
> + unsigned long nr_mfns,
> + unsigned long mfn);
The commit message doesn't mention anything about the addition of
unmap_mmio_regions as a new interface.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 08/14] xen/common: move memory_type_changed() function to common code
2014-05-25 10:51 ` [PATCH v8 08/14] xen/common: move memory_type_changed() function to common code Arianna Avanzini
@ 2014-05-25 16:15 ` Julien Grall
2014-05-26 9:58 ` Jan Beulich
0 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2014-05-25 16:15 UTC (permalink / raw)
To: Arianna Avanzini, xen-devel
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, Ian.Campbell, etrudeau, JBeulich,
andrew.cooper3, viktor.kleinik
Hi Arianna,
On 25/05/14 11:51, Arianna Avanzini wrote:
> Currently, MTRR-related code is not available for the ARM architecture.
> Given that the memory_type_changed() function must be called also from
> common code, its invocation is currently ifdef'd out to be only compiled
> in on an x86 machine. This commit adds an empty stub for ARM, instead,
> also moving the function's definition to common code.
Hrmmm, MTRR doesn't have any sense on ARM, IIRC, this is only
x86-related. So we should not expose this concept (i.e naming the
include and source file) to the common architecture.
I would move the function declaration in xen/p2m*.h and add the stub for
arm in arch/arm/p2m.c
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 10/14] xen/common: move the memory_mapping DOMCTL hypercall to common code
2014-05-25 10:51 ` [PATCH v8 10/14] xen/common: move the memory_mapping DOMCTL hypercall to common code Arianna Avanzini
@ 2014-05-25 16:42 ` Julien Grall
2014-05-26 10:07 ` Jan Beulich
2014-05-26 10:06 ` Jan Beulich
1 sibling, 1 reply; 50+ messages in thread
From: Julien Grall @ 2014-05-25 16:42 UTC (permalink / raw)
To: Arianna Avanzini, xen-devel
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, Ian.Campbell, etrudeau, JBeulich,
andrew.cooper3, viktor.kleinik
Hi Arianna,
On 25/05/14 11:51, Arianna Avanzini wrote:
> +#define mfn_t unsigned long
You should use typedef rather than define a type with a define...
> +#include <xen/p2m-common.h>
> +
That would make more sense to include asm/p2m.h in xen/p2m-common.h
rather than the invert.
This will avoid issue such as, a source file is trying to include
xen/p2m-common.h directly without adding asm/p2m.h before. This will
result to a compilation issue.
Anyway, if the maintainers are fine with this solution, let's stick
> /*
> diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
> new file mode 100644
> index 0000000..8760b9d
> --- /dev/null
> +++ b/xen/include/xen/p2m-common.h
> @@ -0,0 +1,16 @@
> +#ifndef _XEN_P2M_COMMON_H
> +#define _XEN_P2M_COMMON_H
> +
> +/* Map MMIO regions in the p2m: start_gfn and nr_mfns describe the range
> + * in the guest physical address space to map, starting from the machine
> + * frame number mfn. */
> +int map_mmio_regions(struct domain *d,
> + unsigned long start_gfn,
> + unsigned long nr_mfns,
> + mfn_t mfn);
> +int unmap_mmio_regions(struct domain *d,
> + unsigned long start_gfn,
> + unsigned long nr_mfns,
> + mfn_t mfn);
I don't like the idea to expose mfn_t and _mfn to the common code. All
the other interfaces (i.e guest_physmap_*) are using unsigned long here.
Why shouldn't we use the same thing here?
Hence, this is not consistent with the other P2M interface.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 12/14] tools/libxl: handle the iomem parameter with the memory_mapping hcall
2014-05-25 10:51 ` [PATCH v8 12/14] tools/libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
@ 2014-05-25 17:04 ` Julien Grall
2014-06-05 14:27 ` Ian Campbell
0 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2014-05-25 17:04 UTC (permalink / raw)
To: Arianna Avanzini, xen-devel
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, Ian.Campbell, etrudeau, JBeulich,
andrew.cooper3, viktor.kleinik
Hi Arianna,
On 25/05/14 11:51, Arianna Avanzini wrote:
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index 369c3f3..14f9880 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -1650,6 +1650,18 @@ int xc_domain_memory_mapping(
> uint32_t add_mapping)
> {
> DECLARE_DOMCTL;
> + xc_dominfo_t info;
> +
> + if ( xc_domain_getinfo(xch, domid, 1, &info) != 1 )
> + {
You may retrieve the wrong domain here. xc_domain_getinfo return the
information of the first domain with an ID >= domid (see
XEN_DOMCTL_getdomaininfo in xen/common/domctl.c).
> + PERROR("Could not get info for domain");
> + return -EINVAL;
> + }
> + if ( !xc_core_arch_auto_translated_physmap(&info) )
> + {
> + PERROR("memory_mapping only available for auto-translated domains");
PERROR doesn't make sense here. Errno is not set by xc_core_arch_auto_*
Futhermore, I won't add any error message here. Otherwise, we may have
spurious error log for PV with libxl (you are calling this function
unconditionally in the toolstack).
> + return 0;
> + }
>
> domctl.cmd = XEN_DOMCTL_memory_mapping;
> domctl.domain = domid;
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 6aa630e..4de0fb2 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -1113,6 +1113,17 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
> "failed give dom%d access to iomem range %"PRIx64"-%"PRIx64,
> domid, io->start, io->start + io->number - 1);
> ret = ERROR_FAIL;
> + continue;
It should be a "goto error_out" rather than a continue here.
I plan to send a patch fixing the other loop (see
http://lists.xen.org/archives/html/xen-devel/2014-05/msg01831.html), so
I'm fine if you keep as it is this part.
> + }
> + ret = xc_domain_memory_mapping(CTX->xch, domid,
> + io->gfn, io->start,
> + io->number, 1);
> + if (ret < 0) {
> + LOGE(ERROR,
> + "failed to map to dom%d iomem range %"PRIx64"-%"PRIx64
> + " to guest address %"PRIx64,
> + domid, io->start, io->start + io->number - 1, io->gfn);
> + ret = ERROR_FAIL;
Same remark here.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 13/14] tools/libxl: explicitly grant access to needed I/O-memory ranges
2014-05-25 10:51 ` [PATCH v8 13/14] tools/libxl: explicitly grant access to needed I/O-memory ranges Arianna Avanzini
@ 2014-05-25 17:08 ` Julien Grall
2014-05-26 10:11 ` Jan Beulich
2014-05-26 10:10 ` Jan Beulich
1 sibling, 1 reply; 50+ messages in thread
From: Julien Grall @ 2014-05-25 17:08 UTC (permalink / raw)
To: Arianna Avanzini, xen-devel
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, Ian.Campbell, etrudeau, JBeulich,
andrew.cooper3, viktor.kleinik
Hi Arianna,
On 25/05/14 11:51, Arianna Avanzini wrote:
> +
> + /*
> + * If VGA passthru is enabled by domain config, be sure that the
> + * domain can access VGA-related iomem regions.
> + */
> + if (d_config->b_info.u.hvm.gfx_passthru.val) {
> + uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT;
> + ret = xc_domain_iomem_permission(CTX->xch, domid,
> + vga_iomem_start, 0x20, 1);
> + if (ret < 0) {
> + LOGE(ERROR,
> + "failed to give dom%d access to iomem range "
> + "%"PRIx64"-%"PRIx64" for VGA passthru",
> + domid, vga_iomem_start, (vga_iomem_start + 0x20 - 1));
> + goto error_out;
> + }
> + }
IHMO, the guest doesn't need to have permission to this region. When
QEMU ask to map this region to the guest, the hypercall will only check
the permission on the domain where QEMU is running. Therefore, the
permission should be given to the stubdomain.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 07/14] arch/x86: cleanup memory_mapping DOMCTL
2014-05-25 10:51 ` [PATCH v8 07/14] arch/x86: cleanup memory_mapping DOMCTL Arianna Avanzini
@ 2014-05-26 9:57 ` Jan Beulich
0 siblings, 0 replies; 50+ messages in thread
From: Jan Beulich @ 2014-05-26 9:57 UTC (permalink / raw)
To: Arianna Avanzini
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Ian.Jackson, xen-devel,
julien.grall, etrudeau, tim, viktor.kleinik
>>> On 25.05.14 at 12:51, <avanzini.arianna@gmail.com> wrote:
> This commit lets the end mfn be computed only once while handling a
> XEN_DOMCTL_memory_mapping hypercall. Also, the name of the tmp_rc
> local variable is changed to rc.
>
> Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
despite not being certain this is a worthwhile change.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 08/14] xen/common: move memory_type_changed() function to common code
2014-05-25 16:15 ` Julien Grall
@ 2014-05-26 9:58 ` Jan Beulich
2014-06-05 14:08 ` Ian Campbell
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-05-26 9:58 UTC (permalink / raw)
To: Arianna Avanzini, Julien Grall, xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Ian.Jackson, tim, julien.grall,
etrudeau, viktor.kleinik
>>> On 25.05.14 at 18:15, <julien.grall@linaro.org> wrote:
> On 25/05/14 11:51, Arianna Avanzini wrote:
>> Currently, MTRR-related code is not available for the ARM architecture.
>> Given that the memory_type_changed() function must be called also from
>> common code, its invocation is currently ifdef'd out to be only compiled
>> in on an x86 machine. This commit adds an empty stub for ARM, instead,
>> also moving the function's definition to common code.
>
> Hrmmm, MTRR doesn't have any sense on ARM, IIRC, this is only
> x86-related. So we should not expose this concept (i.e naming the
> include and source file) to the common architecture.
>
> I would move the function declaration in xen/p2m*.h and add the stub for
> arm in arch/arm/p2m.c
Seconded.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 09/14] xen/x86: factor out map and unmap from the memory_mapping DOMCTL
2014-05-25 10:51 ` [PATCH v8 09/14] xen/x86: factor out map and unmap from the memory_mapping DOMCTL Arianna Avanzini
@ 2014-05-26 10:04 ` Jan Beulich
0 siblings, 0 replies; 50+ messages in thread
From: Jan Beulich @ 2014-05-26 10:04 UTC (permalink / raw)
To: Arianna Avanzini
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Ian.Jackson, xen-devel,
julien.grall, etrudeau, tim, viktor.kleinik
>>> On 25.05.14 at 12:51, <avanzini.arianna@gmail.com> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -670,17 +670,14 @@ long arch_do_domctl(
> d->domain_id, gfn, mfn, nr_mfns);
>
> ret = iomem_permit_access(d, mfn, mfn_end);
> - if ( !ret && paging_mode_translate(d) )
> + if ( !ret )
> {
> - for ( i = 0; !ret && i < nr_mfns; i++ )
> - ret = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
> + ret = map_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
> if ( ret )
> {
> printk(XENLOG_G_WARNING
> "memory_map:fail: dom%d gfn=%lx mfn=%lx ret:%ld\n",
> - d->domain_id, gfn + i, mfn + i, ret);
> - while ( i-- )
> - clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
> + d->domain_id, gfn, mfn, ret);
I'm not sure how useful retaining this message is now that doesn't
indicated the precise GFN/MFN anymore. At the very least you
should make this explicit by also printing nr_mfns.
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1693,6 +1693,47 @@ unsigned long paging_gva_to_gfn(struct vcpu *v,
> return hostmode->gva_to_gfn(v, hostp2m, va, pfec);
> }
>
> +int map_mmio_regions(struct domain *d,
> + unsigned long start_gfn,
> + unsigned long nr_mfns,
> + mfn_t mfn)
> +{
> + int ret = 0;
> + unsigned long i;
> +
> + if ( !paging_mode_translate(d) )
> + return 0;
> +
> + for ( i = 0; !ret && i < nr_mfns; i++ )
> + {
> + ret = set_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn_x(mfn) + i));
> + if ( ret )
> + {
> + unmap_mmio_regions(d, start_gfn, start_gfn + i, mfn);
Wrong 3rd argument (should be just i).
> +int unmap_mmio_regions(struct domain *d,
> + unsigned long start_gfn,
> + unsigned long nr_mfns,
> + mfn_t mfn)
> +{
> + int ret = 0;
> + unsigned long i;
> +
> + if ( !paging_mode_translate(d) )
> + return 0;
> +
> + for ( i = 0; i < nr_mfns; i++ )
> + ret = clear_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn_x(mfn) + i));
> +
> + return ret;
You're still discarding an eventual error here if the last operation
succeeds.
Also, now that these two are standalone functions, I think it would
make sense to rename "nr_mfns" to just "nr", as the value
expresses both the number of MFNs and GFNs.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 10/14] xen/common: move the memory_mapping DOMCTL hypercall to common code
2014-05-25 10:51 ` [PATCH v8 10/14] xen/common: move the memory_mapping DOMCTL hypercall to common code Arianna Avanzini
2014-05-25 16:42 ` Julien Grall
@ 2014-05-26 10:06 ` Jan Beulich
1 sibling, 0 replies; 50+ messages in thread
From: Jan Beulich @ 2014-05-26 10:06 UTC (permalink / raw)
To: Arianna Avanzini
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Ian.Jackson, xen-devel,
julien.grall, etrudeau, tim, viktor.kleinik
>>> On 25.05.14 at 12:51, <avanzini.arianna@gmail.com> wrote:
> This commit moves to common code the implementation of the memory_mapping
> DOMCTL, currently available only for the x86 architecture. It also adds
> some definitions for data types and constants which are used in common
> code and are not available for the ARM architecture.
>
> Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
On the grounds that the bulk of this is indeed just code motion,
Acked-by: Jan Beulich <jbeulich@suse.com>
for all but the ARM parts of it.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 10/14] xen/common: move the memory_mapping DOMCTL hypercall to common code
2014-05-25 16:42 ` Julien Grall
@ 2014-05-26 10:07 ` Jan Beulich
2014-05-26 11:03 ` Julien Grall
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-05-26 10:07 UTC (permalink / raw)
To: Arianna Avanzini, Julien Grall
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Ian.Jackson, xen-devel,
julien.grall, etrudeau, tim, viktor.kleinik
>>> On 25.05.14 at 18:42, <julien.grall@linaro.org> wrote:
> On 25/05/14 11:51, Arianna Avanzini wrote:
>> +#include <xen/p2m-common.h>
>> +
>
> That would make more sense to include asm/p2m.h in xen/p2m-common.h
> rather than the invert.
>
> This will avoid issue such as, a source file is trying to include
> xen/p2m-common.h directly without adding asm/p2m.h before. This will
> result to a compilation issue.
>
> Anyway, if the maintainers are fine with this solution, let's stick
I actually asked for it to be done this way.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 13/14] tools/libxl: explicitly grant access to needed I/O-memory ranges
2014-05-25 10:51 ` [PATCH v8 13/14] tools/libxl: explicitly grant access to needed I/O-memory ranges Arianna Avanzini
2014-05-25 17:08 ` Julien Grall
@ 2014-05-26 10:10 ` Jan Beulich
1 sibling, 0 replies; 50+ messages in thread
From: Jan Beulich @ 2014-05-26 10:10 UTC (permalink / raw)
To: Arianna Avanzini
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Ian.Jackson, xen-devel,
julien.grall, etrudeau, tim, viktor.kleinik
>>> On 25.05.14 at 12:51, <avanzini.arianna@gmail.com> wrote:
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -1149,6 +1149,23 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
> libxl__spawn_stub_dm(egc, &dcs->dmss);
> else
> libxl__spawn_local_dm(egc, &dcs->dmss.dm);
> +
> + /*
> + * If VGA passthru is enabled by domain config, be sure that the
> + * domain can access VGA-related iomem regions.
> + */
> + if (d_config->b_info.u.hvm.gfx_passthru.val) {
> + uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT;
> + ret = xc_domain_iomem_permission(CTX->xch, domid,
> + vga_iomem_start, 0x20, 1);
> + if (ret < 0) {
> + LOGE(ERROR,
> + "failed to give dom%d access to iomem range "
> + "%"PRIx64"-%"PRIx64" for VGA passthru",
> + domid, vga_iomem_start, (vga_iomem_start + 0x20 - 1));
> + goto error_out;
> + }
> + }
I continue to see a problem with this when more than one GFX card
would get passed through - every guest getting one would get
control over the VGA region, which can't be right.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 13/14] tools/libxl: explicitly grant access to needed I/O-memory ranges
2014-05-25 17:08 ` Julien Grall
@ 2014-05-26 10:11 ` Jan Beulich
2014-05-26 10:58 ` Julien Grall
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-05-26 10:11 UTC (permalink / raw)
To: Julien Grall
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Ian.Jackson, xen-devel,
julien.grall, etrudeau, tim, Arianna Avanzini, viktor.kleinik
>>> On 25.05.14 at 19:08, <julien.grall@linaro.org> wrote:
> On 25/05/14 11:51, Arianna Avanzini wrote:
>> +
>> + /*
>> + * If VGA passthru is enabled by domain config, be sure that the
>> + * domain can access VGA-related iomem regions.
>> + */
>> + if (d_config->b_info.u.hvm.gfx_passthru.val) {
>> + uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT;
>> + ret = xc_domain_iomem_permission(CTX->xch, domid,
>> + vga_iomem_start, 0x20, 1);
>> + if (ret < 0) {
>> + LOGE(ERROR,
>> + "failed to give dom%d access to iomem range "
>> + "%"PRIx64"-%"PRIx64" for VGA passthru",
>> + domid, vga_iomem_start, (vga_iomem_start + 0x20 - 1));
>> + goto error_out;
>> + }
>> + }
>
> IHMO, the guest doesn't need to have permission to this region. When
> QEMU ask to map this region to the guest, the hypercall will only check
> the permission on the domain where QEMU is running. Therefore, the
> permission should be given to the stubdomain.
How would qemu be involved in I/O from/to a passed through
device?
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 13/14] tools/libxl: explicitly grant access to needed I/O-memory ranges
2014-05-26 10:11 ` Jan Beulich
@ 2014-05-26 10:58 ` Julien Grall
2014-05-26 11:15 ` Jan Beulich
0 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2014-05-26 10:58 UTC (permalink / raw)
To: Jan Beulich
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Ian.Jackson, xen-devel,
julien.grall, etrudeau, tim, Arianna Avanzini, viktor.kleinik
On 26/05/14 11:11, Jan Beulich wrote:
>>>> On 25.05.14 at 19:08, <julien.grall@linaro.org> wrote:
>> On 25/05/14 11:51, Arianna Avanzini wrote:
>>> +
>>> + /*
>>> + * If VGA passthru is enabled by domain config, be sure that the
>>> + * domain can access VGA-related iomem regions.
>>> + */
>>> + if (d_config->b_info.u.hvm.gfx_passthru.val) {
>>> + uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT;
>>> + ret = xc_domain_iomem_permission(CTX->xch, domid,
>>> + vga_iomem_start, 0x20, 1);
>>> + if (ret < 0) {
>>> + LOGE(ERROR,
>>> + "failed to give dom%d access to iomem range "
>>> + "%"PRIx64"-%"PRIx64" for VGA passthru",
>>> + domid, vga_iomem_start, (vga_iomem_start + 0x20 - 1));
>>> + goto error_out;
>>> + }
>>> + }
>>
>> IHMO, the guest doesn't need to have permission to this region. When
>> QEMU ask to map this region to the guest, the hypercall will only check
>> the permission on the domain where QEMU is running. Therefore, the
>> permission should be given to the stubdomain.
>
> How would qemu be involved in I/O from/to a passed through
> device?
AFAIU, the mapping of the range 0xa0000-* will be done by QEMU for an
HVM guest (i.e calling xc_domain_memory_mapping).
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 10/14] xen/common: move the memory_mapping DOMCTL hypercall to common code
2014-05-26 10:07 ` Jan Beulich
@ 2014-05-26 11:03 ` Julien Grall
2014-06-05 14:21 ` Ian Campbell
0 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2014-05-26 11:03 UTC (permalink / raw)
To: Jan Beulich, Arianna Avanzini
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Ian.Jackson, xen-devel,
julien.grall, etrudeau, tim, viktor.kleinik
On 26/05/14 11:07, Jan Beulich wrote:
>>>> On 25.05.14 at 18:42, <julien.grall@linaro.org> wrote:
>> On 25/05/14 11:51, Arianna Avanzini wrote:
>>> +#include <xen/p2m-common.h>
>>> +
>>
>> That would make more sense to include asm/p2m.h in xen/p2m-common.h
>> rather than the invert.
>>
>> This will avoid issue such as, a source file is trying to include
>> xen/p2m-common.h directly without adding asm/p2m.h before. This will
>> result to a compilation issue.
>>
>> Anyway, if the maintainers are fine with this solution, let's stick
>
> I actually asked for it to be done this way.
Ok, thanks. I was unable to find the email where you asked it.
In any case, I don't think we should expose mfn_t and _mfn to ARM. All
the P2M interface used in common code, take an unsigned long in parameter.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 13/14] tools/libxl: explicitly grant access to needed I/O-memory ranges
2014-05-26 10:58 ` Julien Grall
@ 2014-05-26 11:15 ` Jan Beulich
2014-06-05 14:31 ` Ian Campbell
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-05-26 11:15 UTC (permalink / raw)
To: Julien Grall
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Ian.Jackson, xen-devel,
julien.grall, etrudeau, tim, Arianna Avanzini, viktor.kleinik
>>> On 26.05.14 at 12:58, <julien.grall@linaro.org> wrote:
>
> On 26/05/14 11:11, Jan Beulich wrote:
>>>>> On 25.05.14 at 19:08, <julien.grall@linaro.org> wrote:
>>> On 25/05/14 11:51, Arianna Avanzini wrote:
>>>> +
>>>> + /*
>>>> + * If VGA passthru is enabled by domain config, be sure that the
>>>> + * domain can access VGA-related iomem regions.
>>>> + */
>>>> + if (d_config->b_info.u.hvm.gfx_passthru.val) {
>>>> + uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT;
>>>> + ret = xc_domain_iomem_permission(CTX->xch, domid,
>>>> + vga_iomem_start, 0x20, 1);
>>>> + if (ret < 0) {
>>>> + LOGE(ERROR,
>>>> + "failed to give dom%d access to iomem range "
>>>> + "%"PRIx64"-%"PRIx64" for VGA passthru",
>>>> + domid, vga_iomem_start, (vga_iomem_start + 0x20 - 1));
>>>> + goto error_out;
>>>> + }
>>>> + }
>>>
>>> IHMO, the guest doesn't need to have permission to this region. When
>>> QEMU ask to map this region to the guest, the hypercall will only check
>>> the permission on the domain where QEMU is running. Therefore, the
>>> permission should be given to the stubdomain.
>>
>> How would qemu be involved in I/O from/to a passed through
>> device?
>
> AFAIU, the mapping of the range 0xa0000-* will be done by QEMU for an
> HVM guest (i.e calling xc_domain_memory_mapping).
If qemu is mapping this _machine_ range to every guest (or every
guest getting a GFX device passed through) that would be wrong
then too afaict.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 02/14] arch/arm: add consistency check to REMOVE p2m changes
2014-05-25 15:50 ` Julien Grall
@ 2014-06-05 13:45 ` Ian Campbell
0 siblings, 0 replies; 50+ messages in thread
From: Ian Campbell @ 2014-06-05 13:45 UTC (permalink / raw)
To: Julien Grall
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, Ian.Campbell, etrudeau,
andrew.cooper3, JBeulich, Arianna Avanzini, viktor.kleinik
On Sun, 2014-05-25 at 16:50 +0100, Julien Grall wrote:
> Hi Arianna,
>
> On 25/05/14 11:51, Arianna Avanzini wrote:
> > + unsigned long mfn = pte.p2m.base;
> > +
> > + /*
> > + * Ensure that the guest address given as argument to
> > + * this function is actually mapped to the specified
> > + * machine address. maddr here is the machine address
> > + * given to the function, while mfn is the machine
> > + * frame number actually mapped to the guest address:
> > + * check if the two correspond.
> > + */
> > + if ( !pte.p2m.valid || maddr != pfn_to_paddr(mfn) )
> > + {
> > + printk("p2m_remove: nonexistent mapping: "
> > + "%"PRIx64" and %"PRIx64"\n",
> > + pfn_to_paddr(mfn), maddr);
> > + /*
> > + * If we have successfully removed other mappings,
> > + * overload flush local variable to store if we need
> > + * to flush TLBs.
> > + */
> > + if (count) flush = 1; else flush = 0;
>
> Hrrm, why do you need this line? Flush is already correctly set
> previously (see flush |= ... few lines above).
That is:
flush |= pte.p2m.valid;
But this is within a !pte.p2m.valid if.
Now I'm not quite sure why we need to flush if this mapping is not
present, and I'm pretty certain that setting flush=0 isn't correct,
since if it was previously true nothing we do should cause us to decide
we don't need to flush any more (or if there is it needs describing very
clearly why this is the case).
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 02/14] arch/arm: add consistency check to REMOVE p2m changes
2014-05-25 10:51 ` [PATCH v8 02/14] arch/arm: add consistency check to REMOVE p2m changes Arianna Avanzini
2014-05-25 15:50 ` Julien Grall
@ 2014-06-05 13:50 ` Ian Campbell
1 sibling, 0 replies; 50+ messages in thread
From: Ian Campbell @ 2014-06-05 13:50 UTC (permalink / raw)
To: Arianna Avanzini
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, xen-devel, julien.grall,
etrudeau, JBeulich, andrew.cooper3, viktor.kleinik
On Sun, 2014-05-25 at 12:51 +0200, Arianna Avanzini wrote:
> case REMOVE:
> {
> + unsigned long mfn = pte.p2m.base;
> +
> + /*
> + * Ensure that the guest address given as argument to
> + * this function is actually mapped to the specified
> + * machine address. maddr here is the machine address
> + * given to the function, while mfn is the machine
> + * frame number actually mapped to the guest address:
> + * check if the two correspond.
> + */
> + if ( !pte.p2m.valid || maddr != pfn_to_paddr(mfn) )
> + {
> + printk("p2m_remove: nonexistent mapping: "
nonexistent is misleading here, since it might be present but not what
we think it should be. Say something like:
p2m_remove: mapping at %"PRIpaddr" is of mfn %"PRIpaddr" not %"PRIpaddr" as expected
and I think it needs to be a gdprintk or something more restircted than
a regular printk -- otherwise a guest might be able to spam the console.
> + "%"PRIx64" and %"PRIx64"\n",
> + pfn_to_paddr(mfn), maddr);
The correct format code for a paddr_t is PRIpaddr.
> + /*
> + * If we have successfully removed other mappings,
> + * overload flush local variable to store if we need
> + * to flush TLBs.
> + */
> + if (count) flush = 1; else flush = 0;
> + rc = -EINVAL;
> + goto out_flush;
> + }
> + }
> + /* fall through */
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 04/14] arch/arm: let map_mmio_regions() use start and count
2014-05-25 15:56 ` Julien Grall
@ 2014-06-05 13:53 ` Ian Campbell
0 siblings, 0 replies; 50+ messages in thread
From: Ian Campbell @ 2014-06-05 13:53 UTC (permalink / raw)
To: Julien Grall
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, Ian.Campbell, etrudeau,
andrew.cooper3, JBeulich, Arianna Avanzini, viktor.kleinik
On Sun, 2014-05-25 at 16:56 +0100, Julien Grall wrote:
> Hi Arianna,
>
> On 25/05/14 11:51, Arianna Avanzini wrote:
> > Currently, the arguments given to the function map_mmio_regions() to
> > describe the memory range to be mapped are the start and end page frame
> > numbers of the range to be mapped. However, this could give rise to
> > issues due to the range being inclusive or exclusive of the end gfn
> > given as parameter. This commit changes the interface of the function
> > to accept the start gfn and the number of gfns to be mapped.
> > This commit also changes the interface of the function map_one_mmio(),
> > helper for the xgene-storm platform, which is a wrapper for the function
> > map_mmio_regions() and does not need its arguments to be paddr_t.
> >
> > NOTE: platform-specific code has not been tested.
>
> FYI, I'm able to run Xen with your serie on the Arndale (an exynos5
> platform). So I believe there is not issue on this platform :).
>
> Acked-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 05/14] arch/arm: unmap partially-mapped I/O-memory regions
2014-05-25 16:04 ` Julien Grall
@ 2014-06-05 14:03 ` Ian Campbell
2014-06-05 14:09 ` Julien Grall
0 siblings, 1 reply; 50+ messages in thread
From: Ian Campbell @ 2014-06-05 14:03 UTC (permalink / raw)
To: Julien Grall
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, Ian.Campbell, etrudeau,
andrew.cooper3, JBeulich, Arianna Avanzini, viktor.kleinik
On Sun, 2014-05-25 at 17:04 +0100, Julien Grall wrote:
> Hi Arianna,
>
> On 25/05/14 11:51, Arianna Avanzini wrote:
> > This commit changes the interface of apply_p2m_changes() to accept
> > optionally the pointer to a counter of successfully performed
> > mappings; such a counter is used only in case of INSERT operation.
> > If an error is encountered during the operation, and therefore the
> > mapping is only partially performed, such a counter is useful to
> > let the caller be able to undo what has just been done.
>
> I don't think you need to introduce another argument to
> apply_p2m_changes. You can directly call apply_p2m_changes with the
> whole range.
Due to the sanity checks which Arianna is adding to the unmap/REMOVE
case I think it would no longer be valid to call unmap on the entire
range unless the entire range was successfully mapped.
I was nearly going to suggest that apply_p2m_changes could return the
number of successful mappings so that the caller could check actual ==
expected, but that removes the possibility to provide a specific error
code as well, so I think that is not acceptable.
I can't remember: did we consider and discard the possibility that
apply_p2m_changes should unwind its own progress on failure to INSERT
(perhaps by recursively calling itself with REMOVE)? That would seem to
be what most callers of INSERT would expect I think (despite many of
them passing NULL for this new argument here)
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 06/14] arch/x86: warn if to-be-removed mapping does not exist
2014-05-25 10:51 ` [PATCH v8 06/14] arch/x86: warn if to-be-removed mapping does not exist Arianna Avanzini
@ 2014-06-05 14:06 ` Ian Campbell
0 siblings, 0 replies; 50+ messages in thread
From: Ian Campbell @ 2014-06-05 14:06 UTC (permalink / raw)
To: Arianna Avanzini
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, xen-devel, julien.grall,
etrudeau, JBeulich, andrew.cooper3, viktor.kleinik
On Sun, 2014-05-25 at 12:51 +0200, Arianna Avanzini wrote:
> Currently, when a memory mapping is removed with the memory_mapping
> DOMCTL, no check is performed on the existence of such a mapping.
> This commit adds such a consistency check to the code performing the
> unmap, emitting a warning message if the check fails.
This is only a warning on x86 but is a fatal error on ARM (from patch #2
of this series). Is this discrepancy deliberate?
I think the ARM behaviour is more correct but I appreciate that it might
be more difficult to switch x86 to that model right away, hence a
warning might consider be more appropriate (for now at least). Is that
the answer to my question?
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 08/14] xen/common: move memory_type_changed() function to common code
2014-05-26 9:58 ` Jan Beulich
@ 2014-06-05 14:08 ` Ian Campbell
0 siblings, 0 replies; 50+ messages in thread
From: Ian Campbell @ 2014-06-05 14:08 UTC (permalink / raw)
To: Jan Beulich
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Julien Grall, Ian.Jackson,
xen-devel, julien.grall, etrudeau, tim, Arianna Avanzini,
viktor.kleinik
On Mon, 2014-05-26 at 10:58 +0100, Jan Beulich wrote:
> >>> On 25.05.14 at 18:15, <julien.grall@linaro.org> wrote:
> > On 25/05/14 11:51, Arianna Avanzini wrote:
> >> Currently, MTRR-related code is not available for the ARM architecture.
> >> Given that the memory_type_changed() function must be called also from
> >> common code, its invocation is currently ifdef'd out to be only compiled
> >> in on an x86 machine. This commit adds an empty stub for ARM, instead,
> >> also moving the function's definition to common code.
> >
> > Hrmmm, MTRR doesn't have any sense on ARM, IIRC, this is only
> > x86-related. So we should not expose this concept (i.e naming the
> > include and source file) to the common architecture.
> >
> > I would move the function declaration in xen/p2m*.h and add the stub for
> > arm in arch/arm/p2m.c
>
> Seconded.
Thirded ;-)
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 05/14] arch/arm: unmap partially-mapped I/O-memory regions
2014-06-05 14:03 ` Ian Campbell
@ 2014-06-05 14:09 ` Julien Grall
0 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2014-06-05 14:09 UTC (permalink / raw)
To: Ian Campbell, Julien Grall
Cc: paolo.valente, keir, stefano.stabellini, tim, dario.faggioli,
Ian.Jackson, xen-devel, Ian.Campbell, etrudeau, andrew.cooper3,
JBeulich, Arianna Avanzini, viktor.kleinik
On 06/05/2014 03:03 PM, Ian Campbell wrote:
> On Sun, 2014-05-25 at 17:04 +0100, Julien Grall wrote:
>> Hi Arianna,
>>
>> On 25/05/14 11:51, Arianna Avanzini wrote:
>>> This commit changes the interface of apply_p2m_changes() to accept
>>> optionally the pointer to a counter of successfully performed
>>> mappings; such a counter is used only in case of INSERT operation.
>>> If an error is encountered during the operation, and therefore the
>>> mapping is only partially performed, such a counter is useful to
>>> let the caller be able to undo what has just been done.
>>
>> I don't think you need to introduce another argument to
>> apply_p2m_changes. You can directly call apply_p2m_changes with the
>> whole range.
>
> Due to the sanity checks which Arianna is adding to the unmap/REMOVE
> case I think it would no longer be valid to call unmap on the entire
> range unless the entire range was successfully mapped.
>
> I was nearly going to suggest that apply_p2m_changes could return the
> number of successful mappings so that the caller could check actual ==
> expected, but that removes the possibility to provide a specific error
> code as well, so I think that is not acceptable.
>
> I can't remember: did we consider and discard the possibility that
> apply_p2m_changes should unwind its own progress on failure to INSERT
> (perhaps by recursively calling itself with REMOVE)? That would seem to
> be what most callers of INSERT would expect I think (despite many of
> them passing NULL for this new argument here)
We didn't consider this solution. I think unwind the mapping in this
function would be better.
Most the function which pass NULL handle only one mapping at the time
(except guest_physmap_add_entry but there is only one caller with an
order > 0 which is located in arch/arm/domain_build.c).
In anycase, I think it's safer to unwind the whole range if we fail to
add on mapping.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 10/14] xen/common: move the memory_mapping DOMCTL hypercall to common code
2014-05-26 11:03 ` Julien Grall
@ 2014-06-05 14:21 ` Ian Campbell
2014-06-05 14:33 ` Tim Deegan
0 siblings, 1 reply; 50+ messages in thread
From: Ian Campbell @ 2014-06-05 14:21 UTC (permalink / raw)
To: Julien Grall
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Ian.Jackson, xen-devel,
julien.grall, etrudeau, Jan Beulich, Arianna Avanzini,
viktor.kleinik
On Mon, 2014-05-26 at 12:03 +0100, Julien Grall wrote:
> In any case, I don't think we should expose mfn_t and _mfn to ARM. All
> the P2M interface used in common code, take an unsigned long in parameter.
I don't object in principal to the idea that common code should be
moving towards more semantic types (i.e. mfn_t rather than unsigned
long) in general. Or even to going as far as x86 has done in the common
code and having mfn_t be more than a typedef (it's a wrapping struct for
type safety), which results in the _mfn() and mfn_x() conversion things.
But I don't think we should be making that transition as part of this
series.
I think the right answer here and now is for map_mmio_regions to take
the mfn as an unsigned long and for the wrapping into a mfn_t to happen
in the x86-specific implementation of map_mmio_regions etc.
That would also get rid of this sort of thing from the previous patch:
_mfn(mfn_x(mfn) + i))
If someone wants to come along later and "type-safe-up" the common code
too, well, lets think about that then...
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 12/14] tools/libxl: handle the iomem parameter with the memory_mapping hcall
2014-05-25 17:04 ` Julien Grall
@ 2014-06-05 14:27 ` Ian Campbell
0 siblings, 0 replies; 50+ messages in thread
From: Ian Campbell @ 2014-06-05 14:27 UTC (permalink / raw)
To: Julien Grall
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, Ian.Campbell, etrudeau,
andrew.cooper3, JBeulich, Arianna Avanzini, viktor.kleinik
On Sun, 2014-05-25 at 18:04 +0100, Julien Grall wrote:
> Hi Arianna,
>
> On 25/05/14 11:51, Arianna Avanzini wrote:
> > diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> > index 369c3f3..14f9880 100644
> > --- a/tools/libxc/xc_domain.c
> > +++ b/tools/libxc/xc_domain.c
> > @@ -1650,6 +1650,18 @@ int xc_domain_memory_mapping(
> > uint32_t add_mapping)
> > {
> > DECLARE_DOMCTL;
> > + xc_dominfo_t info;
> > +
> > + if ( xc_domain_getinfo(xch, domid, 1, &info) != 1 )
> > + {
>
> You may retrieve the wrong domain here. xc_domain_getinfo return the
> information of the first domain with an ID >= domid (see
> XEN_DOMCTL_getdomaininfo in xen/common/domctl.c).
This made me notice that the existing user of
xc_core_arch_auto_translated_physmap is just *before* the check for
info.domid == domid. Oops!
(it's safe/harmless, but silly...)
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 13/14] tools/libxl: explicitly grant access to needed I/O-memory ranges
2014-05-26 11:15 ` Jan Beulich
@ 2014-06-05 14:31 ` Ian Campbell
2014-06-05 14:37 ` Ian Campbell
2014-06-05 14:53 ` Jan Beulich
0 siblings, 2 replies; 50+ messages in thread
From: Ian Campbell @ 2014-06-05 14:31 UTC (permalink / raw)
To: Jan Beulich
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Julien Grall, Ian.Jackson,
xen-devel, julien.grall, etrudeau, tim, Arianna Avanzini,
viktor.kleinik
On Mon, 2014-05-26 at 12:15 +0100, Jan Beulich wrote:
> >>> IHMO, the guest doesn't need to have permission to this region. When
> >>> QEMU ask to map this region to the guest, the hypercall will only check
> >>> the permission on the domain where QEMU is running. Therefore, the
> >>> permission should be given to the stubdomain.
> >>
> >> How would qemu be involved in I/O from/to a passed through
> >> device?
> >
> > AFAIU, the mapping of the range 0xa0000-* will be done by QEMU for an
> > HVM guest (i.e calling xc_domain_memory_mapping).
>
> If qemu is mapping this _machine_ range to every guest (or every
> guest getting a GFX device passed through) that would be wrong
> then too afaict.
How does this work today then? Do no guests get access to 0xa0000 or do
we some how determine which of the multiple GFX devices is the primary
one (with the real 0xa0000 mapped to it)?
I can't see 0xa0000 mapped by anything in xen.git and there are too many
hits on the qemu tree for me to spot it if it is there.
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 10/14] xen/common: move the memory_mapping DOMCTL hypercall to common code
2014-06-05 14:21 ` Ian Campbell
@ 2014-06-05 14:33 ` Tim Deegan
2014-06-05 14:39 ` Ian Campbell
0 siblings, 1 reply; 50+ messages in thread
From: Tim Deegan @ 2014-06-05 14:33 UTC (permalink / raw)
To: Ian Campbell
Cc: julien.grall, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Julien Grall, Ian.Jackson,
xen-devel, Ian.Campbell, etrudeau, Jan Beulich, Arianna Avanzini,
viktor.kleinik
At 15:21 +0100 on 05 Jun (1401978088), Ian Campbell wrote:
> On Mon, 2014-05-26 at 12:03 +0100, Julien Grall wrote:
> > In any case, I don't think we should expose mfn_t and _mfn to ARM. All
> > the P2M interface used in common code, take an unsigned long in parameter.
>
> I don't object in principal to the idea that common code should be
> moving towards more semantic types (i.e. mfn_t rather than unsigned
> long) in general. Or even to going as far as x86 has done in the common
> code and having mfn_t be more than a typedef (it's a wrapping struct for
> type safety), which results in the _mfn() and mfn_x() conversion things.
>
> But I don't think we should be making that transition as part of this
> series.
>
> I think the right answer here and now is for map_mmio_regions to take
> the mfn as an unsigned long and for the wrapping into a mfn_t to happen
> in the x86-specific implementation of map_mmio_regions etc.
+1.
> That would also get rid of this sort of thing from the previous patch:
> _mfn(mfn_x(mfn) + i))
>
> If someone wants to come along later and "type-safe-up" the common code
> too, well, lets think about that then...
Sure. I think it would be a good idea in general to extend those
type-safe conventions, but no hurry to do it here.
Incidentally, I'd be _delighted_ if anyone can suggest a way of making
mfn_t and gfn_t distinct types (i.e. that the compiler won't silently
cast to/from other types) without requiring the _mfn/mfn_x ugliness.
Tim.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 13/14] tools/libxl: explicitly grant access to needed I/O-memory ranges
2014-06-05 14:31 ` Ian Campbell
@ 2014-06-05 14:37 ` Ian Campbell
2014-06-05 14:54 ` Jan Beulich
2014-06-05 14:53 ` Jan Beulich
1 sibling, 1 reply; 50+ messages in thread
From: Ian Campbell @ 2014-06-05 14:37 UTC (permalink / raw)
To: Jan Beulich
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Julien Grall, Ian.Jackson,
xen-devel, julien.grall, etrudeau, tim, Arianna Avanzini,
viktor.kleinik
On Thu, 2014-06-05 at 15:31 +0100, Ian Campbell wrote:
> On Mon, 2014-05-26 at 12:15 +0100, Jan Beulich wrote:
> > >>> IHMO, the guest doesn't need to have permission to this region. When
> > >>> QEMU ask to map this region to the guest, the hypercall will only check
> > >>> the permission on the domain where QEMU is running. Therefore, the
> > >>> permission should be given to the stubdomain.
> > >>
> > >> How would qemu be involved in I/O from/to a passed through
> > >> device?
> > >
> > > AFAIU, the mapping of the range 0xa0000-* will be done by QEMU for an
> > > HVM guest (i.e calling xc_domain_memory_mapping).
> >
> > If qemu is mapping this _machine_ range to every guest (or every
> > guest getting a GFX device passed through) that would be wrong
> > then too afaict.
>
> How does this work today then? Do no guests get access to 0xa0000 or do
> we some how determine which of the multiple GFX devices is the primary
> one (with the real 0xa0000 mapped to it)?
>
> I can't see 0xa0000 mapped by anything in xen.git and there are too many
> hits on the qemu tree for me to spot it if it is there.
Ah, here it is in qemu-trad hw/pt-graphics.c:
int register_vga_regions(struct pt_dev *real_device)
{
u16 vendor_id;
int ret = 0;
if ( !gfx_passthru || real_device->pci_dev->device_class != 0x0300 )
return ret;
...
ret |= xc_domain_memory_mapping(xc_handle, domid,
0xa0000 >> XC_PAGE_SHIFT,
0xa0000 >> XC_PAGE_SHIFT,
0x20,
DPCI_ADD_MAPPING);
AFAICT the only thing which might save us from the scenario you are
worried about would be the device_class == 0x0300 thing, but I don't see
how that could be the case...
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 10/14] xen/common: move the memory_mapping DOMCTL hypercall to common code
2014-06-05 14:33 ` Tim Deegan
@ 2014-06-05 14:39 ` Ian Campbell
0 siblings, 0 replies; 50+ messages in thread
From: Ian Campbell @ 2014-06-05 14:39 UTC (permalink / raw)
To: Tim Deegan
Cc: julien.grall, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Julien Grall, Ian.Jackson,
xen-devel, Ian.Campbell, etrudeau, Jan Beulich, Arianna Avanzini,
viktor.kleinik
On Thu, 2014-06-05 at 16:33 +0200, Tim Deegan wrote:
> Incidentally, I'd be _delighted_ if anyone can suggest a way of making
> mfn_t and gfn_t distinct types (i.e. that the compiler won't silently
> cast to/from other types) without requiring the _mfn/mfn_x ugliness.
I think we could do it with sparse (which allows you to assign "address
spaces" to things) as a separate compilation check/step.
Perhaps there is a similar gcc (or more likely: clang) attribute these
days?
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 13/14] tools/libxl: explicitly grant access to needed I/O-memory ranges
2014-06-05 14:31 ` Ian Campbell
2014-06-05 14:37 ` Ian Campbell
@ 2014-06-05 14:53 ` Jan Beulich
1 sibling, 0 replies; 50+ messages in thread
From: Jan Beulich @ 2014-06-05 14:53 UTC (permalink / raw)
To: Ian Campbell
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Julien Grall, Ian.Jackson,
xen-devel, julien.grall, etrudeau, tim, Arianna Avanzini,
viktor.kleinik
>>> On 05.06.14 at 16:31, <ian.campbell@citrix.com> wrote:
> On Mon, 2014-05-26 at 12:15 +0100, Jan Beulich wrote:
>> >>> IHMO, the guest doesn't need to have permission to this region. When
>> >>> QEMU ask to map this region to the guest, the hypercall will only check
>> >>> the permission on the domain where QEMU is running. Therefore, the
>> >>> permission should be given to the stubdomain.
>> >>
>> >> How would qemu be involved in I/O from/to a passed through
>> >> device?
>> >
>> > AFAIU, the mapping of the range 0xa0000-* will be done by QEMU for an
>> > HVM guest (i.e calling xc_domain_memory_mapping).
>>
>> If qemu is mapping this _machine_ range to every guest (or every
>> guest getting a GFX device passed through) that would be wrong
>> then too afaict.
>
> How does this work today then? Do no guests get access to 0xa0000 or do
> we some how determine which of the multiple GFX devices is the primary
> one (with the real 0xa0000 mapped to it)?
I don't know. All that is clear is that the way it is being proposed here
(no matter whether that's a copy of current qemu behavior) can't be
right.
> I can't see 0xa0000 mapped by anything in xen.git and there are too many
> hits on the qemu tree for me to spot it if it is there.
Right - that's all I can tell too.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 13/14] tools/libxl: explicitly grant access to needed I/O-memory ranges
2014-06-05 14:37 ` Ian Campbell
@ 2014-06-05 14:54 ` Jan Beulich
0 siblings, 0 replies; 50+ messages in thread
From: Jan Beulich @ 2014-06-05 14:54 UTC (permalink / raw)
To: Ian Campbell
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Julien Grall, Ian.Jackson,
xen-devel, julien.grall, etrudeau, tim, Arianna Avanzini,
viktor.kleinik
>>> On 05.06.14 at 16:37, <ian.campbell@citrix.com> wrote:
> On Thu, 2014-06-05 at 15:31 +0100, Ian Campbell wrote:
>> On Mon, 2014-05-26 at 12:15 +0100, Jan Beulich wrote:
>> > >>> IHMO, the guest doesn't need to have permission to this region. When
>> > >>> QEMU ask to map this region to the guest, the hypercall will only check
>> > >>> the permission on the domain where QEMU is running. Therefore, the
>> > >>> permission should be given to the stubdomain.
>> > >>
>> > >> How would qemu be involved in I/O from/to a passed through
>> > >> device?
>> > >
>> > > AFAIU, the mapping of the range 0xa0000-* will be done by QEMU for an
>> > > HVM guest (i.e calling xc_domain_memory_mapping).
>> >
>> > If qemu is mapping this _machine_ range to every guest (or every
>> > guest getting a GFX device passed through) that would be wrong
>> > then too afaict.
>>
>> How does this work today then? Do no guests get access to 0xa0000 or do
>> we some how determine which of the multiple GFX devices is the primary
>> one (with the real 0xa0000 mapped to it)?
>>
>> I can't see 0xa0000 mapped by anything in xen.git and there are too many
>> hits on the qemu tree for me to spot it if it is there.
>
> Ah, here it is in qemu-trad hw/pt-graphics.c:
>
> int register_vga_regions(struct pt_dev *real_device)
> {
> u16 vendor_id;
> int ret = 0;
>
> if ( !gfx_passthru || real_device->pci_dev->device_class != 0x0300 )
> return ret;
>
> ...
> ret |= xc_domain_memory_mapping(xc_handle, domid,
> 0xa0000 >> XC_PAGE_SHIFT,
> 0xa0000 >> XC_PAGE_SHIFT,
> 0x20,
> DPCI_ADD_MAPPING);
>
>
> AFAICT the only thing which might save us from the scenario you are
> worried about would be the device_class == 0x0300 thing, but I don't see
> how that could be the case...
Indeed that would match on most if not all graphics cards.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 01/14] arch/arm: domain build: let dom0 access I/O memory of mapped devices
2014-05-25 10:51 ` [PATCH v8 01/14] arch/arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
@ 2014-06-10 15:04 ` Ian Campbell
0 siblings, 0 replies; 50+ messages in thread
From: Ian Campbell @ 2014-06-10 15:04 UTC (permalink / raw)
To: Arianna Avanzini
Cc: paolo.valente, keir, stefano.stabellini, Ian.Jackson,
dario.faggioli, tim, xen-devel, julien.grall, etrudeau, JBeulich,
andrew.cooper3, viktor.kleinik
On Sun, 2014-05-25 at 12:51 +0200, Arianna Avanzini wrote:
> Currently, dom0 is allowed access to the I/O memory ranges used
> to access devices exposed to it, but it doesn't have those
> ranges in its iomem_caps. This commit implements the correct
> bookkeeping in the generic function which actually maps a
> device's I/O memory to the domain, adding the ranges to the
> domain's iomem_caps.
>
> NOTE: This commit suffers from the following limitations;
> . with this patch, I/O memory ranges pertaining disabled
> devices are not mapped;
> . the "iomem" option could be used to map memory ranges that
> are not described in the device tree.
> In both these cases, this patch does not allow the domain
> the privileges needed to map the needed I/O memory ranges
> afterwards.
>
> Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
> Acked-by: Ian Campbell <Ian.Campbell@eu.citrix.com>
> Acked-by: Julien Grall <julien.grall@citrix.com>
I applied this one.
Patch #2 here had comments and I wasn't brave enough to try and cherry
pick patches from past that because I wasn't sure if there were
dependencies.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM
2014-05-25 10:51 [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (13 preceding siblings ...)
2014-05-25 10:51 ` [PATCH v8 14/14] xen/common: do not implicitly permit access to mapped I/O memory Arianna Avanzini
@ 2014-07-01 10:45 ` Julien Grall
2014-07-01 10:55 ` Arianna Avanzini
14 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2014-07-01 10:45 UTC (permalink / raw)
To: Arianna Avanzini, xen-devel
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, Ian.Campbell, etrudeau, JBeulich,
andrew.cooper3, viktor.kleinik
Hi Arianna,
Do you have any plan to send a new version soon? I'm working on device
pass-through and this series is a dependency/blocker for my series.
IIRC, this version was already in good shape. There is only few comments.
Regards,
On 05/25/2014 11:51 AM, Arianna Avanzini wrote:
> Hello,
>
> this is a new version - the eighth - of my implementation proposal for the
> memory_mapping DOMCTL for the ARM architecture. As usual, this cover letter just
> lists the most relevant changes performed on the patchset, while a more detailed
> description can be found in the description and changelog of each patch; also,
> some more information about the patch series' origin can be found in the last
> full cover letter ([1]).
>
> First of all, the commit descriptions have been reworded to get rid of tentative
> phrases that were pointless in that context, as suggested by Dario Faggioli.
>
> Patch 0002 has been modified to revert some uncorrect changes due to my
> misunderstanding of previous comments, as pointed out by Julien Grall. Also,
> according to his suggestions, an error message has been added to let a failure
> encountered during an unmap be less obscure. The error path has been improved
> to include unmapping previously-created page-table mappings, unlocking the
> p2m_lock and flushing the TLBs in case some mappings have been successfully
> removed.
>
> Patch 0004 has been changed to address issues pointed out by Julien Grall and
> Ian Campbell. Now the correct DIV_ROUND_UP() macro is used instead of the
> ternary operator, and the map_one_mmio() function, defined for the xgene-storm
> platform, accepts pfns as arguments instead of paddr_t. Also, the function
> apply_p2m_changes() is now called from map_mmio_regions() with the correct
> count.
>
> A new patch 0005 has been added to let the map_mmio_regions() function for ARM
> unmap partially-mapped I/O-memory regions if mapping a page fails, as suggested
> by Julien Grall. The approach which has been used includes having the function
> apply_p2m_changes() return, in the case of an error, the number of pages that
> have been successfully mapped, therefore allowing the caller to undo what has
> been done, if necessary. The definition of unmap_mmio_regions() for ARM has been
> moved to this patch and now uses the correct count as indicated by Julien Grall.
>
> The title and description of patch 0006 have been changed to (hopefully) better
> fit the patch content, as requested by Jan Beulich.
>
> The patch factoring out map and unmap operations from the x86 version of the
> memory_mapping DOMCTL has been split into two, as suggested by Jan Beulich, to
> make the review easier: patch 0007 now just cleans up existing code, by letting
> mfn + nr_mfns be computed only once and by changing the name of the tmp_rc local
> variable to rc; patch 0009 instead actually factors out the operations.
>
> A new patch 0008 now adds an empty stub for the ARM architecture of the MTRR-
> related function memory_type_changed(), currently available only for x86, and
> makes its definition common to both x86 and ARM. This was requested by Jan
> Beulich and Ian Campbell.
>
> The call to PERROR() in patch 0012 has been fixed, as it showed an error message
> whose meaning was the opposite of the correct one, as pointed out by Jan
> Beulich.
>
> The patch which implemented a separation of the functions of the memory_mapping
> and iomem_permission DOMCTLs has now been split into two, as suggested by Jan
> Beulich: patch 0013 now contains only libxl-related preparatory changes, while
> patch 0014 actually implements the separation. As for patch 0014, changes
> performed on the iomem_permission DOMCTL handling code have been dropped, while
> the permission checks performed by memory_mapping have been changed to be more
> conservative and check both the calling domain and the grantee domain's
> permissions.
> As of now, patch 0014 performs the separation only for I/O memory and not for
> I/O ports (as was instead suggested by Jan Beulich) because, as far as I know,
> Julien Grall is working on the I/O ports patch series, and I'm afraid I might be
> conflicting with his work. I am however surely willing to add a patch for that
> if he is OK with it.
>
> The code has again been tested on a cubieboard2, with Linux v3.15-rc3 as a dom0
> and ERIKA Enterprise ([2]) as a domU. The x86 bits have been tested on an x86_64
> machine, as I forgot to say in the last versions of the patchset and Jan Beulich
> pointed out; in particular, this last version, has been tested with Linux
> 3.15.0-rc5 as both dom0 and domU.
> Any feedback about this new version of the patchset is more than welcome,
> Arianna
>
> [1] http://lists.xen.org/archives/html/xen-devel/2014-03/msg01724.html
> [2] http://erika.tuxfamily.org/drupal/
>
> Arianna Avanzini (14):
> arch/arm: domain build: let dom0 access I/O memory of mapped devices
> arch/arm: add consistency check to REMOVE p2m changes
> arch/arm: let map_mmio_regions() take pfn as parameters
> arch/arm: let map_mmio_regions() use start and count
> arch/arm: unmap partially-mapped I/O-memory regions
> arch/x86: warn if to-be-removed mapping does not exist
> arch/x86: cleanup memory_mapping DOMCTL
> xen/common: move memory_type_changed() function to common code
> xen/x86: factor out map and unmap from the memory_mapping DOMCTL
> xen/common: move the memory_mapping DOMCTL hypercall to common code
> tools/libxl: parse optional start gfn from the iomem config option
> tools/libxl: handle the iomem parameter with the memory_mapping hcall
> tools/libxl: explicitly grant access to needed I/O-memory ranges
> xen/common: do not implicitly permit access to mapped I/O memory
>
> docs/man/xl.cfg.pod.5 | 18 +++++--
> tools/libxc/xc_domain.c | 12 +++++
> tools/libxl/libxl.h | 10 ++++
> tools/libxl/libxl_create.c | 34 +++++++++++++
> tools/libxl/libxl_internal.h | 1 +
> tools/libxl/libxl_pci.c | 26 ++++------
> tools/libxl/libxl_types.idl | 4 ++
> tools/libxl/xl_cmdimpl.c | 17 ++++---
> xen/arch/arm/Makefile | 1 +
> xen/arch/arm/domain_build.c | 18 +++++--
> xen/arch/arm/gic.c | 15 +++---
> xen/arch/arm/mtrr.c | 6 +++
> xen/arch/arm/p2m.c | 99 ++++++++++++++++++++++++++++--------
> xen/arch/arm/platforms/exynos5.c | 9 ++--
> xen/arch/arm/platforms/omap5.c | 17 +++----
> xen/arch/arm/platforms/xgene-storm.c | 16 +++---
> xen/arch/x86/domctl.c | 76 ---------------------------
> xen/arch/x86/mm/p2m.c | 53 +++++++++++++++++--
> xen/common/domctl.c | 55 +++++++++++++++++++-
> xen/include/asm-arm/mm.h | 2 +
> xen/include/asm-arm/mtrr.h | 6 +++
> xen/include/asm-arm/p2m.h | 11 ++--
> xen/include/asm-x86/mtrr.h | 2 +-
> xen/include/asm-x86/p2m.h | 3 +-
> xen/include/xen/mtrr.h | 8 +++
> xen/include/xen/p2m-common.h | 16 ++++++
> 26 files changed, 365 insertions(+), 170 deletions(-)
> create mode 100644 xen/arch/arm/mtrr.c
> create mode 100644 xen/include/asm-arm/mtrr.h
> create mode 100644 xen/include/xen/mtrr.h
> create mode 100644 xen/include/xen/p2m-common.h
>
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM
2014-07-01 10:45 ` [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Julien Grall
@ 2014-07-01 10:55 ` Arianna Avanzini
0 siblings, 0 replies; 50+ messages in thread
From: Arianna Avanzini @ 2014-07-01 10:55 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, Ian.Campbell, etrudeau, JBeulich,
andrew.cooper3, viktor.kleinik
On 07/01/2014 12:45 PM, Julien Grall wrote:
> Hi Arianna,
>
Hello,
> Do you have any plan to send a new version soon? I'm working on device
> pass-through and this series is a dependency/blocker for my series.
>
> IIRC, this version was already in good shape. There is only few comments.
>
I'm currently working on the v9 series and will be sending it no later than
tomorrow.
Sorry for the delay,
Arianna
> Regards,
>
> On 05/25/2014 11:51 AM, Arianna Avanzini wrote:
>> Hello,
>>
>> this is a new version - the eighth - of my implementation proposal for the
>> memory_mapping DOMCTL for the ARM architecture. As usual, this cover letter just
>> lists the most relevant changes performed on the patchset, while a more detailed
>> description can be found in the description and changelog of each patch; also,
>> some more information about the patch series' origin can be found in the last
>> full cover letter ([1]).
>>
>> First of all, the commit descriptions have been reworded to get rid of tentative
>> phrases that were pointless in that context, as suggested by Dario Faggioli.
>>
>> Patch 0002 has been modified to revert some uncorrect changes due to my
>> misunderstanding of previous comments, as pointed out by Julien Grall. Also,
>> according to his suggestions, an error message has been added to let a failure
>> encountered during an unmap be less obscure. The error path has been improved
>> to include unmapping previously-created page-table mappings, unlocking the
>> p2m_lock and flushing the TLBs in case some mappings have been successfully
>> removed.
>>
>> Patch 0004 has been changed to address issues pointed out by Julien Grall and
>> Ian Campbell. Now the correct DIV_ROUND_UP() macro is used instead of the
>> ternary operator, and the map_one_mmio() function, defined for the xgene-storm
>> platform, accepts pfns as arguments instead of paddr_t. Also, the function
>> apply_p2m_changes() is now called from map_mmio_regions() with the correct
>> count.
>>
>> A new patch 0005 has been added to let the map_mmio_regions() function for ARM
>> unmap partially-mapped I/O-memory regions if mapping a page fails, as suggested
>> by Julien Grall. The approach which has been used includes having the function
>> apply_p2m_changes() return, in the case of an error, the number of pages that
>> have been successfully mapped, therefore allowing the caller to undo what has
>> been done, if necessary. The definition of unmap_mmio_regions() for ARM has been
>> moved to this patch and now uses the correct count as indicated by Julien Grall.
>>
>> The title and description of patch 0006 have been changed to (hopefully) better
>> fit the patch content, as requested by Jan Beulich.
>>
>> The patch factoring out map and unmap operations from the x86 version of the
>> memory_mapping DOMCTL has been split into two, as suggested by Jan Beulich, to
>> make the review easier: patch 0007 now just cleans up existing code, by letting
>> mfn + nr_mfns be computed only once and by changing the name of the tmp_rc local
>> variable to rc; patch 0009 instead actually factors out the operations.
>>
>> A new patch 0008 now adds an empty stub for the ARM architecture of the MTRR-
>> related function memory_type_changed(), currently available only for x86, and
>> makes its definition common to both x86 and ARM. This was requested by Jan
>> Beulich and Ian Campbell.
>>
>> The call to PERROR() in patch 0012 has been fixed, as it showed an error message
>> whose meaning was the opposite of the correct one, as pointed out by Jan
>> Beulich.
>>
>> The patch which implemented a separation of the functions of the memory_mapping
>> and iomem_permission DOMCTLs has now been split into two, as suggested by Jan
>> Beulich: patch 0013 now contains only libxl-related preparatory changes, while
>> patch 0014 actually implements the separation. As for patch 0014, changes
>> performed on the iomem_permission DOMCTL handling code have been dropped, while
>> the permission checks performed by memory_mapping have been changed to be more
>> conservative and check both the calling domain and the grantee domain's
>> permissions.
>> As of now, patch 0014 performs the separation only for I/O memory and not for
>> I/O ports (as was instead suggested by Jan Beulich) because, as far as I know,
>> Julien Grall is working on the I/O ports patch series, and I'm afraid I might be
>> conflicting with his work. I am however surely willing to add a patch for that
>> if he is OK with it.
>>
>> The code has again been tested on a cubieboard2, with Linux v3.15-rc3 as a dom0
>> and ERIKA Enterprise ([2]) as a domU. The x86 bits have been tested on an x86_64
>> machine, as I forgot to say in the last versions of the patchset and Jan Beulich
>> pointed out; in particular, this last version, has been tested with Linux
>> 3.15.0-rc5 as both dom0 and domU.
>> Any feedback about this new version of the patchset is more than welcome,
>> Arianna
>>
>> [1] http://lists.xen.org/archives/html/xen-devel/2014-03/msg01724.html
>> [2] http://erika.tuxfamily.org/drupal/
>>
>> Arianna Avanzini (14):
>> arch/arm: domain build: let dom0 access I/O memory of mapped devices
>> arch/arm: add consistency check to REMOVE p2m changes
>> arch/arm: let map_mmio_regions() take pfn as parameters
>> arch/arm: let map_mmio_regions() use start and count
>> arch/arm: unmap partially-mapped I/O-memory regions
>> arch/x86: warn if to-be-removed mapping does not exist
>> arch/x86: cleanup memory_mapping DOMCTL
>> xen/common: move memory_type_changed() function to common code
>> xen/x86: factor out map and unmap from the memory_mapping DOMCTL
>> xen/common: move the memory_mapping DOMCTL hypercall to common code
>> tools/libxl: parse optional start gfn from the iomem config option
>> tools/libxl: handle the iomem parameter with the memory_mapping hcall
>> tools/libxl: explicitly grant access to needed I/O-memory ranges
>> xen/common: do not implicitly permit access to mapped I/O memory
>>
>> docs/man/xl.cfg.pod.5 | 18 +++++--
>> tools/libxc/xc_domain.c | 12 +++++
>> tools/libxl/libxl.h | 10 ++++
>> tools/libxl/libxl_create.c | 34 +++++++++++++
>> tools/libxl/libxl_internal.h | 1 +
>> tools/libxl/libxl_pci.c | 26 ++++------
>> tools/libxl/libxl_types.idl | 4 ++
>> tools/libxl/xl_cmdimpl.c | 17 ++++---
>> xen/arch/arm/Makefile | 1 +
>> xen/arch/arm/domain_build.c | 18 +++++--
>> xen/arch/arm/gic.c | 15 +++---
>> xen/arch/arm/mtrr.c | 6 +++
>> xen/arch/arm/p2m.c | 99 ++++++++++++++++++++++++++++--------
>> xen/arch/arm/platforms/exynos5.c | 9 ++--
>> xen/arch/arm/platforms/omap5.c | 17 +++----
>> xen/arch/arm/platforms/xgene-storm.c | 16 +++---
>> xen/arch/x86/domctl.c | 76 ---------------------------
>> xen/arch/x86/mm/p2m.c | 53 +++++++++++++++++--
>> xen/common/domctl.c | 55 +++++++++++++++++++-
>> xen/include/asm-arm/mm.h | 2 +
>> xen/include/asm-arm/mtrr.h | 6 +++
>> xen/include/asm-arm/p2m.h | 11 ++--
>> xen/include/asm-x86/mtrr.h | 2 +-
>> xen/include/asm-x86/p2m.h | 3 +-
>> xen/include/xen/mtrr.h | 8 +++
>> xen/include/xen/p2m-common.h | 16 ++++++
>> 26 files changed, 365 insertions(+), 170 deletions(-)
>> create mode 100644 xen/arch/arm/mtrr.c
>> create mode 100644 xen/include/asm-arm/mtrr.h
>> create mode 100644 xen/include/xen/mtrr.h
>> create mode 100644 xen/include/xen/p2m-common.h
>>
>
>
--
/*
* Arianna Avanzini
* avanzini.arianna@gmail.com
* 73628@studenti.unimore.it
*/
^ permalink raw reply [flat|nested] 50+ messages in thread
end of thread, other threads:[~2014-07-01 10:55 UTC | newest]
Thread overview: 50+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-25 10:51 [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
2014-05-25 10:51 ` [PATCH v8 01/14] arch/arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
2014-06-10 15:04 ` Ian Campbell
2014-05-25 10:51 ` [PATCH v8 02/14] arch/arm: add consistency check to REMOVE p2m changes Arianna Avanzini
2014-05-25 15:50 ` Julien Grall
2014-06-05 13:45 ` Ian Campbell
2014-06-05 13:50 ` Ian Campbell
2014-05-25 10:51 ` [PATCH v8 03/14] arch/arm: let map_mmio_regions() take pfn as parameters Arianna Avanzini
2014-05-25 10:51 ` [PATCH v8 04/14] arch/arm: let map_mmio_regions() use start and count Arianna Avanzini
2014-05-25 15:56 ` Julien Grall
2014-06-05 13:53 ` Ian Campbell
2014-05-25 10:51 ` [PATCH v8 05/14] arch/arm: unmap partially-mapped I/O-memory regions Arianna Avanzini
2014-05-25 16:04 ` Julien Grall
2014-06-05 14:03 ` Ian Campbell
2014-06-05 14:09 ` Julien Grall
2014-05-25 10:51 ` [PATCH v8 06/14] arch/x86: warn if to-be-removed mapping does not exist Arianna Avanzini
2014-06-05 14:06 ` Ian Campbell
2014-05-25 10:51 ` [PATCH v8 07/14] arch/x86: cleanup memory_mapping DOMCTL Arianna Avanzini
2014-05-26 9:57 ` Jan Beulich
2014-05-25 10:51 ` [PATCH v8 08/14] xen/common: move memory_type_changed() function to common code Arianna Avanzini
2014-05-25 16:15 ` Julien Grall
2014-05-26 9:58 ` Jan Beulich
2014-06-05 14:08 ` Ian Campbell
2014-05-25 10:51 ` [PATCH v8 09/14] xen/x86: factor out map and unmap from the memory_mapping DOMCTL Arianna Avanzini
2014-05-26 10:04 ` Jan Beulich
2014-05-25 10:51 ` [PATCH v8 10/14] xen/common: move the memory_mapping DOMCTL hypercall to common code Arianna Avanzini
2014-05-25 16:42 ` Julien Grall
2014-05-26 10:07 ` Jan Beulich
2014-05-26 11:03 ` Julien Grall
2014-06-05 14:21 ` Ian Campbell
2014-06-05 14:33 ` Tim Deegan
2014-06-05 14:39 ` Ian Campbell
2014-05-26 10:06 ` Jan Beulich
2014-05-25 10:51 ` [PATCH v8 11/14] tools/libxl: parse optional start gfn from the iomem config option Arianna Avanzini
2014-05-25 10:51 ` [PATCH v8 12/14] tools/libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
2014-05-25 17:04 ` Julien Grall
2014-06-05 14:27 ` Ian Campbell
2014-05-25 10:51 ` [PATCH v8 13/14] tools/libxl: explicitly grant access to needed I/O-memory ranges Arianna Avanzini
2014-05-25 17:08 ` Julien Grall
2014-05-26 10:11 ` Jan Beulich
2014-05-26 10:58 ` Julien Grall
2014-05-26 11:15 ` Jan Beulich
2014-06-05 14:31 ` Ian Campbell
2014-06-05 14:37 ` Ian Campbell
2014-06-05 14:54 ` Jan Beulich
2014-06-05 14:53 ` Jan Beulich
2014-05-26 10:10 ` Jan Beulich
2014-05-25 10:51 ` [PATCH v8 14/14] xen/common: do not implicitly permit access to mapped I/O memory Arianna Avanzini
2014-07-01 10:45 ` [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Julien Grall
2014-07-01 10:55 ` Arianna Avanzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).