[PATCH v3 0/5] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM

[PATCH v3 0/5] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM

[Arianna Avanzini]

Hello,

this patchset is my third attempt at proposing an implementation of the
XEN_DOMCTL_memory_mapping hypercall for the ARM architecture. I optimistically
removed the RFC tag here, I hope this was the right thing to do at this stage.

The first attempt of implementation ([1]) originated directly from my thesis
project, which I am still developing with Paolo Valente ([2]). For my master
thesis, in fact, I am porting an automotive-grade real-time OS (Evidence's
ERIKA Enterprise, [3], [4]) on Xen on ARM; the OS, which is currently being
ported as a domU, needs to access peripherals performing memory-mapped I/O,
and therefore needs the related I/O-memory ranges to be made accessible to it.
As I was working on the first implementation of the DOMCTL after reading a
related xen-devel discussion ([5]), I did not know that Eric Trudeau had already
proposed a similar possible implementation of the hypercall ([6], [7]), and I
afterwards had the benefit of both the feedback given to him and his feedback.

The v2 patchset ([8]), in fact, tried to address some issues pointed out mainly
by Julien Grall and Eric Trudeau, and further discussed with Dario Faggioli.
Such issues concerned correct typing of variables and missing page-alignment
of addresses; v1 also was implemented under the wrong assumption that dom0
could access any range of memory addresses as I/O memory, while v2 instead
added to dom0's iomem_caps the I/O-memory ranges related to devices exposed to
it; finally, the v1 patchset added the new DOMCTL as ARM32-specific, while it
is not.

The first commit in this v3 patchset implements the correct bookkeeping
to make a domain's iomem_caps coherent with the memory-mapping operations
performed during the domain's creation. With respect to v2, it correctly
prints the domain's ID instead of assuming that the domain is privileged,
as pointed out by Julien Grall; it also features a more accurate commit
description than the one given with v2, according to the feedback given by
Ian Campbell.

The second commit adds to the apply_p2m_changes() function some consistency
checks useful for the REMOVE operation. In fact, it may happen that the new
DOMCTL to be introduced is invoked with wrong parameters. More in detail, the
checks added with this commit cover the following two cases, as suggested by
Julien Grall:
. an unmap operation which is meant to be performed for I/O memory must involve
  a gfn mapping which is typed p2m_mmio_direct;
. the gfn given as parameter must be mapped to the mfn given as parameter.

The third commit moves the x86 implementation of the XEN_DOMCTL_memory_mapping
hypercall to xen/common/domctl.c and attempts to adapt the code to be used for
both the x86 and ARM architectures.
As of this commit, with respect to the corresponding previous one, I have been
trying to address the following issues.
. The code has been made common to the ARM and x86 architecture, abstracting
  out the different operations used to map and unmap I/O memory. To this
  purpose, a new pair of functions (map_mmio_regions(), unmap_mmio_regions()
  have been added to the x86 p2m handling library. Also, a new paddr_bits
  variable for ARM has been added.
  The v2 implementation added the new code to xen/arch/arm/domctl.c, while
  that code was almost a complete copy of the already-existing x86 one, as
  pointed out by Ian Campbell and Jan Beulich.
. The unmap_mmio_regions() function now takes pfns as parameters, as requested
  by Julien Grall.
. The end mfn and gfn are now computed only once in variables local to the
  DOMCTL's memory_mapping block, as suggested by Ian Campbell.

The fourth commit attempts to change the libxl code parsing the "iomem"
configuration option in an attempt to comply with a suggestion given by Ian
Campbell, approved by Dario Faggioli and further discussed with Julien Grall
and Stefano Stabellini.
More in detail, it extends the libxl code to parse an additional, optional
parameter to the "iomem" domU configuration option. This allows the user to
specify the start guest address to map the machine I/O-memory address range to;
in the absence of an explicit specification, the code defaults to the solution
implemented with this patchset's v1, that is a 1:1 mapping.

The fifth commit adds to libxl code to handle the iomem configuration option by
invoking the memory_mapping DOMCTL. The code has been ifdef'd to be available
only in case of ARM or x86 HVM guests, as indicated by Ian Campbell, with a
whitelist approach, as suggested by Jan Beulich.

The code has been tested with Linux v3.13 as dom0 and ERIKA as domU.
Again, any feedback about this version of the patches is more than welcome.

Arianna

Arianna Avanzini (5):
  arch, arm: domain build: allow access to I/O memory of mapped devices
  arch, arm: add consistency checks to REMOVE p2m changes
  xen, common: add the XEN_DOMCTL_memory_mapping hypercall
  tools, libxl: parse optional start gfn from the iomem config option
  tools, libxl: handle the iomem parameter with the memory_mapping hcall

 docs/man/xl.cfg.pod.5           |  7 +++--
 tools/libxl/libxl_create.c      | 17 ++++++++++
 tools/libxl/libxl_types.idl     |  1 +
 tools/libxl/xl_cmdimpl.c        | 22 ++++++++-----
 xen/arch/arm/cpu.c              |  2 ++
 xen/arch/arm/domain_build.c     | 11 +++++++
 xen/arch/arm/p2m.c              | 45 ++++++++++++++++++++++++--
 xen/arch/x86/domctl.c           | 70 -----------------------------------------
 xen/arch/x86/mm/p2m.c           | 39 +++++++++++++++++++++++
 xen/common/domctl.c             | 69 ++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/p2m.h       |  4 +++
 xen/include/asm-arm/processor.h |  2 ++
 xen/include/asm-x86/p2m.h       | 10 ++++++
 13 files changed, 217 insertions(+), 82 deletions(-)

-- 
1.9.0

[PATCH v3 1/5] arch, arm: domain build: allow access to I/O memory of mapped devices

[Arianna Avanzini]

Currently, a domain is allowed access to the I/O memory ranges
used to access devices exposed to it, but doesn't have those
ranges in its iomem_caps. This commit attempts to implement the
correct bookkeeping, 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>

---

    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

[PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes

[Arianna Avanzini]

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 that the type of the entry
is correct in case of I/O memory mapping removal; also, 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 checks
to the REMOVE path of apply_p2m_changes(). This is instrumental to
the following commit 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>
---
 xen/arch/arm/p2m.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index d00c882..47bf154 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -243,7 +243,8 @@ 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;
+    p2m_type_t _t;
+    paddr_t addr, _maddr = INVALID_PADDR;
     unsigned long cur_first_page = ~0,
                   cur_first_offset = ~0,
                   cur_second_offset = ~0;
@@ -252,6 +253,20 @@ static int apply_p2m_changes(struct domain *d,
     bool_t populate = (op == INSERT || op == ALLOCATE);
     lpae_t pte;
 
+    /*
+     * As of now, the lookup is needed only in in case
+     * of REMOVE operation, as a consistency check on
+     * the existence of a mapping between the machine
+     * address and the start guest address given as
+     * parameters.
+     */
+    if (op == REMOVE)
+        /*
+         * Be sure to lookup before grabbing the p2m_lock,
+         * as the p2m_lookup() function holds it too.
+         */
+        _maddr = p2m_lookup(d, start_gpaddr, &_t);
+
     spin_lock(&p2m->lock);
 
     if ( d != current->domain )
@@ -367,9 +382,23 @@ static int apply_p2m_changes(struct domain *d,
                     maddr += PAGE_SIZE;
                 }
                 break;
-            case RELINQUISH:
             case REMOVE:
                 {
+                    /*
+                     * Ensure that, if we are trying to unmap I/O memory
+                     * ranges, the given gfn is p2m_mmio_direct.
+                     */
+                    if ( t == p2m_mmio_direct ? _t != p2m_mmio_direct : 0 ||
+                         paddr_to_pfn(_maddr) == INVALID_MFN ||
+                         maddr != _maddr )
+                    {
+                        count++;
+                        break;
+                    }
+                }
+                /* fall through */
+            case RELINQUISH:
+                {
                     if ( !pte.p2m.valid )
                     {
                         count++;
-- 
1.9.0

[PATCH v3 3/5] xen, common: add the XEN_DOMCTL_memory_mapping hypercall

[Arianna Avanzini]

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>

---

    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/cpu.c              |  2 ++
 xen/arch/arm/p2m.c              | 12 +++++++
 xen/arch/x86/domctl.c           | 70 -----------------------------------------
 xen/arch/x86/mm/p2m.c           | 39 +++++++++++++++++++++++
 xen/common/domctl.c             | 69 ++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/p2m.h       |  4 +++
 xen/include/asm-arm/processor.h |  2 ++
 xen/include/asm-x86/p2m.h       | 10 ++++++
 8 files changed, 138 insertions(+), 70 deletions(-)

diff --git a/xen/arch/arm/cpu.c b/xen/arch/arm/cpu.c
index afc564f..28f2611 100644
--- a/xen/arch/arm/cpu.c
+++ b/xen/arch/arm/cpu.c
@@ -17,6 +17,8 @@
 
 #include <asm/processor.h>
 
+unsigned int paddr_bits __read_mostly = 40;
+
 void __cpuinit identify_cpu(struct cpuinfo_arm *c)
 {
         c->midr.bits = READ_SYSREG32(MIDR_EL1);
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 47bf154..6c4c318 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -490,6 +490,18 @@ int map_mmio_regions(struct domain *d,
                              maddr, 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 c0ddef0..f4951b1 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1591,6 +1591,45 @@ 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,
+                     paddr_t start_gaddr,
+                     paddr_t end_gaddr,
+                     paddr_t maddr)
+{
+    int i, ret = 0;
+    unsigned long gfn = paddr_to_pfn(start_gaddr);
+    unsigned long nr_mfns = paddr_to_pfn(end_gaddr) - paddr_to_pfn(start_gaddr);
+    unsigned long mfn = paddr_to_pfn(maddr);
+
+    if ( !paging_mode_translate(d) )
+        return 0;
+
+    for ( i = 0; !ret && i < nr_mfns; i++ )
+        if ( !set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i)) )
+            ret = -EIO;
+    while ( i-- )
+        clear_mmio_p2m_entry(d, gfn + i);
+
+    return ret;
+}
+
+int unmap_mmio_regions(struct domain *d,
+                       unsigned long start_gfn,
+                       unsigned long end_gfn,
+                       unsigned long mfn)
+{
+    unsigned long nr_mfns = end_gfn - start_gfn;
+    int i, ret = 0;
+
+    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..b18fd46 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -818,6 +818,75 @@ 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;
+        long int ret;
+
+        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, PAGE_ALIGN(pfn_to_paddr(gfn)),
+                                       PAGE_ALIGN(
+                                           pfn_to_paddr(gfn_end)) - 1,
+                                       PAGE_ALIGN(pfn_to_paddr(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 3b39c45..00e3bba 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -88,6 +88,10 @@ int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end);
  * address maddr. */
 int map_mmio_regions(struct domain *d, paddr_t start_gaddr,
                      paddr_t end_gaddr, paddr_t maddr);
+int unmap_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-arm/processor.h b/xen/include/asm-arm/processor.h
index 06e638f..4869a75 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -119,6 +119,8 @@
 
 #include <xen/types.h>
 
+extern unsigned int paddr_bits;
+
 struct cpuinfo_arm {
     union {
         uint32_t bits;
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index f4e7253..91ef110 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -657,6 +657,16 @@ void p2m_flush(struct vcpu *v, struct p2m_domain *p2m);
 /* Flushes all nested p2m tables */
 void p2m_flush_nestedp2m(struct domain *d);
 
+/* 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);
+int unmap_mmio_regions(struct domain *d,
+                       unsigned long start_gfn,
+                       unsigned long end_gfn,
+                       unsigned long mfn);
+
 void nestedp2m_write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn,
     l1_pgentry_t *p, mfn_t table_mfn, l1_pgentry_t new, unsigned int level);
 
-- 
1.9.0

[PATCH v3 4/5] tools, libxl: parse optional start gfn from the iomem config option

[Arianna Avanzini]

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 config option 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>
---
 docs/man/xl.cfg.pod.5       |  7 ++++---
 tools/libxl/libxl_types.idl |  1 +
 tools/libxl/xl_cmdimpl.c    | 22 +++++++++++++++-------
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index a6663b9..d9684f2 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -602,12 +602,13 @@ 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.
+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.
 
 It is recommended to use this option only for trusted VMs under
 administrator control.
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 612645c..f0bdb09 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -172,6 +172,7 @@ libxl_ioport_range = Struct("ioport_range", [
 libxl_iomem_range = Struct("iomem_range", [
     ("start", uint64),
     ("number", uint64),
+    ("gfn", uint64),
     ])
 
 libxl_vga_interface_info = Struct("vga_interface_info", [
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 6b1ebfa..cf8d71f 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1220,13 +1220,21 @@ 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);
+            if(sscanf(buf, "%" SCNx64",%" SCNx64"@%" SCNx64,
+                      &b_info->iomem[i].start,
+                      &b_info->iomem[i].number,
+                      &b_info->iomem[i].gfn)
+                  != 3) {
+                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);
+                } else
+                    /* default to 1:1 mapping */
+                    b_info->iomem[i].gfn = b_info->iomem[i].start;
             }
         }
     }
-- 
1.9.0

[PATCH v3 5/5] tools, libxl: handle the iomem parameter with the memory_mapping hcall

[Arianna Avanzini]

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 ARM or x86 HVM guests.

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>

---

    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 | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index d015cf4..dda4df3 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1107,7 +1107,24 @@ 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 defined(__x86_32__) || defined(__x86_64__)
+        if (b_info->type != LIBXL_DOMAIN_TYPE_HVM)
+            continue;
+#endif /* __x86_32__ || __x86_64__ */
+#if defined(__arm__) || defined(__x86_32__) || defined(__x86_64__)
+        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;
+        }
+#endif /* __arm__ || __x86_32__ || __x86_64__ */
     }
 
     switch (d_config->c_info.type) {
-- 
1.9.0

Re: [PATCH v3 1/5] arch, arm: domain build: allow access to I/O memory of mapped devices

[Julien Grall]

Hello Arianna,

Thanks for the patch.

On 15/03/14 20:11, Arianna Avanzini wrote:
> Currently, a domain is allowed access to the I/O memory ranges
> used to access devices exposed to it, but doesn't have those
> ranges in its iomem_caps. This commit attempts to implement the
> correct bookkeeping, adding the ranges to the domain's iomem_caps.

Sorry to ask you another change in the commit message. You remove every 
reference to dom0. Is it normal? IHMO, even if the function is generic, 
for now this patch is dom0 related for now.

Regards,

-- 
Julien Grall

Re: [PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes

[Julien Grall]

Hello Arianna,

Thanks for the patch.

On 15/03/14 20:11, Arianna Avanzini wrote:
> ---
>   xen/arch/arm/p2m.c | 33 +++++++++++++++++++++++++++++++--
>   1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index d00c882..47bf154 100644
> --- a/xen/arcah/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -243,7 +243,8 @@ 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;
> +    p2m_type_t _t;
> +    paddr_t addr, _maddr = INVALID_PADDR;
>       unsigned long cur_first_page = ~0,
>                     cur_first_offset = ~0,
>                     cur_second_offset = ~0;
> @@ -252,6 +253,20 @@ static int apply_p2m_changes(struct domain *d,
>       bool_t populate = (op == INSERT || op == ALLOCATE);
>       lpae_t pte;
>
> +    /*
> +     * As of now, the lookup is needed only in in case
> +     * of REMOVE operation, as a consistency check on
> +     * the existence of a mapping between the machine
> +     * address and the start guest address given as
> +     * parameters.
> +     */
> +    if (op == REMOVE)
> +        /*
> +         * Be sure to lookup before grabbing the p2m_lock,
> +         * as the p2m_lookup() function holds it too.
> +         */
> +        _maddr = p2m_lookup(d, start_gpaddr, &_t);
> +

Did you try remove path? apply_p2m_changes is taking p2m->lock which is 
also taken by p2m_lookup. With this solution it will end up to a deadlock.

Anyway, you don't need to use p2m_lookup because you already have all 
the data in pte (if pte.p2m.valid == 1):
    - pte.p2m.type  = p2m type
    - pte.p2m.base  = MFN

>       spin_lock(&p2m->lock);
>
>       if ( d != current->domain )
> @@ -367,9 +382,23 @@ static int apply_p2m_changes(struct domain *d,
>                       maddr += PAGE_SIZE;
>                   }
>                   break;
> -            case RELINQUISH:
>               case REMOVE:
>                   {
> +                    /*
> +                     * Ensure that, if we are trying to unmap I/O memory
> +                     * ranges, the given gfn is p2m_mmio_direct.
> +                     */

> +                    if ( t == p2m_mmio_direct ? _t != p2m_mmio_direct : 0 ||
> +                         paddr_to_pfn(_maddr) == INVALID_MFN ||

Testing pte.p2m.valid instead of paddr_to(_maddr)... is right answer.

Moreover, why do you need to check t? Every call to 
guest_physmap_remove_page is done with a valid mfn (I guess it can be 
enhanced by a BUG_ON(mfn != INVALID_MFN) in this function).


> +                         maddr != _maddr )

maddr is not incremented during where the page is removed. The next 
iteration will likely fail. You need to increment it in various place.

> +                    {
> +                        count++;
> +                        break;

IHMO, skipping the page is totally wrong. You should return an error here.

Regards,

-- 
Julien Grall

Re: [PATCH v3 3/5] xen, common: add the XEN_DOMCTL_memory_mapping hypercall

[Julien Grall]

Hello Arianna,

Thank for the patch.

On 15/03/14 20:11, Arianna Avanzini wrote:
> diff --git a/xen/arch/arm/cpu.c b/xen/arch/arm/cpu.c
> index afc564f..28f2611 100644
> --- a/xen/arch/arm/cpu.c
> +++ b/xen/arch/arm/cpu.c
> @@ -17,6 +17,8 @@
>
>   #include <asm/processor.h>
>
> +unsigned int paddr_bits __read_mostly = 40;
> +

I would prefer a define for paddr_bits on ARM rather than creating a new 
variable. Something like #define paddr_bits PADDR_BITS in asm-arm/p2m.h

Furthermore, you should not hardcode 40, you must use PADDR_BITS.

[..]

> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 7cf610a..b18fd46 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -818,6 +818,75 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)

[..]

> +                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

Hard tab here. You should use space.

> +        {
> +            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 3b39c45..00e3bba 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -88,6 +88,10 @@ int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end);
>    * address maddr. */
>   int map_mmio_regions(struct domain *d, paddr_t start_gaddr,
>                        paddr_t end_gaddr, paddr_t maddr);
> +int unmap_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-arm/processor.h b/xen/include/asm-arm/processor.h
> index 06e638f..4869a75 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -119,6 +119,8 @@
>
>   #include <xen/types.h>
>
> +extern unsigned int paddr_bits;
> +
>   struct cpuinfo_arm {
>       union {
>           uint32_t bits;
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index f4e7253..91ef110 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -657,6 +657,16 @@ void p2m_flush(struct vcpu *v, struct p2m_domain *p2m);
>   /* Flushes all nested p2m tables */
>   void p2m_flush_nestedp2m(struct domain *d);
>
> +/* 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);
> +int unmap_mmio_regions(struct domain *d,
> +                       unsigned long start_gfn,
> +                       unsigned long end_gfn,
> +                       unsigned long mfn);
> +

The both functions are used on common code. I would prefer to define the 
prototype only once in common header (e.g in include/xen).

At the same time, it's not necessary to give the full addresses on 
argument. GFN, MFN is enough.

I think we already talk about it on V1. I said it wasn't urgent, but as 
the function is common now ... :)

Regards,

-- 
Julien Grall

Re: [PATCH v3 4/5] tools, libxl: parse optional start gfn from the iomem config option

[Julien Grall]

Hello Arianna,

Thanks for the patch.

On 15/03/14 20:11, Arianna Avanzini wrote:
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index a6663b9..d9684f2 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -602,12 +602,13 @@ 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.
> +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.
>
>   It is recommended to use this option only for trusted VMs under
>   administrator control.
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 612645c..f0bdb09 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -172,6 +172,7 @@ libxl_ioport_range = Struct("ioport_range", [
>   libxl_iomem_range = Struct("iomem_range", [
>       ("start", uint64),
>       ("number", uint64),
> +    ("gfn", uint64),

I'm not a libxl expert, but you are modifying an existing structure. Ian 
& Ian, do we need to bump the interface version?

Regards,

-- 
Julien Grall

Re: [PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes

[Arianna Avanzini]

On 03/15/2014 11:19 PM, Julien Grall wrote:
> Hello Arianna,
> 
> Thanks for the patch.
> 

Thank you for the feedback.

> On 15/03/14 20:11, Arianna Avanzini wrote:
>> ---
>>   xen/arch/arm/p2m.c | 33 +++++++++++++++++++++++++++++++--
>>   1 file changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index d00c882..47bf154 100644
>> --- a/xen/arcah/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -243,7 +243,8 @@ 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;
>> +    p2m_type_t _t;
>> +    paddr_t addr, _maddr = INVALID_PADDR;
>>       unsigned long cur_first_page = ~0,
>>                     cur_first_offset = ~0,
>>                     cur_second_offset = ~0;
>> @@ -252,6 +253,20 @@ static int apply_p2m_changes(struct domain *d,
>>       bool_t populate = (op == INSERT || op == ALLOCATE);
>>       lpae_t pte;
>>
>> +    /*
>> +     * As of now, the lookup is needed only in in case
>> +     * of REMOVE operation, as a consistency check on
>> +     * the existence of a mapping between the machine
>> +     * address and the start guest address given as
>> +     * parameters.
>> +     */
>> +    if (op == REMOVE)
>> +        /*
>> +         * Be sure to lookup before grabbing the p2m_lock,
>> +         * as the p2m_lookup() function holds it too.
>> +         */
>> +        _maddr = p2m_lookup(d, start_gpaddr, &_t);
>> +
> 
> Did you try remove path? apply_p2m_changes is taking p2m->lock which is also
> taken by p2m_lookup. With this solution it will end up to a deadlock.
> 

The lookup is performed before grabbing p2m->lock, as stated in the comment.
I'll certainly remove it as it is useless, thank you for the feedback and for
the many suggestions.

> Anyway, you don't need to use p2m_lookup because you already have all the data
> in pte (if pte.p2m.valid == 1):
>    - pte.p2m.type  = p2m type
>    - pte.p2m.base  = MFN
> 
>>       spin_lock(&p2m->lock);
>>
>>       if ( d != current->domain )
>> @@ -367,9 +382,23 @@ static int apply_p2m_changes(struct domain *d,
>>                       maddr += PAGE_SIZE;
>>                   }
>>                   break;
>> -            case RELINQUISH:
>>               case REMOVE:
>>                   {
>> +                    /*
>> +                     * Ensure that, if we are trying to unmap I/O memory
>> +                     * ranges, the given gfn is p2m_mmio_direct.
>> +                     */
> 
>> +                    if ( t == p2m_mmio_direct ? _t != p2m_mmio_direct : 0 ||
>> +                         paddr_to_pfn(_maddr) == INVALID_MFN ||
> 
> Testing pte.p2m.valid instead of paddr_to(_maddr)... is right answer.
> 
> Moreover, why do you need to check t? Every call to guest_physmap_remove_page is
> done with a valid mfn (I guess it can be enhanced by a BUG_ON(mfn !=
> INVALID_MFN) in this function).
> 

I might be wrong, but it seems to me that apply_p2m_changes() is called with op
== REMOVE also from guest_physmap_remove_page(), and in that case t == p2m_invalid.

> 
>> +                         maddr != _maddr )
> 
> maddr is not incremented during where the page is removed. The next iteration
> will likely fail. You need to increment it in various place.
> 

I actually was checking at each iteration the start maddr against the result of
the lookup performed before the loop, which is a mistake. Sorry, and thank you
again for the feedback.

>> +                    {
>> +                        count++;
>> +                        break;
> 
> IHMO, skipping the page is totally wrong. You should return an error here.
> 
> Regards,
> 


-- 
/*
 * Arianna Avanzini
 * avanzini.arianna@gmail.com
 * 73628@studenti.unimore.it
 */

Re: [PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes

[Julien Grall]

On 15/03/14 22:36, Arianna Avanzini wrote:
> On 03/15/2014 11:19 PM, Julien Grall wrote:
>> Did you try remove path? apply_p2m_changes is taking p2m->lock which is also
>> taken by p2m_lookup. With this solution it will end up to a deadlock.
>>
>
> The lookup is performed before grabbing p2m->lock, as stated in the comment.
> I'll certainly remove it as it is useless, thank you for the feedback and for
> the many suggestions.

Oh right, didn't pay attention about it. In any case, you need to keep 
in my mind that p2m_lookup is "slow" (it will map and unmap several 
page). If we can avoid it, it's better.

>> Anyway, you don't need to use p2m_lookup because you already have all the data
>> in pte (if pte.p2m.valid == 1):
>>     - pte.p2m.type  = p2m type
>>     - pte.p2m.base  = MFN
>>
>>>        spin_lock(&p2m->lock);
>>>
>>>        if ( d != current->domain )
>>> @@ -367,9 +382,23 @@ static int apply_p2m_changes(struct domain *d,
>>>                        maddr += PAGE_SIZE;
>>>                    }
>>>                    break;
>>> -            case RELINQUISH:
>>>                case REMOVE:
>>>                    {
>>> +                    /*
>>> +                     * Ensure that, if we are trying to unmap I/O memory
>>> +                     * ranges, the given gfn is p2m_mmio_direct.
>>> +                     */
>>
>>> +                    if ( t == p2m_mmio_direct ? _t != p2m_mmio_direct : 0 ||
>>> +                         paddr_to_pfn(_maddr) == INVALID_MFN ||
>>
>> Testing pte.p2m.valid instead of paddr_to(_maddr)... is right answer.
>>
>> Moreover, why do you need to check t? Every call to guest_physmap_remove_page is
>> done with a valid mfn (I guess it can be enhanced by a BUG_ON(mfn !=
>> INVALID_MFN) in this function).
>>
>
> I might be wrong, but it seems to me that apply_p2m_changes() is called with op
> == REMOVE also from guest_physmap_remove_page(), and in that case t == p2m_invalid.

The important bit is the MFN. I don't think we care about "t".

-- 
Julien Grall

Re: [PATCH v3 3/5] xen, common: add the XEN_DOMCTL_memory_mapping hypercall

[Jan Beulich]

>>> On 15.03.14 at 21:11, Arianna Avanzini <avanzini.arianna@gmail.com> wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1591,6 +1591,45 @@ 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,
> +                     paddr_t start_gaddr,
> +                     paddr_t end_gaddr,
> +                     paddr_t maddr)
> +{
> +    int i, ret = 0;

"i" was unsigned long in the original code, and I don't see any proof
of either the signedness or the width change being correct.

> +    unsigned long gfn = paddr_to_pfn(start_gaddr);
> +    unsigned long nr_mfns = paddr_to_pfn(end_gaddr) - paddr_to_pfn(start_gaddr);

So this implies the range is exclusive of end_gaddr.

> +    unsigned long mfn = paddr_to_pfn(maddr);
> +
> +    if ( !paging_mode_translate(d) )
> +        return 0;
> +
> +    for ( i = 0; !ret && i < nr_mfns; i++ )
> +        if ( !set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i)) )
> +            ret = -EIO;
> +    while ( i-- )
> +        clear_mmio_p2m_entry(d, gfn + i);

So you're clearing all entries unconditionally again right away? This
surely wants to be inside an "if ( ret )".

> +int unmap_mmio_regions(struct domain *d,
> +                       unsigned long start_gfn,
> +                       unsigned long end_gfn,
> +                       unsigned long mfn)
> +{
> +    unsigned long nr_mfns = end_gfn - start_gfn;
> +    int i, ret = 0;

See above.

> +
> +    if ( !paging_mode_translate(d) )
> +        return 0;
> +
> +    for ( i = 0; i < nr_mfns; i++ )
> +        ret |= !clear_mmio_p2m_entry(d, start_gfn + i);

With this you pretty clearly want the function to have bool_t as its
return type.

> +    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;
> +        long int ret;
> +
> +        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, PAGE_ALIGN(pfn_to_paddr(gfn)),
> +                                       PAGE_ALIGN(
> +                                           pfn_to_paddr(gfn_end)) - 1,

Contrary to the above, this implies the range to inclusive of the
ending address.

Furthermore, PAGE_ALIGN(pfn_to_paddr()) is rather pointless a
construct - the outer PAGE_ALIGN() can't possibly have any effect.

> +/* 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);
> +int unmap_mmio_regions(struct domain *d,
> +                       unsigned long start_gfn,
> +                       unsigned long end_gfn,
> +                       unsigned long mfn);

Please have the two interfaces be consistent: Either both take
frame numbers only (preferred), or both take addresses only.

Jan

Re: [PATCH v3 4/5] tools, libxl: parse optional start gfn from the iomem config option

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2779 bytes --]

On sab, 2014-03-15 at 22:35 +0000, Julien Grall wrote:
> On 15/03/14 20:11, Arianna Avanzini wrote:
> > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > index a6663b9..d9684f2 100644
> > --- a/docs/man/xl.cfg.pod.5
> > +++ b/docs/man/xl.cfg.pod.5
> > @@ -602,12 +602,13 @@ 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.
> > +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.
> >
> >   It is recommended to use this option only for trusted VMs under
> >   administrator control.
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 612645c..f0bdb09 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -172,6 +172,7 @@ libxl_ioport_range = Struct("ioport_range", [
> >   libxl_iomem_range = Struct("iomem_range", [
> >       ("start", uint64),
> >       ("number", uint64),
> > +    ("gfn", uint64),
> 
> I'm not a libxl expert, but you are modifying an existing structure. Ian 
> & Ian, do we need to bump the interface version?
> 
I think what we need is the proper LIBXL_HAVE_FOO stuff.

 * In the event that a change is required which cannot be made
 * backwards compatible in this manner a #define of the form
 * LIBXL_HAVE_<interface> will always be added in order to make it
 * possible to write applciations which build against any version of
 * libxl. 

Arianna, try this:
$ grep LIBXL_HAVE_ tools/libxl/libxl.h

And look at one of the examples, e.g.:

/*
 * LIBXL_HAVE_DOMAIN_NODEAFFINITY indicates that a 'nodemap' field
 * (of libxl_bitmap type) is present in libxl_domain_build_info,
 * containing the node-affinity for the domain.
 */
#define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1

The commit this was introduced by is a5d30c23, in case that helps.

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 4/5] tools, libxl: parse optional start gfn from the iomem config option

[Julien Grall]

Hello Arianna,

Few other comments on this patch :)

On 03/15/2014 08:11 PM, Arianna Avanzini wrote:
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index a6663b9..d9684f2 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -602,12 +602,13 @@ 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.
> +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.

I'm wondering if we should add something about IOMMU. On ARM (I don't
know for x86), the IOMMU won't be updated. That would mean every device
protected by an IOMMU *should not* be passthrough via "iomem" parameter.

Regards,

-- 
Julien Grall

Re: [PATCH v3 5/5] tools, libxl: handle the iomem parameter with the memory_mapping hcall

[Julien Grall]

Hello Arianna,

(Adding Mukesh for PVH)

Thanks for the patch.

On 03/15/2014 08:11 PM, Arianna Avanzini wrote:
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index d015cf4..dda4df3 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -1107,7 +1107,24 @@ 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 defined(__x86_32__) || defined(__x86_64__)
> +        if (b_info->type != LIBXL_DOMAIN_TYPE_HVM)

Calling xc_domain_memory_memory_mapping might be also useful for PVH.
AFAIK, on x86 PVH is consider as LIBXL_DOMAIN_TYPE_PV.

The best solution is to check if the domain use auto-translated physmap
(e.g translation between PFN and MFN is done by Xen). But, after talking
with Ian C. it's not possible to know it on libxl.

You might need to introduce an architecture helper (libxl_arm.c and
libxl_x86.c are respectively only compiled on ARM and x86), which check
if you can call xc_domain_memory_mapping or not.

> +            continue;
> +#endif /* __x86_32__ || __x86_64__ */
> +#if defined(__arm__) || defined(__x86_32__) || defined(__x86_64__)

You don't need to ifdef this part. Libxl doesn't support other arch for now.

Regards,

-- 
Julien Grall

Re: [PATCH v3 5/5] tools, libxl: handle the iomem parameter with the memory_mapping hcall

[Arianna Avanzini]

On 03/17/2014 01:35 PM, Julien Grall wrote:
> Hello Arianna,
> 
> (Adding Mukesh for PVH)
> 
> Thanks for the patch.
> 

Thank you for your feedback.

> On 03/15/2014 08:11 PM, Arianna Avanzini wrote:
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index d015cf4..dda4df3 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -1107,7 +1107,24 @@ 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 defined(__x86_32__) || defined(__x86_64__)
>> +        if (b_info->type != LIBXL_DOMAIN_TYPE_HVM)
> 
> Calling xc_domain_memory_memory_mapping might be also useful for PVH.
> AFAIK, on x86 PVH is consider as LIBXL_DOMAIN_TYPE_PV.
> 
> The best solution is to check if the domain use auto-translated physmap
> (e.g translation between PFN and MFN is done by Xen). But, after talking
> with Ian C. it's not possible to know it on libxl.
> 
> You might need to introduce an architecture helper (libxl_arm.c and
> libxl_x86.c are respectively only compiled on ARM and x86), which check
> if you can call xc_domain_memory_mapping or not.
> 

Sorry if I bother you with a question to see if I understood correctly, the
helper should determine whether the guest domain is auto-translated, right?


>> +            continue;
>> +#endif /* __x86_32__ || __x86_64__ */
>> +#if defined(__arm__) || defined(__x86_32__) || defined(__x86_64__)
> 
> You don't need to ifdef this part. Libxl doesn't support other arch for now.
> 

I think it was Jan Beulich who suggested using a whitelist approach here during
the review of the v2 patchset. Is it / is he OK to drop this by now?


> Regards,
> 


-- 
/*
 * Arianna Avanzini
 * avanzini.arianna@gmail.com
 * 73628@studenti.unimore.it
 */

Re: [PATCH v3 5/5] tools, libxl: handle the iomem parameter with the memory_mapping hcall

[Julien Grall]

Hello Arianna,

On 03/18/2014 04:15 PM, Arianna Avanzini wrote:
> On 03/17/2014 01:35 PM, Julien Grall wrote:
>> On 03/15/2014 08:11 PM, Arianna Avanzini wrote:
>>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>>> index d015cf4..dda4df3 100644
>>> --- a/tools/libxl/libxl_create.c
>>> +++ b/tools/libxl/libxl_create.c
>>> @@ -1107,7 +1107,24 @@ 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 defined(__x86_32__) || defined(__x86_64__)
>>> +        if (b_info->type != LIBXL_DOMAIN_TYPE_HVM)
>>
>> Calling xc_domain_memory_memory_mapping might be also useful for PVH.
>> AFAIK, on x86 PVH is consider as LIBXL_DOMAIN_TYPE_PV.
>>
>> The best solution is to check if the domain use auto-translated physmap
>> (e.g translation between PFN and MFN is done by Xen). But, after talking
>> with Ian C. it's not possible to know it on libxl.
>>
>> You might need to introduce an architecture helper (libxl_arm.c and
>> libxl_x86.c are respectively only compiled on ARM and x86), which check
>> if you can call xc_domain_memory_mapping or not.
>>
> 
> Sorry if I bother you with a question to see if I understood correctly, the
> helper should determine whether the guest domain is auto-translated, right?

Right, and we will have to call xc_domain_memory_mapping only if the
domain is auto-translated.

> 
>>> +            continue;
>>> +#endif /* __x86_32__ || __x86_64__ */
>>> +#if defined(__arm__) || defined(__x86_32__) || defined(__x86_64__)
>>
>> You don't need to ifdef this part. Libxl doesn't support other arch for now.
>>
> 
> I think it was Jan Beulich who suggested using a whitelist approach here during
> the review of the v2 patchset. Is it / is he OK to drop this by now?

I think we can remove it as long as we have an arch-function to check
whether this is needed to be called.

If Xen will be ported to a new architecture, the developper will have to
implement the new helper for its architecture.

Regards,

-- 
Julien Grall

Re: [PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes

[Ian Campbell]

On Sat, 2014-03-15 at 21:11 +0100, 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 that the type of the entry
> is correct in case of I/O memory mapping removal; also, 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 checks
> to the REMOVE path of apply_p2m_changes(). This is instrumental to
> the following commit which implements the possibility to trigger
> the removal of p2m ranges via the memory_mapping DOMCTL for ARM.

I'm not sure I follow why this is needed, is there some reason
apply_p2m_changes(REMOVE, ...) should not just remove whatever it is
asked to? What is the downside if the memory_mapping domctl removes
something which is not a memory mapping?

If it's just "a bug" then I think the toolstack should "Not Do That
Then". If the bug might have security implications then perhaps we need
to worry about it, but do you have such a case in mind?

> +                    /*
> +                     * Ensure that, if we are trying to unmap I/O memory
> +                     * ranges, the given gfn is p2m_mmio_direct.
> +                     */
> +                    if ( t == p2m_mmio_direct ? _t != p2m_mmio_direct : 0 ||

If we really do need this sort of behaviour (see above) then I think
this check should be made more generic: 
	if ( t != p2m_invalid && pte.p2m.type != t )
		an error

p2m_invalid is a placeholder in this api for "don't care", since it
doesn't make sense to worry about removing a p2m which doesn't map
anything.

Ian.

Re: [PATCH v3 4/5] tools, libxl: parse optional start gfn from the iomem config option

[Ian Campbell]

On Mon, 2014-03-17 at 11:01 +0100, Dario Faggioli wrote:
> On sab, 2014-03-15 at 22:35 +0000, Julien Grall wrote:
> > On 15/03/14 20:11, Arianna Avanzini wrote:
> > > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > > index a6663b9..d9684f2 100644
> > > --- a/docs/man/xl.cfg.pod.5
> > > +++ b/docs/man/xl.cfg.pod.5
> > > @@ -602,12 +602,13 @@ 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.
> > > +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.
> > >
> > >   It is recommended to use this option only for trusted VMs under
> > >   administrator control.
> > > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > > index 612645c..f0bdb09 100644
> > > --- a/tools/libxl/libxl_types.idl
> > > +++ b/tools/libxl/libxl_types.idl
> > > @@ -172,6 +172,7 @@ libxl_ioport_range = Struct("ioport_range", [
> > >   libxl_iomem_range = Struct("iomem_range", [
> > >       ("start", uint64),
> > >       ("number", uint64),
> > > +    ("gfn", uint64),
> > 
> > I'm not a libxl expert, but you are modifying an existing structure. Ian 
> > & Ian, do we need to bump the interface version?
> > 
> I think what we need is the proper LIBXL_HAVE_FOO stuff.

Correct.

Re: [PATCH v3 4/5] tools, libxl: parse optional start gfn from the iomem config option

[Ian Campbell]

On Sat, 2014-03-15 at 21:11 +0100, Arianna Avanzini wrote:
> 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 config option 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>
> ---
>  docs/man/xl.cfg.pod.5       |  7 ++++---
>  tools/libxl/libxl_types.idl |  1 +
>  tools/libxl/xl_cmdimpl.c    | 22 +++++++++++++++-------
>  3 files changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index a6663b9..d9684f2 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -602,12 +602,13 @@ 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.
> +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.
>  
>  It is recommended to use this option only for trusted VMs under
>  administrator control.
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 612645c..f0bdb09 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -172,6 +172,7 @@ libxl_ioport_range = Struct("ioport_range", [
>  libxl_iomem_range = Struct("iomem_range", [
>      ("start", uint64),
>      ("number", uint64),
> +    ("gfn", uint64),

The existing name "start" is unfortunate since it doesn't really
indicate what it is the start of. Can you please add comments to "gfn"
and "start" (python syntax so # comment after each is fine) to clarify
that one is the host address and the other is the guest address.

>      ])
>  
>  libxl_vga_interface_info = Struct("vga_interface_info", [
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 6b1ebfa..cf8d71f 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1220,13 +1220,21 @@ 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);
> +            if(sscanf(buf, "%" SCNx64",%" SCNx64"@%" SCNx64,
> +                      &b_info->iomem[i].start,
> +                      &b_info->iomem[i].number,
> +                      &b_info->iomem[i].gfn)
> +                  != 3) {
> +                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);

sscanf returns the number of entries it successfully matched and
assigned, so 
        sscanf(buf, "%" SCNx64",%" SCNx64"@%" SCNx64,
will return 2 or 3 depending on what it found. So I think you can
simplify this by switching on the return value of sscanf rather than
reparsing it.

	swtich (sscanf(...)) {
		case 3: break;
		case 2:
			...[i].gfn = ...[i].start;
			break;
		default:
			fpritnf();
			exit
	}
(you get the idea)

Ian.

Re: [PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes

[Julien Grall]

Hi Ian,

On 03/21/2014 10:44 AM, Ian Campbell wrote:
> On Sat, 2014-03-15 at 21:11 +0100, 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 that the type of the entry
>> is correct in case of I/O memory mapping removal; also, 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 checks
>> to the REMOVE path of apply_p2m_changes(). This is instrumental to
>> the following commit which implements the possibility to trigger
>> the removal of p2m ranges via the memory_mapping DOMCTL for ARM.
> 
> I'm not sure I follow why this is needed, is there some reason
> apply_p2m_changes(REMOVE, ...) should not just remove whatever it is
> asked to? What is the downside if the memory_mapping domctl removes
> something which is not a memory mapping?
> 
> If it's just "a bug" then I think the toolstack should "Not Do That
> Then". If the bug might have security implications then perhaps we need
> to worry about it, but do you have such a case in mind?

We have to check somewhere that the removed gfn corresponding to the mfn.
Otherwise the toolstack may be able to remove any page as long as the
MFN is in the iomem permitted range.

I think this is the best approach to check it.

Regards,

-- 
Julien Grall

Re: [PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes

[Ian Campbell]

On Fri, 2014-03-21 at 11:51 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 03/21/2014 10:44 AM, Ian Campbell wrote:
> > On Sat, 2014-03-15 at 21:11 +0100, 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 that the type of the entry
> >> is correct in case of I/O memory mapping removal; also, 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 checks
> >> to the REMOVE path of apply_p2m_changes(). This is instrumental to
> >> the following commit which implements the possibility to trigger
> >> the removal of p2m ranges via the memory_mapping DOMCTL for ARM.
> > 
> > I'm not sure I follow why this is needed, is there some reason
> > apply_p2m_changes(REMOVE, ...) should not just remove whatever it is
> > asked to? What is the downside if the memory_mapping domctl removes
> > something which is not a memory mapping?
> > 
> > If it's just "a bug" then I think the toolstack should "Not Do That
> > Then". If the bug might have security implications then perhaps we need
> > to worry about it, but do you have such a case in mind?
> 
> We have to check somewhere that the removed gfn corresponding to the mfn.

Why? The toolstack can punch whatever holes it wants in the guest
address space, can't it?

> Otherwise the toolstack may be able to remove any page as long as the
> MFN is in the iomem permitted range.

Can't it already do this through other paths?

Maybe there is a security implication there, but I would hope that the
two permissions were pretty closely linked.

> I think this is the best approach to check it.
> 
> Regards,
> 

Re: [PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes

[Julien Grall]

On 03/21/2014 11:54 AM, Ian Campbell wrote:
> On Fri, 2014-03-21 at 11:51 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 03/21/2014 10:44 AM, Ian Campbell wrote:
>>> On Sat, 2014-03-15 at 21:11 +0100, 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 that the type of the entry
>>>> is correct in case of I/O memory mapping removal; also, 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 checks
>>>> to the REMOVE path of apply_p2m_changes(). This is instrumental to
>>>> the following commit which implements the possibility to trigger
>>>> the removal of p2m ranges via the memory_mapping DOMCTL for ARM.
>>>
>>> I'm not sure I follow why this is needed, is there some reason
>>> apply_p2m_changes(REMOVE, ...) should not just remove whatever it is
>>> asked to? What is the downside if the memory_mapping domctl removes
>>> something which is not a memory mapping?
>>>
>>> If it's just "a bug" then I think the toolstack should "Not Do That
>>> Then". If the bug might have security implications then perhaps we need
>>> to worry about it, but do you have such a case in mind?
>>
>> We have to check somewhere that the removed gfn corresponding to the mfn.
> 
> Why? The toolstack can punch whatever holes it wants in the guest
> address space, can't it?

No, every call to apply_p2m_changes is used with a valid mfn given by
Xen directly. The toolstack will only provide the gfn, except for this
function.

> 
>> Otherwise the toolstack may be able to remove any page as long as the
>> MFN is in the iomem permitted range.
> 
> Can't it already do this through other paths?
> 
> Maybe there is a security implication there, but I would hope that the
> two permissions were pretty closely linked.

One the main problem is iomem range permitted won't be anymore in sync.

x86 at least check that the gfn is an MMIO. I think we can safely extend
to check that the GFN use the corresponding MFN.

I don't agree to let the toolstack uses this DOMCTL to do remove any
page in the guess memory.

Regards,

-- 
Julien Grall

Re: [PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes

[Ian Campbell]

On Fri, 2014-03-21 at 12:08 +0000, Julien Grall wrote:
> On 03/21/2014 11:54 AM, Ian Campbell wrote:
> > On Fri, 2014-03-21 at 11:51 +0000, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 03/21/2014 10:44 AM, Ian Campbell wrote:
> >>> On Sat, 2014-03-15 at 21:11 +0100, 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 that the type of the entry
> >>>> is correct in case of I/O memory mapping removal; also, 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 checks
> >>>> to the REMOVE path of apply_p2m_changes(). This is instrumental to
> >>>> the following commit which implements the possibility to trigger
> >>>> the removal of p2m ranges via the memory_mapping DOMCTL for ARM.
> >>>
> >>> I'm not sure I follow why this is needed, is there some reason
> >>> apply_p2m_changes(REMOVE, ...) should not just remove whatever it is
> >>> asked to? What is the downside if the memory_mapping domctl removes
> >>> something which is not a memory mapping?
> >>>
> >>> If it's just "a bug" then I think the toolstack should "Not Do That
> >>> Then". If the bug might have security implications then perhaps we need
> >>> to worry about it, but do you have such a case in mind?
> >>
> >> We have to check somewhere that the removed gfn corresponding to the mfn.
> > 
> > Why? The toolstack can punch whatever holes it wants in the guest
> > address space, can't it?
> 
> No, every call to apply_p2m_changes is used with a valid mfn given by
> Xen directly.

And REMOVE doesn't check it so this isn't actually achieving anything
today.

> The toolstack will only provide the gfn, except for this
> function.

memory_unmap should also only take a gfn, which Xen should lookup to get
an mfn. Notice that on x86 the unmap case doesn't use the mfn argument
and passes only a gfn to clear_mmio_p2m_entry.

It's racy to have the toolstack provide it anyway.

> >> Otherwise the toolstack may be able to remove any page as long as the
> >> MFN is in the iomem permitted range.
> > 
> > Can't it already do this through other paths?
> > 
> > Maybe there is a security implication there, but I would hope that the
> > two permissions were pretty closely linked.
> 
> One the main problem is iomem range permitted won't be anymore in sync.

I don't think this is a big issue. Having permission to have a mapping
does not necessarily imply having the actual mapping, if the toolstack
wants to do it then let it.

> x86 at least check that the gfn is an MMIO. I think we can safely extend
> to check that the GFN use the corresponding MFN.
> 
> I don't agree to let the toolstack uses this DOMCTL to do remove any
> page in the guess memory.

Well, it already can today AFAICS, via remove_from_physmap.

Ian.

Re: [PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes

[Julien Grall]

On 03/21/2014 12:32 PM, Ian Campbell wrote:
>> The toolstack will only provide the gfn, except for this
>> function.
> 
> memory_unmap should also only take a gfn, which Xen should lookup to get
> an mfn. Notice that on x86 the unmap case doesn't use the mfn argument
> and passes only a gfn to clear_mmio_p2m_entry.

The MFN argument is used to check if the domain has access to the
MFN(see iomem_access_permitted at the beginning of the case).

I think we should implement the behavior you described in the common
implementation.

> It's racy to have the toolstack provide it anyway.

>>>> Otherwise the toolstack may be able to remove any page as long as the
>>>> MFN is in the iomem permitted range.
>>>
>>> Can't it already do this through other paths?
>>>
>>> Maybe there is a security implication there, but I would hope that the
>>> two permissions were pretty closely linked.
>>
>> One the main problem is iomem range permitted won't be anymore in sync.
> 
> I don't think this is a big issue. Having permission to have a mapping
> does not necessarily imply having the actual mapping, if the toolstack
> wants to do it then let it.
> 
>> x86 at least check that the gfn is an MMIO. I think we can safely extend
>> to check that the GFN use the corresponding MFN.
>>
>> I don't agree to let the toolstack uses this DOMCTL to do remove any
>> page in the guess memory.
> 
> Well, it already can today AFAICS, via remove_from_physmap.

remove_from_physmap can't remove MMIO region. There is a specific check
in get_page_from_gfn for this purpose.

IHMO, memory_unmap should avoid to remove other region than MMIO. Mainly
because apply_p2m_changes might not remove every reference taken on a page.

-- 
Julien Grall

Re: [PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes

[Ian Campbell]

On Fri, 2014-03-21 at 12:45 +0000, Julien Grall wrote:
> On 03/21/2014 12:32 PM, Ian Campbell wrote:
> >> The toolstack will only provide the gfn, except for this
> >> function.
> > 
> > memory_unmap should also only take a gfn, which Xen should lookup to get
> > an mfn. Notice that on x86 the unmap case doesn't use the mfn argument
> > and passes only a gfn to clear_mmio_p2m_entry.
> 
> The MFN argument is used to check if the domain has access to the
> MFN(see iomem_access_permitted at the beginning of the case).
> 
> I think we should implement the behavior you described in the common
> implementation.
> 
> > It's racy to have the toolstack provide it anyway.
> 
> >>>> Otherwise the toolstack may be able to remove any page as long as the
> >>>> MFN is in the iomem permitted range.
> >>>
> >>> Can't it already do this through other paths?
> >>>
> >>> Maybe there is a security implication there, but I would hope that the
> >>> two permissions were pretty closely linked.
> >>
> >> One the main problem is iomem range permitted won't be anymore in sync.
> > 
> > I don't think this is a big issue. Having permission to have a mapping
> > does not necessarily imply having the actual mapping, if the toolstack
> > wants to do it then let it.
> > 
> >> x86 at least check that the gfn is an MMIO. I think we can safely extend
> >> to check that the GFN use the corresponding MFN.
> >>
> >> I don't agree to let the toolstack uses this DOMCTL to do remove any
> >> page in the guess memory.
> > 
> > Well, it already can today AFAICS, via remove_from_physmap.
> 
> remove_from_physmap can't remove MMIO region. There is a specific check
> in get_page_from_gfn for this purpose.

OK.

> IHMO, memory_unmap should avoid to remove other region than MMIO. Mainly
> because apply_p2m_changes might not remove every reference taken on a page.

OK, but then the changes to apply_p2m_changes should be less specific to
this case and usable even for e.g. sanity checking the removal of ram
mappings too.

Ian.

Re: [PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes

[Julien Grall]

On 03/21/2014 02:09 PM, Ian Campbell wrote:
>> IHMO, memory_unmap should avoid to remove other region than MMIO. Mainly
>> because apply_p2m_changes might not remove every reference taken on a page.
> 
> OK, but then the changes to apply_p2m_changes should be less specific to
> this case and usable even for e.g. sanity checking the removal of ram
> mappings too.

That was my point when I answered to arianna few days ago :).

-- 
Julien Grall