* [PATCH v4 0/7] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM
@ 2014-03-25 2:02 Arianna Avanzini
2014-03-25 2:02 ` [PATCH v4 1/7] arch, arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
` (6 more replies)
0 siblings, 7 replies; 50+ messages in thread
From: Arianna Avanzini @ 2014-03-25 2:02 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
JBeulich, avanzini.arianna, viktor.kleinik
Hello,
here is my fourth attempt at proposing an implementation of the memory_mapping
DOMCTL for the ARM architecture. This patchset contains both the implementation
of the hypercall and some related changes added after having the benefit of
many comments to the previous attempts ([1]). I'm cutting the description a bit
to avoid wasting too much of your time, but more details should be available,
if needed, in the cover letter of the last proposal ([2]).
The v1 patchset suffered from issues concerning correct typing and alignment of
variables, and was implemented under the wrong assumption that dom0 could
access any range of I/O memory; also, the hypercall was implemented as ARM32-
specific, while it is not. The v2 patchset tried to fix these issues, when they
were pointed out by Julien Grall, Eric Trudeau and Dario Faggioli.
The v3 patchset tried to address some more issues indicated by Ian Campbell,
Julien Grall, Jan Beulich and Dario Faggioli, and further discussed with
Stefano Stebellini. The v2 patchset, in fact, did not perform any consistency
check on an I/O-memory mapping before removing it. Also, the implementation of
the hypercall for ARM provided with v2 was almost a complete copy of the x86
version: in v3 the code was made common between both architectures, abstracting
out the different operations performed to map and unmap I/O memory. Also, the v2
patchset did not provide any way to specify the guest address range used to map
the machine I/O-memory range indicated in the domU configuration with the
"iomem" option; the v3 patchset added an optional parameter to that
configuration option, allowing to specify the start guest address used for the
mapping and defaulting to an 1:1 mapping in the absence of such parameter.
Finally, the v2 patchset unconditionally called the new hypercall in the
presence of the "iomem" option, while the v3 patchset attempted to restrict its
use to the cases of ARM or HVM x86 guests.
As of the v4 patchse, the first commit implements the bookkeeping necessary
to make a domain's iomem_caps coherent with the I/O-memory mapping operations
performed during domain build. With respect to v3, it attempts to improve the
commit's description by specifying that, by now, the change is dom0-related,
even if the modified function is generic, as pointed out by Julien Grall.
The second commit adds to the apply_p2m_changes() function some consistency
checks useful for the REMOVE operation. More in detail, the REMOVE switch
of the function checks:
. the validity of the mapping;
. the fact that the mfn whose mapping is to be removed is actually mapped
to the guest address given as parameter.
With respect to v3, this commit attempts to address some issues indicated
by Julien Grall. In fact, it removes an useless and slow lookup performed
on start addresses only, and uses for each mapping the data already available
in the pte. It also correctly increments the local variable used to keep the
machine address whose mapping is currently being removed. It also lets the
REMOVE switch return with an error, instead of just skipping the page, if the
second of the above-described checks fails.
The third commit modifies the interface of the existing map_mmio_regions()
function for ARM according to a suggestion given in the discussion following
the previous proposals by Julien Grall and confirmed by Jan Beulich. In fact,
currently the function takes parameters of paddr_t type; this interface needs
caller functions to correctly page-align addresses. This commit changes lets
this function take page frame numbers as parameters instead, trying also to
modify callers to use the new interface.
The fourth commit moves the existing memory_mapping DOMCTL for x86 to
xen/common/domctl.c and attempts to adapt the code to be used for both the
x86 and ARM architecture. More in detail, the arch-specific operations
necessary to map and unmap memory ranges are abstracted out in a pair of
map_mmio_regions() and unmap_mmio_regions() functions.
With respect to the corresponding one in v3, this commit attempts to address
the following issues.
. It uses a define for the paddr_bits symbol instead of a variable, as
suggested by Julien Grall.
. It defines common prototypes for the newly-introduced functions in a
new common header, as suggested by Julien Grall.
. It makes the signedness and type of some variables used in the new functions
coherent with the ones previously defined in the x86 hypercall, as pointed
out by Jan Beulich. The unmap_mmio_regions() function for x86 still has
int as a return type to let the interface be coherent with the ARM version.
. It clears p2m entries in map_mmio_regions() for x86 only in case of an
error in set_mmio_p2m_entry(), as indicated by Jan Beulich.
. It makes ranges considered in map_mmio_regions() and unmap_mmio_regions()
for x86 inclusive of the end address, while they uncorrectly were not, as
pointed out by Jan Beulich.
. It removes some hard tabs turning them into spaces as suggested by
Julien Grall.
The fifth commit adds to libxl the handling code for a new optional paramter
to the "iomem" configuration option. The new parameter, called "gfn", allows
to specify the start guest address used for the mapping of the given machine
address range. With respect to the v3 patchset, this commit addresses the
following issues.
. Adds the definition of a new LIBXL_HAVE_BUILDINFO_IOMEM_START_GFN macro
to indicate the availability of the new option, as suggested by Dario
Faggioli.
. Simplifies the code by using a switch construct on the return value of
the sscanf() function, instead of calling the function twice, which is
unnecessary, as pointed out by Ian Campbell.
. Adds a short paragraph to the manpage about the use of the iomem option
to passthrough devices protected by an IOMMU, as suggested by Julien
Grall.
. Adds comments to the fields of the iomem structure as the meaning of
"start" is ambiguous, as pointed out by Ian Campbell.
The sixth commit adds two new architecture helpers to libxl, following a
suggestion by Julien Grall. In fact, the v3 patchset allowed the newly-added
DOMCTL to be called only for ARM or HVM x86 guests; the hypercall would be
useful, instead, for all auto-translated guests, which include x86 PVH guests
that are however classified as PV by libxl. The new architecture helpers
should allow the caller to establish if a domain uses an auto-translated
physmap.
The seventh commit adds to libxl code to handle the iomem configuration option
by invoking the memory_mapping DOMCTL. The hypercall is invoked only if the
domain is auto-translated, as suggested by Julien Grall, by using the arch-
specific helpers introduced with the previous commit.
The code has been tested on a cubieboard2, with Linux v3.13 as a dom0 and ERIKA
Enterprise ([3]) as a domU. Any feedback about this new version is more than
welcome.
Arianna
[1] http://markmail.org/message/d5vhhiin5tr2yee7
http://markmail.org/message/dtzsvauzob332gnq
http://markmail.org/message/fy7ioiv37gbp22an
[2] http://lists.xen.org/archives/html/xen-devel/2014-03/msg01724.html
[3] http://erika.tuxfamily.org/drupal/
Arianna Avanzini (7):
arch, arm: domain build: let dom0 access I/O memory of mapped devices
arch, arm: add consistency checks to REMOVE p2m changes
arch, arm: let map_mmio_regions() take pfn as parameters
xen, common: add the XEN_DOMCTL_memory_mapping hypercall
tools, libxl: parse optional start gfn from the iomem config option
tools, libxl: add helpers to establish if guest is auto-translated
tools, libxl: handle the iomem parameter with the memory_mapping hcall
docs/man/xl.cfg.pod.5 | 13 ++++---
tools/libxl/libxl.h | 10 ++++++
tools/libxl/libxl_arch.h | 2 ++
tools/libxl/libxl_arm.c | 5 +++
tools/libxl/libxl_create.c | 13 +++++++
tools/libxl/libxl_types.idl | 5 +--
tools/libxl/libxl_x86.c | 6 ++++
tools/libxl/xl_cmdimpl.c | 21 +++++++----
xen/arch/arm/domain_build.c | 18 ++++++++--
xen/arch/arm/gic.c | 21 ++++++-----
xen/arch/arm/p2m.c | 49 ++++++++++++++++++++-----
xen/arch/arm/platforms/exynos5.c | 13 ++++---
xen/arch/arm/platforms/omap5.c | 25 ++++++++-----
xen/arch/arm/platforms/xgene-storm.c | 4 ++-
xen/arch/x86/domctl.c | 70 ------------------------------------
xen/arch/x86/mm/p2m.c | 40 +++++++++++++++++++++
xen/common/domctl.c | 65 +++++++++++++++++++++++++++++++++
xen/include/asm-arm/p2m.h | 8 ++---
xen/include/asm-x86/p2m.h | 1 +
xen/include/xen/p2m.h | 16 +++++++++
20 files changed, 281 insertions(+), 124 deletions(-)
create mode 100644 xen/include/xen/p2m.h
--
1.9.0
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v4 1/7] arch, arm: domain build: let dom0 access I/O memory of mapped devices
2014-03-25 2:02 [PATCH v4 0/7] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
@ 2014-03-25 2:02 ` Arianna Avanzini
2014-03-25 12:37 ` Julien Grall
2014-03-25 2:02 ` [PATCH v4 2/7] arch, arm: add consistency checks to REMOVE p2m changes Arianna Avanzini
` (5 subsequent siblings)
6 siblings, 1 reply; 50+ messages in thread
From: Arianna Avanzini @ 2014-03-25 2:02 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
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.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Acked-by: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
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 5ca2f15..4a0411c 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>
@@ -733,6 +734,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.0
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v4 2/7] arch, arm: add consistency checks to REMOVE p2m changes
2014-03-25 2:02 [PATCH v4 0/7] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
2014-03-25 2:02 ` [PATCH v4 1/7] arch, arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
@ 2014-03-25 2:02 ` Arianna Avanzini
2014-03-25 12:18 ` Stefano Stabellini
` (2 more replies)
2014-03-25 2:02 ` [PATCH v4 3/7] arch, arm: let map_mmio_regions() take pfn as parameters Arianna Avanzini
` (4 subsequent siblings)
6 siblings, 3 replies; 50+ messages in thread
From: Arianna Avanzini @ 2014-03-25 2:02 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
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>
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: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
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 | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index d00c882..bb0db16 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -243,12 +243,13 @@ static int apply_p2m_changes(struct domain *d,
int rc;
struct p2m_domain *p2m = &d->arch.p2m;
lpae_t *first = NULL, *second = NULL, *third = NULL;
- paddr_t addr;
+ paddr_t addr, _maddr;
unsigned long cur_first_page = ~0,
cur_first_offset = ~0,
cur_second_offset = ~0;
unsigned long count = 0;
unsigned int flush = 0;
+ unsigned long mfn;
bool_t populate = (op == INSERT || op == ALLOCATE);
lpae_t pte;
@@ -258,6 +259,7 @@ static int apply_p2m_changes(struct domain *d,
p2m_load_VTTBR(d);
addr = start_gpaddr;
+ _maddr = maddr;
while ( addr < end_gpaddr )
{
if ( cur_first_page != p2m_first_level_index(addr) )
@@ -327,6 +329,7 @@ static int apply_p2m_changes(struct domain *d,
flush |= pte.p2m.valid;
+ mfn = pte.p2m.base;
/* TODO: Handle other p2m type
*
* It's safe to do the put_page here because page_alloc will
@@ -335,8 +338,6 @@ static int apply_p2m_changes(struct domain *d,
*/
if ( pte.p2m.valid && p2m_is_foreign(pte.p2m.type) )
{
- unsigned long mfn = pte.p2m.base;
-
ASSERT(mfn_valid(mfn));
put_page(mfn_to_page(mfn));
}
@@ -367,9 +368,23 @@ static int apply_p2m_changes(struct domain *d,
maddr += PAGE_SIZE;
}
break;
- case RELINQUISH:
case REMOVE:
{
+ ASSERT(pte.p2m.valid);
+ /*
+ * 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 ( _maddr != pfn_to_paddr(mfn) )
+ return -EINVAL;
+ }
+ /* fall through */
+ case RELINQUISH:
+ {
if ( !pte.p2m.valid )
{
count++;
@@ -408,6 +423,7 @@ static int apply_p2m_changes(struct domain *d,
/* Got the next page */
addr += PAGE_SIZE;
+ _maddr += PAGE_SIZE;
}
if ( flush )
--
1.9.0
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v4 3/7] arch, arm: let map_mmio_regions() take pfn as parameters
2014-03-25 2:02 [PATCH v4 0/7] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
2014-03-25 2:02 ` [PATCH v4 1/7] arch, arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
2014-03-25 2:02 ` [PATCH v4 2/7] arch, arm: add consistency checks to REMOVE p2m changes Arianna Avanzini
@ 2014-03-25 2:02 ` Arianna Avanzini
2014-03-25 12:22 ` Stefano Stabellini
2014-03-25 13:00 ` Julien Grall
2014-03-25 2:02 ` [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall Arianna Avanzini
` (3 subsequent siblings)
6 siblings, 2 replies; 50+ messages in thread
From: Arianna Avanzini @ 2014-03-25 2:02 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
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.
NOTE: platform-specific code has not been tested.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
xen/arch/arm/domain_build.c | 7 ++++---
xen/arch/arm/gic.c | 21 ++++++++++++---------
xen/arch/arm/p2m.c | 13 ++++++++-----
xen/arch/arm/platforms/exynos5.c | 13 ++++++++-----
xen/arch/arm/platforms/omap5.c | 25 ++++++++++++++++---------
xen/arch/arm/platforms/xgene-storm.c | 4 +++-
xen/include/asm-arm/p2m.h | 11 ++++++-----
7 files changed, 57 insertions(+), 37 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4a0411c..10f508d 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -744,9 +744,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(PAGE_ALIGN(addr + size - 1)),
+ 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 074624e..55354d4 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -882,20 +882,23 @@ 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(PAGE_ALIGN(d->arch.vgic.cbase +
+ PAGE_SIZE - 1)),
+ 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(PAGE_ALIGN(d->arch.vgic.cbase +
+ (2 * PAGE_SIZE) - 1)),
+ 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(PAGE_ALIGN(d->arch.vgic.cbase +
+ (2 * PAGE_SIZE) - 1)),
+ 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 bb0db16..c8e77b9 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -469,12 +469,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..fd377ce 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -54,13 +54,16 @@ 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(PAGE_ALIGN(EXYNOS5_PA_CHIPID +
+ PAGE_SIZE - 1)),
+ 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(PAGE_ALIGN(EXYNOS5_PA_TIMER +
+ (PAGE_SIZE * 2) - 1)),
+ 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..143c30d 100644
--- a/xen/arch/arm/platforms/omap5.c
+++ b/xen/arch/arm/platforms/omap5.c
@@ -102,21 +102,28 @@ 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(PAGE_ALIGN(OMAP5_PRM_BASE +
+ (PAGE_SIZE * 2) - 1)),
+ 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(PAGE_ALIGN(OMAP5_PRCM_MPU_BASE +
+ PAGE_SIZE - 1)),
+ 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(PAGE_ALIGN(OMAP5_WKUPGEN_BASE +
+ PAGE_SIZE - 1)),
+ 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(PAGE_ALIGN(OMAP5_SRAM_PA +
+ (PAGE_SIZE * 32) - 1)),
+ 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..48ab323 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(PAGE_ALIGN(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/p2m.h b/xen/include/asm-arm/p2m.h
index 3b39c45..d2d2ce3 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -83,11 +83,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.0
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-03-25 2:02 [PATCH v4 0/7] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (2 preceding siblings ...)
2014-03-25 2:02 ` [PATCH v4 3/7] arch, arm: let map_mmio_regions() take pfn as parameters Arianna Avanzini
@ 2014-03-25 2:02 ` Arianna Avanzini
2014-03-25 9:33 ` Jan Beulich
` (2 more replies)
2014-03-25 2:02 ` [PATCH v4 5/7] tools, libxl: parse optional start gfn from the iomem config option Arianna Avanzini
` (2 subsequent siblings)
6 siblings, 3 replies; 50+ messages in thread
From: Arianna Avanzini @ 2014-03-25 2:02 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
JBeulich, avanzini.arianna, viktor.kleinik
This commit introduces a first attempt of implementation of the
XEN_DOMCTL_memory_mapping hypercall for ARM. As the implementation
would have been almost identical to the one for x86, this code has
been made common to both the architectures. The only difference
between the two procedures lies in the arch-specific implementation
of the map_mmio_regions() and unmap_mmio_regions() functions.
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: 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: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
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.
- 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 and error.
- Make ranges inclusive of the end address in map_mmio_regions()
and unmap_mmio_regions() for x86.
- Turn hard tabs into spaces.
v3:
- Move code to xen/common/domctl.c; abstract out differences between
the x86 and ARM code:
. add map_mmio_regions() and unmap_mmio_regions() functions for x86;
. add a paddr_bits variable for ARM.
- Use pfn as parameters to the unmap_mmio_regions() function.
- Compute gfn + nr_mfns and mfn + nr_mfns only once.
v2:
- Move code to xen/arm/domctl.c.
- 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/arm/p2m.c | 12 ++++++++
xen/arch/x86/domctl.c | 70 -----------------------------------------------
xen/arch/x86/mm/p2m.c | 40 +++++++++++++++++++++++++++
xen/common/domctl.c | 65 +++++++++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/p2m.h | 9 ++----
xen/include/asm-x86/p2m.h | 1 +
xen/include/xen/p2m.h | 16 +++++++++++
7 files changed, 137 insertions(+), 76 deletions(-)
create mode 100644 xen/include/xen/p2m.h
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index c8e77b9..961213d 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -480,6 +480,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 end_gfn,
+ unsigned long mfn)
+{
+ return apply_p2m_changes(d, REMOVE,
+ 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,
unsigned long gpfn,
unsigned long mfn,
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 26635ff..69a7fbf 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -639,76 +639,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;
- int add = domctl->u.memory_mapping.add_mapping;
- unsigned long i;
-
- ret = -EINVAL;
- if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
- ((mfn | (mfn + nr_mfns - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
- (gfn + nr_mfns - 1) < gfn ) /* wrap? */
- break;
-
- ret = -EPERM;
- if ( !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
- break;
-
- ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, 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 + nr_mfns - 1);
- if ( !ret && paging_mode_translate(d) )
- {
- for ( i = 0; !ret && i < nr_mfns; i++ )
- if ( !set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i)) )
- ret = -EIO;
- if ( ret )
- {
- printk(XENLOG_G_WARNING
- "memory_map:fail: dom%d gfn=%lx mfn=%lx\n",
- d->domain_id, gfn + i, mfn + i);
- while ( i-- )
- clear_mmio_p2m_entry(d, gfn + i);
- if ( iomem_deny_access(d, mfn, mfn + nr_mfns - 1) &&
- 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);
- }
- }
- }
- else
- {
- 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++ )
- add |= !clear_mmio_p2m_entry(d, gfn + i);
- ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
- if ( !ret && add )
- ret = -EIO;
- if ( ret && is_hardware_domain(current->domain) )
- printk(XENLOG_ERR
- "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
- ret, add ? "removing" : "denying", d->domain_id,
- mfn, mfn + nr_mfns - 1);
- }
- }
- break;
-
case XEN_DOMCTL_ioport_mapping:
{
#define MAX_IOPORTS 0x10000
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 8f380ed..a44d7a3 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1624,6 +1624,46 @@ 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 end_gfn,
+ unsigned long mfn)
+{
+ int ret = 0;
+ unsigned long i;
+ unsigned long nr_mfns = end_gfn - start_gfn + 1;
+
+ if ( !paging_mode_translate(d) )
+ return 0;
+
+ for ( i = 0; !ret && i < nr_mfns; i++ )
+ if ( !set_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn + i)) )
+ ret = -EIO;
+ if ( ret )
+ while ( i-- )
+ clear_mmio_p2m_entry(d, start_gfn + i);
+
+ return ret;
+}
+
+int unmap_mmio_regions(struct domain *d,
+ unsigned long start_gfn,
+ unsigned long end_gfn,
+ unsigned long mfn)
+{
+ int ret = 0;
+ unsigned long nr_mfns = end_gfn - start_gfn + 1;
+ 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);
+
+ return ret;
+}
+
/*** Audit ***/
#if P2M_AUDIT
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 7cf610a..ae120d9 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -818,6 +818,71 @@ 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;
+ unsigned long gfn_end = gfn + nr_mfns;
+ int add = op->u.memory_mapping.add_mapping;
+
+ ret = -EINVAL;
+ if ( (mfn_end - 1) < mfn || /* wrap? */
+ ((mfn | (mfn_end - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
+ (gfn_end - 1) < gfn ) /* wrap? */
+ return ret;
+
+ ret = -EPERM;
+ if ( !iomem_access_permitted(current->domain, mfn, mfn_end - 1) )
+ return ret;
+
+ ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end - 1, add);
+ if ( ret )
+ return ret;
+
+ 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 - 1);
+ if ( !ret )
+ {
+ ret = map_mmio_regions(d, gfn, gfn_end - 1, mfn);
+ if ( ret )
+ {
+ printk(XENLOG_G_WARNING
+ "memory_map: fail: dom%d gfn=%lx mfn=%lx\n",
+ d->domain_id, gfn, mfn);
+ if ( iomem_deny_access(d, mfn, mfn_end - 1) &&
+ 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 - 1);
+ }
+ }
+ }
+ else
+ {
+ printk(XENLOG_G_INFO
+ "memory_map: remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
+ d->domain_id, gfn, mfn, nr_mfns);
+
+ add = unmap_mmio_regions(d, gfn, gfn_end - 1, mfn);
+ ret = iomem_deny_access(d, mfn, mfn_end - 1);
+ if ( ret && add )
+ ret = -EIO;
+ if ( ret && is_hardware_domain(current->domain) )
+ printk(XENLOG_ERR
+ "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
+ ret, add ? "removing" : "denying", d->domain_id,
+ mfn, mfn_end - 1);
+ }
+ }
+ 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 d2d2ce3..2792d0d 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -2,6 +2,9 @@
#define _XEN_P2M_H
#include <xen/mm.h>
+#include <xen/p2m.h>
+
+#define paddr_bits PADDR_BITS
struct domain;
@@ -83,12 +86,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 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,
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index f4e7253..e7bf06d 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -29,6 +29,7 @@
#include <xen/config.h>
#include <xen/paging.h>
+#include <xen/p2m.h>
#include <asm/mem_sharing.h>
#include <asm/page.h> /* for pagetable_t */
diff --git a/xen/include/xen/p2m.h b/xen/include/xen/p2m.h
new file mode 100644
index 0000000..2be9a86
--- /dev/null
+++ b/xen/include/xen/p2m.h
@@ -0,0 +1,16 @@
+#ifndef _XEN_P2M_COMMON_H
+#define _XEN_P2M_COMMON_H
+
+/* 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 unmap_mmio_regions(struct domain *d,
+ unsigned long start_gfn,
+ unsigned long end_gfn,
+ unsigned long mfn);
+
+#endif /* _XEN_P2M_COMMON_H */
--
1.9.0
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v4 5/7] tools, libxl: parse optional start gfn from the iomem config option
2014-03-25 2:02 [PATCH v4 0/7] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (3 preceding siblings ...)
2014-03-25 2:02 ` [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall Arianna Avanzini
@ 2014-03-25 2:02 ` Arianna Avanzini
2014-03-25 15:39 ` Julien Grall
2014-03-25 2:02 ` [PATCH v4 6/7] tools, libxl: add helpers to establish if guest is auto-translated Arianna Avanzini
2014-03-25 2:02 ` [PATCH v4 7/7] tools, libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
6 siblings, 1 reply; 50+ messages in thread
From: Arianna Avanzini @ 2014-03-25 2:02 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
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>
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: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
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 | 13 +++++++++----
tools/libxl/libxl.h | 10 ++++++++++
tools/libxl/libxl_types.idl | 5 +++--
tools/libxl/xl_cmdimpl.c | 21 ++++++++++++++-------
4 files changed, 36 insertions(+), 13 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index e15a49f..bfb8137 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -588,12 +588,17 @@ 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.
+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.
+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 06bbca6..13f5fe7 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 indicated 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_types.idl b/tools/libxl/libxl_types.idl
index 649ce50..4462586 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -169,8 +169,9 @@ libxl_ioport_range = Struct("ioport_range", [
])
libxl_iomem_range = Struct("iomem_range", [
- ("start", uint64),
- ("number", uint64),
+ ("start", uint64), # start host frame number to be mapped to the guest
+ ("number", uint64), # number of frames to be mapped
+ ("gfn", uint64), # guest frame number used as a start for the mapping
])
libxl_vga_interface_info = Struct("vga_interface_info", [
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 4fc46eb..99a0958 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1208,13 +1208,20 @@ 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);
+ switch(sscanf(buf, "%" SCNx64",%" SCNx64"@%" SCNx64,
+ &b_info->iomem[i].start,
+ &b_info->iomem[i].number,
+ &b_info->iomem[i].gfn)) {
+ case 3: break;
+ case 2:
+ /* default to 1:1 mapping */
+ b_info->iomem[i].gfn = b_info->iomem[i].start;
+ break;
+ default:
+ fprintf(stderr,
+ "xl: Invalid argument parsing iomem: %s\n",
+ buf);
+ exit(1);
}
}
}
--
1.9.0
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v4 6/7] tools, libxl: add helpers to establish if guest is auto-translated
2014-03-25 2:02 [PATCH v4 0/7] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (4 preceding siblings ...)
2014-03-25 2:02 ` [PATCH v4 5/7] tools, libxl: parse optional start gfn from the iomem config option Arianna Avanzini
@ 2014-03-25 2:02 ` Arianna Avanzini
2014-03-25 2:02 ` [PATCH v4 7/7] tools, libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
6 siblings, 0 replies; 50+ messages in thread
From: Arianna Avanzini @ 2014-03-25 2:02 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
JBeulich, avanzini.arianna, viktor.kleinik
This commit adds, for each architecture supported by libxl, a helper
function which establishes if a domain uses an auto-translated
physmap from the libxl_domain_config structure related to it.
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: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
tools/libxl/libxl_arch.h | 2 ++
tools/libxl/libxl_arm.c | 5 +++++
tools/libxl/libxl_x86.c | 6 ++++++
3 files changed, 13 insertions(+)
diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index aee0a91..963d98b 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -22,4 +22,6 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
int libxl__arch_domain_configure(libxl__gc *gc,
libxl_domain_build_info *info,
struct xc_dom_image *dom);
+
+int libxl__arch_auto_translated_physmap(libxl_domain_config *d_config);
#endif
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 0a1c8c5..9f81088 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -11,6 +11,11 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
return 0;
}
+int libxl__arch_auto_translated_physmap(libxl_domain_config *d_config)
+{
+ return 1;
+}
+
static struct arch_info {
const char *guest_type;
const char *timer_compat;
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index b11d036..c6f9c68 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -317,3 +317,9 @@ int libxl__arch_domain_configure(libxl__gc *gc,
{
return 0;
}
+
+int libxl__arch_auto_translated_physmap(libxl_domain_config *d_config)
+{
+ return (d_config->b_info.type == LIBXL_DOMAIN_TYPE_HVM ||
+ libxl_defbool_val(d_config->c_info.pvh));
+}
--
1.9.0
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v4 7/7] tools, libxl: handle the iomem parameter with the memory_mapping hcall
2014-03-25 2:02 [PATCH v4 0/7] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (5 preceding siblings ...)
2014-03-25 2:02 ` [PATCH v4 6/7] tools, libxl: add helpers to establish if guest is auto-translated Arianna Avanzini
@ 2014-03-25 2:02 ` Arianna Avanzini
2014-04-01 15:13 ` Ian Campbell
6 siblings, 1 reply; 50+ messages in thread
From: Arianna Avanzini @ 2014-03-25 2:02 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
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: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
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/libxl/libxl_create.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index a604cd8..fbabb40 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1099,6 +1099,19 @@ 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;
+ }
+ if (!libxl__arch_auto_translated_physmap(d_config))
+ 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.0
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-03-25 2:02 ` [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall Arianna Avanzini
@ 2014-03-25 9:33 ` Jan Beulich
2014-03-28 13:24 ` Arianna Avanzini
2014-03-25 12:35 ` Stefano Stabellini
2014-03-25 13:17 ` Julien Grall
2 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-03-25 9:33 UTC (permalink / raw)
To: Arianna Avanzini
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, julien.grall, etrudeau,
viktor.kleinik
>>> On 25.03.14 at 03:02, <avanzini.arianna@gmail.com> wrote:
> +int unmap_mmio_regions(struct domain *d,
> + unsigned long start_gfn,
> + unsigned long end_gfn,
> + unsigned long mfn)
> +{
> + int ret = 0;
> + unsigned long nr_mfns = end_gfn - start_gfn + 1;
> + 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);
> +
> + return ret;
> +}
Either you keep the function returning a boolean value (but then
please the more conventional 1=success 0=failure), in which case
the function return type should be bool_t, or you make the
function return a proper error code (and propagate that as
necessary in the caller).
Furthermore the mfn parameter is unused here (and effectively
unused also in the ARM variant - it's meaningful only when passing
INSERT to apply_p2m_changes()), so please drop it.
> + 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;
> + unsigned long gfn_end = gfn + nr_mfns;
Afaics all uses of [gm]fn_end involve subtracting 1 - please do this
right here if you already introduce these helper variables.
> --- /dev/null
> +++ b/xen/include/xen/p2m.h
> @@ -0,0 +1,16 @@
> +#ifndef _XEN_P2M_COMMON_H
> +#define _XEN_P2M_COMMON_H
You're on the right track with the guard - please name the header
p2m-common.h, so that it won't violate the common inclusion
hierarchy of the common header including the corresponding arch
one. The alternative would be to keep the name, rename the guard,
and replace all relevant (not necessarily all) #include <asm/p2m.h>
with #include <xen/p2m.h>.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 2/7] arch, arm: add consistency checks to REMOVE p2m changes
2014-03-25 2:02 ` [PATCH v4 2/7] arch, arm: add consistency checks to REMOVE p2m changes Arianna Avanzini
@ 2014-03-25 12:18 ` Stefano Stabellini
2014-03-25 12:51 ` Julien Grall
2014-03-25 17:41 ` Ian Campbell
2 siblings, 0 replies; 50+ messages in thread
From: Stefano Stabellini @ 2014-03-25 12:18 UTC (permalink / raw)
To: Arianna Avanzini
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, xen-devel, julien.grall,
etrudeau, JBeulich, viktor.kleinik
On Tue, 25 Mar 2014, 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>
> 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: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> 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 | 24 ++++++++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index d00c882..bb0db16 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -243,12 +243,13 @@ static int apply_p2m_changes(struct domain *d,
> int rc;
> struct p2m_domain *p2m = &d->arch.p2m;
> lpae_t *first = NULL, *second = NULL, *third = NULL;
> - paddr_t addr;
> + paddr_t addr, _maddr;
> unsigned long cur_first_page = ~0,
> cur_first_offset = ~0,
> cur_second_offset = ~0;
> unsigned long count = 0;
> unsigned int flush = 0;
> + unsigned long mfn;
> bool_t populate = (op == INSERT || op == ALLOCATE);
> lpae_t pte;
>
> @@ -258,6 +259,7 @@ static int apply_p2m_changes(struct domain *d,
> p2m_load_VTTBR(d);
>
> addr = start_gpaddr;
> + _maddr = maddr;
> while ( addr < end_gpaddr )
> {
> if ( cur_first_page != p2m_first_level_index(addr) )
> @@ -327,6 +329,7 @@ static int apply_p2m_changes(struct domain *d,
>
> flush |= pte.p2m.valid;
>
> + mfn = pte.p2m.base;
> /* TODO: Handle other p2m type
> *
> * It's safe to do the put_page here because page_alloc will
> @@ -335,8 +338,6 @@ static int apply_p2m_changes(struct domain *d,
> */
> if ( pte.p2m.valid && p2m_is_foreign(pte.p2m.type) )
> {
> - unsigned long mfn = pte.p2m.base;
> -
> ASSERT(mfn_valid(mfn));
> put_page(mfn_to_page(mfn));
> }
> @@ -367,9 +368,23 @@ static int apply_p2m_changes(struct domain *d,
> maddr += PAGE_SIZE;
> }
> break;
> - case RELINQUISH:
> case REMOVE:
> {
> + ASSERT(pte.p2m.valid);
> + /*
> + * 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 ( _maddr != pfn_to_paddr(mfn) )
> + return -EINVAL;
> + }
> + /* fall through */
> + case RELINQUISH:
> + {
> if ( !pte.p2m.valid )
> {
> count++;
> @@ -408,6 +423,7 @@ static int apply_p2m_changes(struct domain *d,
>
> /* Got the next page */
> addr += PAGE_SIZE;
> + _maddr += PAGE_SIZE;
> }
>
> if ( flush )
> --
> 1.9.0
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 3/7] arch, arm: let map_mmio_regions() take pfn as parameters
2014-03-25 2:02 ` [PATCH v4 3/7] arch, arm: let map_mmio_regions() take pfn as parameters Arianna Avanzini
@ 2014-03-25 12:22 ` Stefano Stabellini
2014-03-25 12:54 ` Julien Grall
2014-03-25 13:00 ` Julien Grall
1 sibling, 1 reply; 50+ messages in thread
From: Stefano Stabellini @ 2014-03-25 12:22 UTC (permalink / raw)
To: Arianna Avanzini
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, xen-devel, julien.grall,
etrudeau, JBeulich, viktor.kleinik
On Tue, 25 Mar 2014, Arianna Avanzini wrote:
> 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.
>
> NOTE: platform-specific code has not been tested.
>
> Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
> Cc: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Paolo Valente <paolo.valente@unimore.it>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Julien Grall <julien.grall@citrix.com>
> Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
> ---
> xen/arch/arm/domain_build.c | 7 ++++---
> xen/arch/arm/gic.c | 21 ++++++++++++---------
> xen/arch/arm/p2m.c | 13 ++++++++-----
> xen/arch/arm/platforms/exynos5.c | 13 ++++++++-----
> xen/arch/arm/platforms/omap5.c | 25 ++++++++++++++++---------
> xen/arch/arm/platforms/xgene-storm.c | 4 +++-
> xen/include/asm-arm/p2m.h | 11 ++++++-----
> 7 files changed, 57 insertions(+), 37 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4a0411c..10f508d 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -744,9 +744,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(PAGE_ALIGN(addr + size - 1)),
> + 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 074624e..55354d4 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -882,20 +882,23 @@ 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(PAGE_ALIGN(d->arch.vgic.cbase +
> + PAGE_SIZE - 1)),
> + 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(PAGE_ALIGN(d->arch.vgic.cbase +
> + (2 * PAGE_SIZE) - 1)),
> + 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(PAGE_ALIGN(d->arch.vgic.cbase +
> + (2 * PAGE_SIZE) - 1)),
> + 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 bb0db16..c8e77b9 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -469,12 +469,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);
> }
This doesn't make any sense. Shouldn't you modify apply_p2m_changes to
take pfns as parameters too?
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-03-25 2:02 ` [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall Arianna Avanzini
2014-03-25 9:33 ` Jan Beulich
@ 2014-03-25 12:35 ` Stefano Stabellini
2014-03-25 14:10 ` Jan Beulich
2014-03-25 13:17 ` Julien Grall
2 siblings, 1 reply; 50+ messages in thread
From: Stefano Stabellini @ 2014-03-25 12:35 UTC (permalink / raw)
To: Arianna Avanzini
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, xen-devel, julien.grall,
etrudeau, JBeulich, viktor.kleinik
On Tue, 25 Mar 2014, Arianna Avanzini wrote:
> This commit introduces a first attempt of implementation of the
> XEN_DOMCTL_memory_mapping hypercall for ARM. As the implementation
> would have been almost identical to the one for x86, this code has
> been made common to both the architectures. The only difference
> between the two procedures lies in the arch-specific implementation
> of the map_mmio_regions() and unmap_mmio_regions() functions.
>
> 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: 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: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
>
> ---
>
> 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.
> - 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 and error.
> - Make ranges inclusive of the end address in map_mmio_regions()
> and unmap_mmio_regions() for x86.
> - Turn hard tabs into spaces.
>
> v3:
> - Move code to xen/common/domctl.c; abstract out differences between
> the x86 and ARM code:
> . add map_mmio_regions() and unmap_mmio_regions() functions for x86;
> . add a paddr_bits variable for ARM.
> - Use pfn as parameters to the unmap_mmio_regions() function.
> - Compute gfn + nr_mfns and mfn + nr_mfns only once.
>
> v2:
> - Move code to xen/arm/domctl.c.
> - 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/arm/p2m.c | 12 ++++++++
> xen/arch/x86/domctl.c | 70 -----------------------------------------------
> xen/arch/x86/mm/p2m.c | 40 +++++++++++++++++++++++++++
> xen/common/domctl.c | 65 +++++++++++++++++++++++++++++++++++++++++++
> xen/include/asm-arm/p2m.h | 9 ++----
> xen/include/asm-x86/p2m.h | 1 +
> xen/include/xen/p2m.h | 16 +++++++++++
> 7 files changed, 137 insertions(+), 76 deletions(-)
> create mode 100644 xen/include/xen/p2m.h
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index c8e77b9..961213d 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -480,6 +480,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 end_gfn,
> + unsigned long mfn)
> +{
> + return apply_p2m_changes(d, REMOVE,
> + 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,
> unsigned long gpfn,
> unsigned long mfn,
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 26635ff..69a7fbf 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -639,76 +639,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;
> - int add = domctl->u.memory_mapping.add_mapping;
> - unsigned long i;
> -
> - ret = -EINVAL;
> - if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
> - ((mfn | (mfn + nr_mfns - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> - (gfn + nr_mfns - 1) < gfn ) /* wrap? */
> - break;
> -
> - ret = -EPERM;
> - if ( !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
> - break;
> -
> - ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, 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 + nr_mfns - 1);
> - if ( !ret && paging_mode_translate(d) )
> - {
> - for ( i = 0; !ret && i < nr_mfns; i++ )
> - if ( !set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i)) )
> - ret = -EIO;
> - if ( ret )
> - {
> - printk(XENLOG_G_WARNING
> - "memory_map:fail: dom%d gfn=%lx mfn=%lx\n",
> - d->domain_id, gfn + i, mfn + i);
> - while ( i-- )
> - clear_mmio_p2m_entry(d, gfn + i);
> - if ( iomem_deny_access(d, mfn, mfn + nr_mfns - 1) &&
> - 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);
> - }
> - }
> - }
> - else
> - {
> - 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++ )
> - add |= !clear_mmio_p2m_entry(d, gfn + i);
> - ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
> - if ( !ret && add )
> - ret = -EIO;
> - if ( ret && is_hardware_domain(current->domain) )
> - printk(XENLOG_ERR
> - "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
> - ret, add ? "removing" : "denying", d->domain_id,
> - mfn, mfn + nr_mfns - 1);
> - }
> - }
> - break;
> -
> case XEN_DOMCTL_ioport_mapping:
> {
> #define MAX_IOPORTS 0x10000
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 8f380ed..a44d7a3 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1624,6 +1624,46 @@ 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 end_gfn,
> + unsigned long mfn)
> +{
> + int ret = 0;
> + unsigned long i;
> + unsigned long nr_mfns = end_gfn - start_gfn + 1;
> +
> + if ( !paging_mode_translate(d) )
> + return 0;
> +
> + for ( i = 0; !ret && i < nr_mfns; i++ )
> + if ( !set_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn + i)) )
> + ret = -EIO;
> + if ( ret )
> + while ( i-- )
> + clear_mmio_p2m_entry(d, start_gfn + i);
> +
> + return ret;
> +}
> +
> +int unmap_mmio_regions(struct domain *d,
> + unsigned long start_gfn,
> + unsigned long end_gfn,
> + unsigned long mfn)
> +{
> + int ret = 0;
> + unsigned long nr_mfns = end_gfn - start_gfn + 1;
> + 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);
> + return ret;
> +}
> +
> /*** Audit ***/
>
> #if P2M_AUDIT
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 7cf610a..ae120d9 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -818,6 +818,71 @@ 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;
> + unsigned long gfn_end = gfn + nr_mfns;
> + int add = op->u.memory_mapping.add_mapping;
> +
> + ret = -EINVAL;
> + if ( (mfn_end - 1) < mfn || /* wrap? */
> + ((mfn | (mfn_end - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> + (gfn_end - 1) < gfn ) /* wrap? */
> + return ret;
> +
> + ret = -EPERM;
> + if ( !iomem_access_permitted(current->domain, mfn, mfn_end - 1) )
> + return ret;
> +
> + ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end - 1, add);
> + if ( ret )
> + return ret;
> +
> + 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 - 1);
> + if ( !ret )
> + {
> + ret = map_mmio_regions(d, gfn, gfn_end - 1, mfn);
> + if ( ret )
> + {
> + printk(XENLOG_G_WARNING
> + "memory_map: fail: dom%d gfn=%lx mfn=%lx\n",
> + d->domain_id, gfn, mfn);
> + if ( iomem_deny_access(d, mfn, mfn_end - 1) &&
> + 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 - 1);
> + }
> + }
> + }
> + else
> + {
> + printk(XENLOG_G_INFO
> + "memory_map: remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
> + d->domain_id, gfn, mfn, nr_mfns);
> +
> + add = unmap_mmio_regions(d, gfn, gfn_end - 1, mfn);
> + ret = iomem_deny_access(d, mfn, mfn_end - 1);
> + if ( ret && add )
> + ret = -EIO;
This is a functional change. The removed code did:
- ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
- if ( !ret && add )
- ret = -EIO;
The change looks correct to me, but I would appreciate if you could
mention it explicitly in the commit message.
> + if ( ret && is_hardware_domain(current->domain) )
> + printk(XENLOG_ERR
> + "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
> + ret, add ? "removing" : "denying", d->domain_id,
> + mfn, mfn_end - 1);
> + }
> + }
> + 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 d2d2ce3..2792d0d 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -2,6 +2,9 @@
> #define _XEN_P2M_H
>
> #include <xen/mm.h>
> +#include <xen/p2m.h>
> +
> +#define paddr_bits PADDR_BITS
>
> struct domain;
>
> @@ -83,12 +86,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 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,
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index f4e7253..e7bf06d 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -29,6 +29,7 @@
>
> #include <xen/config.h>
> #include <xen/paging.h>
> +#include <xen/p2m.h>
> #include <asm/mem_sharing.h>
> #include <asm/page.h> /* for pagetable_t */
>
> diff --git a/xen/include/xen/p2m.h b/xen/include/xen/p2m.h
> new file mode 100644
> index 0000000..2be9a86
> --- /dev/null
> +++ b/xen/include/xen/p2m.h
> @@ -0,0 +1,16 @@
> +#ifndef _XEN_P2M_COMMON_H
> +#define _XEN_P2M_COMMON_H
> +
> +/* 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 unmap_mmio_regions(struct domain *d,
> + unsigned long start_gfn,
> + unsigned long end_gfn,
> + unsigned long mfn);
> +
> +#endif /* _XEN_P2M_COMMON_H */
> --
> 1.9.0
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 1/7] arch, arm: domain build: let dom0 access I/O memory of mapped devices
2014-03-25 2:02 ` [PATCH v4 1/7] arch, arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
@ 2014-03-25 12:37 ` Julien Grall
0 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2014-03-25 12:37 UTC (permalink / raw)
To: Arianna Avanzini
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, Ian.Campbell, etrudeau,
JBeulich, viktor.kleinik
Hi Arianna,
On 03/25/2014 02:02 AM, Arianna Avanzini wrote:
> Currently, dom0 is allowed access to the I/O memory ranges used
> to access devices exposed to it, but it doesn't have those
> ranges in its iomem_caps. This commit 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.
>
> Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
> Acked-by: Ian Campbell <Ian.Campbell@eu.citrix.com>
> Cc: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Paolo Valente <paolo.valente@unimore.it>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
> 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: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 2/7] arch, arm: add consistency checks to REMOVE p2m changes
2014-03-25 2:02 ` [PATCH v4 2/7] arch, arm: add consistency checks to REMOVE p2m changes Arianna Avanzini
2014-03-25 12:18 ` Stefano Stabellini
@ 2014-03-25 12:51 ` Julien Grall
2014-03-25 13:10 ` Julien Grall
2014-03-25 17:41 ` Ian Campbell
2 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2014-03-25 12:51 UTC (permalink / raw)
To: Arianna Avanzini
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, Ian.Campbell, etrudeau,
JBeulich, viktor.kleinik
Hi Arianna,
Thank you for the patch,
On 03/25/2014 02:02 AM, 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>
> 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: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
>
> ---
>
> 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 | 24 ++++++++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index d00c882..bb0db16 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -243,12 +243,13 @@ static int apply_p2m_changes(struct domain *d,
> int rc;
> struct p2m_domain *p2m = &d->arch.p2m;
> lpae_t *first = NULL, *second = NULL, *third = NULL;
> - paddr_t addr;
> + paddr_t addr, _maddr;
> unsigned long cur_first_page = ~0,
> cur_first_offset = ~0,
> cur_second_offset = ~0;
> unsigned long count = 0;
> unsigned int flush = 0;
> + unsigned long mfn;
> bool_t populate = (op == INSERT || op == ALLOCATE);
> lpae_t pte;
>
> @@ -258,6 +259,7 @@ static int apply_p2m_changes(struct domain *d,
> p2m_load_VTTBR(d);
>
> addr = start_gpaddr;
> + _maddr = maddr;
You don't need a temporary variable to hold maddr and increment it.
You can directly use maddr, but you will have to remove "maddr +=
PAGE_SIZE" in INSERT.
You also need to increment maddr when first/second level are not valid.
While reading the code, I'm wondering if we need to return an error if
we are trying to remove a non-existent page. Ian, Stefano, any thoughs?
> while ( addr < end_gpaddr )
> {
> if ( cur_first_page != p2m_first_level_index(addr) )
> @@ -327,6 +329,7 @@ static int apply_p2m_changes(struct domain *d,
>
> flush |= pte.p2m.valid;
>
> + mfn = pte.p2m.base;
I would move mfn = ... in REMOVE part because it may not be valid on
some place and wrongly used in the future.
> /* TODO: Handle other p2m type
> *
> * It's safe to do the put_page here because page_alloc will
> @@ -335,8 +338,6 @@ static int apply_p2m_changes(struct domain *d,
> */
> if ( pte.p2m.valid && p2m_is_foreign(pte.p2m.type) )
> {
> - unsigned long mfn = pte.p2m.base;
> -
> ASSERT(mfn_valid(mfn));
> put_page(mfn_to_page(mfn));
> }
> @@ -367,9 +368,23 @@ static int apply_p2m_changes(struct domain *d,
> maddr += PAGE_SIZE;
> }
> break;
> - case RELINQUISH:
> case REMOVE:
> {
> + ASSERT(pte.p2m.valid);
Why did you add an ASSERT here?
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 3/7] arch, arm: let map_mmio_regions() take pfn as parameters
2014-03-25 12:22 ` Stefano Stabellini
@ 2014-03-25 12:54 ` Julien Grall
2014-03-28 12:51 ` Arianna Avanzini
0 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2014-03-25 12:54 UTC (permalink / raw)
To: Stefano Stabellini
Cc: julien.grall, paolo.valente, keir, tim, dario.faggioli,
Ian.Jackson, xen-devel, Ian.Campbell, etrudeau, JBeulich,
Arianna Avanzini, viktor.kleinik
Hi Stefano and Arianna,
On 03/25/2014 12:22 PM, Stefano Stabellini wrote:
>> - 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);
>> }
>
> This doesn't make any sense. Shouldn't you modify apply_p2m_changes to
> take pfns as parameters too?
If so, I think it should be a separate patch. Her change makes sense
with the current interface.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 3/7] arch, arm: let map_mmio_regions() take pfn as parameters
2014-03-25 2:02 ` [PATCH v4 3/7] arch, arm: let map_mmio_regions() take pfn as parameters Arianna Avanzini
2014-03-25 12:22 ` Stefano Stabellini
@ 2014-03-25 13:00 ` Julien Grall
1 sibling, 0 replies; 50+ messages in thread
From: Julien Grall @ 2014-03-25 13:00 UTC (permalink / raw)
To: Arianna Avanzini
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, Ian.Campbell, etrudeau,
JBeulich, viktor.kleinik
Hi Arianna,
Thank you for the patch.
On 03/25/2014 02:02 AM, Arianna Avanzini wrote:
> 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.
>
> NOTE: platform-specific code has not been tested.
>
> Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
> Cc: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Paolo Valente <paolo.valente@unimore.it>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Julien Grall <julien.grall@citrix.com>
> Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
> ---
> xen/arch/arm/domain_build.c | 7 ++++---
> xen/arch/arm/gic.c | 21 ++++++++++++---------
> xen/arch/arm/p2m.c | 13 ++++++++-----
> xen/arch/arm/platforms/exynos5.c | 13 ++++++++-----
> xen/arch/arm/platforms/omap5.c | 25 ++++++++++++++++---------
> xen/arch/arm/platforms/xgene-storm.c | 4 +++-
> xen/include/asm-arm/p2m.h | 11 ++++++-----
> 7 files changed, 57 insertions(+), 37 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4a0411c..10f508d 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -744,9 +744,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(PAGE_ALIGN(addr + size - 1)),
It would be nice a have a macro for paddr_to_pfn(PAGE_ALIGN(...)). You
are using this pattern multiple times in the patch.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 2/7] arch, arm: add consistency checks to REMOVE p2m changes
2014-03-25 12:51 ` Julien Grall
@ 2014-03-25 13:10 ` Julien Grall
0 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2014-03-25 13:10 UTC (permalink / raw)
To: Arianna Avanzini
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, Ian.Campbell, etrudeau,
JBeulich, viktor.kleinik
On 03/25/2014 12:51 PM, Julien Grall wrote:
> Hi Arianna,
>
> Thank you for the patch,
>
> On 03/25/2014 02:02 AM, 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>
>> 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: Eric Trudeau <etrudeau@broadcom.com>
>> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
>>
>> ---
>>
>> 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 | 24 ++++++++++++++++++++----
>> 1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index d00c882..bb0db16 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -243,12 +243,13 @@ static int apply_p2m_changes(struct domain *d,
>> int rc;
>> struct p2m_domain *p2m = &d->arch.p2m;
>> lpae_t *first = NULL, *second = NULL, *third = NULL;
>> - paddr_t addr;
>> + paddr_t addr, _maddr;
>> unsigned long cur_first_page = ~0,
>> cur_first_offset = ~0,
>> cur_second_offset = ~0;
>> unsigned long count = 0;
>> unsigned int flush = 0;
>> + unsigned long mfn;
>> bool_t populate = (op == INSERT || op == ALLOCATE);
>> lpae_t pte;
>>
>> @@ -258,6 +259,7 @@ static int apply_p2m_changes(struct domain *d,
>> p2m_load_VTTBR(d);
>>
>> addr = start_gpaddr;
>> + _maddr = maddr;
>
> You don't need a temporary variable to hold maddr and increment it.
> You can directly use maddr, but you will have to remove "maddr +=
> PAGE_SIZE" in INSERT.
>
> You also need to increment maddr when first/second level are not valid.
>
> While reading the code, I'm wondering if we need to return an error if
> we are trying to remove a non-existent page. Ian, Stefano, any thoughs?
>
>> while ( addr < end_gpaddr )
>> {
>> if ( cur_first_page != p2m_first_level_index(addr) )
>> @@ -327,6 +329,7 @@ static int apply_p2m_changes(struct domain *d,
>>
>> flush |= pte.p2m.valid;
>>
>> + mfn = pte.p2m.base;
>
> I would move mfn = ... in REMOVE part because it may not be valid on
> some place and wrongly used in the future.
>
>> /* TODO: Handle other p2m type
>> *
>> * It's safe to do the put_page here because page_alloc will
>> @@ -335,8 +338,6 @@ static int apply_p2m_changes(struct domain *d,
>> */
>> if ( pte.p2m.valid && p2m_is_foreign(pte.p2m.type) )
>> {
>> - unsigned long mfn = pte.p2m.base;
>> -
>> ASSERT(mfn_valid(mfn));
>> put_page(mfn_to_page(mfn));
>> }
>> @@ -367,9 +368,23 @@ static int apply_p2m_changes(struct domain *d,
>> maddr += PAGE_SIZE;
>> }
>> break;
>> - case RELINQUISH:
>> case REMOVE:
>> {
>> + ASSERT(pte.p2m.valid);
>
> Why did you add an ASSERT here?
Hmmm ... after reading you patch #4, this assert is wrong. Anyone that
can call XEN_DOMCTL_memory_mapping (e.g the toolstack) can crash Xen
because there is no check that the GFN has a valid mapping.
You should replace this ASSERT by an if ...
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-03-25 2:02 ` [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall Arianna Avanzini
2014-03-25 9:33 ` Jan Beulich
2014-03-25 12:35 ` Stefano Stabellini
@ 2014-03-25 13:17 ` Julien Grall
2014-04-01 14:52 ` Ian Campbell
2 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2014-03-25 13:17 UTC (permalink / raw)
To: Arianna Avanzini
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, Ian.Campbell, etrudeau,
JBeulich, viktor.kleinik
Hi Arianna,
Thank you for the patch.
On 03/25/2014 02:02 AM, Arianna Avanzini wrote:
> This commit introduces a first attempt of implementation of the
> XEN_DOMCTL_memory_mapping hypercall for ARM. As the implementation
> would have been almost identical to the one for x86, this code has
> been made common to both the architectures. The only difference
> between the two procedures lies in the arch-specific implementation
> of the map_mmio_regions() and unmap_mmio_regions() functions.
>
> 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: 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: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
>
> ---
>
> 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.
> - 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 and error.
> - Make ranges inclusive of the end address in map_mmio_regions()
> and unmap_mmio_regions() for x86.
> - Turn hard tabs into spaces.
>
> v3:
> - Move code to xen/common/domctl.c; abstract out differences between
> the x86 and ARM code:
> . add map_mmio_regions() and unmap_mmio_regions() functions for x86;
> . add a paddr_bits variable for ARM.
> - Use pfn as parameters to the unmap_mmio_regions() function.
> - Compute gfn + nr_mfns and mfn + nr_mfns only once.
>
> v2:
> - Move code to xen/arm/domctl.c.
> - 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/arm/p2m.c | 12 ++++++++
> xen/arch/x86/domctl.c | 70 -----------------------------------------------
> xen/arch/x86/mm/p2m.c | 40 +++++++++++++++++++++++++++
> xen/common/domctl.c | 65 +++++++++++++++++++++++++++++++++++++++++++
> xen/include/asm-arm/p2m.h | 9 ++----
> xen/include/asm-x86/p2m.h | 1 +
> xen/include/xen/p2m.h | 16 +++++++++++
> 7 files changed, 137 insertions(+), 76 deletions(-)
> create mode 100644 xen/include/xen/p2m.h
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index c8e77b9..961213d 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -480,6 +480,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 end_gfn,
> + unsigned long mfn)
> +{
> + return apply_p2m_changes(d, REMOVE,
> + 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,
> unsigned long gpfn,
> unsigned long mfn,
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 26635ff..69a7fbf 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -639,76 +639,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;
> - int add = domctl->u.memory_mapping.add_mapping;
> - unsigned long i;
> -
> - ret = -EINVAL;
> - if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
> - ((mfn | (mfn + nr_mfns - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> - (gfn + nr_mfns - 1) < gfn ) /* wrap? */
> - break;
> -
> - ret = -EPERM;
> - if ( !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
> - break;
> -
> - ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, 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 + nr_mfns - 1);
> - if ( !ret && paging_mode_translate(d) )
> - {
> - for ( i = 0; !ret && i < nr_mfns; i++ )
> - if ( !set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i)) )
> - ret = -EIO;
> - if ( ret )
> - {
> - printk(XENLOG_G_WARNING
> - "memory_map:fail: dom%d gfn=%lx mfn=%lx\n",
> - d->domain_id, gfn + i, mfn + i);
> - while ( i-- )
> - clear_mmio_p2m_entry(d, gfn + i);
> - if ( iomem_deny_access(d, mfn, mfn + nr_mfns - 1) &&
> - 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);
> - }
> - }
> - }
> - else
> - {
> - 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++ )
> - add |= !clear_mmio_p2m_entry(d, gfn + i);
> - ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
> - if ( !ret && add )
> - ret = -EIO;
> - if ( ret && is_hardware_domain(current->domain) )
> - printk(XENLOG_ERR
> - "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
> - ret, add ? "removing" : "denying", d->domain_id,
> - mfn, mfn + nr_mfns - 1);
> - }
> - }
> - break;
> -
> case XEN_DOMCTL_ioport_mapping:
> {
> #define MAX_IOPORTS 0x10000
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 8f380ed..a44d7a3 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1624,6 +1624,46 @@ 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 end_gfn,
> + unsigned long mfn)
> +{
> + int ret = 0;
> + unsigned long i;
> + unsigned long nr_mfns = end_gfn - start_gfn + 1;
> +
> + if ( !paging_mode_translate(d) )
> + return 0;
> +
> + for ( i = 0; !ret && i < nr_mfns; i++ )
> + if ( !set_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn + i)) )
> + ret = -EIO;
> + if ( ret )
> + while ( i-- )
> + clear_mmio_p2m_entry(d, start_gfn + i);
> +
> + return ret;
> +}
> +
> +int unmap_mmio_regions(struct domain *d,
> + unsigned long start_gfn,
> + unsigned long end_gfn,
> + unsigned long mfn)
> +{
> + int ret = 0;
> + unsigned long nr_mfns = end_gfn - start_gfn + 1;
> + 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);
> +
> + return ret;
> +}
> +
> /*** Audit ***/
>
> #if P2M_AUDIT
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 7cf610a..ae120d9 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -818,6 +818,71 @@ 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;
> + unsigned long gfn_end = gfn + nr_mfns;
> + int add = op->u.memory_mapping.add_mapping;
> +
> + ret = -EINVAL;
> + if ( (mfn_end - 1) < mfn || /* wrap? */
> + ((mfn | (mfn_end - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> + (gfn_end - 1) < gfn ) /* wrap? */
> + return ret;
Would not it be better to only rely on the GFN when the toolstack is
removing the mapping?
I know this is not the previous behavior, but most of the hypercall
which deal with removing mapping only take GFN.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-03-25 12:35 ` Stefano Stabellini
@ 2014-03-25 14:10 ` Jan Beulich
2014-03-25 15:10 ` Stefano Stabellini
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-03-25 14:10 UTC (permalink / raw)
To: stefano.stabellini, Arianna Avanzini
Cc: Ian.Campbell, paolo.valente, keir, tim, dario.faggioli,
Ian.Jackson, xen-devel, julien.grall, etrudeau, viktor.kleinik
>>> On 25.03.14 at 13:35, <stefano.stabellini@eu.citrix.com> wrote:
> On Tue, 25 Mar 2014, Arianna Avanzini wrote:
>> + add = unmap_mmio_regions(d, gfn, gfn_end - 1, mfn);
>> + ret = iomem_deny_access(d, mfn, mfn_end - 1);
>> + if ( ret && add )
>> + ret = -EIO;
>
> This is a functional change. The removed code did:
>
> - ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
> - if ( !ret && add )
> - ret = -EIO;
>
> The change looks correct to me, but I would appreciate if you could
> mention it explicitly in the commit message.
I overlooked this, and no, it is not correct (and I don't follow why
you think it is): We only want to force ret to -EIO if
iomem_deny_access() didn't already return another error.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-03-25 14:10 ` Jan Beulich
@ 2014-03-25 15:10 ` Stefano Stabellini
2014-03-25 15:36 ` Jan Beulich
0 siblings, 1 reply; 50+ messages in thread
From: Stefano Stabellini @ 2014-03-25 15:10 UTC (permalink / raw)
To: Jan Beulich
Cc: julien.grall, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, xen-devel, Ian.Campbell,
etrudeau, Arianna Avanzini, viktor.kleinik
On Tue, 25 Mar 2014, Jan Beulich wrote:
> >>> On 25.03.14 at 13:35, <stefano.stabellini@eu.citrix.com> wrote:
> > On Tue, 25 Mar 2014, Arianna Avanzini wrote:
> >> + add = unmap_mmio_regions(d, gfn, gfn_end - 1, mfn);
> >> + ret = iomem_deny_access(d, mfn, mfn_end - 1);
> >> + if ( ret && add )
> >> + ret = -EIO;
> >
> > This is a functional change. The removed code did:
> >
> > - ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
> > - if ( !ret && add )
> > - ret = -EIO;
> >
> > The change looks correct to me, but I would appreciate if you could
> > mention it explicitly in the commit message.
>
> I overlooked this, and no, it is not correct (and I don't follow why
> you think it is): We only want to force ret to -EIO if
> iomem_deny_access() didn't already return another error.
I thought that the intention was to return -EIO if unmap_mmio_regions
failed, even if iomem_deny_access is successful.
iomem_deny_access errors should be taken care by the
iomem_access_permitted check above, returning -EPERM.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-03-25 15:10 ` Stefano Stabellini
@ 2014-03-25 15:36 ` Jan Beulich
2014-03-25 15:42 ` Stefano Stabellini
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-03-25 15:36 UTC (permalink / raw)
To: stefano.stabellini
Cc: Ian.Campbell, paolo.valente, keir, tim, dario.faggioli,
Ian.Jackson, xen-devel, julien.grall, etrudeau, Arianna Avanzini,
viktor.kleinik
>>> On 25.03.14 at 16:10, <stefano.stabellini@eu.citrix.com> wrote:
> On Tue, 25 Mar 2014, Jan Beulich wrote:
>> >>> On 25.03.14 at 13:35, <stefano.stabellini@eu.citrix.com> wrote:
>> > On Tue, 25 Mar 2014, Arianna Avanzini wrote:
>> >> + add = unmap_mmio_regions(d, gfn, gfn_end - 1, mfn);
>> >> + ret = iomem_deny_access(d, mfn, mfn_end - 1);
>> >> + if ( ret && add )
>> >> + ret = -EIO;
>> >
>> > This is a functional change. The removed code did:
>> >
>> > - ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
>> > - if ( !ret && add )
>> > - ret = -EIO;
>> >
>> > The change looks correct to me, but I would appreciate if you could
>> > mention it explicitly in the commit message.
>>
>> I overlooked this, and no, it is not correct (and I don't follow why
>> you think it is): We only want to force ret to -EIO if
>> iomem_deny_access() didn't already return another error.
>
> I thought that the intention was to return -EIO if unmap_mmio_regions
> failed, even if iomem_deny_access is successful.
> iomem_deny_access errors should be taken care by the
> iomem_access_permitted check above, returning -EPERM.
It certainly doesn't matter _that_ much which error to return here,
but code movement shouldn't be done with silent functional changes,
the more that the change is buggy beyond altering original behavior:
Consider the case of ret = 0 (iomem_deny_access() succeeded) and
add = 1 (unmap_mmio_regions() failed) - the failure would be lost
with the changed code.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 5/7] tools, libxl: parse optional start gfn from the iomem config option
2014-03-25 2:02 ` [PATCH v4 5/7] tools, libxl: parse optional start gfn from the iomem config option Arianna Avanzini
@ 2014-03-25 15:39 ` Julien Grall
2014-03-25 15:45 ` Julien Grall
2014-03-25 16:27 ` Ian Campbell
0 siblings, 2 replies; 50+ messages in thread
From: Julien Grall @ 2014-03-25 15:39 UTC (permalink / raw)
To: Arianna Avanzini
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, Ian.Campbell, etrudeau,
JBeulich, viktor.kleinik
Hi Arianna,
Thank you for the patch.
On 03/25/2014 02:02 AM, Arianna Avanzini wrote:
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 06bbca6..13f5fe7 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 indicated 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_types.idl b/tools/libxl/libxl_types.idl
> index 649ce50..4462586 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -169,8 +169,9 @@ libxl_ioport_range = Struct("ioport_range", [
> ])
>
> libxl_iomem_range = Struct("iomem_range", [
> - ("start", uint64),
> - ("number", uint64),
> + ("start", uint64), # start host frame number to be mapped to the guest
> + ("number", uint64), # number of frames to be mapped
> + ("gfn", uint64), # guest frame number used as a start for the mapping
> ])
When this structure will be used by another toolstack (e.g not xl), the
default value for gfn will be 0. This is wrong because we won't be able
to differentiate a user that will want to map to the GFN 0 and a 1:1
mapping. -1 seems the best default value for now.
You can use init_val in the IDL to set this value.
> libxl_vga_interface_info = Struct("vga_interface_info", [
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 4fc46eb..99a0958 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1208,13 +1208,20 @@ static void parse_config_data(const char *config_source,
> "xl: Unable to get element %d in iomem list\n", i);
> exit(1);
> }
It's a bug on the current code. We have to init correctly iomem before
with libxl_iomem_range_init(&b_info->iomem);
> - 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);
> + switch(sscanf(buf, "%" SCNx64",%" SCNx64"@%" SCNx64,
> + &b_info->iomem[i].start,
> + &b_info->iomem[i].number,
> + &b_info->iomem[i].gfn)) {
I don't like the switch(sscanf(... it's hard to read.
I would prefer:
ret = sscanf(...);
switch (ret) {
}
> + case 3: break;
> + case 2:
> + /* default to 1:1 mapping */
> + b_info->iomem[i].gfn = b_info->iomem[i].start;
> + break;
If the iomem_range is initialized with the default value. You can defer
this initialization in libxl.
The result code here will be nicer:
ret = sscanf(...)
if ( ret < 2 )
fprintf(stderr, ...);
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-03-25 15:36 ` Jan Beulich
@ 2014-03-25 15:42 ` Stefano Stabellini
2014-04-01 15:01 ` Ian Campbell
0 siblings, 1 reply; 50+ messages in thread
From: Stefano Stabellini @ 2014-03-25 15:42 UTC (permalink / raw)
To: Jan Beulich
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, julien.grall, etrudeau,
Arianna Avanzini, viktor.kleinik
On Tue, 25 Mar 2014, Jan Beulich wrote:
> >>> On 25.03.14 at 16:10, <stefano.stabellini@eu.citrix.com> wrote:
> > On Tue, 25 Mar 2014, Jan Beulich wrote:
> >> >>> On 25.03.14 at 13:35, <stefano.stabellini@eu.citrix.com> wrote:
> >> > On Tue, 25 Mar 2014, Arianna Avanzini wrote:
> >> >> + add = unmap_mmio_regions(d, gfn, gfn_end - 1, mfn);
> >> >> + ret = iomem_deny_access(d, mfn, mfn_end - 1);
> >> >> + if ( ret && add )
> >> >> + ret = -EIO;
> >> >
> >> > This is a functional change. The removed code did:
> >> >
> >> > - ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
> >> > - if ( !ret && add )
> >> > - ret = -EIO;
> >> >
> >> > The change looks correct to me, but I would appreciate if you could
> >> > mention it explicitly in the commit message.
> >>
> >> I overlooked this, and no, it is not correct (and I don't follow why
> >> you think it is): We only want to force ret to -EIO if
> >> iomem_deny_access() didn't already return another error.
> >
> > I thought that the intention was to return -EIO if unmap_mmio_regions
> > failed, even if iomem_deny_access is successful.
> > iomem_deny_access errors should be taken care by the
> > iomem_access_permitted check above, returning -EPERM.
>
> It certainly doesn't matter _that_ much which error to return here,
> but code movement shouldn't be done with silent functional changes,
I agree
> the more that the change is buggy beyond altering original behavior:
> Consider the case of ret = 0 (iomem_deny_access() succeeded) and
> add = 1 (unmap_mmio_regions() failed) - the failure would be lost
> with the changed code.
Yes, you are right, the original code is the correct one.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 5/7] tools, libxl: parse optional start gfn from the iomem config option
2014-03-25 15:39 ` Julien Grall
@ 2014-03-25 15:45 ` Julien Grall
2014-03-25 16:27 ` Ian Campbell
1 sibling, 0 replies; 50+ messages in thread
From: Julien Grall @ 2014-03-25 15:45 UTC (permalink / raw)
To: Arianna Avanzini
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, Ian.Campbell, etrudeau,
JBeulich, viktor.kleinik
On 03/25/2014 03:39 PM, Julien Grall wrote:
>> + case 3: break;
>> + case 2:
>> + /* default to 1:1 mapping */
>> + b_info->iomem[i].gfn = b_info->iomem[i].start;
>> + break;
>
> If the iomem_range is initialized with the default value. You can defer
> this initialization in libxl.
To be more precise, I would move this set in libxl__domain_build_info_setdefault.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 5/7] tools, libxl: parse optional start gfn from the iomem config option
2014-03-25 15:39 ` Julien Grall
2014-03-25 15:45 ` Julien Grall
@ 2014-03-25 16:27 ` Ian Campbell
1 sibling, 0 replies; 50+ messages in thread
From: Ian Campbell @ 2014-03-25 16:27 UTC (permalink / raw)
To: Julien Grall
Cc: paolo.valente, keir, stefano.stabellini, tim, dario.faggioli,
Ian.Jackson, xen-devel, julien.grall, etrudeau, JBeulich,
Arianna Avanzini, viktor.kleinik
On Tue, 2014-03-25 at 15:39 +0000, Julien Grall wrote:
> > libxl_vga_interface_info = Struct("vga_interface_info", [
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 4fc46eb..99a0958 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -1208,13 +1208,20 @@ static void parse_config_data(const char *config_source,
> > "xl: Unable to get element %d in iomem list\n", i);
> > exit(1);
> > }
>
> It's a bug on the current code. We have to init correctly iomem before
> with libxl_iomem_range_init(&b_info->iomem);
You need to do it for each element, so &b_info->iomem[i]
> > + case 3: break;
> > + case 2:
> > + /* default to 1:1 mapping */
> > + b_info->iomem[i].gfn = b_info->iomem[i].start;
> > + break;
>
> If the iomem_range is initialized with the default value. You can defer
> this initialization in libxl.
You will need to make sure that libxl__iomem_range_setdefaults will
correctly set gfn to the 1:1 iff it is set to the sentinel value which
means "default" (probably ~0?). And you will need to make sure that
setdefaults is called when appropriate.
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 2/7] arch, arm: add consistency checks to REMOVE p2m changes
2014-03-25 2:02 ` [PATCH v4 2/7] arch, arm: add consistency checks to REMOVE p2m changes Arianna Avanzini
2014-03-25 12:18 ` Stefano Stabellini
2014-03-25 12:51 ` Julien Grall
@ 2014-03-25 17:41 ` Ian Campbell
2 siblings, 0 replies; 50+ messages in thread
From: Ian Campbell @ 2014-03-25 17:41 UTC (permalink / raw)
To: Arianna Avanzini
Cc: paolo.valente, keir, stefano.stabellini, Ian.Jackson,
dario.faggioli, tim, xen-devel, julien.grall, etrudeau, JBeulich,
viktor.kleinik
On Tue, 2014-03-25 at 03:02 +0100, Arianna Avanzini wrote:
> - paddr_t addr;
> + paddr_t addr, _maddr;
I'm afraid that the _ namespace is reserved, so this is not an
acceptable name for this variable. (I thought I said this before, but my
email archive says not, sorry).
Why can the existing maddr not be used, which might involve incrementing
it in some more cases, in fact I think the increment in the INSERT case
could be pulled out to the same place as the increment of addr without
causing any harm.
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 3/7] arch, arm: let map_mmio_regions() take pfn as parameters
2014-03-25 12:54 ` Julien Grall
@ 2014-03-28 12:51 ` Arianna Avanzini
2014-03-28 13:31 ` Julien Grall
0 siblings, 1 reply; 50+ messages in thread
From: Arianna Avanzini @ 2014-03-28 12:51 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini
Cc: julien.grall, paolo.valente, keir, tim, dario.faggioli,
Ian.Jackson, xen-devel, Ian.Campbell, etrudeau, JBeulich,
viktor.kleinik
On 03/25/2014 01:54 PM, Julien Grall wrote:
> Hi Stefano and Arianna,
>
Hello,
> On 03/25/2014 12:22 PM, Stefano Stabellini wrote:
>>> - 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);
>>> }
>>
>> This doesn't make any sense. Shouldn't you modify apply_p2m_changes to
>> take pfns as parameters too?
>
> If so, I think it should be a separate patch. Her change makes sense
> with the current interface.
>
Thank you both for the feedback. Sorry if I bother you with a question, would
you like a separate patch to modify also apply_p2m_changes() to take pfns as
parameters, or would you prefer to keep the current interface by now?
Thank you again.
> Regards,
>
--
/*
* Arianna Avanzini
* avanzini.arianna@gmail.com
* 73628@studenti.unimore.it
*/
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-03-25 9:33 ` Jan Beulich
@ 2014-03-28 13:24 ` Arianna Avanzini
2014-03-28 13:30 ` Jan Beulich
0 siblings, 1 reply; 50+ messages in thread
From: Arianna Avanzini @ 2014-03-28 13:24 UTC (permalink / raw)
To: Jan Beulich
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, julien.grall, etrudeau,
viktor.kleinik
On 03/25/2014 10:33 AM, Jan Beulich wrote:
>>>> On 25.03.14 at 03:02, <avanzini.arianna@gmail.com> wrote:
>> +int unmap_mmio_regions(struct domain *d,
>> + unsigned long start_gfn,
>> + unsigned long end_gfn,
>> + unsigned long mfn)
>> +{
>> + int ret = 0;
>> + unsigned long nr_mfns = end_gfn - start_gfn + 1;
>> + 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);
>> +
>> + return ret;
>> +}
>
> Either you keep the function returning a boolean value (but then
> please the more conventional 1=success 0=failure), in which case
> the function return type should be bool_t, or you make the
> function return a proper error code (and propagate that as
> necessary in the caller).
>
> Furthermore the mfn parameter is unused here (and effectively
> unused also in the ARM variant - it's meaningful only when passing
> INSERT to apply_p2m_changes()), so please drop it.
>
Thank you for the detailed feedback, I will certainly try to address the issues
you pointed out. Sorry if I bother you with a question on this hunk.
Patch 0002 added to the REMOVE case of apply_p2m_changes() a consistency check
which makes use of the mfn parameter. More in detail, the function should check
whether the mappings to be removed actually involve the gfn and mfn ranges
passed as parameters.
Do you think it is acceptable to keep the mfn parameter to this purpose? In case
you think it is useful/necessary, would you prefer a similar check to be
performed also someplace in the x86-related code?
>> + 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;
>> + unsigned long gfn_end = gfn + nr_mfns;
>
> Afaics all uses of [gm]fn_end involve subtracting 1 - please do this
> right here if you already introduce these helper variables.
>
>> --- /dev/null
>> +++ b/xen/include/xen/p2m.h
>> @@ -0,0 +1,16 @@
>> +#ifndef _XEN_P2M_COMMON_H
>> +#define _XEN_P2M_COMMON_H
>
> You're on the right track with the guard - please name the header
> p2m-common.h, so that it won't violate the common inclusion
> hierarchy of the common header including the corresponding arch
> one. The alternative would be to keep the name, rename the guard,
> and replace all relevant (not necessarily all) #include <asm/p2m.h>
> with #include <xen/p2m.h>.
>
> Jan
>
--
/*
* Arianna Avanzini
* avanzini.arianna@gmail.com
* 73628@studenti.unimore.it
*/
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-03-28 13:24 ` Arianna Avanzini
@ 2014-03-28 13:30 ` Jan Beulich
0 siblings, 0 replies; 50+ messages in thread
From: Jan Beulich @ 2014-03-28 13:30 UTC (permalink / raw)
To: Arianna Avanzini
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, julien.grall, etrudeau,
viktor.kleinik
>>> On 28.03.14 at 14:24, <avanzini.arianna@gmail.com> wrote:
> On 03/25/2014 10:33 AM, Jan Beulich wrote:
>>>>> On 25.03.14 at 03:02, <avanzini.arianna@gmail.com> wrote:
>>> +int unmap_mmio_regions(struct domain *d,
>>> + unsigned long start_gfn,
>>> + unsigned long end_gfn,
>>> + unsigned long mfn)
>>> +{
>>> + int ret = 0;
>>> + unsigned long nr_mfns = end_gfn - start_gfn + 1;
>>> + 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);
>>> +
>>> + return ret;
>>> +}
>>
>> Either you keep the function returning a boolean value (but then
>> please the more conventional 1=success 0=failure), in which case
>> the function return type should be bool_t, or you make the
>> function return a proper error code (and propagate that as
>> necessary in the caller).
>>
>> Furthermore the mfn parameter is unused here (and effectively
>> unused also in the ARM variant - it's meaningful only when passing
>> INSERT to apply_p2m_changes()), so please drop it.
>>
>
> Thank you for the detailed feedback, I will certainly try to address the
> issues
> you pointed out. Sorry if I bother you with a question on this hunk.
> Patch 0002 added to the REMOVE case of apply_p2m_changes() a consistency
> check
> which makes use of the mfn parameter.
That must have been an ARM-only patch then, which I likely only
briefly looked at.
> More in detail, the function should check
> whether the mappings to be removed actually involve the gfn and mfn ranges
> passed as parameters.
> Do you think it is acceptable to keep the mfn parameter to this purpose? In
> case
> you think it is useful/necessary, would you prefer a similar check to be
> performed also someplace in the x86-related code?
Yes - if the check is deemed useful (which I'm not that certain about)
then it should be done in both x86 and ARM (if not doable in common
code in the first place).
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 3/7] arch, arm: let map_mmio_regions() take pfn as parameters
2014-03-28 12:51 ` Arianna Avanzini
@ 2014-03-28 13:31 ` Julien Grall
0 siblings, 0 replies; 50+ messages in thread
From: Julien Grall @ 2014-03-28 13:31 UTC (permalink / raw)
To: Arianna Avanzini
Cc: julien.grall, paolo.valente, keir, Stefano Stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, Ian.Campbell, etrudeau,
JBeulich, viktor.kleinik
On 03/28/2014 12:51 PM, Arianna Avanzini wrote:
> On 03/25/2014 01:54 PM, Julien Grall wrote:
> Hello,
Hi Arianna,
>> On 03/25/2014 12:22 PM, Stefano Stabellini wrote:
>>>> - 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);
>>>> }
>>>
>>> This doesn't make any sense. Shouldn't you modify apply_p2m_changes to
>>> take pfns as parameters too?
>>
>> If so, I think it should be a separate patch. Her change makes sense
>> with the current interface.
>>
>
> Thank you both for the feedback. Sorry if I bother you with a question, would
> you like a separate patch to modify also apply_p2m_changes() to take pfns as
> parameters, or would you prefer to keep the current interface by now?
As you want, I don't think this change is critical for now.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-03-25 13:17 ` Julien Grall
@ 2014-04-01 14:52 ` Ian Campbell
2014-04-01 15:16 ` Julien Grall
0 siblings, 1 reply; 50+ messages in thread
From: Ian Campbell @ 2014-04-01 14:52 UTC (permalink / raw)
To: Julien Grall
Cc: paolo.valente, keir, stefano.stabellini, tim, dario.faggioli,
Ian.Jackson, xen-devel, julien.grall, etrudeau, JBeulich,
Arianna Avanzini, viktor.kleinik
On Tue, 2014-03-25 at 13:17 +0000, Julien Grall wrote:
> > diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> > index 7cf610a..ae120d9 100644
> > --- a/xen/common/domctl.c
> > +++ b/xen/common/domctl.c
> > @@ -818,6 +818,71 @@ 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;
> > + unsigned long gfn_end = gfn + nr_mfns;
> > + int add = op->u.memory_mapping.add_mapping;
> > +
> > + ret = -EINVAL;
> > + if ( (mfn_end - 1) < mfn || /* wrap? */
> > + ((mfn | (mfn_end - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> > + (gfn_end - 1) < gfn ) /* wrap? */
> > + return ret;
>
>
> Would not it be better to only rely on the GFN when the toolstack is
> removing the mapping?
I'm not sure I get what you mean, the gfn is an input to add as well.
> I know this is not the previous behavior, but most of the hypercall
> which deal with removing mapping only take GFN.
Regardless of my confusion above any semantic change should be done
separately from the move of the code from x86 to generic and whatever
minor updates that entails for the ARM case.
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-03-25 15:42 ` Stefano Stabellini
@ 2014-04-01 15:01 ` Ian Campbell
2014-04-01 15:18 ` Jan Beulich
0 siblings, 1 reply; 50+ messages in thread
From: Ian Campbell @ 2014-04-01 15:01 UTC (permalink / raw)
To: Stefano Stabellini
Cc: paolo.valente, keir, tim, dario.faggioli, Ian.Jackson, xen-devel,
julien.grall, etrudeau, Jan Beulich, Arianna Avanzini,
viktor.kleinik
On Tue, 2014-03-25 at 15:42 +0000, Stefano Stabellini wrote:
> On Tue, 25 Mar 2014, Jan Beulich wrote:
> > >>> On 25.03.14 at 16:10, <stefano.stabellini@eu.citrix.com> wrote:
> > > On Tue, 25 Mar 2014, Jan Beulich wrote:
> > >> >>> On 25.03.14 at 13:35, <stefano.stabellini@eu.citrix.com> wrote:
> > >> > On Tue, 25 Mar 2014, Arianna Avanzini wrote:
> > >> >> + add = unmap_mmio_regions(d, gfn, gfn_end - 1, mfn);
> > >> >> + ret = iomem_deny_access(d, mfn, mfn_end - 1);
> > >> >> + if ( ret && add )
> > >> >> + ret = -EIO;
> > >> >
> > >> > This is a functional change. The removed code did:
> > >> >
> > >> > - ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
> > >> > - if ( !ret && add )
> > >> > - ret = -EIO;
> > >> >
> > >> > The change looks correct to me, but I would appreciate if you could
> > >> > mention it explicitly in the commit message.
> > >>
> > >> I overlooked this, and no, it is not correct (and I don't follow why
> > >> you think it is): We only want to force ret to -EIO if
> > >> iomem_deny_access() didn't already return another error.
> > >
> > > I thought that the intention was to return -EIO if unmap_mmio_regions
> > > failed, even if iomem_deny_access is successful.
> > > iomem_deny_access errors should be taken care by the
> > > iomem_access_permitted check above, returning -EPERM.
> >
> > It certainly doesn't matter _that_ much which error to return here,
> > but code movement shouldn't be done with silent functional changes,
>
> I agree
>
>
> > the more that the change is buggy beyond altering original behavior:
> > Consider the case of ret = 0 (iomem_deny_access() succeeded) and
> > add = 1 (unmap_mmio_regions() failed) - the failure would be lost
> > with the changed code.
>
> Yes, you are right, the original code is the correct one.
Although the reuse of the add variable in the else clause if an if (add)
is pretty damned confusing...
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 7/7] tools, libxl: handle the iomem parameter with the memory_mapping hcall
2014-03-25 2:02 ` [PATCH v4 7/7] tools, libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
@ 2014-04-01 15:13 ` Ian Campbell
2014-04-01 15:26 ` Julien Grall
0 siblings, 1 reply; 50+ messages in thread
From: Ian Campbell @ 2014-04-01 15:13 UTC (permalink / raw)
To: Arianna Avanzini
Cc: paolo.valente, keir, stefano.stabellini, Ian.Jackson,
dario.faggioli, tim, xen-devel, julien.grall, etrudeau, JBeulich,
Daniel De Graaf, viktor.kleinik
On Tue, 2014-03-25 at 03:02 +0100, 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.
Sorry for not noticing this sooner but I've just been looking at this
again and it seems that XEN_DOMCTL_memory_mapping is a superset of
XEN_DOMCTL_iomem_permission.
AFAICT XEN_DOMCTL_memory_mapping does exactly the same
iomem_{permit,deny}_access as XEN_DOMCTL_iomem_permission and then iff
the guest is paging_mode_translate sets up a p2m mapping for it.
(There's also some extra debug logging, lets ignore it).
IOW could the toolstack's existing call to XEN_DOMCTL_iomem_permission
not be completely replaced with a call to XEN_DOMCTL_memory_mapping and
have exactly the same affect as this patch, without the need for the
toolstack to infer the paging mode of the guest?
I think the answer is yes, can someone confirm?
One subtle distinction is that it appears that XEN_DOMCTL_memory_mapping
cannot grant access to mfns for which it does not it self have access.
That seems reasonable though.
In fact the fact that XEN_DOMCTL_iomem_permission does not make this
check could be a security issue -- a domain with permission to build
domains could construct a sock puppet domain which it could give access
to ports which it cannot see. Or maybe this is deliberate and isolates
the builder domain from needing h/w permissions, in which case is
XEN_DOMTL_memory_mapping wrong? Daniel?
Ian.
[0] which I am mentioning openly since it is listed in
docs/misc/xsm-flask.txt as being an interface where we will handle issue
publicly.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-04-01 14:52 ` Ian Campbell
@ 2014-04-01 15:16 ` Julien Grall
2014-04-01 15:39 ` Ian Campbell
0 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2014-04-01 15:16 UTC (permalink / raw)
To: Ian Campbell
Cc: paolo.valente, Keir Fraser, Stefano Stabellini, Tim Deegan,
Dario Faggioli, xen.org, xen-devel, Julien Grall, Eric Trudeau,
Jan Beulich, Arianna Avanzini, viktor.kleinik
On 04/01/2014 03:52 PM, Ian Campbell wrote:
> On Tue, 2014-03-25 at 13:17 +0000, Julien Grall wrote:
>>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>>> index 7cf610a..ae120d9 100644
>>> --- a/xen/common/domctl.c
>>> +++ b/xen/common/domctl.c
>>> @@ -818,6 +818,71 @@ 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;
>>> + unsigned long gfn_end = gfn + nr_mfns;
>>> + int add = op->u.memory_mapping.add_mapping;
>>> +
>>> + ret = -EINVAL;
>>> + if ( (mfn_end - 1) < mfn || /* wrap? */
>>> + ((mfn | (mfn_end - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
>>> + (gfn_end - 1) < gfn ) /* wrap? */
>>> + return ret;
>>
>>
>> Would not it be better to only rely on the GFN when the toolstack is
>> removing the mapping?
>
> I'm not sure I get what you mean, the gfn is an input to add as well.
>
>> I know this is not the previous behavior, but most of the hypercall
>> which deal with removing mapping only take GFN.
>
> Regardless of my confusion above any semantic change should be done
> separately from the move of the code from x86 to generic and whatever
> minor updates that entails for the ARM case.
I didn't intend to ask "remove the field mfn". When a mapping is
removed, the MFN is useless because we can retrieve it from the GFN.
It won't impact x86, because removing a GFN that doesn't have mapping
should also fail.
When I was reading the code, x86 seems to lack of some check during
the removing part: a privileged guest (e.g a domain that have permission
to remove a MMIO range) can remove any MMIO range as long as the
MFN is in the permitted range. So the MFN may not be mapped to the GFN.
This will result to a mismatch between the actually mapped MFN
and the permitted range.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-04-01 15:01 ` Ian Campbell
@ 2014-04-01 15:18 ` Jan Beulich
2014-04-01 15:37 ` Ian Campbell
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-04-01 15:18 UTC (permalink / raw)
To: Ian Campbell, Stefano Stabellini
Cc: paolo.valente, keir, tim, dario.faggioli, Ian.Jackson, xen-devel,
julien.grall, etrudeau, AriannaAvanzini, viktor.kleinik
>>> On 01.04.14 at 17:01, <Ian.Campbell@eu.citrix.com> wrote:
> Although the reuse of the add variable in the else clause if an if (add)
> is pretty damned confusing...
I was really surprised that no-one said this earlier on this thread (and
yes, I'm guilty in having introduced this in the x86 code, wanting to
avoid adding another variable).
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 7/7] tools, libxl: handle the iomem parameter with the memory_mapping hcall
2014-04-01 15:13 ` Ian Campbell
@ 2014-04-01 15:26 ` Julien Grall
2014-04-01 15:34 ` Ian Campbell
2014-04-01 20:52 ` Daniel De Graaf
0 siblings, 2 replies; 50+ messages in thread
From: Julien Grall @ 2014-04-01 15:26 UTC (permalink / raw)
To: Ian Campbell
Cc: paolo.valente, Keir Fraser, Stefano Stabellini, Tim Deegan,
Dario Faggioli, xen.org, xen-devel, Julien Grall, Eric Trudeau,
Jan Beulich, Arianna Avanzini, Daniel De Graaf, viktor.kleinik
On 1 April 2014 16:13, Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:
> On Tue, 2014-03-25 at 03:02 +0100, 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.
>
> Sorry for not noticing this sooner but I've just been looking at this
> again and it seems that XEN_DOMCTL_memory_mapping is a superset of
> XEN_DOMCTL_iomem_permission.
>
> AFAICT XEN_DOMCTL_memory_mapping does exactly the same
> iomem_{permit,deny}_access as XEN_DOMCTL_iomem_permission and then iff
> the guest is paging_mode_translate sets up a p2m mapping for it.
> (There's also some extra debug logging, lets ignore it).
>
> IOW could the toolstack's existing call to XEN_DOMCTL_iomem_permission
> not be completely replaced with a call to XEN_DOMCTL_memory_mapping and
> have exactly the same affect as this patch, without the need for the
> toolstack to infer the paging mode of the guest?
>
> I think the answer is yes, can someone confirm?
For x86 HVM, AFAIU only QEMU knows the memory layout of the guest.
So we can't call XEN_DOMCTL_memory_mapping here (at least map
the range in the p2m).
> One subtle distinction is that it appears that XEN_DOMCTL_memory_mapping
> cannot grant access to mfns for which it does not it self have access.
> That seems reasonable though.
>
> In fact the fact that XEN_DOMCTL_iomem_permission does not make this
> check could be a security issue -- a domain with permission to build
> domains could construct a sock puppet domain which it could give access
> to ports which it cannot see. Or maybe this is deliberate and isolates
> the builder domain from needing h/w permissions, in which case is
> XEN_DOMTL_memory_mapping wrong? Daniel?
I think XEN_DOMCTL_memory_mapping is correct (and therefore
XEN_DOMCTL_iomem_permission) wrong. It make senses which the
builder domain patch series from Daniel:
see http://lists.xen.org/archives/html/xen-devel/2014-03/msg03553.html
> [0] which I am mentioning openly since it is listed in
> docs/misc/xsm-flask.txt as being an interface where we will handle issue
> publicly.
There is no reference of [0] in the mail. I guess you were talking
about the last
paragraph? :)
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 7/7] tools, libxl: handle the iomem parameter with the memory_mapping hcall
2014-04-01 15:26 ` Julien Grall
@ 2014-04-01 15:34 ` Ian Campbell
2014-04-01 20:52 ` Daniel De Graaf
1 sibling, 0 replies; 50+ messages in thread
From: Ian Campbell @ 2014-04-01 15:34 UTC (permalink / raw)
To: Julien Grall
Cc: paolo.valente, Keir Fraser, Stefano Stabellini, Tim Deegan,
Dario Faggioli, xen.org, xen-devel, Julien Grall, Eric Trudeau,
Jan Beulich, Arianna Avanzini, Daniel De Graaf, viktor.kleinik
On Tue, 2014-04-01 at 16:26 +0100, Julien Grall wrote:
> On 1 April 2014 16:13, Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:
> > On Tue, 2014-03-25 at 03:02 +0100, 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.
> >
> > Sorry for not noticing this sooner but I've just been looking at this
> > again and it seems that XEN_DOMCTL_memory_mapping is a superset of
> > XEN_DOMCTL_iomem_permission.
> >
> > AFAICT XEN_DOMCTL_memory_mapping does exactly the same
> > iomem_{permit,deny}_access as XEN_DOMCTL_iomem_permission and then iff
> > the guest is paging_mode_translate sets up a p2m mapping for it.
> > (There's also some extra debug logging, lets ignore it).
> >
> > IOW could the toolstack's existing call to XEN_DOMCTL_iomem_permission
> > not be completely replaced with a call to XEN_DOMCTL_memory_mapping and
> > have exactly the same affect as this patch, without the need for the
> > toolstack to infer the paging mode of the guest?
> >
> > I think the answer is yes, can someone confirm?
>
> For x86 HVM, AFAIU only QEMU knows the memory layout of the guest.
> So we can't call XEN_DOMCTL_memory_mapping here (at least map
> the range in the p2m).
This usecase is the iomem= in the config file, which qemu isn't involved
in. So using this option on x86 HVM guests has basically the same issues
as using it on ARM guests regarding the need to understand what the
hypervisor (including qemu) is doing with the guest address space.
QEMU only uses this hypercall for PCI passthrough, which is a different
option in the guest cfg file.
> > One subtle distinction is that it appears that XEN_DOMCTL_memory_mapping
> > cannot grant access to mfns for which it does not it self have access.
> > That seems reasonable though.
> >
> > In fact the fact that XEN_DOMCTL_iomem_permission does not make this
> > check could be a security issue -- a domain with permission to build
> > domains could construct a sock puppet domain which it could give access
> > to ports which it cannot see. Or maybe this is deliberate and isolates
> > the builder domain from needing h/w permissions, in which case is
> > XEN_DOMTL_memory_mapping wrong? Daniel?
>
> I think XEN_DOMCTL_memory_mapping is correct (and therefore
> XEN_DOMCTL_iomem_permission) wrong. It make senses which the
> builder domain patch series from Daniel:
> see http://lists.xen.org/archives/html/xen-devel/2014-03/msg03553.html
>
> > [0] which I am mentioning openly since it is listed in
> > docs/misc/xsm-flask.txt as being an interface where we will handle issue
> > publicly.
>
> There is no reference of [0] in the mail. I guess you were talking
> about the last
> paragraph? :)
oops, yes. It went after "security issue".
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-04-01 15:18 ` Jan Beulich
@ 2014-04-01 15:37 ` Ian Campbell
0 siblings, 0 replies; 50+ messages in thread
From: Ian Campbell @ 2014-04-01 15:37 UTC (permalink / raw)
To: Jan Beulich
Cc: paolo.valente, keir, Stefano Stabellini, tim, dario.faggioli,
Ian.Jackson, xen-devel, julien.grall, etrudeau, AriannaAvanzini,
viktor.kleinik
On Tue, 2014-04-01 at 16:18 +0100, Jan Beulich wrote:
> >>> On 01.04.14 at 17:01, <Ian.Campbell@eu.citrix.com> wrote:
> > Although the reuse of the add variable in the else clause if an if (add)
> > is pretty damned confusing...
>
> I was really surprised that no-one said this earlier on this thread (and
> yes, I'm guilty in having introduced this in the x86 code, wanting to
> avoid adding another variable).
;-)
I only noticed it because I was about to say "how can this code be
right, we are in the else clause".
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-04-01 15:16 ` Julien Grall
@ 2014-04-01 15:39 ` Ian Campbell
2014-04-01 16:00 ` Julien Grall
0 siblings, 1 reply; 50+ messages in thread
From: Ian Campbell @ 2014-04-01 15:39 UTC (permalink / raw)
To: Julien Grall
Cc: paolo.valente, Keir Fraser, Stefano Stabellini, Tim Deegan,
Dario Faggioli, xen.org, xen-devel, Julien Grall, Eric Trudeau,
Jan Beulich, Arianna Avanzini, viktor.kleinik
On Tue, 2014-04-01 at 16:16 +0100, Julien Grall wrote:
> On 04/01/2014 03:52 PM, Ian Campbell wrote:
> > On Tue, 2014-03-25 at 13:17 +0000, Julien Grall wrote:
> >>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> >>> index 7cf610a..ae120d9 100644
> >>> --- a/xen/common/domctl.c
> >>> +++ b/xen/common/domctl.c
> >>> @@ -818,6 +818,71 @@ 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;
> >>> + unsigned long gfn_end = gfn + nr_mfns;
> >>> + int add = op->u.memory_mapping.add_mapping;
> >>> +
> >>> + ret = -EINVAL;
> >>> + if ( (mfn_end - 1) < mfn || /* wrap? */
> >>> + ((mfn | (mfn_end - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> >>> + (gfn_end - 1) < gfn ) /* wrap? */
> >>> + return ret;
> >>
> >>
> >> Would not it be better to only rely on the GFN when the toolstack is
> >> removing the mapping?
> >
> > I'm not sure I get what you mean, the gfn is an input to add as well.
> >
> >> I know this is not the previous behavior, but most of the hypercall
> >> which deal with removing mapping only take GFN.
> >
> > Regardless of my confusion above any semantic change should be done
> > separately from the move of the code from x86 to generic and whatever
> > minor updates that entails for the ARM case.
>
>
> I didn't intend to ask "remove the field mfn". When a mapping is
> removed, the MFN is useless because we can retrieve it from the GFN.
>
> It won't impact x86, because removing a GFN that doesn't have mapping
> should also fail.
>
> When I was reading the code, x86 seems to lack of some check during
> the removing part: a privileged guest (e.g a domain that have permission
> to remove a MMIO range) can remove any MMIO range as long as the
> MFN is in the permitted range. So the MFN may not be mapped to the GFN.
>
> This will result to a mismatch between the actually mapped MFN
> and the permitted range.
Is the solution to that not to add the checks rather than remove them?
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-04-01 15:39 ` Ian Campbell
@ 2014-04-01 16:00 ` Julien Grall
2014-04-02 9:43 ` Ian Campbell
0 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2014-04-01 16:00 UTC (permalink / raw)
To: Ian Campbell
Cc: paolo.valente, Keir Fraser, Stefano Stabellini, Tim Deegan,
Dario Faggioli, xen.org, xen-devel, Julien Grall, Eric Trudeau,
Jan Beulich, Arianna Avanzini, viktor.kleinik
On 04/01/2014 04:39 PM, Ian Campbell wrote:
> On Tue, 2014-04-01 at 16:16 +0100, Julien Grall wrote:
>> On 04/01/2014 03:52 PM, Ian Campbell wrote:
>>> On Tue, 2014-03-25 at 13:17 +0000, Julien Grall wrote:
>>>>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>>>>> index 7cf610a..ae120d9 100644
>>>>> --- a/xen/common/domctl.c
>>>>> +++ b/xen/common/domctl.c
>>>>> @@ -818,6 +818,71 @@ 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;
>>>>> + unsigned long gfn_end = gfn + nr_mfns;
>>>>> + int add = op->u.memory_mapping.add_mapping;
>>>>> +
>>>>> + ret = -EINVAL;
>>>>> + if ( (mfn_end - 1) < mfn || /* wrap? */
>>>>> + ((mfn | (mfn_end - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
>>>>> + (gfn_end - 1) < gfn ) /* wrap? */
>>>>> + return ret;
>>>>
>>>>
>>>> Would not it be better to only rely on the GFN when the toolstack is
>>>> removing the mapping?
>>>
>>> I'm not sure I get what you mean, the gfn is an input to add as well.
>>>
>>>> I know this is not the previous behavior, but most of the hypercall
>>>> which deal with removing mapping only take GFN.
>>>
>>> Regardless of my confusion above any semantic change should be done
>>> separately from the move of the code from x86 to generic and whatever
>>> minor updates that entails for the ARM case.
>>
>>
>> I didn't intend to ask "remove the field mfn". When a mapping is
>> removed, the MFN is useless because we can retrieve it from the GFN.
>>
>> It won't impact x86, because removing a GFN that doesn't have mapping
>> should also fail.
>>
>> When I was reading the code, x86 seems to lack of some check during
>> the removing part: a privileged guest (e.g a domain that have permission
>> to remove a MMIO range) can remove any MMIO range as long as the
>> MFN is in the permitted range. So the MFN may not be mapped to the GFN.
>>
>> This will result to a mismatch between the actually mapped MFN
>> and the permitted range.
>
> Is the solution to that not to add the checks rather than remove them?
Which check? To give an example:
In the p2m we have this list of directly MMIO
gfn 0xab => mfn 0x2b
gfn 0xac => mfn 0x2c
gfn 0xad => mfn 0x3b
gfn 0xae => mfn 0x3c
The current domain as permission on: 0x2b-0x2c
It's valid (but wrong) to call DOMCTL_memory_mapping with:
add = 0
gfn = 0xac
mfn = 0x2b
nr_mfns = 1
As you can see mfn doesn't match the gfn, but the code will let you do that.
What I suggest is two have something similar to:
for (i = 0; i < nr_mfns; i++)
{
mfn = p2m_lookup(d, gfn);
clear p2m
remove rigth
}
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 7/7] tools, libxl: handle the iomem parameter with the memory_mapping hcall
2014-04-01 15:26 ` Julien Grall
2014-04-01 15:34 ` Ian Campbell
@ 2014-04-01 20:52 ` Daniel De Graaf
2014-04-02 9:45 ` Ian Campbell
1 sibling, 1 reply; 50+ messages in thread
From: Daniel De Graaf @ 2014-04-01 20:52 UTC (permalink / raw)
To: Julien Grall, Ian Campbell
Cc: paolo.valente, Keir Fraser, Stefano Stabellini, Tim Deegan,
Dario Faggioli, xen.org, xen-devel, Julien Grall, Eric Trudeau,
Jan Beulich, Arianna Avanzini, viktor.kleinik
On 04/01/2014 11:26 AM, Julien Grall wrote:
> On 1 April 2014 16:13, Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:
>> On Tue, 2014-03-25 at 03:02 +0100, 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.
>>
>> Sorry for not noticing this sooner but I've just been looking at this
>> again and it seems that XEN_DOMCTL_memory_mapping is a superset of
>> XEN_DOMCTL_iomem_permission.
>>
>> AFAICT XEN_DOMCTL_memory_mapping does exactly the same
>> iomem_{permit,deny}_access as XEN_DOMCTL_iomem_permission and then iff
>> the guest is paging_mode_translate sets up a p2m mapping for it.
>> (There's also some extra debug logging, lets ignore it).
>>
>> IOW could the toolstack's existing call to XEN_DOMCTL_iomem_permission
>> not be completely replaced with a call to XEN_DOMCTL_memory_mapping and
>> have exactly the same affect as this patch, without the need for the
>> toolstack to infer the paging mode of the guest?
>>
>> I think the answer is yes, can someone confirm?
>
> For x86 HVM, AFAIU only QEMU knows the memory layout of the guest.
> So we can't call XEN_DOMCTL_memory_mapping here (at least map
> the range in the p2m).
>
>> One subtle distinction is that it appears that XEN_DOMCTL_memory_mapping
>> cannot grant access to mfns for which it does not it self have access.
>> That seems reasonable though.
>>
>> In fact the fact that XEN_DOMCTL_iomem_permission does not make this
>> check could be a security issue -- a domain with permission to build
>> domains could construct a sock puppet domain which it could give access
>> to ports which it cannot see. Or maybe this is deliberate and isolates
>> the builder domain from needing h/w permissions, in which case is
>> XEN_DOMTL_memory_mapping wrong? Daniel?
>
> I think XEN_DOMCTL_memory_mapping is correct (and therefore
> XEN_DOMCTL_iomem_permission) wrong. It make senses which the
> builder domain patch series from Daniel:
> see http://lists.xen.org/archives/html/xen-devel/2014-03/msg03553.html
Currently, XEN_DOMCTL_memory_mapping is allowed to device model domains
whereas XEN_DOMCTL_iomem_permission is restricted to dom0 only. This is
probably the reason why an iomem_access_permitted check is not present
in XEN_DOMCTL_iomem_permission.
If FLASK is enabled, both domctls do the same permission checking based
on the security label of the memory range: that the current domain has
the RESOURCE__{ADD,REMOVE}_IOMEM permission, and the target domain has
the RESOURCE__USE permission. This prevents the sock-puppet method from
being used to permit arbitrary accesses to created domains, but requires
that these restrictions be done at the granularity of the security
labels, which may not be as flexible as preferred in some setups.
While the current design does allow for a domain builder to manage
resources that it cannot directly use on its own, I don't think this was
ever really a design decision. There are few (if any) security gains
from being able to block a domain builder from accessing resources if it
can create domains that access these resources, since it can just create
sock-puppet domains or corrupt the domain with access.
I think changing XEN_DOMCTL_iomem_permission to require the current
domain to pass an iomem_access_permitted check before permitting access
is reasonable. It will require some adjustments to my domain builder
series which currently relies on the old behavior, but those should be
fairly simple (cloning the rangesets instead of swapping). If this
change is made, I think similar changes to the other rangeset domctls
(irq, ioport) should be done at the same time.
--
Daniel De Graaf
National Security Agency
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-04-01 16:00 ` Julien Grall
@ 2014-04-02 9:43 ` Ian Campbell
2014-04-02 10:06 ` Jan Beulich
0 siblings, 1 reply; 50+ messages in thread
From: Ian Campbell @ 2014-04-02 9:43 UTC (permalink / raw)
To: Julien Grall
Cc: paolo.valente, Keir Fraser, Stefano Stabellini, Tim Deegan,
Dario Faggioli, xen.org, xen-devel, Julien Grall, Eric Trudeau,
Jan Beulich, Arianna Avanzini, viktor.kleinik
On Tue, 2014-04-01 at 17:00 +0100, Julien Grall wrote:
> >>>>> + ret = -EINVAL;
> >>>>> + if ( (mfn_end - 1) < mfn || /* wrap? */
> >>>>> + ((mfn | (mfn_end - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> >>>>> + (gfn_end - 1) < gfn ) /* wrap? */
> >>>>> + return ret;
> >
> > Is the solution to that not to add the checks rather than remove them?
>
> Which check?
You were, I thought, proposing to remove some of the check quoted above
in the remove case? I was asking why we wouldn't instead check that the
given mfn is mapped by the given gfn.
> To give an example:
> In the p2m we have this list of directly MMIO
> gfn 0xab => mfn 0x2b
> gfn 0xac => mfn 0x2c
> gfn 0xad => mfn 0x3b
> gfn 0xae => mfn 0x3c
> The current domain as permission on: 0x2b-0x2c
>
> It's valid (but wrong) to call DOMCTL_memory_mapping with:
> add = 0
> gfn = 0xac
> mfn = 0x2b
> nr_mfns = 1
>
> As you can see mfn doesn't match the gfn, but the code will let you do that.
>
> What I suggest is two have something similar to:
> for (i = 0; i < nr_mfns; i++)
> {
> mfn = p2m_lookup(d, gfn);
> clear p2m
> remove rigth
> }
Over the range of gfns given by the hypercall this algorithm is O(n^2)
which we don't want. And it iterated over all guest mfns which is also
something we don't want, or at least we don't want to add new instances
of.
Wouldn't it be better to do the p2m_lookup and check that the result
equates to the mfn given in the argument?
This interface is already pretty broken for the case where a toolstack
maps the same mfns into a guest twice. But that is a pretty broken thing
for a toolstack to do. We could perhaps just modify this hypercall to
fail if the guest already has permission to access the given mfn, the
assumption being that that permissions must be given by
DOMCTL_memory_mapping and not DOMCTL_iomem_perm.
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 7/7] tools, libxl: handle the iomem parameter with the memory_mapping hcall
2014-04-01 20:52 ` Daniel De Graaf
@ 2014-04-02 9:45 ` Ian Campbell
2014-04-02 14:14 ` Daniel De Graaf
0 siblings, 1 reply; 50+ messages in thread
From: Ian Campbell @ 2014-04-02 9:45 UTC (permalink / raw)
To: Daniel De Graaf
Cc: paolo.valente, Keir Fraser, Stefano Stabellini, xen.org,
Dario Faggioli, Julien Grall, Tim Deegan, xen-devel,
Julien Grall, Eric Trudeau, Jan Beulich, Arianna Avanzini,
viktor.kleinik
On Tue, 2014-04-01 at 16:52 -0400, Daniel De Graaf wrote:
> Currently, XEN_DOMCTL_memory_mapping is allowed to device model domains
> whereas XEN_DOMCTL_iomem_permission is restricted to dom0 only. This is
> probably the reason why an iomem_access_permitted check is not present
> in XEN_DOMCTL_iomem_permission.
>
> If FLASK is enabled, both domctls do the same permission checking based
> on the security label of the memory range: that the current domain has
> the RESOURCE__{ADD,REMOVE}_IOMEM permission, and the target domain has
> the RESOURCE__USE permission. This prevents the sock-puppet method from
> being used to permit arbitrary accesses to created domains, but requires
> that these restrictions be done at the granularity of the security
> labels, which may not be as flexible as preferred in some setups.
So the builder domain would have permission per iomem_access_permitted
to use the range, but would not actually be able to do so due to lack of
RESOURCE__USE?
And it can give that access to someone else by virtue of
RESOURCE__{ADD,REMOVE}_IOMEM?
> While the current design does allow for a domain builder to manage
> resources that it cannot directly use on its own, I don't think this was
> ever really a design decision. There are few (if any) security gains
> from being able to block a domain builder from accessing resources if it
> can create domains that access these resources, since it can just create
> sock-puppet domains or corrupt the domain with access.
Right.
> I think changing XEN_DOMCTL_iomem_permission to require the current
> domain to pass an iomem_access_permitted check before permitting access
> is reasonable. It will require some adjustments to my domain builder
> series which currently relies on the old behavior, but those should be
> fairly simple (cloning the rangesets instead of swapping). If this
> change is made, I think similar changes to the other rangeset domctls
> (irq, ioport) should be done at the same time.
Yes, consistency here would be good.
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-04-02 9:43 ` Ian Campbell
@ 2014-04-02 10:06 ` Jan Beulich
2014-04-02 10:19 ` Ian Campbell
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-04-02 10:06 UTC (permalink / raw)
To: Ian Campbell
Cc: paolo.valente, Keir Fraser, Stefano Stabellini, Tim Deegan,
Dario Faggioli, Julien Grall, xen.org, xen-devel, JulienGrall,
Eric Trudeau, Arianna Avanzini, viktor.kleinik
>>> On 02.04.14 at 11:43, <Ian.Campbell@eu.citrix.com> wrote:
> This interface is already pretty broken for the case where a toolstack
> maps the same mfns into a guest twice. But that is a pretty broken thing
> for a toolstack to do.
Is it? In the context of the VGA ROM issue in qemu (discussed recently)
it seemed reasonable to me to map this at both the PCI BAR specified
location and the legacy ISA C0000h one. Similar considerations might
apply to other PCI option ROMs needed for booting (PXE comes to mind).
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-04-02 10:06 ` Jan Beulich
@ 2014-04-02 10:19 ` Ian Campbell
2014-04-02 10:53 ` Jan Beulich
0 siblings, 1 reply; 50+ messages in thread
From: Ian Campbell @ 2014-04-02 10:19 UTC (permalink / raw)
To: Jan Beulich
Cc: paolo.valente, Keir Fraser, Stefano Stabellini, Tim Deegan,
Dario Faggioli, Julien Grall, xen.org, xen-devel, JulienGrall,
Eric Trudeau, Arianna Avanzini, viktor.kleinik
On Wed, 2014-04-02 at 11:06 +0100, Jan Beulich wrote:
> >>> On 02.04.14 at 11:43, <Ian.Campbell@eu.citrix.com> wrote:
> > This interface is already pretty broken for the case where a toolstack
> > maps the same mfns into a guest twice. But that is a pretty broken thing
> > for a toolstack to do.
>
> Is it? In the context of the VGA ROM issue in qemu (discussed recently)
> it seemed reasonable to me to map this at both the PCI BAR specified
> location and the legacy ISA C0000h one.
That's emulated though isn't it? This interface is about real machine
iomem pages.
Anyway, the issue is that the permission is not reference counted so you
can do:
map mfn 0x12345 at gfn 0xc0000
map mfn 0x12345 at gfn 0xf0000000
unmap mfn 0x12345 from 0xf0000000
Now the guest no longer has permission (as in iomem_access_permitted) to
access mfn 0x12345 but it still has a mapping at gfn 0xc0000 and can use
it (unless perhaps some iommu stuff goes on which I haven't grokked).
Perhaps we don't care about that and expect the toolstack/device model
to ensure that it unmaps everything or nothing. (I didn't check if a
subsequent unmap from 0xc0000 would work BTW).
If we do care about this issue then our choices are:
* Implement proper referencing counting of iomem permissions
(tricky?)
* Force the reference counting to be trivial by requiring that the
reference is at most 1
* Split the manipulation of iomem permissions out of
XEN_DOMCTL_memory_mapping and require the toolstack to issue
XEN_DOMCTL_iomem_permission first (and last after all mappings
are removed).
* Do some sort of exhaustive search on unmap like Julien suggested
* ???
Ian.
> Similar considerations might
> apply to other PCI option ROMs needed for booting (PXE comes to mind).
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-04-02 10:19 ` Ian Campbell
@ 2014-04-02 10:53 ` Jan Beulich
2014-04-05 12:08 ` Arianna Avanzini
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-04-02 10:53 UTC (permalink / raw)
To: Ian Campbell
Cc: paolo.valente, Keir Fraser, Stefano Stabellini, Tim Deegan,
Dario Faggioli, Julien Grall, xen.org, xen-devel, JulienGrall,
Eric Trudeau, Arianna Avanzini, viktor.kleinik
>>> On 02.04.14 at 12:19, <Ian.Campbell@eu.citrix.com> wrote:
> On Wed, 2014-04-02 at 11:06 +0100, Jan Beulich wrote:
>> >>> On 02.04.14 at 11:43, <Ian.Campbell@eu.citrix.com> wrote:
>> > This interface is already pretty broken for the case where a toolstack
>> > maps the same mfns into a guest twice. But that is a pretty broken thing
>> > for a toolstack to do.
>>
>> Is it? In the context of the VGA ROM issue in qemu (discussed recently)
>> it seemed reasonable to me to map this at both the PCI BAR specified
>> location and the legacy ISA C0000h one.
>
> That's emulated though isn't it? This interface is about real machine
> iomem pages.
Could be either (VGA passthrough, or PXE boot ROM on a passed
through NIC), at least in theory.
> Anyway, the issue is that the permission is not reference counted so you
> can do:
> map mfn 0x12345 at gfn 0xc0000
> map mfn 0x12345 at gfn 0xf0000000
> unmap mfn 0x12345 from 0xf0000000
>
> Now the guest no longer has permission (as in iomem_access_permitted) to
> access mfn 0x12345 but it still has a mapping at gfn 0xc0000 and can use
> it (unless perhaps some iommu stuff goes on which I haven't grokked).
That's ugly indeed.
> Perhaps we don't care about that and expect the toolstack/device model
> to ensure that it unmaps everything or nothing. (I didn't check if a
> subsequent unmap from 0xc0000 would work BTW).
In fact we wouldn't, at the example of the ROMs again: The mapping
in the legacy ISA area should always be there, whereas the one at
the PCI BAR specified location should only be there if the ROM is
enabled.
> If we do care about this issue then our choices are:
> * Implement proper referencing counting of iomem permissions
> (tricky?)
> * Force the reference counting to be trivial by requiring that the
> reference is at most 1
> * Split the manipulation of iomem permissions out of
> XEN_DOMCTL_memory_mapping and require the toolstack to issue
> XEN_DOMCTL_iomem_permission first (and last after all mappings
> are removed).
This one seems the only reasonable approach to me. Yet in order to
prevent the (disaggregated) tool stack from revoking the permission
without removing the mapping, some reference counting would still
be needed. Or maybe we don't need to care about that case, as long
as the memory range can't be assigned to another guest not
controlled by the same tool stack instance.
> * Do some sort of exhaustive search on unmap like Julien suggested
Wrt unmapping, I think the MFN mapped at the specified GFN
shouldn't really matter - if the tool stack says "unmap", so be it.
And hence there's nothing really to search for.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 7/7] tools, libxl: handle the iomem parameter with the memory_mapping hcall
2014-04-02 9:45 ` Ian Campbell
@ 2014-04-02 14:14 ` Daniel De Graaf
0 siblings, 0 replies; 50+ messages in thread
From: Daniel De Graaf @ 2014-04-02 14:14 UTC (permalink / raw)
To: Ian Campbell
Cc: paolo.valente, Keir Fraser, Stefano Stabellini, xen.org,
Dario Faggioli, Julien Grall, Tim Deegan, xen-devel,
Julien Grall, Eric Trudeau, Jan Beulich, Arianna Avanzini,
viktor.kleinik
On 04/02/2014 05:45 AM, Ian Campbell wrote:
> On Tue, 2014-04-01 at 16:52 -0400, Daniel De Graaf wrote:
>> Currently, XEN_DOMCTL_memory_mapping is allowed to device model domains
>> whereas XEN_DOMCTL_iomem_permission is restricted to dom0 only. This is
>> probably the reason why an iomem_access_permitted check is not present
>> in XEN_DOMCTL_iomem_permission.
>>
>> If FLASK is enabled, both domctls do the same permission checking based
>> on the security label of the memory range: that the current domain has
>> the RESOURCE__{ADD,REMOVE}_IOMEM permission, and the target domain has
>> the RESOURCE__USE permission. This prevents the sock-puppet method from
>> being used to permit arbitrary accesses to created domains, but requires
>> that these restrictions be done at the granularity of the security
>> labels, which may not be as flexible as preferred in some setups.
>
> So the builder domain would have permission per iomem_access_permitted
> to use the range, but would not actually be able to do so due to lack of
> RESOURCE__USE?
Without changes, the domain builder would not need to have permission per
the rangeset. Since the modification to the rangeset is what triggers the
RESOURCE__USE check, if the domain builder did not have USE then it would
not pass iomem_access_permitted. With the access check added, the USE
permission would be required to for the add/remove permissions to do
anything. The permission isn't a complete subset because any relabeling
of resources makes this more complicated.
> And it can give that access to someone else by virtue of
> RESOURCE__{ADD,REMOVE}_IOMEM?
Right.
>> While the current design does allow for a domain builder to manage
>> resources that it cannot directly use on its own, I don't think this was
>> ever really a design decision. There are few (if any) security gains
>> from being able to block a domain builder from accessing resources if it
>> can create domains that access these resources, since it can just create
>> sock-puppet domains or corrupt the domain with access.
>
> Right.
>
>> I think changing XEN_DOMCTL_iomem_permission to require the current
>> domain to pass an iomem_access_permitted check before permitting access
>> is reasonable. It will require some adjustments to my domain builder
>> series which currently relies on the old behavior, but those should be
>> fairly simple (cloning the rangesets instead of swapping). If this
>> change is made, I think similar changes to the other rangeset domctls
>> (irq, ioport) should be done at the same time.
>
> Yes, consistency here would be good.
>
> Ian.
--
Daniel De Graaf
National Security Agency
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-04-02 10:53 ` Jan Beulich
@ 2014-04-05 12:08 ` Arianna Avanzini
2014-04-06 16:23 ` Stefano Stabellini
2014-04-07 7:01 ` Jan Beulich
0 siblings, 2 replies; 50+ messages in thread
From: Arianna Avanzini @ 2014-04-05 12:08 UTC (permalink / raw)
To: Jan Beulich, Ian Campbell, JulienGrall
Cc: paolo.valente, Keir Fraser, Stefano Stabellini, Tim Deegan,
Dario Faggioli, Julien Grall, xen.org, xen-devel, Eric Trudeau,
viktor.kleinik
On 04/02/2014 12:53 PM, Jan Beulich wrote:
>>>> On 02.04.14 at 12:19, <Ian.Campbell@eu.citrix.com> wrote:
>> On Wed, 2014-04-02 at 11:06 +0100, Jan Beulich wrote:
>>>>>> On 02.04.14 at 11:43, <Ian.Campbell@eu.citrix.com> wrote:
>>>> This interface is already pretty broken for the case where a toolstack
>>>> maps the same mfns into a guest twice. But that is a pretty broken thing
>>>> for a toolstack to do.
>>>
>>> Is it? In the context of the VGA ROM issue in qemu (discussed recently)
>>> it seemed reasonable to me to map this at both the PCI BAR specified
>>> location and the legacy ISA C0000h one.
>>
>> That's emulated though isn't it? This interface is about real machine
>> iomem pages.
>
> Could be either (VGA passthrough, or PXE boot ROM on a passed
> through NIC), at least in theory.
>
>> Anyway, the issue is that the permission is not reference counted so you
>> can do:
>> map mfn 0x12345 at gfn 0xc0000
>> map mfn 0x12345 at gfn 0xf0000000
>> unmap mfn 0x12345 from 0xf0000000
>>
>> Now the guest no longer has permission (as in iomem_access_permitted) to
>> access mfn 0x12345 but it still has a mapping at gfn 0xc0000 and can use
>> it (unless perhaps some iommu stuff goes on which I haven't grokked).
>
> That's ugly indeed.
>
>> Perhaps we don't care about that and expect the toolstack/device model
>> to ensure that it unmaps everything or nothing. (I didn't check if a
>> subsequent unmap from 0xc0000 would work BTW).
>
> In fact we wouldn't, at the example of the ROMs again: The mapping
> in the legacy ISA area should always be there, whereas the one at
> the PCI BAR specified location should only be there if the ROM is
> enabled.
>
>> If we do care about this issue then our choices are:
>> * Implement proper referencing counting of iomem permissions
>> (tricky?)
>> * Force the reference counting to be trivial by requiring that the
>> reference is at most 1
>> * Split the manipulation of iomem permissions out of
>> XEN_DOMCTL_memory_mapping and require the toolstack to issue
>> XEN_DOMCTL_iomem_permission first (and last after all mappings
>> are removed).
>
> This one seems the only reasonable approach to me. Yet in order to
> prevent the (disaggregated) tool stack from revoking the permission
> without removing the mapping, some reference counting would still
> be needed. Or maybe we don't need to care about that case, as long
> as the memory range can't be assigned to another guest not
> controlled by the same tool stack instance.
>
>> * Do some sort of exhaustive search on unmap like Julien suggested
>
> Wrt unmapping, I think the MFN mapped at the specified GFN
> shouldn't really matter - if the tool stack says "unmap", so be it.
> And hence there's nothing really to search for.
>
Hello,
sorry if I intrude with a question. I have been working on a new version of the
patch series, just to try to get out of the way the most immediate issues that
everybody pointed out.
Is everybody OK if I send it or would you prefer me to wait for a decision in
this new thread of discussion and try to include in the next patchset something
related to it? I'm obviously fine either way.
Thank you for all your comments and, again, sorry for the question.
--
/*
* Arianna Avanzini
* avanzini.arianna@gmail.com
* 73628@studenti.unimore.it
*/
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-04-05 12:08 ` Arianna Avanzini
@ 2014-04-06 16:23 ` Stefano Stabellini
2014-04-07 7:01 ` Jan Beulich
1 sibling, 0 replies; 50+ messages in thread
From: Stefano Stabellini @ 2014-04-06 16:23 UTC (permalink / raw)
To: Arianna Avanzini
Cc: JulienGrall, paolo.valente, Keir Fraser, Stefano Stabellini,
Tim Deegan, Dario Faggioli, Julien Grall, xen.org, xen-devel,
Ian Campbell, Eric Trudeau, Jan Beulich, viktor.kleinik
On Sat, 5 Apr 2014, Arianna Avanzini wrote:
> On 04/02/2014 12:53 PM, Jan Beulich wrote:
> >>>> On 02.04.14 at 12:19, <Ian.Campbell@eu.citrix.com> wrote:
> >> On Wed, 2014-04-02 at 11:06 +0100, Jan Beulich wrote:
> >>>>>> On 02.04.14 at 11:43, <Ian.Campbell@eu.citrix.com> wrote:
> >>>> This interface is already pretty broken for the case where a toolstack
> >>>> maps the same mfns into a guest twice. But that is a pretty broken thing
> >>>> for a toolstack to do.
> >>>
> >>> Is it? In the context of the VGA ROM issue in qemu (discussed recently)
> >>> it seemed reasonable to me to map this at both the PCI BAR specified
> >>> location and the legacy ISA C0000h one.
> >>
> >> That's emulated though isn't it? This interface is about real machine
> >> iomem pages.
> >
> > Could be either (VGA passthrough, or PXE boot ROM on a passed
> > through NIC), at least in theory.
> >
> >> Anyway, the issue is that the permission is not reference counted so you
> >> can do:
> >> map mfn 0x12345 at gfn 0xc0000
> >> map mfn 0x12345 at gfn 0xf0000000
> >> unmap mfn 0x12345 from 0xf0000000
> >>
> >> Now the guest no longer has permission (as in iomem_access_permitted) to
> >> access mfn 0x12345 but it still has a mapping at gfn 0xc0000 and can use
> >> it (unless perhaps some iommu stuff goes on which I haven't grokked).
> >
> > That's ugly indeed.
> >
> >> Perhaps we don't care about that and expect the toolstack/device model
> >> to ensure that it unmaps everything or nothing. (I didn't check if a
> >> subsequent unmap from 0xc0000 would work BTW).
> >
> > In fact we wouldn't, at the example of the ROMs again: The mapping
> > in the legacy ISA area should always be there, whereas the one at
> > the PCI BAR specified location should only be there if the ROM is
> > enabled.
> >
> >> If we do care about this issue then our choices are:
> >> * Implement proper referencing counting of iomem permissions
> >> (tricky?)
> >> * Force the reference counting to be trivial by requiring that the
> >> reference is at most 1
> >> * Split the manipulation of iomem permissions out of
> >> XEN_DOMCTL_memory_mapping and require the toolstack to issue
> >> XEN_DOMCTL_iomem_permission first (and last after all mappings
> >> are removed).
> >
> > This one seems the only reasonable approach to me. Yet in order to
> > prevent the (disaggregated) tool stack from revoking the permission
> > without removing the mapping, some reference counting would still
> > be needed. Or maybe we don't need to care about that case, as long
> > as the memory range can't be assigned to another guest not
> > controlled by the same tool stack instance.
> >
> >> * Do some sort of exhaustive search on unmap like Julien suggested
> >
> > Wrt unmapping, I think the MFN mapped at the specified GFN
> > shouldn't really matter - if the tool stack says "unmap", so be it.
> > And hence there's nothing really to search for.
> >
>
> Hello,
>
> sorry if I intrude with a question. I have been working on a new version of the
> patch series, just to try to get out of the way the most immediate issues that
> everybody pointed out.
>
> Is everybody OK if I send it or would you prefer me to wait for a decision in
> this new thread of discussion and try to include in the next patchset something
> related to it? I'm obviously fine either way.
>
> Thank you for all your comments and, again, sorry for the question.
Feel free to send a new version on the series. It is probably going to
help the people here make a decision.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-04-05 12:08 ` Arianna Avanzini
2014-04-06 16:23 ` Stefano Stabellini
@ 2014-04-07 7:01 ` Jan Beulich
1 sibling, 0 replies; 50+ messages in thread
From: Jan Beulich @ 2014-04-07 7:01 UTC (permalink / raw)
To: Arianna Avanzini
Cc: Ian Campbell, paolo.valente, Keir Fraser, Stefano Stabellini,
Tim Deegan, Dario Faggioli, Julien Grall, xen.org, xen-devel,
JulienGrall, Eric Trudeau, viktor.kleinik
>>> On 05.04.14 at 14:08, <avanzini.arianna@gmail.com> wrote:
> On 04/02/2014 12:53 PM, Jan Beulich wrote:
>> Wrt unmapping, I think the MFN mapped at the specified GFN
>> shouldn't really matter - if the tool stack says "unmap", so be it.
>> And hence there's nothing really to search for.
>
> sorry if I intrude with a question. I have been working on a new version of
> the
> patch series, just to try to get out of the way the most immediate issues
> that
> everybody pointed out.
>
> Is everybody OK if I send it or would you prefer me to wait for a decision
> in
> this new thread of discussion and try to include in the next patchset
> something
> related to it? I'm obviously fine either way.
So am I - in the end all you risk is that you need to produce another
revision of the series.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
end of thread, other threads:[~2014-04-07 7:01 UTC | newest]
Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-25 2:02 [PATCH v4 0/7] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
2014-03-25 2:02 ` [PATCH v4 1/7] arch, arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
2014-03-25 12:37 ` Julien Grall
2014-03-25 2:02 ` [PATCH v4 2/7] arch, arm: add consistency checks to REMOVE p2m changes Arianna Avanzini
2014-03-25 12:18 ` Stefano Stabellini
2014-03-25 12:51 ` Julien Grall
2014-03-25 13:10 ` Julien Grall
2014-03-25 17:41 ` Ian Campbell
2014-03-25 2:02 ` [PATCH v4 3/7] arch, arm: let map_mmio_regions() take pfn as parameters Arianna Avanzini
2014-03-25 12:22 ` Stefano Stabellini
2014-03-25 12:54 ` Julien Grall
2014-03-28 12:51 ` Arianna Avanzini
2014-03-28 13:31 ` Julien Grall
2014-03-25 13:00 ` Julien Grall
2014-03-25 2:02 ` [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall Arianna Avanzini
2014-03-25 9:33 ` Jan Beulich
2014-03-28 13:24 ` Arianna Avanzini
2014-03-28 13:30 ` Jan Beulich
2014-03-25 12:35 ` Stefano Stabellini
2014-03-25 14:10 ` Jan Beulich
2014-03-25 15:10 ` Stefano Stabellini
2014-03-25 15:36 ` Jan Beulich
2014-03-25 15:42 ` Stefano Stabellini
2014-04-01 15:01 ` Ian Campbell
2014-04-01 15:18 ` Jan Beulich
2014-04-01 15:37 ` Ian Campbell
2014-03-25 13:17 ` Julien Grall
2014-04-01 14:52 ` Ian Campbell
2014-04-01 15:16 ` Julien Grall
2014-04-01 15:39 ` Ian Campbell
2014-04-01 16:00 ` Julien Grall
2014-04-02 9:43 ` Ian Campbell
2014-04-02 10:06 ` Jan Beulich
2014-04-02 10:19 ` Ian Campbell
2014-04-02 10:53 ` Jan Beulich
2014-04-05 12:08 ` Arianna Avanzini
2014-04-06 16:23 ` Stefano Stabellini
2014-04-07 7:01 ` Jan Beulich
2014-03-25 2:02 ` [PATCH v4 5/7] tools, libxl: parse optional start gfn from the iomem config option Arianna Avanzini
2014-03-25 15:39 ` Julien Grall
2014-03-25 15:45 ` Julien Grall
2014-03-25 16:27 ` Ian Campbell
2014-03-25 2:02 ` [PATCH v4 6/7] tools, libxl: add helpers to establish if guest is auto-translated Arianna Avanzini
2014-03-25 2:02 ` [PATCH v4 7/7] tools, libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
2014-04-01 15:13 ` Ian Campbell
2014-04-01 15:26 ` Julien Grall
2014-04-01 15:34 ` Ian Campbell
2014-04-01 20:52 ` Daniel De Graaf
2014-04-02 9:45 ` Ian Campbell
2014-04-02 14:14 ` Daniel De Graaf
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.