* [PATCH v7 00/10] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM
@ 2014-05-05 15:54 Arianna Avanzini
2014-05-05 15:54 ` [PATCH v7 01/10] arch/arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
` (10 more replies)
0 siblings, 11 replies; 45+ messages in thread
From: Arianna Avanzini @ 2014-05-05 15:54 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,
here is my seventh attempt at proposing an implementation of the memory_mapping
DOMCTL for the ARM architecture. This cover letter just lists the main changes
performed on the patchset, while a more detailed description can be found in the
commit descriptions and in the changelog of each patch; also, more about the
previous versions of the patchset and its origins can be found in the last
full cover letter ([1]).
The commit description of patch 0001 has been changed to highlight some
limitations of the implementation, which were pointed out by Julien Grall.
As far as I know, he already has in his passthru-related series a follow-up
patch which addresses such limitations.
Patch 0002 has been changed to address an issue pointed out by Julien Grall; the
check added to ARM-related code on the existence of a mapping to be removed now
silently ignores that the gfn is not mapped on removal.
When using paddr_to_pfn_aligned(), now patch 0003 does not compute an useless
decrement on the addresses to be converted to pfns, as suggested by
Ian Campbell.
Also, this patchset finally implements a change to the interface of the
{map|unmap}_mmio_regions() functions, allowing them to accept a description of
the address range to map as a "start_gfn, nr_mfns pair". The change is performed
in the 0004 patch of this series, which substitutes the previous one, that
instead changed the map_mmio_regions() function to accept a range inclusive of
the end_gfn.
This change was requested by Ian Campbell and Andrew Cooper. The latter also
asked that all relevant functions touched by this patchset and still relying on
a "start_gfn, end_gfn" pair given as parameter were adapted to use the
"start_gfn, nr_mfns" model, which is not prone to misinterpretations about the
range being inclusive or exclusive of the end gfn.
The 0004 patch in this series, however, still produces the minimum necessary
changes to the functions which are to be introduced by this patch and to the
already-existing function map_mmio_regions() for the ARM architecture. Also, Jan
Beulich pointed out that a "start, count" pair cannot fully describe the address
range.
I'm including the requested change in the patch series just to have feedback
(and hoping this is the correct way to ask for feedback), if you consider the
previous implementation to be more appropriate I can easily revert to it.
Patch 0005, which adds to x86-related code a check on the existence of a to-be-
removed mapping, now lets the check just emit a warning on failure instead of
returning an error, as per Jan Beulich's suggestion.
Patch 0006 and 0007 have been hopefully improved as regards typing and
correct use of memory handling functions, following Jan Beulich's comments.
Patch 0008 now uses LIBXL_INVALID_GFN when useful, as suggested by Ian Campbell;
the manpage now also mentions the iomem option defaulting to an 1:1 mapping
when no gfn parameter is given and that it is for auto-translated domains, as
requested by Ian Campbell and Julien Grall.
Patch 0009 now checks if a domain is auto-translated in xc_domain_memory_mapping
to avoid to expose this information to libxl, as indicated by Ian Campbell; the
commit introducing libxl helpers to this purpose has been therefore dropped.
Patch 0010, which attempts to separate operations performed by the hypercalls
iomem_permission and memory_mapping, has been changed to address some issues
pointed out by Jan Beulich. It now lets I/O ports and I/O memory be treated the
same way when allowing access to a domain. Also, now it is iomem_permission
that checks if the calling domain is allowed to access to-be-mapped memory
ranges. As suggested by Ian Campbell, finally, this patch changes the construct
used by libxl during the initialization of PCI devices to (hopefully) better
suit the new execution flow.
The code has again been tested on a cubieboard2, with Linux v3.13 as a dom0 and
ERIKA Enterprise ([2]) as a 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 (10):
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/x86: check if mapping exists before memory_mapping removes it
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
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/domain_build.c | 18 +++++++--
xen/arch/arm/gic.c | 15 +++----
xen/arch/arm/p2m.c | 44 +++++++++++++++++----
xen/arch/arm/platforms/exynos5.c | 9 ++---
xen/arch/arm/platforms/omap5.c | 17 ++++----
xen/arch/arm/platforms/xgene-storm.c | 4 +-
xen/arch/x86/domctl.c | 76 ------------------------------------
xen/arch/x86/mm/p2m.c | 53 +++++++++++++++++++++++--
xen/common/domctl.c | 70 ++++++++++++++++++++++++++++++---
xen/include/asm-arm/mm.h | 2 +
xen/include/asm-arm/p2m.h | 11 +++---
xen/include/asm-x86/p2m.h | 3 +-
xen/include/xen/p2m-common.h | 16 ++++++++
21 files changed, 306 insertions(+), 154 deletions(-)
create mode 100644 xen/include/xen/p2m-common.h
--
1.9.2
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v7 01/10] arch/arm: domain build: let dom0 access I/O memory of mapped devices
2014-05-05 15:54 [PATCH v7 00/10] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
@ 2014-05-05 15:54 ` Arianna Avanzini
2014-05-05 15:54 ` [PATCH v7 02/10] arch/arm: add consistency check to REMOVE p2m changes Arianna Avanzini
` (9 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Arianna Avanzini @ 2014-05-05 15:54 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 attempts to implement 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>
---
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 187e071..1802b6e 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/setup.h>
#include <asm/platform.h>
#include <asm/psci.h>
@@ -740,6 +741,16 @@ static int map_device(struct domain *d, const 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] 45+ messages in thread
* [PATCH v7 02/10] arch/arm: add consistency check to REMOVE p2m changes
2014-05-05 15:54 [PATCH v7 00/10] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
2014-05-05 15:54 ` [PATCH v7 01/10] arch/arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
@ 2014-05-05 15:54 ` Arianna Avanzini
2014-05-06 16:51 ` Julien Grall
2014-05-05 15:54 ` [PATCH v7 03/10] arch/arm: let map_mmio_regions() take pfn as parameters Arianna Avanzini
` (8 subsequent siblings)
10 siblings, 1 reply; 45+ messages in thread
From: Arianna Avanzini @ 2014-05-05 15:54 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 attempts to add 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>
---
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 | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 403fd89..17b0635 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -406,12 +406,26 @@ 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) )
+ return -EINVAL;
+ }
+ /* fall through */
+ case RELINQUISH:
+ {
if ( !pte.p2m.valid )
{
count++;
@@ -450,6 +464,7 @@ static int apply_p2m_changes(struct domain *d,
/* Got the next page */
addr += PAGE_SIZE;
+ maddr += PAGE_SIZE;
}
if ( flush )
--
1.9.2
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v7 03/10] arch/arm: let map_mmio_regions() take pfn as parameters
2014-05-05 15:54 [PATCH v7 00/10] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
2014-05-05 15:54 ` [PATCH v7 01/10] arch/arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
2014-05-05 15:54 ` [PATCH v7 02/10] arch/arm: add consistency check to REMOVE p2m changes Arianna Avanzini
@ 2014-05-05 15:54 ` Arianna Avanzini
2014-05-05 15:54 ` [PATCH v7 04/10] arch/arm: let map_mmio_regions() use start and count Arianna Avanzini
` (7 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Arianna Avanzini @ 2014-05-05 15:54 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 attempts 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 1802b6e..fc3f110 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -751,9 +751,10 @@ static int map_device(struct domain *d, const 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 8168b7b..1eeeea4 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -888,20 +888,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 17b0635..f88cddc 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -510,12 +510,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 af3b71c..f3662fd 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] 45+ messages in thread
* [PATCH v7 04/10] arch/arm: let map_mmio_regions() use start and count
2014-05-05 15:54 [PATCH v7 00/10] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (2 preceding siblings ...)
2014-05-05 15:54 ` [PATCH v7 03/10] arch/arm: let map_mmio_regions() take pfn as parameters Arianna Avanzini
@ 2014-05-05 15:54 ` Arianna Avanzini
2014-05-05 18:55 ` Julien Grall
` (2 more replies)
2014-05-05 15:54 ` [PATCH v7 05/10] arch/x86: check if mapping exists before memory_mapping removes it Arianna Avanzini
` (6 subsequent siblings)
10 siblings, 3 replies; 45+ messages in thread
From: Arianna Avanzini @ 2014-05-05 15:54 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.
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/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 | 2 +-
xen/include/asm-arm/p2m.h | 7 ++++---
7 files changed, 17 insertions(+), 27 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index fc3f110..23874a8 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -753,7 +753,7 @@ static int map_device(struct domain *d, const struct dt_device_node *dev)
}
res = map_mmio_regions(d,
paddr_to_pfn(addr & PAGE_MASK),
- paddr_to_pfn_aligned(addr + size),
+ size / PAGE_SIZE + (size % PAGE_SIZE ? 1 : 0),
paddr_to_pfn(addr & PAGE_MASK));
if ( res )
{
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 1eeeea4..5a5f155 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -888,22 +888,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 f88cddc..2c7f542 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -511,12 +511,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 + 1),
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 f3662fd..a7f4c8b 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -48,7 +48,7 @@ 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, paddr_to_pfn(start),
- paddr_to_pfn_aligned(end),
+ paddr_to_pfn_aligned(end) - paddr_to_pfn(start) + 1,
paddr_to_pfn(start));
if ( ret )
printk("Failed to map %s @ %"PRIpaddr" to dom%d\n",
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] 45+ messages in thread
* [PATCH v7 05/10] arch/x86: check if mapping exists before memory_mapping removes it
2014-05-05 15:54 [PATCH v7 00/10] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (3 preceding siblings ...)
2014-05-05 15:54 ` [PATCH v7 04/10] arch/arm: let map_mmio_regions() use start and count Arianna Avanzini
@ 2014-05-05 15:54 ` Arianna Avanzini
2014-05-06 8:25 ` Jan Beulich
2014-05-05 15:54 ` [PATCH v7 06/10] xen/x86: factor out map and unmap from the memory_mapping DOMCTL Arianna Avanzini
` (5 subsequent siblings)
10 siblings, 1 reply; 45+ messages in thread
From: Arianna Avanzini @ 2014-05-05 15:54 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 attempts to add 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>
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:
- 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 ae29a56..c466063 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 1d0528b..a99e222 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -801,10 +801,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);
@@ -813,15 +813,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 86847e9..e901085 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -513,7 +513,7 @@ void p2m_memory_type_changed(struct domain *d);
/* 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] 45+ messages in thread
* [PATCH v7 06/10] xen/x86: factor out map and unmap from the memory_mapping DOMCTL
2014-05-05 15:54 [PATCH v7 00/10] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (4 preceding siblings ...)
2014-05-05 15:54 ` [PATCH v7 05/10] arch/x86: check if mapping exists before memory_mapping removes it Arianna Avanzini
@ 2014-05-05 15:54 ` Arianna Avanzini
2014-05-06 8:35 ` Jan Beulich
2014-05-05 15:54 ` [PATCH v7 07/10] xen/common: move the memory_mapping DOMCTL hypercall to common code Arianna Avanzini
` (4 subsequent siblings)
10 siblings, 1 reply; 45+ messages in thread
From: Arianna Avanzini @ 2014-05-05 15:54 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>
---
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 | 45 +++++++++++++++++++--------------------------
xen/arch/x86/mm/p2m.c | 41 +++++++++++++++++++++++++++++++++++++++++
xen/include/asm-x86/p2m.h | 12 ++++++++++++
3 files changed, 72 insertions(+), 26 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index c466063..daaa371 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -646,19 +646,21 @@ 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;
+ unsigned long gfn_end = gfn + 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)) ||
- (gfn + nr_mfns - 1) < gfn ) /* wrap? */
+ if ( mfn_end < mfn || /* wrap? */
+ ((mfn | mfn_end) >> (paddr_bits - PAGE_SHIFT)) ||
+ gfn_end < 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,49 +670,40 @@ 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);
- if ( !ret && paging_mode_translate(d) )
+ ret = iomem_permit_access(d, mfn, mfn_end);
+ 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));
- if ( iomem_deny_access(d, mfn, mfn + nr_mfns - 1) &&
+ 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 + nr_mfns - 1);
+ d->domain_id, mfn, mfn_end);
}
}
}
else
{
- int tmp_rc = 0;
+ int rc;
printk(XENLOG_G_INFO
"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 )
- tmp_rc = ret;
- }
- ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
+ rc = unmap_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
+ 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);
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index a99e222..65fc8d9 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1650,6 +1650,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);
+ ret = -EIO;
+ }
+
+ 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));
+
+ if ( ret )
+ return -EIO;
+ return 0;
+}
+
/*** Audit ***/
#if P2M_AUDIT
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index e901085..e712916 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] 45+ messages in thread
* [PATCH v7 07/10] xen/common: move the memory_mapping DOMCTL hypercall to common code
2014-05-05 15:54 [PATCH v7 00/10] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (5 preceding siblings ...)
2014-05-05 15:54 ` [PATCH v7 06/10] xen/x86: factor out map and unmap from the memory_mapping DOMCTL Arianna Avanzini
@ 2014-05-05 15:54 ` Arianna Avanzini
2014-05-06 8:40 ` Jan Beulich
2014-05-06 16:54 ` Julien Grall
2014-05-05 15:54 ` [PATCH v7 08/10] tools/libxl: parse optional start gfn from the iomem config option Arianna Avanzini
` (3 subsequent siblings)
10 siblings, 2 replies; 45+ messages in thread
From: Arianna Avanzini @ 2014-05-05 15:54 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. This commit
also adds to ARM-related code an unmap_mmio_regions() function to match
the corresponding one available for x86.
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:
- Pass p2m_invalid as last parameter to unmap_mmio_regions() for ARM
as it is not (and currently must not be) used.
- 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/arm/p2m.c | 12 ++++++++
xen/arch/x86/domctl.c | 69 ------------------------------------------
xen/common/domctl.c | 71 ++++++++++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/p2m.h | 13 ++++----
xen/include/asm-x86/p2m.h | 13 +-------
xen/include/xen/p2m-common.h | 16 ++++++++++
6 files changed, 106 insertions(+), 88 deletions(-)
create mode 100644 xen/include/xen/p2m-common.h
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 2c7f542..d2568ad 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -521,6 +521,18 @@ int map_mmio_regions(struct domain *d,
MATTR_DEV, p2m_mmio_direct);
}
+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 + 1),
+ pfn_to_paddr(mfn),
+ MATTR_DEV, p2m_invalid);
+}
+
int guest_physmap_add_entry(struct domain *d,
unsigned long gpfn,
unsigned long mfn,
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index daaa371..55f0605 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -641,75 +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;
- unsigned long gfn_end = gfn + 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_end < 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;
-
- 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:
{
#define MAX_IOPORTS 0x10000
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index af3614b..866338b 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -822,6 +822,77 @@ 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;
+ unsigned long gfn_end = gfn + 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_end < 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;
+
+ 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);
+ }
+#ifdef CONFIG_X86
+ /* Do this unconditionally to cover errors on above failure paths. */
+ memory_type_changed(d);
+#endif
+ }
+ 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 6d56daa..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,13 +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 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 e712916..9b0f08b 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] 45+ messages in thread
* [PATCH v7 08/10] tools/libxl: parse optional start gfn from the iomem config option
2014-05-05 15:54 [PATCH v7 00/10] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (6 preceding siblings ...)
2014-05-05 15:54 ` [PATCH v7 07/10] xen/common: move the memory_mapping DOMCTL hypercall to common code Arianna Avanzini
@ 2014-05-05 15:54 ` Arianna Avanzini
2014-05-05 15:54 ` [PATCH v7 09/10] tools/libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
` (2 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Arianna Avanzini @ 2014-05-05 15:54 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 c8ce6c1..96c2b67 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 b2c3015..a78944d 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -95,6 +95,16 @@
#define LIBXL_HAVE_BUILDINFO_EVENT_CHANNELS 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 0f7bbf8..997cadd 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 0b38b32..9a88dcb 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] 45+ messages in thread
* [PATCH v7 09/10] tools/libxl: handle the iomem parameter with the memory_mapping hcall
2014-05-05 15:54 [PATCH v7 00/10] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (7 preceding siblings ...)
2014-05-05 15:54 ` [PATCH v7 08/10] tools/libxl: parse optional start gfn from the iomem config option Arianna Avanzini
@ 2014-05-05 15:54 ` Arianna Avanzini
2014-05-06 8:44 ` Jan Beulich
2014-05-07 11:16 ` Ian Campbell
2014-05-05 15:54 ` [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory Arianna Avanzini
2014-05-06 8:21 ` [PATCH v7 00/10] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Jan Beulich
10 siblings, 2 replies; 45+ messages in thread
From: Arianna Avanzini @ 2014-05-05 15:54 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>
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:
- 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..2de0d11 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 not 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] 45+ messages in thread
* [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory
2014-05-05 15:54 [PATCH v7 00/10] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (8 preceding siblings ...)
2014-05-05 15:54 ` [PATCH v7 09/10] tools/libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
@ 2014-05-05 15:54 ` Arianna Avanzini
2014-05-06 9:06 ` Jan Beulich
2014-05-06 8:21 ` [PATCH v7 00/10] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Jan Beulich
10 siblings, 1 reply; 45+ messages in thread
From: Arianna Avanzini @ 2014-05-05 15:54 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 attempts to separate 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.
This commit also attempts to change existing code to be sure to grant
access permission to PCI-related I/O memory ranges (for passthrough of
PCI devices specified in the domain's configuration) and to VGA-related
memory ranges (for VGA passthrough, if gfx_passthru = 1 in the domain
configuration). As of the latter, VGA needs some extra memory ranges to
be mapped with respect to PCI; the access to those memory ranges was
previously given implicitly when calling xc_domain_memory_mapping, while
now it must be explicitly 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>
---
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.
- Do not handle I/O ports and I/O memory differently when allowing
to a domain to access a PCI device.
- 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 +++++++++--------------
xen/common/domctl.c | 51 +++++++++++++++++-----------------------------
3 files changed, 46 insertions(+), 48 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 */
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 866338b..8ee72eb 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -803,18 +803,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
{
unsigned long mfn = op->u.iomem_permission.first_mfn;
unsigned long nr_mfns = op->u.iomem_permission.nr_mfns;
+ unsigned long mfn_end = mfn + nr_mfns - 1;
int allow = op->u.iomem_permission.allow_access;
ret = -EINVAL;
- if ( (mfn + nr_mfns - 1) < mfn ) /* wrap? */
+ if ( mfn_end < mfn ) /* wrap? */
break;
- if ( xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, allow) )
- ret = -EPERM;
- else if ( allow )
- ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1);
+ ret = -EPERM;
+ if ( !iomem_access_permitted(current->domain, mfn, mfn_end) ||
+ xsm_iomem_permission(XSM_HOOK, d, mfn, mfn_end, allow) )
+ break;
+
+ if ( allow )
+ ret = iomem_permit_access(d, mfn, mfn_end);
else
- ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
+ ret = iomem_deny_access(d, mfn, mfn_end);
#ifdef CONFIG_X86
if ( !ret )
memory_type_changed(d);
@@ -838,7 +842,7 @@ 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(d, mfn, mfn_end) )
break;
ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
@@ -851,40 +855,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;
-
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);
}
#ifdef CONFIG_X86
/* Do this unconditionally to cover errors on above failure paths. */
--
1.9.2
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH v7 04/10] arch/arm: let map_mmio_regions() use start and count
2014-05-05 15:54 ` [PATCH v7 04/10] arch/arm: let map_mmio_regions() use start and count Arianna Avanzini
@ 2014-05-05 18:55 ` Julien Grall
2014-05-07 11:03 ` Ian Campbell
2014-05-19 13:47 ` Julien Grall
2 siblings, 0 replies; 45+ messages in thread
From: Julien Grall @ 2014-05-05 18:55 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,
Thank you for the patch.
On 05/05/14 16:54, 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.
>
> 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/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 | 2 +-
> xen/include/asm-arm/p2m.h | 7 ++++---
> 7 files changed, 17 insertions(+), 27 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index fc3f110..23874a8 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -753,7 +753,7 @@ static int map_device(struct domain *d, const struct dt_device_node *dev)
> }
> res = map_mmio_regions(d,
> paddr_to_pfn(addr & PAGE_MASK),
> - paddr_to_pfn_aligned(addr + size),
> + size / PAGE_SIZE + (size % PAGE_SIZE ? 1 : 0),
Would not it be easier to use paddr_to_pfn(PAGE_ALIGN(size))?
> return 0;
> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
> index f3662fd..a7f4c8b 100644
> --- a/xen/arch/arm/platforms/xgene-storm.c
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -48,7 +48,7 @@ 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, paddr_to_pfn(start),
> - paddr_to_pfn_aligned(end),
> + paddr_to_pfn_aligned(end) - paddr_to_pfn(start) + 1,
I would directly give the pfn in argument of map_one_mmio.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 00/10] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM
2014-05-05 15:54 [PATCH v7 00/10] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (9 preceding siblings ...)
2014-05-05 15:54 ` [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory Arianna Avanzini
@ 2014-05-06 8:21 ` Jan Beulich
10 siblings, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2014-05-06 8:21 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 05.05.14 at 17:54, <avanzini.arianna@gmail.com> wrote:
> The code has again been tested on a cubieboard2, with Linux v3.13 as a dom0
> and ERIKA Enterprise ([2]) as a domU.
And hopefully the x86 parts of it also got tested...
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 05/10] arch/x86: check if mapping exists before memory_mapping removes it
2014-05-05 15:54 ` [PATCH v7 05/10] arch/x86: check if mapping exists before memory_mapping removes it Arianna Avanzini
@ 2014-05-06 8:25 ` Jan Beulich
0 siblings, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2014-05-06 8:25 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 05.05.14 at 17:54, <avanzini.arianna@gmail.com> 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 attempts to add 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>
With the title changed to describe the actual behavior
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: 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:
> - 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 ae29a56..c466063 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 1d0528b..a99e222 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -801,10 +801,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);
> @@ -813,15 +813,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 86847e9..e901085 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -513,7 +513,7 @@ void p2m_memory_type_changed(struct domain *d);
>
> /* 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 [flat|nested] 45+ messages in thread
* Re: [PATCH v7 06/10] xen/x86: factor out map and unmap from the memory_mapping DOMCTL
2014-05-05 15:54 ` [PATCH v7 06/10] xen/x86: factor out map and unmap from the memory_mapping DOMCTL Arianna Avanzini
@ 2014-05-06 8:35 ` Jan Beulich
0 siblings, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2014-05-06 8:35 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 05.05.14 at 17:54, <avanzini.arianna@gmail.com> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -646,19 +646,21 @@ 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;
> + unsigned long gfn_end = gfn + 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)) ||
> - (gfn + nr_mfns - 1) < gfn ) /* wrap? */
> + if ( mfn_end < mfn || /* wrap? */
> + ((mfn | mfn_end) >> (paddr_bits - PAGE_SHIFT)) ||
> + gfn_end < 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;
>
With the switch to passing nr_mfns to the helper functions all of the
above and some of the change further down (i.e. anything substituting
[gm]fn_end are now unrelated to the patch. If you feel strongly about
doing this replacement, this should be a separate patch, making the
review of this one easier.
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1650,6 +1650,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));
The |= it pointless considering the loop termination condition. Just
use =.
> + if ( ret )
> + {
> + unmap_mmio_regions(d, start_gfn, start_gfn + i, mfn);
> + ret = -EIO;
And without using |= there's then also no point in overriding the
return value from set_mmio_p2m_entry().
> +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));
> +
> + if ( ret )
> + return -EIO;
Same here regarding the return value override: Avoid the |= and
instead latch either the first or last error (if any), just like done by
the original code.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 07/10] xen/common: move the memory_mapping DOMCTL hypercall to common code
2014-05-05 15:54 ` [PATCH v7 07/10] xen/common: move the memory_mapping DOMCTL hypercall to common code Arianna Avanzini
@ 2014-05-06 8:40 ` Jan Beulich
2014-05-07 11:09 ` Ian Campbell
2014-05-07 11:10 ` Ian Campbell
2014-05-06 16:54 ` Julien Grall
1 sibling, 2 replies; 45+ messages in thread
From: Jan Beulich @ 2014-05-06 8:40 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 05.05.14 at 17:54, <avanzini.arianna@gmail.com> wrote:
> 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.
Was this requested by one of the ARM maintainers? Do memory types
not exist at all on ARM? I would have expected the function to be
empty only until someone would get to implement it properly... Other
architectures (ia64 at least, since that's the one I know next best after
x86) would surely have needed this too.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 09/10] tools/libxl: handle the iomem parameter with the memory_mapping hcall
2014-05-05 15:54 ` [PATCH v7 09/10] tools/libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
@ 2014-05-06 8:44 ` Jan Beulich
2014-05-07 11:16 ` Ian Campbell
1 sibling, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2014-05-06 8:44 UTC (permalink / raw)
To: Arianna Avanzini, xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Ian.Jackson, tim, julien.grall,
etrudeau, viktor.kleinik
>>> On 05.05.14 at 17:54, <avanzini.arianna@gmail.com> wrote:
> --- 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 not available for auto-translated domains");
s/not/only/?
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory
2014-05-05 15:54 ` [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory Arianna Avanzini
@ 2014-05-06 9:06 ` Jan Beulich
2014-05-10 1:10 ` Arianna Avanzini
2014-05-25 17:14 ` Julien Grall
0 siblings, 2 replies; 45+ messages in thread
From: Jan Beulich @ 2014-05-06 9: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 05.05.14 at 17:54, <avanzini.arianna@gmail.com> wrote:
> tools/libxl/libxl_create.c | 17 ++++++++++++++++
> tools/libxl/libxl_pci.c | 26 +++++++++--------------
> xen/common/domctl.c | 51 +++++++++++++++++-----------------------------
First of all - is it (from a functionality pov) really necessary to mix
tools and hypervisor changes here.
> --- 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;
> + }
> + }
While you added some text to the description regarding this change,
it still remains unclear why this is really needed, since no other code
is being removed in its place. So my minimum expectation here would
be for you to point out which code this replaces/duplicates, and why
the original needs to remain in place.
Furthermore (I'm not sure if the same mistake is being done
elsewhere, you may not be the original one to blame for this) I think
it is a mistake to mix up VGA and graphics pass-through: When you
want a graphics card (usually the primary one) to behave as VGA, it
needs the legacy VGA memory and I/O port ranges to be under its
control. Any other (usually secondary) card would not need this, as
can be easily seen by the otherwise resulting conflict if you pass
through multiple graphics cards to distinct domains.
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -803,18 +803,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> {
> unsigned long mfn = op->u.iomem_permission.first_mfn;
> unsigned long nr_mfns = op->u.iomem_permission.nr_mfns;
> + unsigned long mfn_end = mfn + nr_mfns - 1;
> int allow = op->u.iomem_permission.allow_access;
>
> ret = -EINVAL;
> - if ( (mfn + nr_mfns - 1) < mfn ) /* wrap? */
> + if ( mfn_end < mfn ) /* wrap? */
> break;
>
> - if ( xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, allow) )
> - ret = -EPERM;
> - else if ( allow )
> - ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1);
> + ret = -EPERM;
> + if ( !iomem_access_permitted(current->domain, mfn, mfn_end) ||
> + xsm_iomem_permission(XSM_HOOK, d, mfn, mfn_end, allow) )
> + break;
> +
> + if ( allow )
> + ret = iomem_permit_access(d, mfn, mfn_end);
> else
I'd prefer this to remain an if/else-if sequence, like done in
http://lists.xenproject.org/archives/html/xen-devel/2014-04/msg03988.html.
Perhaps that patch (once I get to submit v2) could serve as a
prereq for this change of yours?
> @@ -838,7 +842,7 @@ 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(d, mfn, mfn_end) )
I'm not sure if we shouldn't rather be conservative here and check
both domains' permissions.
And now that I reached the end of the patch I'm still missing the
similar adjustments for I/O port handling, while I think I saw you
claim somewhere that in this version you did deal with that.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 02/10] arch/arm: add consistency check to REMOVE p2m changes
2014-05-05 15:54 ` [PATCH v7 02/10] arch/arm: add consistency check to REMOVE p2m changes Arianna Avanzini
@ 2014-05-06 16:51 ` Julien Grall
2014-05-06 16:52 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2014-05-06 16:51 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 05/05/2014 04:54 PM, Arianna Avanzini wrote:
> 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 attempts to add 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>
>
> ---
>
> v7:
> - Silently ignore the fact that, when removing a mapping, the specified
> gfn is not mapped at all.
I think you misunderstood my previous comment. I didn't ask to remove
"maddr += ...". This code was right. Now the failure (i.e the MFN
doesn't match the GFN) is obscure.
On x86, Xen will continue to unmap even if we fail to remove one entry.
Of course, it will return an error at the end.
> ---
> xen/arch/arm/p2m.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 403fd89..17b0635 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -406,12 +406,26 @@ 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) )
> + return -EINVAL;
I didn't catch it until now, this is wrong. In case of an error you have to:
- unmap PT table mapping
- unlock p2m lock
- flush the TLBs (if we successfully removed other mapping)
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 02/10] arch/arm: add consistency check to REMOVE p2m changes
2014-05-06 16:51 ` Julien Grall
@ 2014-05-06 16:52 ` Julien Grall
0 siblings, 0 replies; 45+ messages in thread
From: Julien Grall @ 2014-05-06 16:52 UTC (permalink / raw)
To: Arianna Avanzini, xen-devel
Cc: paolo.valente, keir, stefano.stabellini, tim, dario.faggioli,
Ian.Jackson, Ian.Campbell, etrudeau, JBeulich, andrew.cooper3,
viktor.kleinik
On 05/06/2014 05:51 PM, Julien Grall wrote:
> Hi Arianna,
>
> On 05/05/2014 04:54 PM, Arianna Avanzini wrote:
>> 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 attempts to add 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>
>>
>> ---
>>
>> v7:
>> - Silently ignore the fact that, when removing a mapping, the specified
>> gfn is not mapped at all.
>
> I think you misunderstood my previous comment. I didn't ask to remove
> "maddr += ...". This code was right. Now the failure (i.e the MFN
> doesn't match the GFN) is obscure.
>
> On x86, Xen will continue to unmap even if we fail to remove one entry.
> Of course, it will return an error at the end.
I forgot to add: it would be nice to have some print message as x86 does.
--
Julien Grall
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 07/10] xen/common: move the memory_mapping DOMCTL hypercall to common code
2014-05-05 15:54 ` [PATCH v7 07/10] xen/common: move the memory_mapping DOMCTL hypercall to common code Arianna Avanzini
2014-05-06 8:40 ` Jan Beulich
@ 2014-05-06 16:54 ` Julien Grall
2014-05-10 1:20 ` Arianna Avanzini
1 sibling, 1 reply; 45+ messages in thread
From: Julien Grall @ 2014-05-06 16:54 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 05/05/2014 04:54 PM, Arianna Avanzini wrote:
> + ret = iomem_permit_access(d, mfn, mfn_end);
> + if ( !ret )
> + {
> + ret = map_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
The code of map_mmio_regions differs between x86 on ARM.
On the former architecture, if Xen fails to map a page, it will unmap
all the previous page. It's not the case on ARM.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 04/10] arch/arm: let map_mmio_regions() use start and count
2014-05-05 15:54 ` [PATCH v7 04/10] arch/arm: let map_mmio_regions() use start and count Arianna Avanzini
2014-05-05 18:55 ` Julien Grall
@ 2014-05-07 11:03 ` Ian Campbell
2014-05-19 13:47 ` Julien Grall
2 siblings, 0 replies; 45+ messages in thread
From: Ian Campbell @ 2014-05-07 11:03 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 Mon, 2014-05-05 at 17:54 +0200, Arianna Avanzini wrote:
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index fc3f110..23874a8 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -753,7 +753,7 @@ static int map_device(struct domain *d, const struct dt_device_node *dev)
> }
> res = map_mmio_regions(d,
> paddr_to_pfn(addr & PAGE_MASK),
> - paddr_to_pfn_aligned(addr + size),
> + size / PAGE_SIZE + (size % PAGE_SIZE ? 1 : 0),
Is this trying to do ROUNDUP? Or perhaps DIV_ROUND_UP?
Ian.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 07/10] xen/common: move the memory_mapping DOMCTL hypercall to common code
2014-05-06 8:40 ` Jan Beulich
@ 2014-05-07 11:09 ` Ian Campbell
2014-05-10 0:26 ` Arianna Avanzini
2014-05-07 11:10 ` Ian Campbell
1 sibling, 1 reply; 45+ messages in thread
From: Ian Campbell @ 2014-05-07 11:09 UTC (permalink / raw)
To: Jan Beulich
Cc: tim, paolo.valente, keir, stefano.stabellini, andrew.cooper3,
dario.faggioli, Ian.Jackson, xen-devel, julien.grall, etrudeau,
Arianna Avanzini, viktor.kleinik
On Tue, 2014-05-06 at 09:40 +0100, Jan Beulich wrote:
> >>> On 05.05.14 at 17:54, <avanzini.arianna@gmail.com> wrote:
> > 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.
>
> Was this requested by one of the ARM maintainers?
I don't think it was me.
> Do memory types not exist at all on ARM?
This is types in "MTRR" sense rather than p2m type sense I think? These
are part of the PT mappings, so I would expect something like this to
probably be needed for any IOMMU stuff at some point.
> I would have expected the function to be
> empty only until someone would get to implement it properly...
Me too.
> Other
> architectures (ia64 at least, since that's the one I know next best after
> x86) would surely have needed this too.
>
> Jan
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 07/10] xen/common: move the memory_mapping DOMCTL hypercall to common code
2014-05-06 8:40 ` Jan Beulich
2014-05-07 11:09 ` Ian Campbell
@ 2014-05-07 11:10 ` Ian Campbell
1 sibling, 0 replies; 45+ messages in thread
From: Ian Campbell @ 2014-05-07 11:10 UTC (permalink / raw)
To: Jan Beulich
Cc: tim, paolo.valente, keir, stefano.stabellini, andrew.cooper3,
dario.faggioli, Ian.Jackson, xen-devel, julien.grall, etrudeau,
Arianna Avanzini, viktor.kleinik
On Tue, 2014-05-06 at 09:40 +0100, Jan Beulich wrote:
> >>> On 05.05.14 at 17:54, <avanzini.arianna@gmail.com> wrote:
> > 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.
>
> Was this requested by one of the ARM maintainers?
I don't think it was me.
> Do memory types not exist at all on ARM?
This is types in "MTRR" sense rather than p2m type sense I think? These
are part of the PT mappings, so I would expect something like this to
probably be needed for any IOMMU stuff at some point.
> I would have expected the function to be
> empty only until someone would get to implement it properly...
Me too.
> Other
> architectures (ia64 at least, since that's the one I know next best after
> x86) would surely have needed this too.
>
> Jan
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 09/10] tools/libxl: handle the iomem parameter with the memory_mapping hcall
2014-05-05 15:54 ` [PATCH v7 09/10] tools/libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
2014-05-06 8:44 ` Jan Beulich
@ 2014-05-07 11:16 ` Ian Campbell
1 sibling, 0 replies; 45+ messages in thread
From: Ian Campbell @ 2014-05-07 11:16 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 Mon, 2014-05-05 at 17:54 +0200, Arianna Avanzini wrote:
> 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>
> 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>
xc_core_arch_auto_translated_physmap is already pretty strangely named,
but lets not shave that yack now:
Acked-by: 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:
> - 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..2de0d11 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 not 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;
> }
> }
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 07/10] xen/common: move the memory_mapping DOMCTL hypercall to common code
2014-05-07 11:09 ` Ian Campbell
@ 2014-05-10 0:26 ` Arianna Avanzini
2014-05-12 8:29 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Arianna Avanzini @ 2014-05-10 0:26 UTC (permalink / raw)
To: Ian Campbell, Jan Beulich
Cc: tim, paolo.valente, keir, stefano.stabellini, andrew.cooper3,
dario.faggioli, Ian.Jackson, xen-devel, julien.grall, etrudeau,
viktor.kleinik
On 05/07/2014 01:09 PM, Ian Campbell wrote:
> On Tue, 2014-05-06 at 09:40 +0100, Jan Beulich wrote:
>>>>> On 05.05.14 at 17:54, <avanzini.arianna@gmail.com> wrote:
>>> 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.
>>
>> Was this requested by one of the ARM maintainers?
>
> I don't think it was me.
>
>> Do memory types not exist at all on ARM?
>
> This is types in "MTRR" sense rather than p2m type sense I think? These
> are part of the PT mappings, so I would expect something like this to
> probably be needed for any IOMMU stuff at some point.
>
>> I would have expected the function to be
>> empty only until someone would get to implement it properly...
>
> Me too.
>
Yes, in the previous version I had defined an empty stub for a
memory_type_changed() function for ARM. While trying to move its prototype to a
common header according to Jan Beulich's suggestion, however, I have noticed
that the invocation of that same function, performed from the common code
handling the iomem_permission domctl, is ifdef'd out to be compiled only on an
x86 machine. I therefore thought to do the same for the memory_mapping domctl.
Do you prefer that an empty stub is added? If so, would you like also the
handling of memory_type_changed() in iomem_permission to be modified in this
patchset?
>> Other
>> architectures (ia64 at least, since that's the one I know next best after
>> x86) would surely have needed this too.
>>
>> Jan
>>
>
>
--
/*
* Arianna Avanzini
* avanzini.arianna@gmail.com
* 73628@studenti.unimore.it
*/
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory
2014-05-06 9:06 ` Jan Beulich
@ 2014-05-10 1:10 ` Arianna Avanzini
2014-05-12 8:35 ` Jan Beulich
2014-05-25 17:14 ` Julien Grall
1 sibling, 1 reply; 45+ messages in thread
From: Arianna Avanzini @ 2014-05-10 1:10 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, viktor.kleinik
On 05/06/2014 11:06 AM, Jan Beulich wrote:
>>>> On 05.05.14 at 17:54, <avanzini.arianna@gmail.com> wrote:
>> tools/libxl/libxl_create.c | 17 ++++++++++++++++
>> tools/libxl/libxl_pci.c | 26 +++++++++--------------
>> xen/common/domctl.c | 51 +++++++++++++++++-----------------------------
>
> First of all - is it (from a functionality pov) really necessary to mix
> tools and hypervisor changes here.
>
I think it's not. Thank you for pointing that out, I will split the patch
according to your suggestion.
>> --- 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;
>> + }
>> + }
>
> While you added some text to the description regarding this change,
> it still remains unclear why this is really needed, since no other code
> is being removed in its place. So my minimum expectation here would
> be for you to point out which code this replaces/duplicates, and why
> the original needs to remain in place.
>
Right, the commit description is still very confused, thank you for pointing
that out.
QEMU seems to rely only on the memory_mapping domctl to map VGA-related memory
areas in case gfx passthru is enabled, if I'm seeing things correctly. The
xc_domain_memory_mapping() function is invoked from the register_vga_regions()
function (defined in hw/pt-graphics.c). The latter function seems to be executed
upon registration of a physical device (register_real_device() ->
pt_register_regions() in hw/pass-through.c), and to be actually executed only if
gfx passthru is enabled for the device (if it is not, the function seems to
immediately return without performing any mapping operation of I/O memory or ports).
Since this patch aims at separating the functions of the memory_mapping and
iomem_permission domctls, memory_mapping does not implicitly grants permission
to mapped I/O-memory ranges; having QEMU invoking just the memory_mapping domctl
would lead to a failure. This hunk was supposed to allow to the domain access to
the necessary VGA-related memory ranges in case gfx passthru is enabled by
domain configuration.
> Furthermore (I'm not sure if the same mistake is being done
> elsewhere, you may not be the original one to blame for this) I think
> it is a mistake to mix up VGA and graphics pass-through: When you
> want a graphics card (usually the primary one) to behave as VGA, it
> needs the legacy VGA memory and I/O port ranges to be under its
> control. Any other (usually secondary) card would not need this, as
> can be easily seen by the otherwise resulting conflict if you pass
> through multiple graphics cards to distinct domains.
>
I see, thank you for the clear explanation. Unfortunately I just relied on the
QEMU code, which seems to execute the function (and map those memory ranges) for
all devices, provided that gfx passthru is enabled and there is a VGA
controller. I most probably overlooked something.
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -803,18 +803,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>> {
>> unsigned long mfn = op->u.iomem_permission.first_mfn;
>> unsigned long nr_mfns = op->u.iomem_permission.nr_mfns;
>> + unsigned long mfn_end = mfn + nr_mfns - 1;
>> int allow = op->u.iomem_permission.allow_access;
>>
>> ret = -EINVAL;
>> - if ( (mfn + nr_mfns - 1) < mfn ) /* wrap? */
>> + if ( mfn_end < mfn ) /* wrap? */
>> break;
>>
>> - if ( xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, allow) )
>> - ret = -EPERM;
>> - else if ( allow )
>> - ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1);
>> + ret = -EPERM;
>> + if ( !iomem_access_permitted(current->domain, mfn, mfn_end) ||
>> + xsm_iomem_permission(XSM_HOOK, d, mfn, mfn_end, allow) )
>> + break;
>> +
>> + if ( allow )
>> + ret = iomem_permit_access(d, mfn, mfn_end);
>> else
>
> I'd prefer this to remain an if/else-if sequence, like done in
> http://lists.xenproject.org/archives/html/xen-devel/2014-04/msg03988.html.
> Perhaps that patch (once I get to submit v2) could serve as a
> prereq for this change of yours?
>
Certainly, thank you for the pointer.
>> @@ -838,7 +842,7 @@ 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(d, mfn, mfn_end) )
>
> I'm not sure if we shouldn't rather be conservative here and check
> both domains' permissions.
>
Sure, thank you for the suggestion.
> And now that I reached the end of the patch I'm still missing the
> similar adjustments for I/O port handling, while I think I saw you
> claim somewhere that in this version you did deal with that.
>
The previous version of the patch gave access permission to I/O ports (in
do_pci_add()) only to paravirtualized domains. This version of the patch
executes also for HVM domains all the code that was previously executed only for
PV domains, including the invocation of ioport_permission. As Ian Campbell
suggested (and hoping I understood his suggestion correctly), it does:
if (hvm)
device_model_related_code();
previously_pv_specific_code();
> Jan
>
--
/*
* Arianna Avanzini
* avanzini.arianna@gmail.com
* 73628@studenti.unimore.it
*/
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 07/10] xen/common: move the memory_mapping DOMCTL hypercall to common code
2014-05-06 16:54 ` Julien Grall
@ 2014-05-10 1:20 ` Arianna Avanzini
2014-05-10 9:03 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Arianna Avanzini @ 2014-05-10 1:20 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 05/06/2014 06:54 PM, Julien Grall wrote:
> Hi Arianna,
>
> On 05/05/2014 04:54 PM, Arianna Avanzini wrote:
>> + ret = iomem_permit_access(d, mfn, mfn_end);
>> + if ( !ret )
>> + {
>> + ret = map_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
>
> The code of map_mmio_regions differs between x86 on ARM.
>
> On the former architecture, if Xen fails to map a page, it will unmap
> all the previous page. It's not the case on ARM.
>
The map_mmio_regions() implementation for x86 (factored out from the
x86-specific code in patch 0006) includes also unmapping the pages in the case
of an error (even if I just realized that I made a mistake there by still using
the end gfn instead of the number of gfns). The ARM implementation is still the
original one, calling apply_p2m_changes().
> Regards,
>
--
/*
* Arianna Avanzini
* avanzini.arianna@gmail.com
* 73628@studenti.unimore.it
*/
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 07/10] xen/common: move the memory_mapping DOMCTL hypercall to common code
2014-05-10 1:20 ` Arianna Avanzini
@ 2014-05-10 9:03 ` Julien Grall
0 siblings, 0 replies; 45+ messages in thread
From: Julien Grall @ 2014-05-10 9:03 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 10/05/14 02:20, Arianna Avanzini wrote:
> On 05/06/2014 06:54 PM, Julien Grall wrote:
>> Hi Arianna,
>>
>> On 05/05/2014 04:54 PM, Arianna Avanzini wrote:
>>> + ret = iomem_permit_access(d, mfn, mfn_end);
>>> + if ( !ret )
>>> + {
>>> + ret = map_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
>>
>> The code of map_mmio_regions differs between x86 on ARM.
>>
>> On the former architecture, if Xen fails to map a page, it will unmap
>> all the previous page. It's not the case on ARM.
>>
>
> The map_mmio_regions() implementation for x86 (factored out from the
> x86-specific code in patch 0006) includes also unmapping the pages in the case
> of an error (even if I just realized that I made a mistake there by still using
> the end gfn instead of the number of gfns). The ARM implementation is still the
> original one, calling apply_p2m_changes().
For hypercall like memory_mapping DOMCTL, failing to map the whole
region doesn't necessary mean the domain will be destroyed right after.
So it's not possible to have the region partially mapped in the guest.
I think you should call unmap in map_mmio_regions if Xen fails to map
the whole region.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 07/10] xen/common: move the memory_mapping DOMCTL hypercall to common code
2014-05-10 0:26 ` Arianna Avanzini
@ 2014-05-12 8:29 ` Jan Beulich
0 siblings, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2014-05-12 8:29 UTC (permalink / raw)
To: Ian Campbell, Arianna Avanzini
Cc: tim, paolo.valente, keir, stefano.stabellini, andrew.cooper3,
dario.faggioli, Ian.Jackson, xen-devel, julien.grall, etrudeau,
viktor.kleinik
>>> On 10.05.14 at 02:26, <avanzini.arianna@gmail.com> wrote:
> On 05/07/2014 01:09 PM, Ian Campbell wrote:
>> On Tue, 2014-05-06 at 09:40 +0100, Jan Beulich wrote:
>>>>>> On 05.05.14 at 17:54, <avanzini.arianna@gmail.com> wrote:
>>>> 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.
>>>
>>> Was this requested by one of the ARM maintainers?
>>
>> I don't think it was me.
>>
>>> Do memory types not exist at all on ARM?
>>
>> This is types in "MTRR" sense rather than p2m type sense I think? These
>> are part of the PT mappings, so I would expect something like this to
>> probably be needed for any IOMMU stuff at some point.
>>
>>> I would have expected the function to be
>>> empty only until someone would get to implement it properly...
>>
>> Me too.
>>
>
> Yes, in the previous version I had defined an empty stub for a
> memory_type_changed() function for ARM. While trying to move its prototype
> to a
> common header according to Jan Beulich's suggestion, however, I have noticed
> that the invocation of that same function, performed from the common code
> handling the iomem_permission domctl, is ifdef'd out to be compiled only on
> an
> x86 machine. I therefore thought to do the same for the memory_mapping
> domctl.
>
> Do you prefer that an empty stub is added? If so, would you like also the
> handling of memory_type_changed() in iomem_permission to be modified in this
> patchset?
I think that would make sense.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory
2014-05-10 1:10 ` Arianna Avanzini
@ 2014-05-12 8:35 ` Jan Beulich
0 siblings, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2014-05-12 8:35 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 10.05.14 at 03:10, <avanzini.arianna@gmail.com> wrote:
> On 05/06/2014 11:06 AM, Jan Beulich wrote:
>> And now that I reached the end of the patch I'm still missing the
>> similar adjustments for I/O port handling, while I think I saw you
>> claim somewhere that in this version you did deal with that.
>>
>
> The previous version of the patch gave access permission to I/O ports (in
> do_pci_add()) only to paravirtualized domains. This version of the patch
> executes also for HVM domains all the code that was previously executed only
> for
> PV domains, including the invocation of ioport_permission. As Ian Campbell
> suggested (and hoping I understood his suggestion correctly), it does:
> if (hvm)
> device_model_related_code();
> previously_pv_specific_code();
Now, I was talking about the hypervisor code only, whereas your
reply seems to refer to tools side code. I.e. I was expecting the
MMIO related changes to the domctl handling to get suitably
mirrored to the similar but not identical I/O port handling.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 04/10] arch/arm: let map_mmio_regions() use start and count
2014-05-05 15:54 ` [PATCH v7 04/10] arch/arm: let map_mmio_regions() use start and count Arianna Avanzini
2014-05-05 18:55 ` Julien Grall
2014-05-07 11:03 ` Ian Campbell
@ 2014-05-19 13:47 ` Julien Grall
2 siblings, 0 replies; 45+ messages in thread
From: Julien Grall @ 2014-05-19 13:47 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
On 05/05/2014 04:54 PM, Arianna Avanzini wrote:
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index f88cddc..2c7f542 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -511,12 +511,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 + 1),
This is wrong, apply_p2m_changes range is [spfn, epfn[. Here you will
map one page more. This remark is also valid for unmap_mmio_regions in
patch #7.
Once you're series will be pushed in Xen upstream, I plan to rework
apply_p2m_changes to use gfn + nr_pages which is less confusing and
avoid using non aligned address as arguments (actually the function is
assuming that the address is page-aligned).
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory
2014-05-06 9:06 ` Jan Beulich
2014-05-10 1:10 ` Arianna Avanzini
@ 2014-05-25 17:14 ` Julien Grall
2014-05-26 9:03 ` Jan Beulich
1 sibling, 1 reply; 45+ messages in thread
From: Julien Grall @ 2014-05-25 17:14 UTC (permalink / raw)
To: Jan Beulich, Arianna Avanzini
Cc: julien.grall, paolo.valente, keir, stefano.stabellini,
andrew.cooper3, dario.faggioli, Ian.Jackson, xen-devel,
Ian.Campbell, etrudeau, tim, viktor.kleinik
Hi Jan,
On 06/05/14 10:06, Jan Beulich wrote:
>> @@ -838,7 +842,7 @@ 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(d, mfn, mfn_end) )
>
> I'm not sure if we shouldn't rather be conservative here and check
> both domains' permissions.
I think we only need to check the permission of the current domain.
Checking the permission of the guest is not necessary here. Hence, the
code to map a region will be more difficult. We will have first call
iomem_permission then call memory_mapping.
I would prefer to keep the current permission check ie:
if ( !iomem_access_permitted(current->domain, ...) )
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory
2014-05-25 17:14 ` Julien Grall
@ 2014-05-26 9:03 ` Jan Beulich
2014-05-26 10:14 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2014-05-26 9:03 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 19:14, <julien.grall@linaro.org> wrote:
> On 06/05/14 10:06, Jan Beulich wrote:
>>> @@ -838,7 +842,7 @@ 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(d, mfn, mfn_end) )
>>
>> I'm not sure if we shouldn't rather be conservative here and check
>> both domains' permissions.
>
> I think we only need to check the permission of the current domain.
> Checking the permission of the guest is not necessary here. Hence, the
> code to map a region will be more difficult. We will have first call
> iomem_permission then call memory_mapping.
>
> I would prefer to keep the current permission check ie:
>
> if ( !iomem_access_permitted(current->domain, ...) )
In fact my earlier comment was wrong - for some reason I was
thinking this was the mapping operation that gets modified here,
not the permission one. For the permission one it is of course
wrong to check the subject domain's permissions, as they're only
about to be granted.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory
2014-05-26 9:03 ` Jan Beulich
@ 2014-05-26 10:14 ` Jan Beulich
2014-05-26 10:53 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2014-05-26 10:14 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 26.05.14 at 11:03, <JBeulich@suse.com> wrote:
>>>> On 25.05.14 at 19:14, <julien.grall@linaro.org> wrote:
>> On 06/05/14 10:06, Jan Beulich wrote:
>>>> @@ -838,7 +842,7 @@ 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(d, mfn, mfn_end) )
>>>
>>> I'm not sure if we shouldn't rather be conservative here and check
>>> both domains' permissions.
>>
>> I think we only need to check the permission of the current domain.
>> Checking the permission of the guest is not necessary here. Hence, the
>> code to map a region will be more difficult. We will have first call
>> iomem_permission then call memory_mapping.
>>
>> I would prefer to keep the current permission check ie:
>>
>> if ( !iomem_access_permitted(current->domain, ...) )
>
> In fact my earlier comment was wrong - for some reason I was
> thinking this was the mapping operation that gets modified here,
> not the permission one. For the permission one it is of course
> wrong to check the subject domain's permissions, as they're only
> about to be granted.
Or maybe I wasn't wrong - the patch context doesn't really make
clear whether it's the granting or mapping operation that gets
adjusted here (since an earlier patch moved the mapping one into
this function).
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory
2014-05-26 10:14 ` Jan Beulich
@ 2014-05-26 10:53 ` Julien Grall
2014-05-26 11:14 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2014-05-26 10:53 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:14, Jan Beulich wrote:
>
> Or maybe I wasn't wrong - the patch context doesn't really make
> clear whether it's the granting or mapping operation that gets
> adjusted here (since an earlier patch moved the mapping one into
> this function).
ret = -EPERM;
- if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
+ if ( !iomem_access_permitted(d, mfn, mfn_end) )
break;
ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
There is an xsm_iomem_mapping just after, so the change has been done in
XEN_DOMCTL_memory_mapping.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory
2014-05-26 10:53 ` Julien Grall
@ 2014-05-26 11:14 ` Jan Beulich
2014-05-26 11:24 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2014-05-26 11:14 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 26.05.14 at 12:53, <julien.grall@linaro.org> wrote:
>
> On 26/05/14 11:14, Jan Beulich wrote:
>>
>> Or maybe I wasn't wrong - the patch context doesn't really make
>> clear whether it's the granting or mapping operation that gets
>> adjusted here (since an earlier patch moved the mapping one into
>> this function).
>
> ret = -EPERM;
> - if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
> + if ( !iomem_access_permitted(d, mfn, mfn_end) )
> break;
>
> ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
>
> There is an xsm_iomem_mapping just after, so the change has been done in
> XEN_DOMCTL_memory_mapping.
In which case I indeed stick to my original comment - it's perhaps
best to check _both_.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory
2014-05-26 11:14 ` Jan Beulich
@ 2014-05-26 11:24 ` Julien Grall
2014-05-26 11:37 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2014-05-26 11:24 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 12:14, Jan Beulich wrote:
>>>> On 26.05.14 at 12:53, <julien.grall@linaro.org> wrote:
>
>>
>> On 26/05/14 11:14, Jan Beulich wrote:
>>>
>>> Or maybe I wasn't wrong - the patch context doesn't really make
>>> clear whether it's the granting or mapping operation that gets
>>> adjusted here (since an earlier patch moved the mapping one into
>>> this function).
>>
>> ret = -EPERM;
>> - if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
>> + if ( !iomem_access_permitted(d, mfn, mfn_end) )
>> break;
>>
>> ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
>>
>> There is an xsm_iomem_mapping just after, so the change has been done in
>> XEN_DOMCTL_memory_mapping.
>
> In which case I indeed stick to my original comment - it's perhaps
> best to check _both_.
Why? We may want to map the region in the guest P2M without giving the
permission to the guest (I'm thinking about ARM passthrough case).
With your requirements, we have to call 2 hypercalls rather than one for
memory mapping, even if we don't want to allow the guest modifying iomem
range.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory
2014-05-26 11:24 ` Julien Grall
@ 2014-05-26 11:37 ` Jan Beulich
2014-05-26 11:42 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2014-05-26 11:37 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 13:24, <julien.grall@linaro.org> wrote:
> On 26/05/14 12:14, Jan Beulich wrote:
>>>>> On 26.05.14 at 12:53, <julien.grall@linaro.org> wrote:
>>> On 26/05/14 11:14, Jan Beulich wrote:
>>>>
>>>> Or maybe I wasn't wrong - the patch context doesn't really make
>>>> clear whether it's the granting or mapping operation that gets
>>>> adjusted here (since an earlier patch moved the mapping one into
>>>> this function).
>>>
>>> ret = -EPERM;
>>> - if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
>>> + if ( !iomem_access_permitted(d, mfn, mfn_end) )
>>> break;
>>>
>>> ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
>>>
>>> There is an xsm_iomem_mapping just after, so the change has been done in
>>> XEN_DOMCTL_memory_mapping.
>>
>> In which case I indeed stick to my original comment - it's perhaps
>> best to check _both_.
>
> Why? We may want to map the region in the guest P2M without giving the
> permission to the guest (I'm thinking about ARM passthrough case).
How can you put a mapping of memory into a guest's P2M for which
that guest has no access permission? To me this reads like you're
intending to create a security issue here.
> With your requirements, we have to call 2 hypercalls rather than one for
> memory mapping, even if we don't want to allow the guest modifying iomem
> range.
While I can see you not allowing modification, even r/o access may
(and likely will) be problematic for MMIO.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory
2014-05-26 11:37 ` Jan Beulich
@ 2014-05-26 11:42 ` Julien Grall
2014-05-26 11:51 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2014-05-26 11:42 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 12:37, Jan Beulich wrote:
>>>> On 26.05.14 at 13:24, <julien.grall@linaro.org> wrote:
>> On 26/05/14 12:14, Jan Beulich wrote:
>>>>>> On 26.05.14 at 12:53, <julien.grall@linaro.org> wrote:
>>>> On 26/05/14 11:14, Jan Beulich wrote:
>>>>>
>>>>> Or maybe I wasn't wrong - the patch context doesn't really make
>>>>> clear whether it's the granting or mapping operation that gets
>>>>> adjusted here (since an earlier patch moved the mapping one into
>>>>> this function).
>>>>
>>>> ret = -EPERM;
>>>> - if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
>>>> + if ( !iomem_access_permitted(d, mfn, mfn_end) )
>>>> break;
>>>>
>>>> ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
>>>>
>>>> There is an xsm_iomem_mapping just after, so the change has been done in
>>>> XEN_DOMCTL_memory_mapping.
>>>
>>> In which case I indeed stick to my original comment - it's perhaps
>>> best to check _both_.
>>
>> Why? We may want to map the region in the guest P2M without giving the
>> permission to the guest (I'm thinking about ARM passthrough case).
>
> How can you put a mapping of memory into a guest's P2M for which
> that guest has no access permission? To me this reads like you're
> intending to create a security issue here.
iomem_access_permitted is used to check if we allow the current guest to
map a region in another guest P2M.
Once the mapping is done, at least on ARM, we don't use anymore the
permission check. This is because there is no trap involved afterwards.
In a such case, I don't see any posssible security issue.
>> With your requirements, we have to call 2 hypercalls rather than one for
>> memory mapping, even if we don't want to allow the guest modifying iomem
>> range.
>
> While I can see you not allowing modification, even r/o access may
> (and likely will) be problematic for MMIO.
AFAIU, iomem_access_permitted is only here to allow modification of this
range via hypercall.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory
2014-05-26 11:42 ` Julien Grall
@ 2014-05-26 11:51 ` Jan Beulich
2014-05-26 12:15 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2014-05-26 11:51 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 13:42, <julien.grall@linaro.org> wrote:
>
> On 26/05/14 12:37, Jan Beulich wrote:
>>>>> On 26.05.14 at 13:24, <julien.grall@linaro.org> wrote:
>>> On 26/05/14 12:14, Jan Beulich wrote:
>>>>>>> On 26.05.14 at 12:53, <julien.grall@linaro.org> wrote:
>>>>> On 26/05/14 11:14, Jan Beulich wrote:
>>>>>>
>>>>>> Or maybe I wasn't wrong - the patch context doesn't really make
>>>>>> clear whether it's the granting or mapping operation that gets
>>>>>> adjusted here (since an earlier patch moved the mapping one into
>>>>>> this function).
>>>>>
>>>>> ret = -EPERM;
>>>>> - if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
>>>>> + if ( !iomem_access_permitted(d, mfn, mfn_end) )
>>>>> break;
>>>>>
>>>>> ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
>>>>>
>>>>> There is an xsm_iomem_mapping just after, so the change has been done in
>>>>> XEN_DOMCTL_memory_mapping.
>>>>
>>>> In which case I indeed stick to my original comment - it's perhaps
>>>> best to check _both_.
>>>
>>> Why? We may want to map the region in the guest P2M without giving the
>>> permission to the guest (I'm thinking about ARM passthrough case).
>>
>> How can you put a mapping of memory into a guest's P2M for which
>> that guest has no access permission? To me this reads like you're
>> intending to create a security issue here.
>
> iomem_access_permitted is used to check if we allow the current guest to
> map a region in another guest P2M.
>
> Once the mapping is done, at least on ARM, we don't use anymore the
> permission check. This is because there is no trap involved afterwards.
I don't see how absence or presence of traps is involved here. The
problem I see is that by putting in such a P2M entrry you allow a
guest access to memory that it wasn't granted access to.
>>> With your requirements, we have to call 2 hypercalls rather than one for
>>> memory mapping, even if we don't want to allow the guest modifying iomem
>>> range.
>>
>> While I can see you not allowing modification, even r/o access may
>> (and likely will) be problematic for MMIO.
>
> AFAIU, iomem_access_permitted is only here to allow modification of this
> range via hypercall.
I don't think I understand what you're trying to tell me here.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory
2014-05-26 11:51 ` Jan Beulich
@ 2014-05-26 12:15 ` Julien Grall
2014-05-26 13:22 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2014-05-26 12:15 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 12:51, Jan Beulich wrote:
>>>> On 26.05.14 at 13:42, <julien.grall@linaro.org> wrote:
>
>>
>> On 26/05/14 12:37, Jan Beulich wrote:
>>>>>> On 26.05.14 at 13:24, <julien.grall@linaro.org> wrote:
>>>> On 26/05/14 12:14, Jan Beulich wrote:
>>>>>>>> On 26.05.14 at 12:53, <julien.grall@linaro.org> wrote:
>>>>>> On 26/05/14 11:14, Jan Beulich wrote:
>>>>>>>
>>>>>>> Or maybe I wasn't wrong - the patch context doesn't really make
>>>>>>> clear whether it's the granting or mapping operation that gets
>>>>>>> adjusted here (since an earlier patch moved the mapping one into
>>>>>>> this function).
>>>>>>
>>>>>> ret = -EPERM;
>>>>>> - if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
>>>>>> + if ( !iomem_access_permitted(d, mfn, mfn_end) )
>>>>>> break;
>>>>>>
>>>>>> ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
>>>>>>
>>>>>> There is an xsm_iomem_mapping just after, so the change has been done in
>>>>>> XEN_DOMCTL_memory_mapping.
>>>>>
>>>>> In which case I indeed stick to my original comment - it's perhaps
>>>>> best to check _both_.
>>>>
>>>> Why? We may want to map the region in the guest P2M without giving the
>>>> permission to the guest (I'm thinking about ARM passthrough case).
>>>
>>> How can you put a mapping of memory into a guest's P2M for which
>>> that guest has no access permission? To me this reads like you're
>>> intending to create a security issue here.
>>
>> iomem_access_permitted is used to check if we allow the current guest to
>> map a region in another guest P2M.
>>
>> Once the mapping is done, at least on ARM, we don't use anymore the
>> permission check. This is because there is no trap involved afterwards.
>
> I don't see how absence or presence of traps is involved here. The
> problem I see is that by putting in such a P2M entrry you allow a
> guest access to memory that it wasn't granted access to.
In the case of an HVM guest (or ARM guest), the permission seems to be
used only during DOMCTL_memory_mapping hypercall. So I understand the
permission as "I'm allowed to map/unmap this MMIO range from a guest P2M".
If we request the guest to have the permission on this range, we also
allow the guest to map in its P2M (assuming XSM is not there) theses MMIOs.
I don't think, at least on ARM, we want to let the guest doing what it
wants with the mapping MMIO region.
>>>> With your requirements, we have to call 2 hypercalls rather than one for
>>>> memory mapping, even if we don't want to allow the guest modifying iomem
>>>> range.
>>>
>>> While I can see you not allowing modification, even r/o access may
>>> (and likely will) be problematic for MMIO.
>>
>> AFAIU, iomem_access_permitted is only here to allow modification of this
>> range via hypercall.
Sorry, I should have read again what I wrote. I tried to be clearer above.
--
Julien Grall
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory
2014-05-26 12:15 ` Julien Grall
@ 2014-05-26 13:22 ` Jan Beulich
2014-05-26 14:26 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2014-05-26 13:22 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 14:15, <julien.grall@linaro.org> wrote:
>
> On 26/05/14 12:51, Jan Beulich wrote:
>>>>> On 26.05.14 at 13:42, <julien.grall@linaro.org> wrote:
>>
>>>
>>> On 26/05/14 12:37, Jan Beulich wrote:
>>>>>>> On 26.05.14 at 13:24, <julien.grall@linaro.org> wrote:
>>>>> On 26/05/14 12:14, Jan Beulich wrote:
>>>>>>>>> On 26.05.14 at 12:53, <julien.grall@linaro.org> wrote:
>>>>>>> On 26/05/14 11:14, Jan Beulich wrote:
>>>>>>>>
>>>>>>>> Or maybe I wasn't wrong - the patch context doesn't really make
>>>>>>>> clear whether it's the granting or mapping operation that gets
>>>>>>>> adjusted here (since an earlier patch moved the mapping one into
>>>>>>>> this function).
>>>>>>>
>>>>>>> ret = -EPERM;
>>>>>>> - if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
>>>>>>> + if ( !iomem_access_permitted(d, mfn, mfn_end) )
>>>>>>> break;
>>>>>>>
>>>>>>> ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
>>>>>>>
>>>>>>> There is an xsm_iomem_mapping just after, so the change has been done in
>>>>>>> XEN_DOMCTL_memory_mapping.
>>>>>>
>>>>>> In which case I indeed stick to my original comment - it's perhaps
>>>>>> best to check _both_.
>>>>>
>>>>> Why? We may want to map the region in the guest P2M without giving the
>>>>> permission to the guest (I'm thinking about ARM passthrough case).
>>>>
>>>> How can you put a mapping of memory into a guest's P2M for which
>>>> that guest has no access permission? To me this reads like you're
>>>> intending to create a security issue here.
>>>
>>> iomem_access_permitted is used to check if we allow the current guest to
>>> map a region in another guest P2M.
>>>
>>> Once the mapping is done, at least on ARM, we don't use anymore the
>>> permission check. This is because there is no trap involved afterwards.
>>
>> I don't see how absence or presence of traps is involved here. The
>> problem I see is that by putting in such a P2M entrry you allow a
>> guest access to memory that it wasn't granted access to.
>
> In the case of an HVM guest (or ARM guest), the permission seems to be
> used only during DOMCTL_memory_mapping hypercall. So I understand the
> permission as "I'm allowed to map/unmap this MMIO range from a guest P2M".
Ah, I start seeing where you're coming from: Taking current uses of
a certain construct to imply a meaning of that construct is kind of
backwards. You should start with the meaning, and all of
{iomem,ioports,pirq}_access_permitted() are inquiries to find out
whether the specified domain is permitted to access the specified
resource.
And the fact that for HVM/ARM there's currently only that one use
in the domctl handling isn't indicative of anything. Just take the use
in xen/common/grant_table.c - if the paging_mode_external()
check got dropped (assuming the backing functions can handle this)
you'd all of the sudden have another use. And the check here, from
all I know, isn't there because such an operation makes sense only
for PV guests, but because the same operation just can't be handled
for HVM/PVH/ARM ones. The moment someone finds a legitimate use
of e.g. granting pages from a passed-through frame buffer (to e.g.
snapshot its contents directly to storage) this would need to be
fixed.
> If we request the guest to have the permission on this range, we also
> allow the guest to map in its P2M (assuming XSM is not there) theses MMIOs.
>
> I don't think, at least on ARM, we want to let the guest doing what it
> wants with the mapping MMIO region.
And I didn't say that.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory
2014-05-26 13:22 ` Jan Beulich
@ 2014-05-26 14:26 ` Julien Grall
2014-05-26 15:00 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2014-05-26 14:26 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 14:22, Jan Beulich wrote:
>>>> On 26.05.14 at 14:15, <julien.grall@linaro.org> wrote:
>
>>
>> On 26/05/14 12:51, Jan Beulich wrote:
>>>>>> On 26.05.14 at 13:42, <julien.grall@linaro.org> wrote:
>>>
>>>>
>>>> On 26/05/14 12:37, Jan Beulich wrote:
>>>>>>>> On 26.05.14 at 13:24, <julien.grall@linaro.org> wrote:
>>>>>> On 26/05/14 12:14, Jan Beulich wrote:
>>>>>>>>>> On 26.05.14 at 12:53, <julien.grall@linaro.org> wrote:
>>>>>>>> On 26/05/14 11:14, Jan Beulich wrote:
>>>>>>>>>
>>>>>>>>> Or maybe I wasn't wrong - the patch context doesn't really make
>>>>>>>>> clear whether it's the granting or mapping operation that gets
>>>>>>>>> adjusted here (since an earlier patch moved the mapping one into
>>>>>>>>> this function).
>>>>>>>>
>>>>>>>> ret = -EPERM;
>>>>>>>> - if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
>>>>>>>> + if ( !iomem_access_permitted(d, mfn, mfn_end) )
>>>>>>>> break;
>>>>>>>>
>>>>>>>> ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
>>>>>>>>
>>>>>>>> There is an xsm_iomem_mapping just after, so the change has been done in
>>>>>>>> XEN_DOMCTL_memory_mapping.
>>>>>>>
>>>>>>> In which case I indeed stick to my original comment - it's perhaps
>>>>>>> best to check _both_.
>>>>>>
>>>>>> Why? We may want to map the region in the guest P2M without giving the
>>>>>> permission to the guest (I'm thinking about ARM passthrough case).
>>>>>
>>>>> How can you put a mapping of memory into a guest's P2M for which
>>>>> that guest has no access permission? To me this reads like you're
>>>>> intending to create a security issue here.
>>>>
>>>> iomem_access_permitted is used to check if we allow the current guest to
>>>> map a region in another guest P2M.
>>>>
>>>> Once the mapping is done, at least on ARM, we don't use anymore the
>>>> permission check. This is because there is no trap involved afterwards.
>>>
>>> I don't see how absence or presence of traps is involved here. The
>>> problem I see is that by putting in such a P2M entrry you allow a
>>> guest access to memory that it wasn't granted access to.
>>
>> In the case of an HVM guest (or ARM guest), the permission seems to be
>> used only during DOMCTL_memory_mapping hypercall. So I understand the
>> permission as "I'm allowed to map/unmap this MMIO range from a guest P2M".
>
> Ah, I start seeing where you're coming from: Taking current uses of
> a certain construct to imply a meaning of that construct is kind of
> backwards. You should start with the meaning, and all of
> {iomem,ioports,pirq}_access_permitted() are inquiries to find out
> whether the specified domain is permitted to access the specified
> resource.
>
> And the fact that for HVM/ARM there's currently only that one use
> in the domctl handling isn't indicative of anything. Just take the use
> in xen/common/grant_table.c - if the paging_mode_external()
> check got dropped (assuming the backing functions can handle this)
> you'd all of the sudden have another use. And the check here, from
> all I know, isn't there because such an operation makes sense only
> for PV guests, but because the same operation just can't be handled
> for HVM/PVH/ARM ones. The moment someone finds a legitimate use
> of e.g. granting pages from a passed-through frame buffer (to e.g.
> snapshot its contents directly to storage) this would need to be
> fixed.
I got your point now. Many thanks for your explanation.
I'm still not convince, it's perfectly valid to:
1) call iomem_permission to set the permission
2) call memory_mapping
3) call iomem_permission to remove the permission
In this case the memory mapping will be there without any granting.
Hence, we won't be able to remove those mappings.
IHMO, this make the requirement of the permission useless and add more
code in the toolstack to map an MMIO region for peanuts.
>> If we request the guest to have the permission on this range, we also
>> allow the guest to map in its P2M (assuming XSM is not there) theses MMIOs.
>>
>> I don't think, at least on ARM, we want to let the guest doing what it
>> wants with the mapping MMIO region.
>
> And I didn't say that.
I didn't intend to blame you with this sentence.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory
2014-05-26 14:26 ` Julien Grall
@ 2014-05-26 15:00 ` Jan Beulich
0 siblings, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2014-05-26 15:00 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 16:26, <julien.grall@linaro.org> wrote:
> I'm still not convince, it's perfectly valid to:
> 1) call iomem_permission to set the permission
> 2) call memory_mapping
> 3) call iomem_permission to remove the permission
>
> In this case the memory mapping will be there without any granting.
> Hence, we won't be able to remove those mappings.
This would be a tool stack bug imo. And yes, this is possible, but no,
I don't think one should rely on this being possible (i.e. if we
introduced ways to find such mappings and refused the permission
removal, nothing should break).
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
end of thread, other threads:[~2014-05-26 15:00 UTC | newest]
Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-05 15:54 [PATCH v7 00/10] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
2014-05-05 15:54 ` [PATCH v7 01/10] arch/arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
2014-05-05 15:54 ` [PATCH v7 02/10] arch/arm: add consistency check to REMOVE p2m changes Arianna Avanzini
2014-05-06 16:51 ` Julien Grall
2014-05-06 16:52 ` Julien Grall
2014-05-05 15:54 ` [PATCH v7 03/10] arch/arm: let map_mmio_regions() take pfn as parameters Arianna Avanzini
2014-05-05 15:54 ` [PATCH v7 04/10] arch/arm: let map_mmio_regions() use start and count Arianna Avanzini
2014-05-05 18:55 ` Julien Grall
2014-05-07 11:03 ` Ian Campbell
2014-05-19 13:47 ` Julien Grall
2014-05-05 15:54 ` [PATCH v7 05/10] arch/x86: check if mapping exists before memory_mapping removes it Arianna Avanzini
2014-05-06 8:25 ` Jan Beulich
2014-05-05 15:54 ` [PATCH v7 06/10] xen/x86: factor out map and unmap from the memory_mapping DOMCTL Arianna Avanzini
2014-05-06 8:35 ` Jan Beulich
2014-05-05 15:54 ` [PATCH v7 07/10] xen/common: move the memory_mapping DOMCTL hypercall to common code Arianna Avanzini
2014-05-06 8:40 ` Jan Beulich
2014-05-07 11:09 ` Ian Campbell
2014-05-10 0:26 ` Arianna Avanzini
2014-05-12 8:29 ` Jan Beulich
2014-05-07 11:10 ` Ian Campbell
2014-05-06 16:54 ` Julien Grall
2014-05-10 1:20 ` Arianna Avanzini
2014-05-10 9:03 ` Julien Grall
2014-05-05 15:54 ` [PATCH v7 08/10] tools/libxl: parse optional start gfn from the iomem config option Arianna Avanzini
2014-05-05 15:54 ` [PATCH v7 09/10] tools/libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
2014-05-06 8:44 ` Jan Beulich
2014-05-07 11:16 ` Ian Campbell
2014-05-05 15:54 ` [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory Arianna Avanzini
2014-05-06 9:06 ` Jan Beulich
2014-05-10 1:10 ` Arianna Avanzini
2014-05-12 8:35 ` Jan Beulich
2014-05-25 17:14 ` Julien Grall
2014-05-26 9:03 ` Jan Beulich
2014-05-26 10:14 ` Jan Beulich
2014-05-26 10:53 ` Julien Grall
2014-05-26 11:14 ` Jan Beulich
2014-05-26 11:24 ` Julien Grall
2014-05-26 11:37 ` Jan Beulich
2014-05-26 11:42 ` Julien Grall
2014-05-26 11:51 ` Jan Beulich
2014-05-26 12:15 ` Julien Grall
2014-05-26 13:22 ` Jan Beulich
2014-05-26 14:26 ` Julien Grall
2014-05-26 15:00 ` Jan Beulich
2014-05-06 8:21 ` [PATCH v7 00/10] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.