[RFC PATCH 0/3] Implement the XEN_DOMCTL_memory_mapping hypecall for arm32

[RFC PATCH 0/3] Implement the XEN_DOMCTL_memory_mapping hypecall for arm32

[Arianna Avanzini]

Hello,

this patchset adds a first attempt of implementation of the
XEN_DOMCTL_memory_mapping hypercall for the arm32 architecture.

As a part of my thesis project (developed with Paolo Valente,
in Italy, [1]), I am porting an automotive-grade real-time OS
(Evidence's ERIKA Enterprise, [2], [3]) on Xen on ARM, and the
port of the OS as a domU has required the OS to be able to access
peripherals performing memory-mapped I/O; as a consequence, the
I/O-memory ranges related to such peripherals had to be made
accessible to the domU.

I have been working on this patchset for the last weeks after reading
a related xen-devel discussion ([4]), and I had no idea that Eric Trudeau
had already proposed to this mailing list an implementation of the
hypercall ([5], [6]). Any comment is obviously welcome on this point, and if
he wants so I will of course add his Signed-off-by to the patches.

Any feedback about the patches is more than welcome. The code has been
tested with Linux v3.13 as dom0 and ERIKA Enterprise as domU.

[1] http://www.unimore.it/
[2] http://www.evidence.eu.com/
[3] http://erika.tuxfamily.org/drupal/
[4] http://lists.xen.org/archives/html/xen-devel/2013-06/msg00870.html
[5] http://marc.info/?l=xen-devel&m=137338996422503
[6] http://lists.xen.org/archives/html/xen-devel/2014-02/msg02514.html

Arianna Avanzini (3):
  arch, arm32: add definition of paddr_bits
  arch, arm32: add the XEN_DOMCTL_memory_mapping hypercall
  tools, libxl: handle the iomem parameter with the memory_mapping hcall

 tools/libxl/libxl_create.c      | 10 ++++++
 xen/arch/arm/arm32/cpu.h        |  7 ++++
 xen/arch/arm/arm32/domctl.c     | 74 +++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/p2m.c              |  9 +++++
 xen/include/asm-arm/p2m.h       |  2 ++
 xen/include/asm-arm/processor.h |  2 ++
 6 files changed, 104 insertions(+)
 create mode 100644 xen/arch/arm/arm32/cpu.h

-- 
1.8.5.3

[RFC PATCH 1/3] arch, arm32: add definition of paddr_bits

[Arianna Avanzini]

Currently, the paddr_bits variable, which keeps the machine word size,
is not defined for the arm32 architecture. This commits adds its
definition for arm32 in a new header in the architecture-specific
directory.
This change is instrumental to the implementation of the
XEN_DOMCTL_memory_mapping hypercall, added in the following
commit.

Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
 xen/arch/arm/arm32/cpu.h        | 7 +++++++
 xen/include/asm-arm/processor.h | 2 ++
 2 files changed, 9 insertions(+)
 create mode 100644 xen/arch/arm/arm32/cpu.h

diff --git a/xen/arch/arm/arm32/cpu.h b/xen/arch/arm/arm32/cpu.h
new file mode 100644
index 0000000..1091051
--- /dev/null
+++ b/xen/arch/arm/arm32/cpu.h
@@ -0,0 +1,7 @@
+#ifndef __ARM_ARM32_CPU_H
+#define __ARM_ARM32_CPU_H
+
+/* ARM-v7 features LPAE (machine word size is 40 bit) */
+unsigned int paddr_bits = 40;
+
+#endif /* __ARM_ARM32_CPU_H */
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 06e638f..65ec3c7 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -226,6 +226,8 @@ struct cpuinfo_arm {
  * capabilities of CPUs
  */
 
+extern unsigned int paddr_bits;
+
 extern struct cpuinfo_arm boot_cpu_data;
 
 extern void identify_cpu(struct cpuinfo_arm *);
-- 
1.8.5.3

[RFC PATCH 2/3] arch, arm32: 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 arm32. The following
warnings must be taken into consideration:
. currently, the hypercall simply performs an 1:1 mapping of I/O
  memory ranges (mfn == gfn);
. the range of I/O memory addresses is mapped all at once with
  map_mmio_regions();
. the hypercall takes for granted that any I/O memory range can
  be mapped to a domU if the current domain is dom0.

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: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
 xen/arch/arm/arm32/domctl.c | 74 +++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/p2m.c          |  9 ++++++
 xen/include/asm-arm/p2m.h   |  2 ++
 3 files changed, 85 insertions(+)

diff --git a/xen/arch/arm/arm32/domctl.c b/xen/arch/arm/arm32/domctl.c
index c2ca4d3..67cf734 100644
--- a/xen/arch/arm/arm32/domctl.c
+++ b/xen/arch/arm/arm32/domctl.c
@@ -10,8 +10,11 @@
 #include <xen/errno.h>
 #include <xen/sched.h>
 #include <xen/hypercall.h>
+#include <xen/iocap.h>
 #include <public/domctl.h>
 
+#include "cpu.h"
+
 long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
                XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 {
@@ -19,6 +22,77 @@ long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
     {
     case XEN_DOMCTL_set_address_size:
         return domctl->u.address_size.size == 32 ? 0 : -EINVAL;
+    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;
+        long int ret;
+
+        ret = -EINVAL;
+        if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
+            ((mfn | (mfn + nr_mfns - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
+            (gfn + nr_mfns - 1) < gfn ) /* wrap? */
+            return ret;
+
+        ret = -EPERM;
+	/*
+	 * NOTE: dom0 seems to have empty iomem_caps but to be however able to
+	 *       access I/O memory ranges. The following check takes for granted
+	 *       that any iomem range can be mapped to a domU if the current
+	 *       domain is dom0.
+	 */
+        if ( current->domain->domain_id != 0 &&
+             !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
+            return ret;
+
+        ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 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 + nr_mfns - 1);
+            if ( !ret )
+            {
+                /* 1:1 iomem mapping (gfn == mfn) */
+                ret = map_mmio_regions(d, gfn, gfn + nr_mfns - 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 + 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);
+
+            add = unmap_mmio_regions(d, gfn, gfn + nr_mfns - 1, mfn);
+            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);
+	}
+        return ret;
+    }
     default:
         return -ENOSYS;
     }
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index d00c882..710f74e 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -461,6 +461,15 @@ int map_mmio_regions(struct domain *d,
                              maddr, MATTR_DEV, p2m_mmio_direct);
 }
 
+int unmap_mmio_regions(struct domain *d,
+                       paddr_t start_gaddr,
+                       paddr_t end_gaddr,
+                       paddr_t maddr)
+{
+    return apply_p2m_changes(d, REMOVE, start_gaddr, end_gaddr,
+                             maddr, MATTR_DEV, p2m_mmio_direct);
+}
+
 int guest_physmap_add_entry(struct domain *d,
                             unsigned long gpfn,
                             unsigned long mfn,
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 3b39c45..907ce4a 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -88,6 +88,8 @@ 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, paddr_t start_gaddr,
+                       paddr_t end_gaddr, paddr_t maddr);
 
 int guest_physmap_add_entry(struct domain *d,
                             unsigned long gfn,
-- 
1.8.5.3

[RFC PATCH 3/3] 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.

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: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
 tools/libxl/libxl_create.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index a604cd8..7da0778 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1099,6 +1099,16 @@ 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;
+        } else {
+            ret = xc_domain_memory_mapping(CTX->xch, domid,
+                                           io->start, io->start,
+                                           io->number, 1);
+            if (ret < 0) {
+                LOGE(ERROR,
+                     "failed to map to dom%d iomem range %"PRIx64"-%"PRIx64,
+                     domid, io->start, io->start + io->number - 1);
+                ret = ERROR_FAIL;
+            }
         }
     }
 
-- 
1.8.5.3

Re: [RFC PATCH 0/3] Implement the XEN_DOMCTL_memory_mapping hypecall for arm32

[Julien Grall]

On 02/03/14 08:49, Arianna Avanzini wrote:
> Hello,

Hello Arianna,

> this patchset adds a first attempt of implementation of the
> XEN_DOMCTL_memory_mapping hypercall for the arm32 architecture.

Thanks for this patch series. I have added Ian Campbell and Tim Deegan, 
they are the maintainers of the ARM port.

> Any feedback about the patches is more than welcome. The code has been
> tested with Linux v3.13 as dom0 and ERIKA Enterprise as domU.

See my answers on the different patch for feedbacks.

Regards,


-- 
Julien Grall

Re: [RFC PATCH 1/3] arch, arm32: add definition of paddr_bits

[Julien Grall]

Hello Arianna,

On 02/03/14 08:49, Arianna Avanzini wrote:
> Currently, the paddr_bits variable, which keeps the machine word size,
> is not defined for the arm32 architecture. This commits adds its
> definition for arm32 in a new header in the architecture-specific
> directory.
> This change is instrumental to the implementation of the
> XEN_DOMCTL_memory_mapping hypercall, added in the following
> commit.

paddr_bits doesn't keep the machine word size but the size of the 
physical address.

On ARM, this value is fixed and defined as PADDR_BITS in
xen/include/asm-arm/page.h. I don't think we need to introduce a 
variable for this purpose. x86 defines paddr_bits because the value can 
change on some processor.

> diff --git a/xen/arch/arm/arm32/cpu.h b/xen/arch/arm/arm32/cpu.h
> new file mode 100644
> index 0000000..1091051
> --- /dev/null
> +++ b/xen/arch/arm/arm32/cpu.h
> @@ -0,0 +1,7 @@
> +#ifndef __ARM_ARM32_CPU_H
> +#define __ARM_ARM32_CPU_H
> +
> +/* ARM-v7 features LPAE (machine word size is 40 bit) */
> +unsigned int paddr_bits = 40;
> +

Global variable must not be defined in headers. If someone decides to 
include this header in 2 differents C files, the variable will be 
defined twice. Therefore the linker-step will fail.

You should define it in a c-file.

Regards,

-- 
Julien Grall

Re: [RFC PATCH 2/3] arch, arm32: add the XEN_DOMCTL_memory_mapping hypercall

[Julien Grall]

Hello Arianna,

On 02/03/14 08:49, Arianna Avanzini wrote:
> This commit introduces a first attempt of implementation of the
> XEN_DOMCTL_memory_mapping hypercall for arm32. The following
> warnings must be taken into consideration:
> . currently, the hypercall simply performs an 1:1 mapping of I/O
>    memory ranges (mfn == gfn);
> . the range of I/O memory addresses is mapped all at once with
>    map_mmio_regions();
> . the hypercall takes for granted that any I/O memory range can
>    be mapped to a domU if the current domain is dom0.
>
> 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: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
> ---
>   xen/arch/arm/arm32/domctl.c | 74 +++++++++++++++++++++++++++++++++++++++++++++

The implementation of XEN_DOMCTL_memory_mapping is not ARM32 specific. 
It can be implemented in arch/arm/domctl.c.

>   xen/arch/arm/p2m.c          |  9 ++++++
>   xen/include/asm-arm/p2m.h   |  2 ++
>   3 files changed, 85 insertions(+)
>
> diff --git a/xen/arch/arm/arm32/domctl.c b/xen/arch/arm/arm32/domctl.c
> index c2ca4d3..67cf734 100644
> --- a/xen/arch/arm/arm32/domctl.c
> +++ b/xen/arch/arm/arm32/domctl.c
> @@ -10,8 +10,11 @@
>   #include <xen/errno.h>
>   #include <xen/sched.h>
>   #include <xen/hypercall.h>
> +#include <xen/iocap.h>
>   #include <public/domctl.h>
>
> +#include "cpu.h"
> +
>   long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>   {
> @@ -19,6 +22,77 @@ long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>       {
>       case XEN_DOMCTL_set_address_size:
>           return domctl->u.address_size.size == 32 ? 0 : -EINVAL;
> +    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;
> +        long int ret;
> +
> +        ret = -EINVAL;
> +        if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
> +            ((mfn | (mfn + nr_mfns - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> +            (gfn + nr_mfns - 1) < gfn ) /* wrap? */
> +            return ret;
> diff --git a/xen/arch/arm/arm32/domctl.c b/xen/arch/arm/
> +
> +        ret = -EPERM;
> +	/*
> +	 * NOTE: dom0 seems to have empty iomem_caps but to be however able to
> +	 *       access I/O memory ranges. The following check takes for granted
> +	 *       that any iomem range can be mapped to a domU if the current
> +	 *       domain is dom0.
> +	 */
> +        if ( current->domain->domain_id != 0 &&
> +             !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
> +            return ret;

This check can be removed by adding in construct_dom0 
(arch/arm/domain_build.c) something like that:
   /* DOM0 is permitted full I/O capabilities */
   rc = iomem_permit_access(d, 0UL, ~OUL);

I'm wondering if we can even restrict dom0 I/0 access by only permit 
access on devices passthrough to it. Because dom0 should not be allowed 
to map I/O ranges which belong to device used by Xen e.g : GIC, RAM,...

I think we should at least restrict dom0 to use the hypercall for 
mapping device memory. Otherwise dom0 may be allowed to map Xen address 
range, do wrong thing with foreign mapping...

> +
> +        ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 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 + nr_mfns - 1);
> +            if ( !ret )
> +            {
> +                /* 1:1 iomem mapping (gfn == mfn) */
> +                ret = map_mmio_regions(d, gfn, gfn + nr_mfns - 1, mfn);

The comment "1:1 iomem mapping (gfn == mfn)" seems wrong here. The 
implementation you wrote allow gfn != mfn.

Regards,

-- 
Julien Grall

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

[Julien Grall]

Hello Arianna,

Adding Ian Jackson, maintainer of libxl.

On 02/03/14 08:49, 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.

Do you plan to give I/O mem range which is not describe by the device tree?

IHMO, allow the user to specify which I/O mem region will be mapped to 
the guest on ARM is wrong. As the platform is described via the device 
tree, the guest configuration should have a list of node to map to domU. 
Then {lib,}xl will parse/map/update the DT node to the guest.

> 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: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
> ---
>   tools/libxl/libxl_create.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index a604cd8..7da0778 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -1099,6 +1099,16 @@ 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;
> +        } else {
> +            ret = xc_domain_memory_mapping(CTX->xch, domid,
> +                                           io->start, io->start,
> +                                           io->number, 1);
> +            if (ret < 0) {
> +                LOGE(ERROR,
> +                     "failed to map to dom%d iomem range %"PRIx64"-%"PRIx64,
> +                     domid, io->start, io->start + io->number - 1);
> +                ret = ERROR_FAIL;
> +            }

This code is common on both x86 and ARM. On x86, we don't want to map
the I/O mem region in the guest here. QEMU will do it later.

The 1:1 mapping doesn't seem correct because the range could clash with 
the guest layout (e.g: GIC base address, RAM base address) which is for 
now hardcoded (see xen/include/public/arch-arm.h:358).

I have various solution to fix this issue:
   - Relocate the GIC/RAM base address and notify Xen that the GIC base 
address has changed. So we can keep the 1:1 mapping.
   - Let libxl decide where to map the I/O mem range in the guest.

For the latter solution, we will have to find a way to tell to the guest 
: "This I/O mem range is here ...". Perhaps via the device tree.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Sun, 2014-03-02 at 18:33 +0800, Julien Grall wrote:
> Hello Arianna,
> 
> Adding Ian Jackson, maintainer of libxl.
> 
> On 02/03/14 08:49, 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.
> 
> Do you plan to give I/O mem range which is not describe by the device tree?
> 
> IHMO, allow the user to specify which I/O mem region will be mapped to 
> the guest on ARM is wrong. As the platform is described via the device 
> tree, the guest configuration should have a list of node to map to domU. 
> Then {lib,}xl will parse/map/update the DT node to the guest.

I think there is room for both models. People just doing basic device
pass-through will probably want a "user friendly" DT based option, but
others might want to use a lower level raw interface, just like on x86
where we provide both the low level iomem/ioport/irq interface and the
pci passthrough syntax.

If they want to hard code an address in both their cfg file and their
guest kernel then that is up to them, they get to keep both pieces when
we changes things of course.

> The 1:1 mapping doesn't seem correct because the range could clash with 
> the guest layout (e.g: GIC base address, RAM base address) which is for 
> now hardcoded (see xen/include/public/arch-arm.h:358).
> 
> I have various solution to fix this issue:
>    - Relocate the GIC/RAM base address and notify Xen that the GIC base 
> address has changed. So we can keep the 1:1 mapping.
>    - Let libxl decide where to map the I/O mem range in the guest.

We might need some sort of ARM equivalent to the x86 "e820_host" option
-- which makes the guest address space look like the hosts (similar to
what we do for dom0 on ARM).

> For the latter solution, we will have to find a way to tell to the guest 
> : "This I/O mem range is here ...". Perhaps via the device tree.
> 
> Regards,
> 

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

[Dario Faggioli]

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

On dom, 2014-03-02 at 11:27 +0000, Ian Campbell wrote:
> On Sun, 2014-03-02 at 18:33 +0800, Julien Grall wrote:
> > Do you plan to give I/O mem range which is not describe by the device tree?
> > 
> > IHMO, allow the user to specify which I/O mem region will be mapped to 
> > the guest on ARM is wrong. As the platform is described via the device 
> > tree, the guest configuration should have a list of node to map to domU. 
> > Then {lib,}xl will parse/map/update the DT node to the guest.
> 
> I think there is room for both models. People just doing basic device
> pass-through will probably want a "user friendly" DT based option, but
> others might want to use a lower level raw interface, just like on x86
> where we provide both the low level iomem/ioport/irq interface and the
> pci passthrough syntax.
> 
That indeed is the case. The automotive OS Arianna is porting on Xen on
ARM doesn't know anything about device trees. Actually, a DT parser
would probably be more code than the OS itself! :-P

AFAIUI, that's Eric's case too. In fact this is, I think, quite common
in small/embedded OSes, and it would be a good thing for Xen to allow
them to work as guests, without necessarily requiring them to go for DT
(of course, the cost of keeping things consistent is on them!).

> If they want to hard code an address in both their cfg file and their
> guest kernel then that is up to them, they get to keep both pieces when
> we changes things of course.
> 
Right. So, just to make sure I understand it correctly, you're saying
that it is fine to have the hypercall implemented and called sort of
like how Arianna is doing it, modulo moving the call outside of common
ARM and x86 code, as Julien pointed out, is this correct? If yes, I
(FWIW) totally concur. :-)

For the sake of this series, do you think there is something that
Arianna should do, if not to ensure 100% consistency (which is not Xen's
job, in this case) but, perhaps, just to put things on the safe side?

> We might need some sort of ARM equivalent to the x86 "e820_host" option
> -- which makes the guest address space look like the hosts (similar to
> what we do for dom0 on ARM).
> 
Right. Again, and sorry for not getting it just from the above, do you
think this is something Arianna should take care of, when reposting the
series, or can/should it be done also later?

Thanks and 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: [RFC PATCH 3/3] tools, libxl: handle the iomem parameter with the memory_mapping hcall

[Dario Faggioli]

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

On dom, 2014-03-02 at 18:33 +0800, Julien Grall wrote:
> > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > index a604cd8..7da0778 100644
> > --- a/tools/libxl/libxl_create.c
> > +++ b/tools/libxl/libxl_create.c
> > @@ -1099,6 +1099,16 @@ 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;
> > +        } else {
> > +            ret = xc_domain_memory_mapping(CTX->xch, domid,
> > +                                           io->start, io->start,
> > +                                           io->number, 1);
> > +            if (ret < 0) {
> > +                LOGE(ERROR,
> > +                     "failed to map to dom%d iomem range %"PRIx64"-%"PRIx64,
> > +                     domid, io->start, io->start + io->number - 1);
> > +                ret = ERROR_FAIL;
> > +            }
> 
> This code is common on both x86 and ARM. On x86, we don't want to map
> the I/O mem region in the guest here. QEMU will do it later.
> 
Right. I took a quick look and, as you're saying, I found the call sites
of xc_domain_memory_mapping() in QEMU's code (in _pt_iomem_helper() ).
At the same time, I see QEMU being created as a device model in this
same function, below this hunk... But that does _not_ _always_ happen, I
think, does it?

In particular, what about PV guests for which libxl__need_xenpv_qemu()
returns 0? Who calls xc_domain_memory_mapping() on the specified iomem
region(s) for them?

I'm asking because libxl__need_xenpv_qemu() does not look like it's
considering num_iomem / num_irqs at all and, although I know ARM guests
are not 'full PV', we don't have QEMU there (yet) either... so I think
it's a similar situation.

Sorry if I'm missing something obvious, I'm not super familiar with this
code, and I guess Arianna can just try investigate this a bit further,
but if someone has any ideas... :-P

Thanks and 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: [RFC PATCH 2/3] arch, arm32: add the XEN_DOMCTL_memory_mapping hypercall

[Dario Faggioli]

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

On dom, 2014-03-02 at 17:56 +0800, Julien Grall wrote:
> On 02/03/14 08:49, Arianna Avanzini wrote:

> > +
> > +        ret = -EPERM;
> > +	/*
> > +	 * NOTE: dom0 seems to have empty iomem_caps but to be however able to
> > +	 *       access I/O memory ranges. The following check takes for granted
> > +	 *       that any iomem range can be mapped to a domU if the current
> > +	 *       domain is dom0.
> > +	 */
> > +        if ( current->domain->domain_id != 0 &&
> > +             !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
> > +            return ret;
> 
> This check can be removed by adding in construct_dom0 
> (arch/arm/domain_build.c) something like that:
>    /* DOM0 is permitted full I/O capabilities */
>    rc = iomem_permit_access(d, 0UL, ~OUL);
> 
Right. FTR, xen/arch/x86/domain_build.c, has this (also in
construct_dom0):

    /* DOM0 is permitted full I/O capabilities. */
    rc |= ioports_permit_access(dom0, 0, 0xFFFF);
    rc |= iomem_permit_access(dom0, 0UL, ~0UL);
    rc |= irqs_permit_access(dom0, 1, nr_irqs_gsi - 1);

Do you want a patch to that/similar effect?

> I'm wondering if we can even restrict dom0 I/0 access by only permit 
> access on devices passthrough to it. Because dom0 should not be allowed 
> to map I/O ranges which belong to device used by Xen e.g : GIC, RAM,...
> 
So, this is probably me messing up with the terminology, but what
'devices passthrough to it [dom0]' would mean, for example in cases
where we don't have an IOMMU (like cubie* boards), and hence where
proper passtrhogh will never be, I think, supported? Or do we plan to
have it working there too?

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: [RFC PATCH 3/3] tools, libxl: handle the iomem parameter with the memory_mapping hcall

[Julien Grall]

On 03/03/14 18:32, Dario Faggioli wrote:
> Right. So, just to make sure I understand it correctly, you're saying
> that it is fine to have the hypercall implemented and called sort of
> like how Arianna is doing it, modulo moving the call outside of common
> ARM and x86 code, as Julien pointed out, is this correct? If yes, I
> (FWIW) totally concur. :-)

I think in both solution, this hypercall should be implemented :).

> For the sake of this series, do you think there is something that
> Arianna should do, if not to ensure 100% consistency (which is not Xen's
> job, in this case) but, perhaps, just to put things on the safe side?
>
> Right. Again, and sorry for not getting it just from the above, do you
> think this is something Arianna should take care of, when reposting the
> series, or can/should it be done also later?

I think Arianna solution's might not work if the 1:1 mapping clash with 
existing layout. I might help Arianna on this subject because it will be 
useful to passthrough a device to the guest.

-- 
Julien Grall

Re: [RFC PATCH 2/3] arch, arm32: add the XEN_DOMCTL_memory_mapping hypercall

[Julien Grall]

On 03/03/14 19:56, Dario Faggioli wrote:
> On dom, 2014-03-02 at 17:56 +0800, Julien Grall wrote:
>> On 02/03/14 08:49, Arianna Avanzini wrote:
>
>>> +
>>> +        ret = -EPERM;
>>> +	/*
>>> +	 * NOTE: dom0 seems to have empty iomem_caps but to be however able to
>>> +	 *       access I/O memory ranges. The following check takes for granted
>>> +	 *       that any iomem range can be mapped to a domU if the current
>>> +	 *       domain is dom0.
>>> +	 */
>>> +        if ( current->domain->domain_id != 0 &&
>>> +             !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
>>> +            return ret;
>>
>> This check can be removed by adding in construct_dom0
>> (arch/arm/domain_build.c) something like that:
>>     /* DOM0 is permitted full I/O capabilities */
>>     rc = iomem_permit_access(d, 0UL, ~OUL);
>>
> Right. FTR, xen/arch/x86/domain_build.c, has this (also in
> construct_dom0):
>
>      /* DOM0 is permitted full I/O capabilities. */
>      rc |= ioports_permit_access(dom0, 0, 0xFFFF);
>      rc |= iomem_permit_access(dom0, 0UL, ~0UL);
>      rc |= irqs_permit_access(dom0, 1, nr_irqs_gsi - 1);
>
> Do you want a patch to that/similar effect?

Yes. Maybe a bit more smarter than permitting full I/0 caps for dom0.

>> I'm wondering if we can even restrict dom0 I/0 access by only permit
>> access on devices passthrough to it. Because dom0 should not be allowed
>> to map I/O ranges which belong to device used by Xen e.g : GIC, RAM,...
>>
> So, this is probably me messing up with the terminology, but what
> 'devices passthrough to it [dom0]' would mean, for example in cases
> where we don't have an IOMMU (like cubie* boards), and hence where
> proper passtrhogh will never be, I think, supported? Or do we plan to
> have it working there too?

My term seems to be wrong for dom0 :).

On ARM, some device is used by Xen and therefore not exposed to dom0. By 
passthrough to dom0 I meant every device that are given to dom0, no 
matter if the platform has an IOMMU.

I think DOM0 should only be able to map theses devices to a guest. It 
seems stupid to allow dom0 mapping RAM or the UART used by Xen.

-- 
Julien Grall

Re: [RFC PATCH 2/3] arch, arm32: add the XEN_DOMCTL_memory_mapping hypercall

[Dario Faggioli]

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

On lun, 2014-03-03 at 23:20 +0800, Julien Grall wrote:
> On 03/03/14 19:56, Dario Faggioli wrote:

> > Right. FTR, xen/arch/x86/domain_build.c, has this (also in
> > construct_dom0):
> >
> >      /* DOM0 is permitted full I/O capabilities. */
> >      rc |= ioports_permit_access(dom0, 0, 0xFFFF);
> >      rc |= iomem_permit_access(dom0, 0UL, ~0UL);
> >      rc |= irqs_permit_access(dom0, 1, nr_irqs_gsi - 1);
> >
> > Do you want a patch to that/similar effect?
> 
> Yes. Maybe a bit more smarter than permitting full I/0 caps for dom0.
> 
EhEh... do you mean to say that x86 is "not smart" ? :-)

> > So, this is probably me messing up with the terminology, but what
> > 'devices passthrough to it [dom0]' would mean, for example in cases
> > where we don't have an IOMMU (like cubie* boards), and hence where
> > proper passtrhogh will never be, I think, supported? Or do we plan to
> > have it working there too?
> 
> My term seems to be wrong for dom0 :).
> 
> On ARM, some device is used by Xen and therefore not exposed to dom0. By 
> passthrough to dom0 I meant every device that are given to dom0, no 
> matter if the platform has an IOMMU.
> 
Ok, now I got it.

> I think DOM0 should only be able to map theses devices to a guest. It 
> seems stupid to allow dom0 mapping RAM or the UART used by Xen.
> 
I see, and it does make sense.

Of course, I really don't know what these devices are and what the best
approach would be to hide/revoke permissions to their iomem/irqs (e.g.,
allow everything and then revoke some, allow selectively in the first
place, etc.).

I'm quite sure Arianna is keen on helping with this, but it better be,
if possible, someone of you ARM folks suggesting her what, where and
how. :-)

Thanks and 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: [RFC PATCH 2/3] arch, arm32: add the XEN_DOMCTL_memory_mapping hypercall

[Eric Trudeau]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@linaro.org]
> Sent: Monday, March 03, 2014 10:21 AM
> To: Dario Faggioli
> Cc: Arianna Avanzini; xen-devel@lists.xen.org; paolo.valente@unimore.it;
> stefano.stabellini@eu.citrix.com; julien.grall@citrix.com; Eric Trudeau;
> viktor.kleinik@globallogic.com; Ian Campbell; Tim Deegan
> Subject: Re: [Xen-devel] [RFC PATCH 2/3] arch, arm32: add the
> XEN_DOMCTL_memory_mapping hypercall
> 
> 
> 
> On 03/03/14 19:56, Dario Faggioli wrote:
> > On dom, 2014-03-02 at 17:56 +0800, Julien Grall wrote:
> >> On 02/03/14 08:49, Arianna Avanzini wrote:
> >
> >>> +
> >>> +        ret = -EPERM;
> >>> +	/*
> >>> +	 * NOTE: dom0 seems to have empty iomem_caps but to be however
> able to
> >>> +	 *       access I/O memory ranges. The following check takes for granted
> >>> +	 *       that any iomem range can be mapped to a domU if the current
> >>> +	 *       domain is dom0.
> >>> +	 */
> >>> +        if ( current->domain->domain_id != 0 &&
> >>> +             !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns -
> 1) )
> >>> +            return ret;
> >>
> >> This check can be removed by adding in construct_dom0
> >> (arch/arm/domain_build.c) something like that:
> >>     /* DOM0 is permitted full I/O capabilities */
> >>     rc = iomem_permit_access(d, 0UL, ~OUL);
> >>
> > Right. FTR, xen/arch/x86/domain_build.c, has this (also in
> > construct_dom0):
> >
> >      /* DOM0 is permitted full I/O capabilities. */
> >      rc |= ioports_permit_access(dom0, 0, 0xFFFF);
> >      rc |= iomem_permit_access(dom0, 0UL, ~0UL);
> >      rc |= irqs_permit_access(dom0, 1, nr_irqs_gsi - 1);
> >
> > Do you want a patch to that/similar effect?
> 
> Yes. Maybe a bit more smarter than permitting full I/0 caps for dom0.
> 

Our implementation does not require Dom0 access permission in order
for it to grant access permission to a DomU.  I suppose it wouldn't hurt
for iomem_permit_access because we allow iomem regions to be mapped
into multiple domains; however, I think the irqs_permit_access call keeps
multiple domains from "owning" the same IRQ.  I might be wrong about that.

> >> I'm wondering if we can even restrict dom0 I/0 access by only permit
> >> access on devices passthrough to it. Because dom0 should not be allowed
> >> to map I/O ranges which belong to device used by Xen e.g : GIC, RAM,...
> >>
> > So, this is probably me messing up with the terminology, but what
> > 'devices passthrough to it [dom0]' would mean, for example in cases
> > where we don't have an IOMMU (like cubie* boards), and hence where
> > proper passtrhogh will never be, I think, supported? Or do we plan to
> > have it working there too?
> 
> My term seems to be wrong for dom0 :).
> 
> On ARM, some device is used by Xen and therefore not exposed to dom0. By
> passthrough to dom0 I meant every device that are given to dom0, no
> matter if the platform has an IOMMU.
> 
> I think DOM0 should only be able to map theses devices to a guest. It
> seems stupid to allow dom0 mapping RAM or the UART used by Xen.
> 
> --
> Julien Grall

Re: [RFC PATCH 2/3] arch, arm32: add the XEN_DOMCTL_memory_mapping hypercall

[Dario Faggioli]

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

On lun, 2014-03-03 at 16:25 +0000, Eric Trudeau wrote:

> > > Right. FTR, xen/arch/x86/domain_build.c, has this (also in
> > > construct_dom0):
> > >
> > >      /* DOM0 is permitted full I/O capabilities. */
> > >      rc |= ioports_permit_access(dom0, 0, 0xFFFF);
> > >      rc |= iomem_permit_access(dom0, 0UL, ~0UL);
> > >      rc |= irqs_permit_access(dom0, 1, nr_irqs_gsi - 1);
> > >
> > > Do you want a patch to that/similar effect?
> > 
> > Yes. Maybe a bit more smarter than permitting full I/0 caps for dom0.
> > 
> 
> Our implementation does not require Dom0 access permission in order
> for it to grant access permission to a DomU.  I suppose it wouldn't hurt
> for iomem_permit_access because we allow iomem regions to be mapped
> into multiple domains; however, I think the irqs_permit_access call keeps
> multiple domains from "owning" the same IRQ.  I might be wrong about that.
> 
As far as I understood it, it is not required here either. And in fact,
such permission is not there, and things works for Arianna too.

However, it seemed a sane check to have in place (e.g., the x86
implementation does check for that), that's why she's trying to
introduce it properly. :-)

After all, as far as I remember, you do have something like this:

    if ( current->domain->domain_id != 0 )
        break;

don't you?

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: [RFC PATCH 2/3] arch, arm32: add the XEN_DOMCTL_memory_mapping hypercall

[Eric Trudeau]

> -----Original Message-----
> From: Arianna Avanzini [mailto:avanzini.arianna@gmail.com]
> Sent: Saturday, March 01, 2014 7:49 PM
> To: xen-devel@lists.xen.org
> Cc: avanzini.arianna@gmail.com; dario.faggioli@citrix.com;
> paolo.valente@unimore.it; stefano.stabellini@eu.citrix.com;
> julien.grall@citrix.com; Eric Trudeau; viktor.kleinik@globallogic.com
> Subject: [RFC PATCH 2/3] arch, arm32: add the
> XEN_DOMCTL_memory_mapping hypercall
> 
> This commit introduces a first attempt of implementation of the
> XEN_DOMCTL_memory_mapping hypercall for arm32. The following
> warnings must be taken into consideration:
> . currently, the hypercall simply performs an 1:1 mapping of I/O
>   memory ranges (mfn == gfn);
> . the range of I/O memory addresses is mapped all at once with
>   map_mmio_regions();
> . the hypercall takes for granted that any I/O memory range can
>   be mapped to a domU if the current domain is dom0.
> 
> 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: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
> ---
>  xen/arch/arm/arm32/domctl.c | 74
> +++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/p2m.c          |  9 ++++++
>  xen/include/asm-arm/p2m.h   |  2 ++
>  3 files changed, 85 insertions(+)
> 
> diff --git a/xen/arch/arm/arm32/domctl.c b/xen/arch/arm/arm32/domctl.c
> index c2ca4d3..67cf734 100644
> --- a/xen/arch/arm/arm32/domctl.c
> +++ b/xen/arch/arm/arm32/domctl.c
> @@ -10,8 +10,11 @@
>  #include <xen/errno.h>
>  #include <xen/sched.h>
>  #include <xen/hypercall.h>
> +#include <xen/iocap.h>
>  #include <public/domctl.h>
> 
> +#include "cpu.h"
> +
>  long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>                 XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>  {
> @@ -19,6 +22,77 @@ long subarch_do_domctl(struct xen_domctl *domctl,
> struct domain *d,
>      {
>      case XEN_DOMCTL_set_address_size:
>          return domctl->u.address_size.size == 32 ? 0 : -EINVAL;
> +    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;
> +        long int ret;
> +
> +        ret = -EINVAL;
> +        if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
> +            ((mfn | (mfn + nr_mfns - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> +            (gfn + nr_mfns - 1) < gfn ) /* wrap? */
> +            return ret;
> +
> +        ret = -EPERM;
> +	/*
> +	 * NOTE: dom0 seems to have empty iomem_caps but to be however
> able to
> +	 *       access I/O memory ranges. The following check takes for granted
> +	 *       that any iomem range can be mapped to a domU if the current
> +	 *       domain is dom0.
> +	 */
> +        if ( current->domain->domain_id != 0 &&
> +             !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
> +            return ret;
> +
> +        ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 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 + nr_mfns - 1);
> +            if ( !ret )
> +            {
> +                /* 1:1 iomem mapping (gfn == mfn) */
> +                ret = map_mmio_regions(d, gfn, gfn + nr_mfns - 1, mfn);

Last I checked, map_mmio_regions takes paddr_t, not frame numbers.

Beware:  I found that map_mmio_regions was off by one page unless I changed
"gfn + nr_mfns - 1" to "gfn + nr_mfns".
This may have been fixed in more recent upstream code.

> +                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 + 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);
> +
> +            add = unmap_mmio_regions(d, gfn, gfn + nr_mfns - 1, mfn);
> +            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);
> +	}
> +        return ret;
> +    }
>      default:
>          return -ENOSYS;
>      }
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index d00c882..710f74e 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -461,6 +461,15 @@ int map_mmio_regions(struct domain *d,
>                               maddr, MATTR_DEV, p2m_mmio_direct);
>  }
> 
> +int unmap_mmio_regions(struct domain *d,
> +                       paddr_t start_gaddr,
> +                       paddr_t end_gaddr,
> +                       paddr_t maddr)
> +{
> +    return apply_p2m_changes(d, REMOVE, start_gaddr, end_gaddr,
> +                             maddr, MATTR_DEV, p2m_mmio_direct);
> +}
> +
>  int guest_physmap_add_entry(struct domain *d,
>                              unsigned long gpfn,
>                              unsigned long mfn,
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 3b39c45..907ce4a 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -88,6 +88,8 @@ 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, paddr_t start_gaddr,
> +                       paddr_t end_gaddr, paddr_t maddr);
> 
>  int guest_physmap_add_entry(struct domain *d,
>                              unsigned long gfn,
> --
> 1.8.5.3

Re: [RFC PATCH 2/3] arch, arm32: add the XEN_DOMCTL_memory_mapping hypercall

[Eric Trudeau]

> -----Original Message-----
> From: Dario Faggioli [mailto:dario.faggioli@citrix.com]
> Sent: Monday, March 03, 2014 11:35 AM
> To: Eric Trudeau
> Cc: Julien Grall; Arianna Avanzini; xen-devel@lists.xen.org;
> paolo.valente@unimore.it; stefano.stabellini@eu.citrix.com;
> julien.grall@citrix.com; viktor.kleinik@globallogic.com; Ian Campbell; Tim
> Deegan
> Subject: Re: [Xen-devel] [RFC PATCH 2/3] arch, arm32: add the
> XEN_DOMCTL_memory_mapping hypercall
> 
> On lun, 2014-03-03 at 16:25 +0000, Eric Trudeau wrote:
> 
> > > > Right. FTR, xen/arch/x86/domain_build.c, has this (also in
> > > > construct_dom0):
> > > >
> > > >      /* DOM0 is permitted full I/O capabilities. */
> > > >      rc |= ioports_permit_access(dom0, 0, 0xFFFF);
> > > >      rc |= iomem_permit_access(dom0, 0UL, ~0UL);
> > > >      rc |= irqs_permit_access(dom0, 1, nr_irqs_gsi - 1);
> > > >
> > > > Do you want a patch to that/similar effect?
> > >
> > > Yes. Maybe a bit more smarter than permitting full I/0 caps for dom0.
> > >
> >
> > Our implementation does not require Dom0 access permission in order
> > for it to grant access permission to a DomU.  I suppose it wouldn't hurt
> > for iomem_permit_access because we allow iomem regions to be mapped
> > into multiple domains; however, I think the irqs_permit_access call keeps
> > multiple domains from "owning" the same IRQ.  I might be wrong about that.
> >
> As far as I understood it, it is not required here either. And in fact,
> such permission is not there, and things works for Arianna too.
> 
> However, it seemed a sane check to have in place (e.g., the x86
> implementation does check for that), that's why she's trying to
> introduce it properly. :-)
> 
> After all, as far as I remember, you do have something like this:
> 
>     if ( current->domain->domain_id != 0 )
>         break;
> 
> don't you?
> 
Yes, we only allow Domain 0 to perform the memory_mapping hypercall.
As mentioned in one of the related threads to the patch, in embedded 
system implementations like ours, we are partitioning devices to various
DomU's and Dom0 is really the control domain with a few shared devices.
If it gets hacked we want to make it that much harder to get access to
a device because it does not have access to it, although the hacker
potentially would make the hypercall to get access, etc.  We think of it
in the same way that normally on Linux, you run without privileges and
use sudo when you really want to get more control temporarily.

I checked the flow for when we route an IRQ to multiple domains and
it does not check for more than one domain having permission to access
the PIRQ, it checks whether another domain already has the IRQ routed
to it.  So, even with IRQs, it should not be a problem to have Dom0 have
permission to all non-hypervisor IRQs.

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

Re: [RFC PATCH 2/3] arch, arm32: add the XEN_DOMCTL_memory_mapping hypercall

[Julien Grall]

Hi Dario,

On 03/03/14 23:33, Dario Faggioli wrote:
> On lun, 2014-03-03 at 23:20 +0800, Julien Grall wrote:
>> On 03/03/14 19:56, Dario Faggioli wrote:
>
>>> Right. FTR, xen/arch/x86/domain_build.c, has this (also in
>>> construct_dom0):
>>>
>>>       /* DOM0 is permitted full I/O capabilities. */
>>>       rc |= ioports_permit_access(dom0, 0, 0xFFFF);
>>>       rc |= iomem_permit_access(dom0, 0UL, ~0UL);
>>>       rc |= irqs_permit_access(dom0, 1, nr_irqs_gsi - 1);
>>>
>>> Do you want a patch to that/similar effect?
>>
>> Yes. Maybe a bit more smarter than permitting full I/0 caps for dom0.
>>
> EhEh... do you mean to say that x86 is "not smart" ? :-)

No :).

>
>>> So, this is probably me messing up with the terminology, but what
>>> 'devices passthrough to it [dom0]' would mean, for example in cases
>>> where we don't have an IOMMU (like cubie* boards), and hence where
>>> proper passtrhogh will never be, I think, supported? Or do we plan to
>>> have it working there too?
>>
>> My term seems to be wrong for dom0 :).
>>
>> On ARM, some device is used by Xen and therefore not exposed to dom0. By
>> passthrough to dom0 I meant every device that are given to dom0, no
>> matter if the platform has an IOMMU.
>>
> Ok, now I got it.
>
>> I think DOM0 should only be able to map theses devices to a guest. It
>> seems stupid to allow dom0 mapping RAM or the UART used by Xen.
>>
> I see, and it does make sense.
>
> Of course, I really don't know what these devices are and what the best
> approach would be to hide/revoke permissions to their iomem/irqs (e.g.,
> allow everything and then revoke some, allow selectively in the first
> place, etc.).

I think the best place to permit access is in map_device 
(xen/arch/arm/domain_build.c).

This function is called for every devices used  by dom0 to map I/O mem 
and IRQ.

>
> I'm quite sure Arianna is keen on helping with this, but it better be,
> if possible, someone of you ARM folks suggesting her what, where and
> how. :-)

Either the ML or #xenarm channel on Freenode are the best to ask question.
Arianna, I advice you to join the IRC channel if it's not already the 
case ;).

Cheers,

-- 
Julien Grall

Re: [RFC PATCH 2/3] arch, arm32: add the XEN_DOMCTL_memory_mapping hypercall

[Julien Grall]

Hello Eric,

On 04/03/14 02:06, Eric Trudeau wrote:
> Last I checked, map_mmio_regions takes paddr_t, not frame numbers.

Right.

> Beware:  I found that map_mmio_regions was off by one page unless I changed
> "gfn + nr_mfns - 1" to "gfn + nr_mfns".
> This may have been fixed in more recent upstream code.

That makes me think, map_mmio_regions is buggy if the range is not 
correctly aligned. For instance:

start_gaddr = 0x1001 and end_gaddr = 0x2001, we only map 0x1000-0x2000 
to the domain.

The problem is the same with p2m_populate_ram (even if I doubt that RAM 
range is not page-aligned).

I see 2 solutions to fix it:
	- Change map_mmio_regions prototype to use frame is instead address.
         - Aligned the input addresses.

IHMO, the former seems to be better, at least it avoid developer to 
discover that some bits was mapped under his feet.

Ian, do you have any opinion?

Regards,

-- 
Julien Grall

Re: [RFC PATCH 2/3] arch, arm32: add the XEN_DOMCTL_memory_mapping hypercall

[Arianna Avanzini]

On 03/02/2014 10:56 AM, Julien Grall wrote:
> Hello Arianna,
> 

Hello,

thank you all for your comments. I'm currently trying to address at my best the
issues pointed out throughout the discussion. Also, sorry for the delay in replying.


> On 02/03/14 08:49, Arianna Avanzini wrote:
>> This commit introduces a first attempt of implementation of the
>> XEN_DOMCTL_memory_mapping hypercall for arm32. The following
>> warnings must be taken into consideration:
>> . currently, the hypercall simply performs an 1:1 mapping of I/O
>>    memory ranges (mfn == gfn);
>> . the range of I/O memory addresses is mapped all at once with
>>    map_mmio_regions();
>> . the hypercall takes for granted that any I/O memory range can
>>    be mapped to a domU if the current domain is dom0.
>>
>> 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: Eric Trudeau <etrudeau@broadcom.com>
>> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
>> ---
>>   xen/arch/arm/arm32/domctl.c | 74 +++++++++++++++++++++++++++++++++++++++++++++
> 
> The implementation of XEN_DOMCTL_memory_mapping is not ARM32 specific. It can be
> implemented in arch/arm/domctl.c.
> 
>>   xen/arch/arm/p2m.c          |  9 ++++++
>>   xen/include/asm-arm/p2m.h   |  2 ++
>>   3 files changed, 85 insertions(+)
>>
>> diff --git a/xen/arch/arm/arm32/domctl.c b/xen/arch/arm/arm32/domctl.c
>> index c2ca4d3..67cf734 100644
>> --- a/xen/arch/arm/arm32/domctl.c
>> +++ b/xen/arch/arm/arm32/domctl.c
>> @@ -10,8 +10,11 @@
>>   #include <xen/errno.h>
>>   #include <xen/sched.h>
>>   #include <xen/hypercall.h>
>> +#include <xen/iocap.h>
>>   #include <public/domctl.h>
>>
>> +#include "cpu.h"
>> +
>>   long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>   {
>> @@ -19,6 +22,77 @@ long subarch_do_domctl(struct xen_domctl *domctl, struct
>> domain *d,
>>       {
>>       case XEN_DOMCTL_set_address_size:
>>           return domctl->u.address_size.size == 32 ? 0 : -EINVAL;
>> +    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;
>> +        long int ret;
>> +
>> +        ret = -EINVAL;
>> +        if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
>> +            ((mfn | (mfn + nr_mfns - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
>> +            (gfn + nr_mfns - 1) < gfn ) /* wrap? */
>> +            return ret;
>> diff --git a/xen/arch/arm/arm32/domctl.c b/xen/arch/arm/
>> +
>> +        ret = -EPERM;
>> +    /*
>> +     * NOTE: dom0 seems to have empty iomem_caps but to be however able to
>> +     *       access I/O memory ranges. The following check takes for granted
>> +     *       that any iomem range can be mapped to a domU if the current
>> +     *       domain is dom0.
>> +     */
>> +        if ( current->domain->domain_id != 0 &&
>> +             !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
>> +            return ret;
> 
> This check can be removed by adding in construct_dom0 (arch/arm/domain_build.c)
> something like that:
>   /* DOM0 is permitted full I/O capabilities */
>   rc = iomem_permit_access(d, 0UL, ~OUL);
> 
> I'm wondering if we can even restrict dom0 I/0 access by only permit access on
> devices passthrough to it. Because dom0 should not be allowed to map I/O ranges
> which belong to device used by Xen e.g : GIC, RAM,...
> 
> I think we should at least restrict dom0 to use the hypercall for mapping device
> memory. Otherwise dom0 may be allowed to map Xen address range, do wrong thing
> with foreign mapping...
> 
>> +
>> +        ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 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 + nr_mfns - 1);
>> +            if ( !ret )
>> +            {
>> +                /* 1:1 iomem mapping (gfn == mfn) */
>> +                ret = map_mmio_regions(d, gfn, gfn + nr_mfns - 1, mfn);
> 
> The comment "1:1 iomem mapping (gfn == mfn)" seems wrong here. The
> implementation you wrote allow gfn != mfn.
> 

Right, thank you.
Sorry if I bother you with one more question about this hunk; aside from the
error pointed out by Eric with the usage of pfn instead of paddr_t, is it OK to
map the address range all at once with map_mmio_regions() as proposed? I noticed
that the x86-related code seems to map the range one mfn at a time with
set_mmio_p2m_entry(), and I wonder if this different approach has any benefit I
didn't think about.



> Regards,
> 


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

Re: [RFC PATCH 2/3] arch, arm32: add the XEN_DOMCTL_memory_mapping hypercall

[Julien Grall]

On 05/03/14 21:59, Arianna Avanzini wrote:
> Hello,

Hello Arianna,

>> The comment "1:1 iomem mapping (gfn == mfn)" seems wrong here. The
>> implementation you wrote allow gfn != mfn.
>>
>
> Right, thank you.
> Sorry if I bother you with one more question about this hunk; aside from the
> error pointed out by Eric with the usage of pfn instead of paddr_t, is it OK to
> map the address range all at once with map_mmio_regions() as proposed? I noticed
> that the x86-related code seems to map the range one mfn at a time with
> set_mmio_p2m_entry(), and I wonder if this different approach has any benefit I
> didn't think about.

map_mmio_regions will take p2m lock and do MMU management (e.g TLB 
flush...). For the latter the code  has been written to only do TLB 
flush, if needed, once per call. More the range is large, better it is.

So, it's better to call the function only once for the whole range.

Regards,

-- 
Julien Grall

Re: [RFC PATCH 1/3] arch, arm32: add definition of paddr_bits

[Arianna Avanzini]

On 03/02/2014 09:13 AM, Julien Grall wrote:
> Hello Arianna,
> 

Hello,

> On 02/03/14 08:49, Arianna Avanzini wrote:
>> Currently, the paddr_bits variable, which keeps the machine word size,
>> is not defined for the arm32 architecture. This commits adds its
>> definition for arm32 in a new header in the architecture-specific
>> directory.
>> This change is instrumental to the implementation of the
>> XEN_DOMCTL_memory_mapping hypercall, added in the following
>> commit.
> 
> paddr_bits doesn't keep the machine word size but the size of the physical address.
> 
> On ARM, this value is fixed and defined as PADDR_BITS in
> xen/include/asm-arm/page.h. I don't think we need to introduce a variable for
> this purpose. x86 defines paddr_bits because the value can change on some
> processor.
> 

Thank you for explaining that. I've tried to fix as suggested.

>> diff --git a/xen/arch/arm/arm32/cpu.h b/xen/arch/arm/arm32/cpu.h
>> new file mode 100644
>> index 0000000..1091051
>> --- /dev/null
>> +++ b/xen/arch/arm/arm32/cpu.h
>> @@ -0,0 +1,7 @@
>> +#ifndef __ARM_ARM32_CPU_H
>> +#define __ARM_ARM32_CPU_H
>> +
>> +/* ARM-v7 features LPAE (machine word size is 40 bit) */
>> +unsigned int paddr_bits = 40;
>> +
> 
> Global variable must not be defined in headers. If someone decides to include
> this header in 2 differents C files, the variable will be defined twice.
> Therefore the linker-step will fail.
> 
> You should define it in a c-file.
> 

OK, thank you again.


> Regards,
> 


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

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

[Arianna Avanzini]

On 03/03/2014 04:13 PM, Julien Grall wrote:
> 
> 
> On 03/03/14 18:32, Dario Faggioli wrote:
>> Right. So, just to make sure I understand it correctly, you're saying
>> that it is fine to have the hypercall implemented and called sort of
>> like how Arianna is doing it, modulo moving the call outside of common
>> ARM and x86 code, as Julien pointed out, is this correct? If yes, I
>> (FWIW) totally concur. :-)
> 
> I think in both solution, this hypercall should be implemented :).
> 
>> For the sake of this series, do you think there is something that
>> Arianna should do, if not to ensure 100% consistency (which is not Xen's
>> job, in this case) but, perhaps, just to put things on the safe side?
>>
>> Right. Again, and sorry for not getting it just from the above, do you
>> think this is something Arianna should take care of, when reposting the
>> series, or can/should it be done also later?
> 
> I think Arianna solution's might not work if the 1:1 mapping clash with existing
> layout. I might help Arianna on this subject because it will be useful to
> passthrough a device to the guest.
> 

Sorry for the delay and for bothering you with another question. In order to
ensure that the 1:1 mapping of a new range of I/O memory does not clash with the
existing layout, we should then check that nothing else has been mapped to that
address range in the domU's address space?
Again, sorry for the probably obvious question.



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

Re: [RFC PATCH 2/3] arch, arm32: add the XEN_DOMCTL_memory_mapping hypercall

[Arianna Avanzini]

On 03/04/2014 03:42 AM, Julien Grall wrote:
> Hi Dario,
> 
> On 03/03/14 23:33, Dario Faggioli wrote:
>> On lun, 2014-03-03 at 23:20 +0800, Julien Grall wrote:
>>> On 03/03/14 19:56, Dario Faggioli wrote:
>>
>>>> Right. FTR, xen/arch/x86/domain_build.c, has this (also in
>>>> construct_dom0):
>>>>
>>>>       /* DOM0 is permitted full I/O capabilities. */
>>>>       rc |= ioports_permit_access(dom0, 0, 0xFFFF);
>>>>       rc |= iomem_permit_access(dom0, 0UL, ~0UL);
>>>>       rc |= irqs_permit_access(dom0, 1, nr_irqs_gsi - 1);
>>>>
>>>> Do you want a patch to that/similar effect?
>>>
>>> Yes. Maybe a bit more smarter than permitting full I/0 caps for dom0.
>>>
>> EhEh... do you mean to say that x86 is "not smart" ? :-)
> 
> No :).
> 
>>
>>>> So, this is probably me messing up with the terminology, but what
>>>> 'devices passthrough to it [dom0]' would mean, for example in cases
>>>> where we don't have an IOMMU (like cubie* boards), and hence where
>>>> proper passtrhogh will never be, I think, supported? Or do we plan to
>>>> have it working there too?
>>>
>>> My term seems to be wrong for dom0 :).
>>>
>>> On ARM, some device is used by Xen and therefore not exposed to dom0. By
>>> passthrough to dom0 I meant every device that are given to dom0, no
>>> matter if the platform has an IOMMU.
>>>
>> Ok, now I got it.
>>
>>> I think DOM0 should only be able to map theses devices to a guest. It
>>> seems stupid to allow dom0 mapping RAM or the UART used by Xen.
>>>
>> I see, and it does make sense.
>>
>> Of course, I really don't know what these devices are and what the best
>> approach would be to hide/revoke permissions to their iomem/irqs (e.g.,
>> allow everything and then revoke some, allow selectively in the first
>> place, etc.).
> 
> I think the best place to permit access is in map_device
> (xen/arch/arm/domain_build.c).
> 
> This function is called for every devices used  by dom0 to map I/O mem and IRQ.
> 

Thank you both for making this point more clear to me. I have tried to fix as
specified.

>>
>> I'm quite sure Arianna is keen on helping with this, but it better be,
>> if possible, someone of you ARM folks suggesting her what, where and
>> how. :-)
> 
> Either the ML or #xenarm channel on Freenode are the best to ask question.
> Arianna, I advice you to join the IRC channel if it's not already the case ;).
> 
> Cheers,
> 


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

Re: [RFC PATCH 2/3] arch, arm32: add the XEN_DOMCTL_memory_mapping hypercall

[Arianna Avanzini]

On 03/04/2014 04:08 AM, Julien Grall wrote:
> Hello Eric,
> 
> On 04/03/14 02:06, Eric Trudeau wrote:
>> Last I checked, map_mmio_regions takes paddr_t, not frame numbers.
> 
> Right.
> 

Thank you both, I have tried to fix by using the correct conversion macro.

>> Beware:  I found that map_mmio_regions was off by one page unless I changed
>> "gfn + nr_mfns - 1" to "gfn + nr_mfns".
>> This may have been fixed in more recent upstream code.
> 
> That makes me think, map_mmio_regions is buggy if the range is not correctly
> aligned. For instance:
> 
> start_gaddr = 0x1001 and end_gaddr = 0x2001, we only map 0x1000-0x2000 to the
> domain.
> 
> The problem is the same with p2m_populate_ram (even if I doubt that RAM range is
> not page-aligned).
> 
> I see 2 solutions to fix it:
>     - Change map_mmio_regions prototype to use frame is instead address.
>         - Aligned the input addresses.
> 
> IHMO, the former seems to be better, at least it avoid developer to discover
> that some bits was mapped under his feet.
> 

Sorry if I intrude here, thank you for pointing that out. As of now I have tried
to simply pass aligned addresses to map_mmio_regions() when it is invoked by the
memory_mapping hypercall.
Sorry for the additional question, do you think that it would be appropriate to
add to the series a patch implementing one of the above-described solutions (the
one believed to be better)?


> Ian, do you have any opinion?
> 
> Regards,
> 


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

Re: [RFC PATCH 2/3] arch, arm32: add the XEN_DOMCTL_memory_mapping hypercall

[Arianna Avanzini]

On 03/06/2014 04:41 AM, Julien Grall wrote:
> 
> 
> On 05/03/14 21:59, Arianna Avanzini wrote:
>> Hello,
> 
> Hello Arianna,
> 
>>> The comment "1:1 iomem mapping (gfn == mfn)" seems wrong here. The
>>> implementation you wrote allow gfn != mfn.
>>>
>>
>> Right, thank you.
>> Sorry if I bother you with one more question about this hunk; aside from the
>> error pointed out by Eric with the usage of pfn instead of paddr_t, is it OK to
>> map the address range all at once with map_mmio_regions() as proposed? I noticed
>> that the x86-related code seems to map the range one mfn at a time with
>> set_mmio_p2m_entry(), and I wonder if this different approach has any benefit I
>> didn't think about.
> 
> map_mmio_regions will take p2m lock and do MMU management (e.g TLB flush...).
> For the latter the code  has been written to only do TLB flush, if needed, once
> per call. More the range is large, better it is.
> 
> So, it's better to call the function only once for the whole range.
> 

OK, thank you for the clear explanation.


> Regards,
> 


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

Re: [RFC PATCH 2/3] arch, arm32: add the XEN_DOMCTL_memory_mapping hypercall

[Julien Grall]

Hello Arianna,

On 07/03/14 08:56, Arianna Avanzini wrote:
>>> Beware:  I found that map_mmio_regions was off by one page unless I changed
>>> "gfn + nr_mfns - 1" to "gfn + nr_mfns".
>>> This may have been fixed in more recent upstream code.
>>
>> That makes me think, map_mmio_regions is buggy if the range is not correctly
>> aligned. For instance:
>>
>> start_gaddr = 0x1001 and end_gaddr = 0x2001, we only map 0x1000-0x2000 to the
>> domain.
>>
>> The problem is the same with p2m_populate_ram (even if I doubt that RAM range is
>> not page-aligned).
>>
>> I see 2 solutions to fix it:
>>      - Change map_mmio_regions prototype to use frame is instead address.
>>          - Aligned the input addresses.
>>
>> IHMO, the former seems to be better, at least it avoid developer to discover
>> that some bits was mapped under his feet.
>>
>
> Sorry if I intrude here, thank you for pointing that out. As of now I have tried
> to simply pass aligned addresses to map_mmio_regions() when it is invoked by the
> memory_mapping hypercall.

FYI, the end address should be PAGE-aligned minus 1. Otherwise you will 
map a spurious page to the guest.

> Sorry for the additional question, do you think that it would be appropriate to
> add to the series a patch implementing one of the above-described solutions (the
> one believed to be better)?

I'd like to have some input from Ian before. I think for now it's fine 
to stay with the current implementation.

Regards,

-- 
Julien Grall

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

[Julien Grall]

Hello Arianna,

On 07/03/14 08:45, Arianna Avanzini wrote:
>>
>> I think Arianna solution's might not work if the 1:1 mapping clash with existing
>> layout. I might help Arianna on this subject because it will be useful to
>> passthrough a device to the guest.
>>
>
> Sorry for the delay and for bothering you with another question. In order to
> ensure that the 1:1 mapping of a new range of I/O memory does not clash with the
> existing layout, we should then check that nothing else has been mapped to that
> address range in the domU's address space?

I think checking is not the right solution, what will you do if the 
range is effectively clash to another one?

On a previous mail (see 
http://lists.xenproject.org/archives/html/xen-devel/2014-03/msg00036.html), 
Ian has proposed to exposed the layout of the host to the guest. That 
will allow you to map the memory in 1:1.

I will need this item for device passthrough soon. I can take care of 
this item if you want.

Regards,

-- 
Julien Grall

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

[Julien Grall]

On 03/03/14 19:19, Dario Faggioli wrote:
>> This code is common on both x86 and ARM. On x86, we don't want to map
>> the I/O mem region in the guest here. QEMU will do it later.
>>
> Right. I took a quick look and, as you're saying, I found the call sites
> of xc_domain_memory_mapping() in QEMU's code (in _pt_iomem_helper() ).
> At the same time, I see QEMU being created as a device model in this
> same function, below this hunk... But that does _not_ _always_ happen, I
> think, does it?
>
> In particular, what about PV guests for which libxl__need_xenpv_qemu()
> returns 0? Who calls xc_domain_memory_mapping() on the specified iomem
> region(s) for them?

Only the PV guest knows its memory layout. I guess 
xc_domain_memory_mapping is called by itself.

Adding Tim and Jan that might be able to answer to this question.

-- 
Julien Grall

Re: [RFC PATCH 2/3] arch, arm32: add the XEN_DOMCTL_memory_mapping hypercall

[Arianna Avanzini]

On 03/07/2014 04:41 AM, Julien Grall wrote:
> 
> Hello Arianna,
> 
> On 07/03/14 08:56, Arianna Avanzini wrote:
>>>> Beware:  I found that map_mmio_regions was off by one page unless I changed
>>>> "gfn + nr_mfns - 1" to "gfn + nr_mfns".
>>>> This may have been fixed in more recent upstream code.
>>>
>>> That makes me think, map_mmio_regions is buggy if the range is not correctly
>>> aligned. For instance:
>>>
>>> start_gaddr = 0x1001 and end_gaddr = 0x2001, we only map 0x1000-0x2000 to the
>>> domain.
>>>
>>> The problem is the same with p2m_populate_ram (even if I doubt that RAM range is
>>> not page-aligned).
>>>
>>> I see 2 solutions to fix it:
>>>      - Change map_mmio_regions prototype to use frame is instead address.
>>>          - Aligned the input addresses.
>>>
>>> IHMO, the former seems to be better, at least it avoid developer to discover
>>> that some bits was mapped under his feet.
>>>
>>
>> Sorry if I intrude here, thank you for pointing that out. As of now I have tried
>> to simply pass aligned addresses to map_mmio_regions() when it is invoked by the
>> memory_mapping hypercall.
> 
> FYI, the end address should be PAGE-aligned minus 1. Otherwise you will map a
> spurious page to the guest.
> 

Thank you for pointing that out.

>> Sorry for the additional question, do you think that it would be appropriate to
>> add to the series a patch implementing one of the above-described solutions (the
>> one believed to be better)?
> 
> I'd like to have some input from Ian before. I think for now it's fine to stay
> with the current implementation.
> 

OK. Thank you for making this clear.


> Regards,
> 


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

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

[Arianna Avanzini]

On 03/07/2014 05:03 AM, Julien Grall wrote:
> Hello Arianna,
> 
> On 07/03/14 08:45, Arianna Avanzini wrote:
>>>
>>> I think Arianna solution's might not work if the 1:1 mapping clash with existing
>>> layout. I might help Arianna on this subject because it will be useful to
>>> passthrough a device to the guest.
>>>
>>
>> Sorry for the delay and for bothering you with another question. In order to
>> ensure that the 1:1 mapping of a new range of I/O memory does not clash with the
>> existing layout, we should then check that nothing else has been mapped to that
>> address range in the domU's address space?
> 
> I think checking is not the right solution, what will you do if the range is
> effectively clash to another one?
> 

Right, sorry.

> On a previous mail (see
> http://lists.xenproject.org/archives/html/xen-devel/2014-03/msg00036.html), Ian
> has proposed to exposed the layout of the host to the guest. That will allow you
> to map the memory in 1:1.
> 
> I will need this item for device passthrough soon. I can take care of this item
> if you want.
> 

OK, thank you again for replying and making this clear.


> Regards,
> 


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