[RFC PATCH v2 0/3] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM

[RFC PATCH v2 0/3] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM

[Arianna Avanzini]

Hello,

this patchset is the v2 of my previously-proposed ([1]) attempt of an
implementation of the XEN_DOMCTL_memory_mapping hypercall for the ARM
architecture.

As I already wrote in the cover letter of the first version, as a part of my
thesis project (developed with Paolo Valente, [2]), I am porting an automotive-
grade real-time OS (Evidence's ERIKA Enterprise, [3], [4]) on Xen on ARM.
The port of the OS as a domU has required it 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 the first version of the patchset after reading a related
xen-devel discussion ([5]), and I afterwards found another topic where Eric
Trudeau had already proposed a possible implementation of the hypercall ([6],
[7]).

The first commit in this patchset gives dom0 access to memory ranges used
for memory-mapped I/O to devices exposed to it, as suggested by Julien Grall.
The previous implementation simply assumed that dom0 could access any range of
I/O memory.

The second commit adds an implementation of the XEN_DOMCTL_memory_mapping
hypercall to xen/arm/domctl.c. As of this commit, with respect to the previous
version, I have been trying to address the following issues.
. The code has been moved to xen/arm/domctl.c, since it is ARM-specific but not
  ARM32-specific, as suggested by Julien Grall. The previous version incorrectly
  implemented the hypercall in xen/arm/arm32/domctl.c.
. The code uses the PADDR_BITS constant already defined for ARM, indicated by
  Julien Grall. The previous implementation wrongly defined a new ARM32-specific
  variable to keep the physical address size. 
. The v2 patchset uses paddr_t as arguments to the map_mmio_regions() function,
  as indicated by Eric Trudeau. The previous version incorrectly used pfn.
. This version of the code aligns addresses given as arguments to the function
  map_mmio_regions(). The previous implementation did not page-align addresses:
  in this way, the wrong number of pages might be mapped, as pointed out by Eric
  Trudeau and Julien Grall.

The third commit adds to libxl code to handle the iomem configuration option by
invoking the newly-implemented hypercall. This still leaves the following
outstanding issue, pointed out by Julien Grall.
. The code used to handle the iomem configuration option by invoking the
  newly-implemented hypercall is common to x86 and ARM. It implements
  a simple 1:1 mapping which could clash with the domU's existing memory
  layout if the range has already been used in the guest's address space.

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

Arianna

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

Arianna Avanzini (3):
  arch, arm: allow dom0 access to I/O memory of mapped devices
  arch, arm: add the XEN_DOMCTL_memory_mapping hypercall
  tools, libxl: handle the iomem parameter with the memory_mapping hcall

 tools/libxl/libxl_create.c  | 17 +++++++++++
 xen/arch/arm/domain_build.c | 10 +++++++
 xen/arch/arm/domctl.c       | 69 +++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/p2m.c          |  9 ++++++
 xen/include/asm-arm/p2m.h   |  2 ++
 5 files changed, 107 insertions(+)

-- 
1.9.0

[RFC PATCH v2 1/3] arch, arm: allow dom0 access to I/O memory of mapped devices

[Arianna Avanzini]

Currently, dom0 is not allowed access to the I/O memory ranges
used to access devices exposed to it. This commit attempts
to give it access to those memory ranges during domain build
by adding the ranges to dom0's iomem_caps.

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: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
 xen/arch/arm/domain_build.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 5ca2f15..0b283d8 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,15 @@ 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 dom0 access to"
+                   " 0x%"PRIx64" - 0x%"PRIx64"\n",
+                   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

Re: [RFC PATCH v2 1/3] arch, arm: allow dom0 access to I/O memory of mapped devices

[Julien Grall]

Hello Arianna,

On 03/10/2014 08:25 AM, Arianna Avanzini wrote:
> Currently, dom0 is not allowed access to the I/O memory ranges
> used to access devices exposed to it. This commit attempts
> to give it access to those memory ranges during domain build
> by adding the ranges to dom0's iomem_caps.
> 
> 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: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
> ---
>  xen/arch/arm/domain_build.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 5ca2f15..0b283d8 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,15 @@ 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 dom0 access to"

Can you print the right domain id instead of using "dom0"?

I have a patch to remove the other hardcoded "dom0" in this function.

Regards,

-- 
Julien Grall

Re: [RFC PATCH v2 1/3] arch, arm: allow dom0 access to I/O memory of mapped devices

[Arianna Avanzini]

On 03/10/2014 12:30 PM, Julien Grall wrote:
> Hello Arianna,
> 

Hello,

thank you again for the feedback.


> On 03/10/2014 08:25 AM, Arianna Avanzini wrote:
>> Currently, dom0 is not allowed access to the I/O memory ranges
>> used to access devices exposed to it. This commit attempts
>> to give it access to those memory ranges during domain build
>> by adding the ranges to dom0's iomem_caps.
>>
>> 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: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> Cc: Eric Trudeau <etrudeau@broadcom.com>
>> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
>> ---
>>  xen/arch/arm/domain_build.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 5ca2f15..0b283d8 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,15 @@ 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 dom0 access to"
> 
> Can you print the right domain id instead of using "dom0"?
> 

Sure, thank you for pointing that out.


> I have a patch to remove the other hardcoded "dom0" in this function.
> 
> Regards,
> 


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

Re: [RFC PATCH v2 1/3] arch, arm: allow dom0 access to I/O memory of mapped devices

[Ian Campbell]

On Mon, 2014-03-10 at 09:25 +0100, Arianna Avanzini wrote:
> Currently, dom0 is not allowed access to the I/O memory ranges
> used to access devices exposed to it. This commit attempts
> to give it access to those memory ranges during domain build
> by adding the ranges to dom0's iomem_caps.

Just to make sure I've got it straight: Strictly speaking dom0 is
allowed this access, because we unconditionally create the required
mappings. What this commit changes is to make sure that the bookkeeping
actually reflects that reality.

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

With Julien's comment addressed please add:
Acked-by: Ian Campbell <Ian.Campbell@eu.citrix.com>

> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
> ---
>  xen/arch/arm/domain_build.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 5ca2f15..0b283d8 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,15 @@ 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 dom0 access to"
> +                   " 0x%"PRIx64" - 0x%"PRIx64"\n",
> +                   addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);

Not your fault but I wonder why we are using u64 here for addr and
friends and not paddr_t.

> +            return res;
> +        }
>          res = map_mmio_regions(d, addr & PAGE_MASK,
>                                 PAGE_ALIGN(addr + size) - 1,
>                                 addr & PAGE_MASK);

Re: [RFC PATCH v2 1/3] arch, arm: allow dom0 access to I/O memory of mapped devices

[Julien Grall]

Hi Ian,

On 03/13/2014 03:27 PM, Ian Campbell wrote:
>> @@ -733,6 +734,15 @@ 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 dom0 access to"
>> +                   " 0x%"PRIx64" - 0x%"PRIx64"\n",
>> +                   addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
> 
> Not your fault but I wonder why we are using u64 here for addr and
> friends and not paddr_t.

dt_device_get_address is taken 2 pointers to u64 (e.g address and size)
in parameter. It was copy from Linux DT code.

I can send a patch to replace u64 by paddr_t for these calls.

Regards,

-- 
Julien Grall

[RFC PATCH v2 2/3] arch, arm: 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. The range of I/O
memory addresses is mapped all at once with map_mmio_regions().

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

diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index 45974e7..078b165 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -10,6 +10,7 @@
 #include <xen/errno.h>
 #include <xen/sched.h>
 #include <xen/hypercall.h>
+#include <xen/iocap.h>
 #include <public/domctl.h>
 
 long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
@@ -30,7 +31,75 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
 
         return p2m_cache_flush(d, s, e);
     }
+    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;
+        if ( !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 )
+            {
+                ret = map_mmio_regions(d, PAGE_ALIGN(pfn_to_paddr(gfn)),
+                                       PAGE_ALIGN(
+                                           pfn_to_paddr(gfn + nr_mfns)) - 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 + 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, PAGE_ALIGN(pfn_to_paddr(gfn)),
+                                     PAGE_ALIGN(
+                                         pfn_to_paddr(gfn + nr_mfns)) - 1,
+                                     PAGE_ALIGN(pfn_to_paddr(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 subarch_do_domctl(domctl, d, u_domctl);
     }
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.9.0

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

[Julien Grall]

Hello Arianna,

On 03/10/2014 08:25 AM, Arianna Avanzini wrote:
> This commit introduces a first attempt of implementation of the
> XEN_DOMCTL_memory_mapping hypercall for ARM. The range of I/O
> memory addresses is mapped all at once with map_mmio_regions().
> 
> 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: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
> ---
>  xen/arch/arm/domctl.c     | 69 +++++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/p2m.c        |  9 +++++++
>  xen/include/asm-arm/p2m.h |  2 ++
>  3 files changed, 80 insertions(+)
> 
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index 45974e7..078b165 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -10,6 +10,7 @@
>  #include <xen/errno.h>
>  #include <xen/sched.h>
>  #include <xen/hypercall.h>
> +#include <xen/iocap.h>
>  #include <public/domctl.h>
>  
>  long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
> @@ -30,7 +31,75 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>  
>          return p2m_cache_flush(d, s, e);
>      }
> +    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;
> +        if ( !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 )
> +            {
> +                ret = map_mmio_regions(d, PAGE_ALIGN(pfn_to_paddr(gfn)),
> +                                       PAGE_ALIGN(
> +                                           pfn_to_paddr(gfn + nr_mfns)) - 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 + 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, PAGE_ALIGN(pfn_to_paddr(gfn)),
> +                                     PAGE_ALIGN(
> +                                         pfn_to_paddr(gfn + nr_mfns)) - 1,
> +                                     PAGE_ALIGN(pfn_to_paddr(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);


The unmap part doesn't seem correct to me. With your solution, you are
allowed to remove any gfn as long as the current domain is permitted to
modify the mfn. No matter if gfn is effectively mapped to the mfn or not.

You should at least check that gfn is typed p2m_mmio_direct (This is
done by clean_mmio_p2m_entry on x86). You can also check that the gfn is
mapped to the mfn.

I would do that in the switch REMOVE in apply_p2m_changes.

> +	}
> +        return ret;
> +    }
>      default:
>          return subarch_do_domctl(domctl, d, u_domctl);
>      }
> 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)
> +{

Can you use pfn instead of physical address?

Regards,

-- 
Julien Grall

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

[Arianna Avanzini]

On 03/10/2014 01:03 PM, Julien Grall wrote:
> Hello Arianna,
> 
> On 03/10/2014 08:25 AM, Arianna Avanzini wrote:
>> This commit introduces a first attempt of implementation of the
>> XEN_DOMCTL_memory_mapping hypercall for ARM. The range of I/O
>> memory addresses is mapped all at once with map_mmio_regions().
>>
>> 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: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> Cc: Eric Trudeau <etrudeau@broadcom.com>
>> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
>> ---
>>  xen/arch/arm/domctl.c     | 69 +++++++++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/p2m.c        |  9 +++++++
>>  xen/include/asm-arm/p2m.h |  2 ++
>>  3 files changed, 80 insertions(+)
>>
>> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
>> index 45974e7..078b165 100644
>> --- a/xen/arch/arm/domctl.c
>> +++ b/xen/arch/arm/domctl.c
>> @@ -10,6 +10,7 @@
>>  #include <xen/errno.h>
>>  #include <xen/sched.h>
>>  #include <xen/hypercall.h>
>> +#include <xen/iocap.h>
>>  #include <public/domctl.h>
>>  
>>  long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>> @@ -30,7 +31,75 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>  
>>          return p2m_cache_flush(d, s, e);
>>      }
>> +    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;
>> +        if ( !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 )
>> +            {
>> +                ret = map_mmio_regions(d, PAGE_ALIGN(pfn_to_paddr(gfn)),
>> +                                       PAGE_ALIGN(
>> +                                           pfn_to_paddr(gfn + nr_mfns)) - 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 + 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, PAGE_ALIGN(pfn_to_paddr(gfn)),
>> +                                     PAGE_ALIGN(
>> +                                         pfn_to_paddr(gfn + nr_mfns)) - 1,
>> +                                     PAGE_ALIGN(pfn_to_paddr(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);
> 
> 
> The unmap part doesn't seem correct to me. With your solution, you are
> allowed to remove any gfn as long as the current domain is permitted to
> modify the mfn. No matter if gfn is effectively mapped to the mfn or not.
> 
> You should at least check that gfn is typed p2m_mmio_direct (This is
> done by clean_mmio_p2m_entry on x86). You can also check that the gfn is
> mapped to the mfn.
> 
> I would do that in the switch REMOVE in apply_p2m_changes.
> 

OK, thank you for the detailed feedback and for the suggestions. I'll certainly
try to implement the checks you indicated.


>> +	}
>> +        return ret;
>> +    }
>>      default:
>>          return subarch_do_domctl(domctl, d, u_domctl);
>>      }
>> 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)
>> +{
> 
> Can you use pfn instead of physical address?
> 

Sure, thank you for the feedback.
Sorry if I bother you with another question, unmap_mmio_regions() is a wrapper
to apply_p2m_changes(), which takes paddr_t as parameters. Is it OK to just have
unmap_mmio_regions() take pfn as parameters and then convert them to paddr_t
when calling apply_p2m_changes(), or would you prefer that changes are performed
also to apply_p2m_changes()?


> Regards,
> 


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

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

[Ian Campbell]

(x86 guys -- scope for making code common discussed below)

On Mon, 2014-03-10 at 09:25 +0100, Arianna Avanzini wrote:
> This commit introduces a first attempt of implementation of the
> XEN_DOMCTL_memory_mapping hypercall for ARM. The range of I/O
> memory addresses is mapped all at once with map_mmio_regions().
> 
> 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: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
> ---
>  xen/arch/arm/domctl.c     | 69 +++++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/p2m.c        |  9 +++++++
>  xen/include/asm-arm/p2m.h |  2 ++
>  3 files changed, 80 insertions(+)
> 
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index 45974e7..078b165 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -10,6 +10,7 @@
>  #include <xen/errno.h>
>  #include <xen/sched.h>
>  #include <xen/hypercall.h>
> +#include <xen/iocap.h>
>  #include <public/domctl.h>
>  
>  long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
> @@ -30,7 +31,75 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>  
>          return p2m_cache_flush(d, s, e);
>      }
> +    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? */

"foo + foo_mfns" appears quite a lot here. I think you could calculate
those once into foo_start and foo_end and use throughout which would
make the remainder much easier to follow.

Oh, I see this came from the x86 function of which this is an almost
complete copy -- can it not be made common?

The difference seems to be map_mmio_regions vs. a loop over
set_mmio_p2m_entry which seems easy enough to abstract out.

> +             ((mfn | (mfn + nr_mfns - 1)) >> (PADDR_BITS - PAGE_SHIFT)) ||
> +             (gfn + nr_mfns - 1) < gfn ) /* wrap? */
> +            return ret;
> +
> +        ret = -EPERM;
> +        if ( !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 )
> +            {
> +                ret = map_mmio_regions(d, PAGE_ALIGN(pfn_to_paddr(gfn)),
> +                                       PAGE_ALIGN(
> +                                           pfn_to_paddr(gfn + nr_mfns)) - 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 + 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, PAGE_ALIGN(pfn_to_paddr(gfn)),
> +                                     PAGE_ALIGN(
> +                                         pfn_to_paddr(gfn + nr_mfns)) - 1,
> +                                     PAGE_ALIGN(pfn_to_paddr(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 subarch_do_domctl(domctl, d, u_domctl);
>      }
> 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,

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

[Jan Beulich]

>>> On 13.03.14 at 16:29, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> @@ -30,7 +31,75 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>  
>>          return p2m_cache_flush(d, s, e);
>>      }
>> +    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? */
> 
> "foo + foo_mfns" appears quite a lot here. I think you could calculate
> those once into foo_start and foo_end and use throughout which would
> make the remainder much easier to follow.
> 
> Oh, I see this came from the x86 function of which this is an almost
> complete copy -- can it not be made common?
> 
> The difference seems to be map_mmio_regions vs. a loop over
> set_mmio_p2m_entry which seems easy enough to abstract out.

Indeed.

Jan

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

[Dario Faggioli]

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

On gio, 2014-03-13 at 15:36 +0000, Jan Beulich wrote:
> >>> On 13.03.14 at 16:29, Ian Campbell <Ian.Campbell@citrix.com> wrote:

> > "foo + foo_mfns" appears quite a lot here. I think you could calculate
> > those once into foo_start and foo_end and use throughout which would
> > make the remainder much easier to follow.
> > 
> > Oh, I see this came from the x86 function of which this is an almost
> > complete copy -- can it not be made common?
> > 
> > The difference seems to be map_mmio_regions vs. a loop over
> > set_mmio_p2m_entry which seems easy enough to abstract out.
> 
> Indeed.
> 
But, if I can ask, from where does this difference come from? If Arianna
is to make this common code, should she retain such difference or kill
it? I mean, do we want to keep having x86 mapping in loop and ARM
mapping all at once, or can we just merge that part two? If the former,
why?

ISTR, Arianna asked this already, but the answer was something like
"mapping all at once" is correct (someone correct me if I'm wrong).

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 v2 2/3] arch, arm: add the XEN_DOMCTL_memory_mapping hypercall

[Ian Campbell]

On Thu, 2014-03-13 at 16:51 +0100, Dario Faggioli wrote:
> On gio, 2014-03-13 at 15:36 +0000, Jan Beulich wrote:
> > >>> On 13.03.14 at 16:29, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> 
> > > "foo + foo_mfns" appears quite a lot here. I think you could calculate
> > > those once into foo_start and foo_end and use throughout which would
> > > make the remainder much easier to follow.
> > > 
> > > Oh, I see this came from the x86 function of which this is an almost
> > > complete copy -- can it not be made common?
> > > 
> > > The difference seems to be map_mmio_regions vs. a loop over
> > > set_mmio_p2m_entry which seems easy enough to abstract out.
> > 
> > Indeed.
> > 
> But, if I can ask, from where does this difference come from? If Arianna
> is to make this common code, should she retain such difference or kill
> it? I mean, do we want to keep having x86 mapping in loop and ARM
> mapping all at once, or can we just merge that part two? If the former,
> why?

If it were me I'd make an x86 equivalent to arms map_mmio_regions which
did the loop of set_mmio_p2m_entry.

Ian.

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

[Jan Beulich]

>>> On 13.03.14 at 16:51, Dario Faggioli <dario.faggioli@citrix.com> wrote:
> On gio, 2014-03-13 at 15:36 +0000, Jan Beulich wrote:
>> >>> On 13.03.14 at 16:29, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> 
>> > "foo + foo_mfns" appears quite a lot here. I think you could calculate
>> > those once into foo_start and foo_end and use throughout which would
>> > make the remainder much easier to follow.
>> > 
>> > Oh, I see this came from the x86 function of which this is an almost
>> > complete copy -- can it not be made common?
>> > 
>> > The difference seems to be map_mmio_regions vs. a loop over
>> > set_mmio_p2m_entry which seems easy enough to abstract out.
>> 
>> Indeed.
>> 
> But, if I can ask, from where does this difference come from? If Arianna
> is to make this common code, should she retain such difference or kill
> it? I mean, do we want to keep having x86 mapping in loop and ARM
> mapping all at once, or can we just merge that part two? If the former,
> why?

For one I'm the wrong one to ask - I didn't write that code.
And second - map_mmio_regions() is (so far) an ARM specific
function. Hence perhaps putting the loop into an identically
named function on x86 would be the right thing to do?

Jan

[RFC PATCH v2 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, so that the address range can be mapped
to the address space of the domU.
NOTE: the added code is still common to both x86 and ARM; it also
      implements a simple 1:1 mapping that could clash with the domU's
      existing memory layout if the range is already in use in the
      guest's address space.

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: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
 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 a604cd8..6c206c3 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1099,6 +1099,23 @@ 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 {
+            /*
+             * NOTE: the following code is still common to both x86
+             *       and ARM; it also implements a simple 1:1 mapping
+             *       that could clash with the domU's existing memory
+             *       layout if the range is already in use in the
+             *       guest's address space.
+             */
+            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.9.0

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

[Ian Campbell]

On Mon, 2014-03-10 at 09:25 +0100, Arianna Avanzini wrote:
> Currently, the configuration-parsing code concerning the handling of the
> iomem parameter only invokes the XEN_DOMCTL_iomem_permission hypercall.
> This commit lets the XEN_DOMCTL_memory_mapping hypercall be invoked
> after XEN_DOMCTL_iomem_permission when the iomem parameter is parsed
> from a domU configuration file, so that the address range can be mapped
> to the address space of the domU.
> NOTE: the added code is still common to both x86 and ARM; it also
>       implements a simple 1:1 mapping that could clash with the domU's
>       existing memory layout if the range is already in use in the
>       guest's address space.

In that case you need to CC the x86 maintainers (Jan, Keir, Tim) here.
It doesn't seem to me that this is going to be the correct thing to do
for either x86 PV or x86 HVM guests.

My gut feeling is that this should be ifdef'd or otherwise made
conditional.

> 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: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
> ---
>  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 a604cd8..6c206c3 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -1099,6 +1099,23 @@ 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;

Please add a continue here and drop the else brining the remainder of
the added code out an indentation level.

The existing error handling in this function seems very sketchy to me,
there's a bunch of places where we set ret but then carry on regardless,
more than likely overwriting ret again. It's possible that a bunch of
goto error_out's should be added.

> +        } else {
> +            /*
> +             * NOTE: the following code is still common to both x86
> +             *       and ARM; it also implements a simple 1:1 mapping
> +             *       that could clash with the domU's existing memory
> +             *       layout if the range is already in use in the
> +             *       guest's address space.

The right thing to do here is to add a guest address field to
libxl_iomem_range and to extend the xl parse to accept
	iomem = [ "MFN,NR@PFN" ] syntax
where @PFN is optional and the default is 1:1.

> +             */
> +            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;
> +            }
>          }
>      }
>  

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

[Julien Grall]

Hi Ian,

On 03/13/2014 03:27 PM, Ian Campbell wrote:
> On Mon, 2014-03-10 at 09:25 +0100, Arianna Avanzini wrote:
>> Currently, the configuration-parsing code concerning the handling of the
>> iomem parameter only invokes the XEN_DOMCTL_iomem_permission hypercall.
>> This commit lets the XEN_DOMCTL_memory_mapping hypercall be invoked
>> after XEN_DOMCTL_iomem_permission when the iomem parameter is parsed
>> from a domU configuration file, so that the address range can be mapped
>> to the address space of the domU.
>> NOTE: the added code is still common to both x86 and ARM; it also
>>       implements a simple 1:1 mapping that could clash with the domU's
>>       existing memory layout if the range is already in use in the
>>       guest's address space.
> 
> In that case you need to CC the x86 maintainers (Jan, Keir, Tim) here.
> It doesn't seem to me that this is going to be the correct thing to do
> for either x86 PV or x86 HVM guests.

>From my reply on V1, the XEN_DOMCTL_memory_mapping hypercall is called
by QEMU for HVM. I was unable to define where this call is made by PV.

>> 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: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> Cc: Eric Trudeau <etrudeau@broadcom.com>
>> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
>> ---
>>  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 a604cd8..6c206c3 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -1099,6 +1099,23 @@ 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;
> 
> Please add a continue here and drop the else brining the remainder of
> the added code out an indentation level.
> 
> The existing error handling in this function seems very sketchy to me,
> there's a bunch of places where we set ret but then carry on regardless,
> more than likely overwriting ret again. It's possible that a bunch of
> goto error_out's should be added.
> 
>> +        } else {
>> +            /*
>> +             * NOTE: the following code is still common to both x86
>> +             *       and ARM; it also implements a simple 1:1 mapping
>> +             *       that could clash with the domU's existing memory
>> +             *       layout if the range is already in use in the
>> +             *       guest's address space.
> 
> The right thing to do here is to add a guest address field to
> libxl_iomem_range and to extend the xl parse to accept
> 	iomem = [ "MFN,NR@PFN" ] syntax
> where @PFN is optional and the default is 1:1.

I disagree with this solution, the user doesn't know the guest layout.
How is it possible to let him choose the pfn?

I think, for now the best solution is to expose the same memory layout
as the host when iomem is set.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Thu, 2014-03-13 at 15:34 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 03/13/2014 03:27 PM, Ian Campbell wrote:
> > On Mon, 2014-03-10 at 09:25 +0100, Arianna Avanzini wrote:
> >> Currently, the configuration-parsing code concerning the handling of the
> >> iomem parameter only invokes the XEN_DOMCTL_iomem_permission hypercall.
> >> This commit lets the XEN_DOMCTL_memory_mapping hypercall be invoked
> >> after XEN_DOMCTL_iomem_permission when the iomem parameter is parsed
> >> from a domU configuration file, so that the address range can be mapped
> >> to the address space of the domU.
> >> NOTE: the added code is still common to both x86 and ARM; it also
> >>       implements a simple 1:1 mapping that could clash with the domU's
> >>       existing memory layout if the range is already in use in the
> >>       guest's address space.
> > 
> > In that case you need to CC the x86 maintainers (Jan, Keir, Tim) here.
> > It doesn't seem to me that this is going to be the correct thing to do
> > for either x86 PV or x86 HVM guests.
> 
> From my reply on V1, the XEN_DOMCTL_memory_mapping hypercall is called
> by QEMU for HVM. I was unable to define where this call is made by PV.

It's probably not. A PV guest will simply write entries to its page
tables mapping these machine addresses and Xen will use the caps arrays
to decide if that is allowed or not.

IOW XEN_DOMCTL_memory_mapping is only useful for externally translated
guests (i.e. x86 HVM and ARM).

Ian.
> 
> >> 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: Ian Jackson <Ian.Jackson@eu.citrix.com>
> >> Cc: Eric Trudeau <etrudeau@broadcom.com>
> >> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
> >> ---
> >>  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 a604cd8..6c206c3 100644
> >> --- a/tools/libxl/libxl_create.c
> >> +++ b/tools/libxl/libxl_create.c
> >> @@ -1099,6 +1099,23 @@ 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;
> > 
> > Please add a continue here and drop the else brining the remainder of
> > the added code out an indentation level.
> > 
> > The existing error handling in this function seems very sketchy to me,
> > there's a bunch of places where we set ret but then carry on regardless,
> > more than likely overwriting ret again. It's possible that a bunch of
> > goto error_out's should be added.
> > 
> >> +        } else {
> >> +            /*
> >> +             * NOTE: the following code is still common to both x86
> >> +             *       and ARM; it also implements a simple 1:1 mapping
> >> +             *       that could clash with the domU's existing memory
> >> +             *       layout if the range is already in use in the
> >> +             *       guest's address space.
> > 
> > The right thing to do here is to add a guest address field to
> > libxl_iomem_range and to extend the xl parse to accept
> > 	iomem = [ "MFN,NR@PFN" ] syntax
> > where @PFN is optional and the default is 1:1.
> 
> I disagree with this solution, the user doesn't know the guest layout.
> How is it possible to let him choose the pfn?
> 
> I think, for now the best solution is to expose the same memory layout
> as the host when iomem is set.
> 
> Regards,
> 

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

[Dario Faggioli]

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

On gio, 2014-03-13 at 15:34 +0000, Julien Grall wrote:

> >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> >> index a604cd8..6c206c3 100644
> >> --- a/tools/libxl/libxl_create.c
> >> +++ b/tools/libxl/libxl_create.c
> >> @@ -1099,6 +1099,23 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,

> >> +        } else {
> >> +            /*
> >> +             * NOTE: the following code is still common to both x86
> >> +             *       and ARM; it also implements a simple 1:1 mapping
> >> +             *       that could clash with the domU's existing memory
> >> +             *       layout if the range is already in use in the
> >> +             *       guest's address space.
> > 
> > The right thing to do here is to add a guest address field to
> > libxl_iomem_range and to extend the xl parse to accept
> > 	iomem = [ "MFN,NR@PFN" ] syntax
> > where @PFN is optional and the default is 1:1.
> 
> I disagree with this solution, the user doesn't know the guest layout.
> How is it possible to let him choose the pfn?
> 
I agree that the user will very much likely not know the guest layout.
Still, making @PFN optional and defaulting to 1:1, as Ian was
suggesting, seems the more flexible solution possible, as it does the
right thing (in combination with "layout passthrough", of course, as you
say below), but provide enough flexibility for some strange use case we
can't predict yet.

What's the downside of that?

> I think, for now the best solution is to expose the same memory layout
> as the host when iomem is set.
> 
Sure, but I don't think I see any conflict between this and the approach
Ian proposed above. What am I missing?

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

[Julien Grall]

On 03/13/2014 04:36 PM, Dario Faggioli wrote:
> On gio, 2014-03-13 at 15:34 +0000, Julien Grall wrote:
> 
>>>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>>>> index a604cd8..6c206c3 100644
>>>> --- a/tools/libxl/libxl_create.c
>>>> +++ b/tools/libxl/libxl_create.c
>>>> @@ -1099,6 +1099,23 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
> 
>>>> +        } else {
>>>> +            /*
>>>> +             * NOTE: the following code is still common to both x86
>>>> +             *       and ARM; it also implements a simple 1:1 mapping
>>>> +             *       that could clash with the domU's existing memory
>>>> +             *       layout if the range is already in use in the
>>>> +             *       guest's address space.
>>>
>>> The right thing to do here is to add a guest address field to
>>> libxl_iomem_range and to extend the xl parse to accept
>>> 	iomem = [ "MFN,NR@PFN" ] syntax
>>> where @PFN is optional and the default is 1:1.
>>
>> I disagree with this solution, the user doesn't know the guest layout.
>> How is it possible to let him choose the pfn?
>>
> I agree that the user will very much likely not know the guest layout.
> Still, making @PFN optional and defaulting to 1:1, as Ian was
> suggesting, seems the more flexible solution possible, as it does the
> right thing (in combination with "layout passthrough", of course, as you
> say below), but provide enough flexibility for some strange use case we
> can't predict yet.
> 
> What's the downside of that?

There is no need to extend libxl_iomem_range if we have a "layout
passthrough". The user will just have to specify the host MFN.

>> I think, for now the best solution is to expose the same memory layout
>> as the host when iomem is set.
>>
> Sure, but I don't think I see any conflict between this and the approach
> Ian proposed above. What am I missing?

I believe, Ian was assuming that the user know the layout because this
solution will be used to a very specific case (I think mostly when the
device tree won't describe the hardware).

I'm wondering, if we can let the kernel calling the hypercall. He knows
what is the memory layout of the VM.

-- 
Julien Grall

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

[Ian Campbell]

On Thu, 2014-03-13 at 16:47 +0000, Julien Grall wrote:

> >> I think, for now the best solution is to expose the same memory layout
> >> as the host when iomem is set.
> >>
> > Sure, but I don't think I see any conflict between this and the approach
> > Ian proposed above. What am I missing?
> 
> I believe, Ian was assuming that the user know the layout because this
> solution will be used to a very specific case (I think mostly when the
> device tree won't describe the hardware).

Right, my assumption was that the kernel was also compiled for the exact
hardware layout as part of some sort of embedded/appliance situation...

> I'm wondering, if we can let the kernel calling the hypercall. He knows
> what is the memory layout of the VM.

This would be somewhat analogous to what happens with an x86 PV guest.
It would have to be an physmap call or something since this domctl
wouldn't be accessible by the guest.

That makes a lot of sense actually since this domctl seems to have been
intended for use by the x86 HVM device model (qemu).
Ian.

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

[Dario Faggioli]

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

On gio, 2014-03-13 at 17:32 +0000, Ian Campbell wrote:
> On Thu, 2014-03-13 at 16:47 +0000, Julien Grall wrote:

> > > Sure, but I don't think I see any conflict between this and the approach
> > > Ian proposed above. What am I missing?
> > 
> > I believe, Ian was assuming that the user know the layout because this
> > solution will be used to a very specific case (I think mostly when the
> > device tree won't describe the hardware).
> 
> Right, my assumption was that the kernel was also compiled for the exact
> hardware layout as part of some sort of embedded/appliance situation...
> 
Exactly, that may very well be the case. It may not, in Arianna's case,
but it well can be true for others, or even for her, in future.

Therefore, I keep failing to see why to prevent this to be the case.

> > I'm wondering, if we can let the kernel calling the hypercall. He knows
> > what is the memory layout of the VM.
> 
> This would be somewhat analogous to what happens with an x86 PV guest.
> It would have to be an physmap call or something since this domctl
> wouldn't be accessible by the guest.
> 
> That makes a lot of sense actually since this domctl seems to have been
> intended for use by the x86 HVM device model (qemu).
>
I thought about that too. The reason why this was the taken approach is
this xen-devel discussion:
http://lists.xen.org/archives/html/xen-devel/2013-06/msg00870.html

in particular, this Julien's message:
http://lists.xen.org/archives/html/xen-devel/2013-06/msg00902.html

Also, Eric and Viktor where asking for/working on something similar, so
there perhaps would be some good in having this...

Eric, Viktor, can you comment why you need this call and how you use, or
want to use it for?
Would it be the same for you to have it in the form of a physmap call,
and invoke it from within the guest kernel?

In Arianna's case, it think it would be more than fine to implement it
that way, and call it from within the OS, isn't this the case, Arianna?

One thing I don't see right now is, in the in-kernel case, what we
should do when finding the "iomem=[]" option in a config file.

Also, just trying to recap, for Arianna's sake, moving the
implementation of the DOMCTL in common code (and implementing the
missing bits to make it works properly, of course) is still something we
want, right?

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

[Julien Grall]

On 13/03/14 18:37, Dario Faggioli wrote:
> I thought about that too. The reason why this was the taken approach is
> this xen-devel discussion:
> http://lists.xen.org/archives/html/xen-devel/2013-06/msg00870.html
>
> in particular, this Julien's message:
> http://lists.xen.org/archives/html/xen-devel/2013-06/msg00902.html

I was a newbie when I wrote this mail ;).

IMHO, I think we will have to implement a similar solution when the 
passthrough via device tree will be handled.
But ... for the "iomem", the best solution seems to let the guest mapped 
itself the range (see my previous comment on the thread).

> One thing I don't see right now is, in the in-kernel case, what we
> should do when finding the "iomem=[]" option in a config file.

Keep the current behavior in libxl. E.g give the permission to map the 
I/O range.

> Also, just trying to recap, for Arianna's sake, moving the
> implementation of the DOMCTL in common code (and implementing the
> missing bits to make it works properly, of course) is still something we
> want, right?

I think yes.

Regards,

-- 
Julien Grall

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

[Dario Faggioli]

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

On gio, 2014-03-13 at 20:29 +0000, Julien Grall wrote:
> 
> On 13/03/14 18:37, Dario Faggioli wrote:
> > I thought about that too. The reason why this was the taken approach is
> > this xen-devel discussion:
> > http://lists.xen.org/archives/html/xen-devel/2013-06/msg00870.html
> >
> > in particular, this Julien's message:
> > http://lists.xen.org/archives/html/xen-devel/2013-06/msg00902.html
> 
> I was a newbie when I wrote this mail ;).
> 
EhEh... And I by no means wanted to blame you or anyone else... Just
making sure the context is clear enough. :-)

> IMHO, I think we will have to implement a similar solution when the 
> passthrough via device tree will be handled.
> But ... for the "iomem", the best solution seems to let the guest mapped 
> itself the range (see my previous comment on the thread).
> 
Which one (comment)? :-)

Since you say "let the guest", I take it you're talking about the
solution of having the _guest's_ _kernel_ do the mapping?

> > One thing I don't see right now is, in the in-kernel case, what we
> > should do when finding the "iomem=[]" option in a config file.
> 
> Keep the current behavior in libxl. E.g give the permission to map the 
> I/O range.
> 
Ok... What I was asking was, how do we trigger a call to the proper
hypercall (which will be a physmap op, rather than a DOMCTL at that
point) inside the guest kernel?

As you say, right now, we just grant the guest the permission then, in
the x86 HVM case, QEMU will do the mapping later. How do QEMU knows what
to map? I tried to track that, but I did not found much, perhaps I know
too few technical details about PCI passthrough in QEMU.

However, more than figuring out how that happens, I'm interesting in
understanding who you think should inform the guest kernel to map
something, how to do this and what addresses. Because, I think, this is
going to be what Arianna (and Eric? And Viktor?) will need to do...

> > Also, just trying to recap, for Arianna's sake, moving the
> > implementation of the DOMCTL in common code (and implementing the
> > missing bits to make it works properly, of course) is still something we
> > want, right?
> 
> I think yes.
> 
So, again, you're saying we want both the DOMCTL, which no one will be
calling, and the physmap, which the guest kernel will be calling
"somehow", with the "somehow" part not well defined?

I guess the reason for this is that you'll need the DOMCTL at some
point, even for proper PCI passthrough? (yes, you said something about
this above, but I'm not sure I understood it there either)

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

[Ian Campbell]

On Thu, 2014-03-13 at 19:37 +0100, Dario Faggioli wrote:
> On gio, 2014-03-13 at 17:32 +0000, Ian Campbell wrote:
> > On Thu, 2014-03-13 at 16:47 +0000, Julien Grall wrote:
> 
> > > > Sure, but I don't think I see any conflict between this and the approach
> > > > Ian proposed above. What am I missing?
> > > 
> > > I believe, Ian was assuming that the user know the layout because this
> > > solution will be used to a very specific case (I think mostly when the
> > > device tree won't describe the hardware).
> > 
> > Right, my assumption was that the kernel was also compiled for the exact
> > hardware layout as part of some sort of embedded/appliance situation...
> > 
> Exactly, that may very well be the case. It may not, in Arianna's case,

Does Arianna's OS support device tree then? I had thought not. If it
doesn't support device tree then it is necessarily tied to a specific
version of Xen, since in the absence of DT it must hardcode some
addresses.

> but it well can be true for others, or even for her, in future.
> 
> Therefore, I keep failing to see why to prevent this to be the case.

Prevent what to be the case?

> > > I'm wondering, if we can let the kernel calling the hypercall. He knows
> > > what is the memory layout of the VM.
> > 
> > This would be somewhat analogous to what happens with an x86 PV guest.
> > It would have to be an physmap call or something since this domctl
> > wouldn't be accessible by the guest.
> > 
> > That makes a lot of sense actually since this domctl seems to have been
> > intended for use by the x86 HVM device model (qemu).
> >
> I thought about that too. The reason why this was the taken approach is
> this xen-devel discussion:
> http://lists.xen.org/archives/html/xen-devel/2013-06/msg00870.html
> 
> in particular, this Julien's message:
> http://lists.xen.org/archives/html/xen-devel/2013-06/msg00902.html
> 
> Also, Eric and Viktor where asking for/working on something similar, so
> there perhaps would be some good in having this...
> 
> Eric, Viktor, can you comment why you need this call and how you use, or
> want to use it for?
> Would it be the same for you to have it in the form of a physmap call,
> and invoke it from within the guest kernel?
> 
> In Arianna's case, it think it would be more than fine to implement it
> that way, and call it from within the OS, isn't this the case, Arianna?

It's certainly an option, and it would make a lot of the toolstack side
issues moot but I'm not at all sure it is the right answer. In
particular although it might be easy to bodge a mapping into many OSes I
can imagine getting such a think into something generic like Linux might
be more tricky -- in which case perhaps the toolstack should be taking
care of it, and that does have a certain appeal from the simplicity of
the guest interface side of things.

> One thing I don't see right now is, in the in-kernel case, what we
> should do when finding the "iomem=[]" option in a config file.

Even for an x86 HVM guest with iomem there is no call to
xc_domain_memory_mapping (even from qemu) it is called only for PCI
passthrough. I've no idea if/how iomem=[] works for x86 HVM guests.
Based on a cursory glance it looks to me like it wouldn't and if it did
work it would have the same problems wrt where to map it as we have
today with the ARM guests, except perhaps on a PC the sorts of things
you would pass through with this can be done 1:1 because they are legacy
PC peripherals etc.

I think we just don't know what the right answer is yet and I'm unlikely
to be able to say directly what the right answer is. I'm hoping that
people who want to use this low level functionality can provide a
consistent story for how it should work (a "design" if you like) to
which I can say "yes, that seems sensible" or "hmm, that seems odd
because of X". At the moment X is "the 1:1 mapping seems undesirable to
me". There have been some suggestions for how to fix that, someone with
a horse in the race should have a think about it and provide an
iteration on the design until we are happy with it.

> Also, just trying to recap, for Arianna's sake, moving the
> implementation of the DOMCTL in common code (and implementing the
> missing bits to make it works properly, of course) is still something we
> want, right?

*If* we go the route of having the kernel make the mapping then there is
no need, is there?

Ian.

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

[Julien Grall]

Hi Ian,

On 03/14/2014 09:46 AM, Ian Campbell wrote:
>> In Arianna's case, it think it would be more than fine to implement it
>> that way, and call it from within the OS, isn't this the case, Arianna?
> 
> It's certainly an option, and it would make a lot of the toolstack side
> issues moot but I'm not at all sure it is the right answer. In
> particular although it might be easy to bodge a mapping into many OSes I
> can imagine getting such a think into something generic like Linux might
> be more tricky -- in which case perhaps the toolstack should be taking
> care of it, and that does have a certain appeal from the simplicity of
> the guest interface side of things.

The generic way for Linux (and other oses) is to use device tree
passthrough.
I still think the "iomem" is a hackish way to passthrough the hardware
to guest. People who will use this solution are aware of there kernel
should map itself the region.

>> Also, just trying to recap, for Arianna's sake, moving the
>> implementation of the DOMCTL in common code (and implementing the
>> missing bits to make it works properly, of course) is still something we
>> want, right?
> 
> *If* we go the route of having the kernel make the mapping then there is
> no need, is there?

How the kernel will map the region?

Regards,

-- 
Julien Grall

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

[Dario Faggioli]

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

On ven, 2014-03-14 at 09:46 +0000, Ian Campbell wrote:
> On Thu, 2014-03-13 at 19:37 +0100, Dario Faggioli wrote:

> > Exactly, that may very well be the case. It may not, in Arianna's case,
> 
> Does Arianna's OS support device tree then? I had thought not. 
>
It does not. The "it's not the case" was referred to the hardcoded
addresses of the IO registers, as she's for now using the 1:1 mapping.
But as I said, that can change, and that's why I was arguing in favor of
your solution of being able to specify a PFN too.

> If it
> doesn't support device tree then it is necessarily tied to a specific
> version of Xen, since in the absence of DT it must hardcode some
> addresses.
> 
That's fine, at least for now, for that OS, as well as, I'm quite sure,
for many embedded OSes, should they want to run on Xen.

> > but it well can be true for others, or even for her, in future.
> > 
> > Therefore, I keep failing to see why to prevent this to be the case.
> 
> Prevent what to be the case?
> 
Julien was saying he does not want the extended "iomem=[MFN,NR@PFN]"
syntax. In particular, he does not want the "@PFN" part, which I happen
to like. :-)

> > In Arianna's case, it think it would be more than fine to implement it
> > that way, and call it from within the OS, isn't this the case, Arianna?
> 
> It's certainly an option, and it would make a lot of the toolstack side
> issues moot but I'm not at all sure it is the right answer. In
> particular although it might be easy to bodge a mapping into many OSes I
> can imagine getting such a think into something generic like Linux might
> be more tricky -- in which case perhaps the toolstack should be taking
> care of it, and that does have a certain appeal from the simplicity of
> the guest interface side of things.
> 
If the toolstack needs to support iomem, yes.

A way to see it could be that, as of now, we have embedded OSes asking
for it already, and, at the same time, it's unlikely that more generic
system such as Linux would want something similar, for one because
they'll have full DT/ACPI support for this.

The fact that, if what you say below is true, "iomem" does not work at
all, and no one complained from the Linux world so far, seems to me to 

> > One thing I don't see right now is, in the in-kernel case, what we
> > should do when finding the "iomem=[]" option in a config file.
> 
> Even for an x86 HVM guest with iomem there is no call to
> xc_domain_memory_mapping (even from qemu) it is called only for PCI
> passthrough. I've no idea if/how iomem=[] works for x86 HVM guests.
> Based on a cursory glance it looks to me like it wouldn't and if it did
> work it would have the same problems wrt where to map it as we have
> today with the ARM guests, except perhaps on a PC the sorts of things
> you would pass through with this can be done 1:1 because they are legacy
> PC peripherals etc.
> 
Aha, interesting! As said to Julien, I tried to look for how these iomem
regions get to the xc_domain_memory_map in QEMU and I also found nothing
(except for PCI passthrough stuff)... I was assuming I was missing
something... apparently, I wasn't. :-)

I can try (even just out of curiosity!) to dig in the tree and see what
happened with this feature...

BTW, doesn't this make this discussion even more relevant? I mean, if
it's there but it's not working, shouldn't we make it work (for some
definition of "work"), if we decide it's useful, or kill it, if not?

> I think we just don't know what the right answer is yet and I'm unlikely
> to be able to say directly what the right answer is. I'm hoping that
> people who want to use this low level functionality can provide a
> consistent story for how it should work (a "design" if you like) to
> which I can say "yes, that seems sensible" or "hmm, that seems odd
> because of X". At the moment X is "the 1:1 mapping seems undesirable to
> me". There have been some suggestions for how to fix that, someone with
> a horse in the race should have a think about it and provide an
> iteration on the design until we are happy with it.
> 
Again, I'll look more into the history of this feature.

In the meantime, the man page entry says:

"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.

It is recommended to use this option only for trusted VMs under
administrator control."

For one, the "Allow guest to access" there leaves a lot of room for
interpretation, I think. It doesn't say anything about mapping, so one
can interpret it as 'this only grant you mapping permission, up to you
to map'. However, it says "access", so one can interpret it as 'if I can
access it, it's mapped already'.

Wherever this discussion will land, I think that, if we keep this
option, we should make the documentation less ambiguous.

That being said, allow me to play, in the rest of this e-mail, the role
of one which expects the mapping to be actually instated by just
specifying "iomem=[]" in the config file, which is (correct me guys if
I'm wrong) what Eric, Viktor and Arianna thought when reading the entry
above.

So:
 1. it says nothing about where the mapping ends up in guest's memory,
 2. it looks quite clear (to me?) that this is a raw/low level feature,
    to be used under controlled conditions (which an embedded product
    often is)

To me, a legitimate use case is this: I want to run version X of my non
DT capable OS on version Z of Xen, on release K of board B. In such
configuration, the GPIO controller is at MFN 0x000abcd, and I want only
VM V to have direct access to it (board B dos not have an IOMMU).

I would also assume that one is in full control of the guest address
space, so it is be ok to hardcode the addresses of registers somewhere.
Of course, that may be an issue, when putting Xen underneath, as Xen's
mangling with the such address space can cause troubles.

Arianna, can you confirm that this is pretty much the case of Erika, or
amend what I did get wrong?

I certainly don't claim to have the right answer but, in the described
scenario, either:
 1) the combination of iomem=[ MFN,NR@PFN ]", defaulting to 1:1 if  
    "@PFN is missing, and e820_host
 2) calling (the equivalent of) XEN_DOMCTL_memory_map from the guest 
    kernel

would be good solutions, to the point that I think we could even support
both. The main point being that, I think, even in the worst case, any
erroneous usage of either, would "just" destroy the guest, and that's
acceptable.

Actually, if going for 1), I think (when both the pieces are there)
things should be pretty safe. Furthermore, implementing 1) seems to me
the only way of having the "iomem=[]" parameter causing both permissions
granting and mapping. Downside is (libxl side of) the implementation
indeed looks cumbersome.

If going for _only_ 2), then "iomem=[]" would just be there to ensure
the future mapping operation to be successful, i.e., for granting
mapping rights, as it's doing right now. It would be up to the guest
kernel to make sure the MFN it is trying to map are consistent with what
was specified in "iomem=[]". Given the use case we're talking about, I
don't think this is an unreasonable request, as far as we make the iomem
man entry more clearly stating this.

Again, Arianna, do you confirm both solution are possible for you?

I certainly agree that the thread could benefit from some opinion from
people actually wanting to use this. In addition to Arianna, I have
pinged Eirc and Viktor repeatedly... let's see if they have time to let
us all know something more about their own needs and requirements wrt
this.

> > Also, just trying to recap, for Arianna's sake, moving the
> > implementation of the DOMCTL in common code (and implementing the
> > missing bits to make it works properly, of course) is still something we
> > want, right?
> 
> *If* we go the route of having the kernel make the mapping then there is
> no need, is there?
> 
Yeah, well, me not being sure is the reason I asked... And Julien said
he thinks we still want it... :-P

As I was saying above, I think there is room for both, but I don't mind
picking up one. However, if we want to fix iomem=[] and go as far as
having it doing the mapping, then I think we all agree we need the
DOMCTL.

So, looks like the discussion resolves to something like:
 - do we need the DOMCTL for other purposes than iomem=[] ?
 - if no, what do we want to do with iomem=[] ?

Sorry to have brought you all deep down into this can of worms! :-/

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

[Arianna Avanzini]

On 03/14/2014 01:15 PM, Dario Faggioli wrote:
> On ven, 2014-03-14 at 09:46 +0000, Ian Campbell wrote:
>> On Thu, 2014-03-13 at 19:37 +0100, Dario Faggioli wrote:
> 
>>> Exactly, that may very well be the case. It may not, in Arianna's case,
>>
>> Does Arianna's OS support device tree then? I had thought not. 
>>
> It does not. The "it's not the case" was referred to the hardcoded
> addresses of the IO registers, as she's for now using the 1:1 mapping.
> But as I said, that can change, and that's why I was arguing in favor of
> your solution of being able to specify a PFN too.
> 
>> If it
>> doesn't support device tree then it is necessarily tied to a specific
>> version of Xen, since in the absence of DT it must hardcode some
>> addresses.
>>
> That's fine, at least for now, for that OS, as well as, I'm quite sure,
> for many embedded OSes, should they want to run on Xen.
> 
>>> but it well can be true for others, or even for her, in future.
>>>
>>> Therefore, I keep failing to see why to prevent this to be the case.
>>
>> Prevent what to be the case?
>>
> Julien was saying he does not want the extended "iomem=[MFN,NR@PFN]"
> syntax. In particular, he does not want the "@PFN" part, which I happen
> to like. :-)
> 
>>> In Arianna's case, it think it would be more than fine to implement it
>>> that way, and call it from within the OS, isn't this the case, Arianna?
>>
>> It's certainly an option, and it would make a lot of the toolstack side
>> issues moot but I'm not at all sure it is the right answer. In
>> particular although it might be easy to bodge a mapping into many OSes I
>> can imagine getting such a think into something generic like Linux might
>> be more tricky -- in which case perhaps the toolstack should be taking
>> care of it, and that does have a certain appeal from the simplicity of
>> the guest interface side of things.
>>
> If the toolstack needs to support iomem, yes.
> 
> A way to see it could be that, as of now, we have embedded OSes asking
> for it already, and, at the same time, it's unlikely that more generic
> system such as Linux would want something similar, for one because
> they'll have full DT/ACPI support for this.
> 
> The fact that, if what you say below is true, "iomem" does not work at
> all, and no one complained from the Linux world so far, seems to me to 
> 
>>> One thing I don't see right now is, in the in-kernel case, what we
>>> should do when finding the "iomem=[]" option in a config file.
>>
>> Even for an x86 HVM guest with iomem there is no call to
>> xc_domain_memory_mapping (even from qemu) it is called only for PCI
>> passthrough. I've no idea if/how iomem=[] works for x86 HVM guests.
>> Based on a cursory glance it looks to me like it wouldn't and if it did
>> work it would have the same problems wrt where to map it as we have
>> today with the ARM guests, except perhaps on a PC the sorts of things
>> you would pass through with this can be done 1:1 because they are legacy
>> PC peripherals etc.
>>
> Aha, interesting! As said to Julien, I tried to look for how these iomem
> regions get to the xc_domain_memory_map in QEMU and I also found nothing
> (except for PCI passthrough stuff)... I was assuming I was missing
> something... apparently, I wasn't. :-)
> 
> I can try (even just out of curiosity!) to dig in the tree and see what
> happened with this feature...
> 
> BTW, doesn't this make this discussion even more relevant? I mean, if
> it's there but it's not working, shouldn't we make it work (for some
> definition of "work"), if we decide it's useful, or kill it, if not?
> 
>> I think we just don't know what the right answer is yet and I'm unlikely
>> to be able to say directly what the right answer is. I'm hoping that
>> people who want to use this low level functionality can provide a
>> consistent story for how it should work (a "design" if you like) to
>> which I can say "yes, that seems sensible" or "hmm, that seems odd
>> because of X". At the moment X is "the 1:1 mapping seems undesirable to
>> me". There have been some suggestions for how to fix that, someone with
>> a horse in the race should have a think about it and provide an
>> iteration on the design until we are happy with it.
>>
> Again, I'll look more into the history of this feature.
> 
> In the meantime, the man page entry says:
> 
> "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.
> 
> It is recommended to use this option only for trusted VMs under
> administrator control."
> 
> For one, the "Allow guest to access" there leaves a lot of room for
> interpretation, I think. It doesn't say anything about mapping, so one
> can interpret it as 'this only grant you mapping permission, up to you
> to map'. However, it says "access", so one can interpret it as 'if I can
> access it, it's mapped already'.
> 
> Wherever this discussion will land, I think that, if we keep this
> option, we should make the documentation less ambiguous.
> 
> That being said, allow me to play, in the rest of this e-mail, the role
> of one which expects the mapping to be actually instated by just
> specifying "iomem=[]" in the config file, which is (correct me guys if
> I'm wrong) what Eric, Viktor and Arianna thought when reading the entry
> above.
> 
> So:
>  1. it says nothing about where the mapping ends up in guest's memory,
>  2. it looks quite clear (to me?) that this is a raw/low level feature,
>     to be used under controlled conditions (which an embedded product
>     often is)
> 
> To me, a legitimate use case is this: I want to run version X of my non
> DT capable OS on version Z of Xen, on release K of board B. In such
> configuration, the GPIO controller is at MFN 0x000abcd, and I want only
> VM V to have direct access to it (board B dos not have an IOMMU).
> 
> I would also assume that one is in full control of the guest address
> space, so it is be ok to hardcode the addresses of registers somewhere.
> Of course, that may be an issue, when putting Xen underneath, as Xen's
> mangling with the such address space can cause troubles.
> 
> Arianna, can you confirm that this is pretty much the case of Erika, or
> amend what I did get wrong?
> 

I confirm that this is the case of ERIKA Enterprise, whose domU port is intended
to have exclusive access to some peripherals' I/O memory ranges. I also confirm
that, as you wrote, ERIKA doesn't currently support DT parsing and only relies
on hardcoded I/O memory addresses.

> I certainly don't claim to have the right answer but, in the described
> scenario, either:
>  1) the combination of iomem=[ MFN,NR@PFN ]", defaulting to 1:1 if  
>     "@PFN is missing, and e820_host
>  2) calling (the equivalent of) XEN_DOMCTL_memory_map from the guest 
>     kernel
> 
> would be good solutions, to the point that I think we could even support
> both. The main point being that, I think, even in the worst case, any
> erroneous usage of either, would "just" destroy the guest, and that's
> acceptable.
> 
> Actually, if going for 1), I think (when both the pieces are there)
> things should be pretty safe. Furthermore, implementing 1) seems to me
> the only way of having the "iomem=[]" parameter causing both permissions
> granting and mapping. Downside is (libxl side of) the implementation
> indeed looks cumbersome.
> 
> If going for _only_ 2), then "iomem=[]" would just be there to ensure
> the future mapping operation to be successful, i.e., for granting
> mapping rights, as it's doing right now. It would be up to the guest
> kernel to make sure the MFN it is trying to map are consistent with what
> was specified in "iomem=[]". Given the use case we're talking about, I
> don't think this is an unreasonable request, as far as we make the iomem
> man entry more clearly stating this.
> 
> Again, Arianna, do you confirm both solution are possible for you?
> 

Yes, I believe both solutions are applicable as far as it concerns my use case.

> I certainly agree that the thread could benefit from some opinion from
> people actually wanting to use this. In addition to Arianna, I have
> pinged Eirc and Viktor repeatedly... let's see if they have time to let
> us all know something more about their own needs and requirements wrt
> this.
> 
>>> Also, just trying to recap, for Arianna's sake, moving the
>>> implementation of the DOMCTL in common code (and implementing the
>>> missing bits to make it works properly, of course) is still something we
>>> want, right?
>>
>> *If* we go the route of having the kernel make the mapping then there is
>> no need, is there?
>>
> Yeah, well, me not being sure is the reason I asked... And Julien said
> he thinks we still want it... :-P
> 
> As I was saying above, I think there is room for both, but I don't mind
> picking up one. However, if we want to fix iomem=[] and go as far as
> having it doing the mapping, then I think we all agree we need the
> DOMCTL.
> 
> So, looks like the discussion resolves to something like:
>  - do we need the DOMCTL for other purposes than iomem=[] ?
>  - if no, what do we want to do with iomem=[] ?
> 
> Sorry to have brought you all deep down into this can of worms! :-/
> 
> Regards,
> Dario
> 


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

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

[Ian Campbell]

On Fri, 2014-03-14 at 13:15 +0100, Dario Faggioli wrote:
> > > In Arianna's case, it think it would be more than fine to implement it
> > > that way, and call it from within the OS, isn't this the case, Arianna?
> > 
> > It's certainly an option, and it would make a lot of the toolstack side
> > issues moot but I'm not at all sure it is the right answer. In
> > particular although it might be easy to bodge a mapping into many OSes I
> > can imagine getting such a think into something generic like Linux might
> > be more tricky -- in which case perhaps the toolstack should be taking
> > care of it, and that does have a certain appeal from the simplicity of
> > the guest interface side of things.
> > 
> If the toolstack needs to support iomem, yes.
> 
> A way to see it could be that, as of now, we have embedded OSes asking
> for it already, and, at the same time, it's unlikely that more generic
> system such as Linux would want something similar, for one because
> they'll have full DT/ACPI support for this.
> 
> The fact that, if what you say below is true, "iomem" does not work at
> all, and no one complained from the Linux world so far, seems to me to 

iomem *does* work just fine for x86 PV guests, which is what it was
added for. Apparently it was never extended to HVM guests.

> > > One thing I don't see right now is, in the in-kernel case, what we
> > > should do when finding the "iomem=[]" option in a config file.
> > 
> > Even for an x86 HVM guest with iomem there is no call to
> > xc_domain_memory_mapping (even from qemu) it is called only for PCI
> > passthrough. I've no idea if/how iomem=[] works for x86 HVM guests.
> > Based on a cursory glance it looks to me like it wouldn't and if it did
> > work it would have the same problems wrt where to map it as we have
> > today with the ARM guests, except perhaps on a PC the sorts of things
> > you would pass through with this can be done 1:1 because they are legacy
> > PC peripherals etc.
> > 
> Aha, interesting! As said to Julien, I tried to look for how these iomem
> regions get to the xc_domain_memory_map in QEMU and I also found nothing
> (except for PCI passthrough stuff)... I was assuming I was missing
> something... apparently, I wasn't. :-)
> 
> I can try (even just out of curiosity!) to dig in the tree and see what
> happened with this feature...
> 
> BTW, doesn't this make this discussion even more relevant? I mean, if
> it's there but it's not working, shouldn't we make it work (for some
> definition of "work"), if we decide it's useful, or kill it, if not?

Apparently no one is using this with x86 HVM guests, so apparently no
one cares. If there are users then we should fix it. Maybe we will fix
this as a side effect of making it work on ARM, I don't know.

> > I think we just don't know what the right answer is yet and I'm unlikely
> > to be able to say directly what the right answer is. I'm hoping that
> > people who want to use this low level functionality can provide a
> > consistent story for how it should work (a "design" if you like) to
> > which I can say "yes, that seems sensible" or "hmm, that seems odd
> > because of X". At the moment X is "the 1:1 mapping seems undesirable to
> > me". There have been some suggestions for how to fix that, someone with
> > a horse in the race should have a think about it and provide an
> > iteration on the design until we are happy with it.
> > 
> Again, I'll look more into the history of this feature.
> 
> In the meantime, the man page entry says:
> 
> "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.
> 
> It is recommended to use this option only for trusted VMs under
> administrator control."
> 
> For one, the "Allow guest to access" there leaves a lot of room for
> interpretation, I think.

Not if you think about it in terms of being a PV guest option, where the
mapping just happens when the guest makes a PTE to map it.

Probably this option is currently misplaced in the man page, it should
be PV specific.

>  It doesn't say anything about mapping, so one
> can interpret it as 'this only grant you mapping permission, up to you
> to map'. However, it says "access", so one can interpret it as 'if I can
> access it, it's mapped already'.
> 
> Wherever this discussion will land, I think that, if we keep this
> option, we should make the documentation less ambiguous.
> 
> That being said, allow me to play, in the rest of this e-mail, the role
> of one which expects the mapping to be actually instated by just
> specifying "iomem=[]" in the config file, which is (correct me guys if
> I'm wrong) what Eric, Viktor and Arianna thought when reading the entry
> above.
> 
> So:
>  1. it says nothing about where the mapping ends up in guest's memory,

it currently says nothing because it is a PV feature.

>  2. it looks quite clear (to me?) that this is a raw/low level feature,
>     to be used under controlled conditions (which an embedded product
>     often is)

Correct.

> To me, a legitimate use case is this: I want to run version X of my non
> DT capable OS on version Z of Xen, on release K of board B. In such
> configuration, the GPIO controller is at MFN 0x000abcd, and I want only
> VM V to have direct access to it (board B dos not have an IOMMU).
> 
> I would also assume that one is in full control of the guest address

If "one" here is the user then I don't think so.

A given version of Xen will provide a particular memory layout to the
guest. If you want to run non-single image OSes (i.e. things without
device tree) on Xen then you will need to build a specific kernel binary
for that version of Xen hardcoding the particular layout of that version
of Xen. If you upgrade Xen then you will need to rebuild your guest to
use the correct address layout.

If the user doesn't want that, i.e. they want a single binary to run on
multiple versions of Xen, then they had better implement device tree
support in their kernel.

As a second aspect of this it means that the user will need to change
their config file to update their iomem mappings to new safe values when
they change the version of Xen. This is possibly even true if they use
device tree (in which case maybe we want a way for them to add
descriptions of these things to dt).

> space, so it is be ok to hardcode the addresses of registers somewhere.
> Of course, that may be an issue, when putting Xen underneath, as Xen's
> mangling with the such address space can cause troubles.
> 
> Arianna, can you confirm that this is pretty much the case of Erika, or
> amend what I did get wrong?
> 
> I certainly don't claim to have the right answer but, in the described
> scenario, either:
>  1) the combination of iomem=[ MFN,NR@PFN ]", defaulting to 1:1 if  
>     "@PFN is missing, and e820_host
>  2) calling (the equivalent of) XEN_DOMCTL_memory_map from the guest 
>     kernel
> 
> would be good solutions, to the point that I think we could even support
> both. The main point being that, I think, even in the worst case, any
> erroneous usage of either, would "just" destroy the guest, and that's
> acceptable.

I don't think we want both and I'm leaning towards #1 right now, but
with the e820_host thing being unnecessary in the first instance.

> Actually, if going for 1), I think (when both the pieces are there)
> things should be pretty safe. Furthermore, implementing 1) seems to me
> the only way of having the "iomem=[]" parameter causing both permissions
> granting and mapping. Downside is (libxl side of) the implementation
> indeed looks cumbersome.
> 
> If going for _only_ 2), then "iomem=[]" would just be there to ensure
> the future mapping operation to be successful, i.e., for granting
> mapping rights, as it's doing right now. It would be up to the guest
> kernel to make sure the MFN it is trying to map are consistent with what
> was specified in "iomem=[]". Given the use case we're talking about, I
> don't think this is an unreasonable request, as far as we make the iomem
> man entry more clearly stating this.

My worry with this one is that it makes might make it harder to DTRT in
the future, e.g. by adding device tree nodes to represent things mapped
with iomem=[], by committing us to a world where the guest makes these
mappings and not the tools.

> Again, Arianna, do you confirm both solution are possible for you?
> 
> I certainly agree that the thread could benefit from some opinion from
> people actually wanting to use this. In addition to Arianna, I have
> pinged Eirc and Viktor repeatedly... let's see if they have time to let
> us all know something more about their own needs and requirements wrt
> this.
> 
> > > Also, just trying to recap, for Arianna's sake, moving the
> > > implementation of the DOMCTL in common code (and implementing the
> > > missing bits to make it works properly, of course) is still something we
> > > want, right?
> > 
> > *If* we go the route of having the kernel make the mapping then there is
> > no need, is there?
> > 
> Yeah, well, me not being sure is the reason I asked... And Julien said
> he thinks we still want it... :-P

The answer to this question will necessarily depend on the design
decisions made above. It will either be needed or not.

> As I was saying above, I think there is room for both, but I don't mind
> picking up one. However, if we want to fix iomem=[] and go as far as
> having it doing the mapping, then I think we all agree we need the
> DOMCTL.
> 
> So, looks like the discussion resolves to something like:
>  - do we need the DOMCTL for other purposes than iomem=[] ?
>  - if no, what do we want to do with iomem=[] ?

Please come up with a design which answers this, I've given my opinions
above but if you think some other design is better then argue for it.

> 
> Sorry to have brought you all deep down into this can of worms! :-/
> 
> Regards,
> Dario
> 

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

[Stefano Stabellini]

On Fri, 14 Mar 2014, Ian Campbell wrote:
> > Of course, that may be an issue, when putting Xen underneath, as Xen's
> > mangling with the such address space can cause troubles.
> > 
> > Arianna, can you confirm that this is pretty much the case of Erika, or
> > amend what I did get wrong?
> > 
> > I certainly don't claim to have the right answer but, in the described
> > scenario, either:
> >  1) the combination of iomem=[ MFN,NR@PFN ]", defaulting to 1:1 if  
> >     "@PFN is missing, and e820_host
> >  2) calling (the equivalent of) XEN_DOMCTL_memory_map from the guest 
> >     kernel
> > 
> > would be good solutions, to the point that I think we could even support
> > both. The main point being that, I think, even in the worst case, any
> > erroneous usage of either, would "just" destroy the guest, and that's
> > acceptable.
> 
> I don't think we want both and I'm leaning towards #1 right now, but
> with the e820_host thing being unnecessary in the first instance.

1) looks like the better option to me


> > Actually, if going for 1), I think (when both the pieces are there)
> > things should be pretty safe. Furthermore, implementing 1) seems to me
> > the only way of having the "iomem=[]" parameter causing both permissions
> > granting and mapping. Downside is (libxl side of) the implementation
> > indeed looks cumbersome.
> > 
> > If going for _only_ 2), then "iomem=[]" would just be there to ensure
> > the future mapping operation to be successful, i.e., for granting
> > mapping rights, as it's doing right now. It would be up to the guest
> > kernel to make sure the MFN it is trying to map are consistent with what
> > was specified in "iomem=[]". Given the use case we're talking about, I
> > don't think this is an unreasonable request, as far as we make the iomem
> > man entry more clearly stating this.
> 
> My worry with this one is that it makes might make it harder to DTRT in
> the future, e.g. by adding device tree nodes to represent things mapped
> with iomem=[], by committing us to a world where the guest makes these
> mappings and not the tools.

Yeah, also it requires more guest kernel modifications that might not
always be possible.

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

[Dario Faggioli]

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

On ven, 2014-03-14 at 12:49 +0000, Ian Campbell wrote:
> On Fri, 2014-03-14 at 13:15 +0100, Dario Faggioli wrote:
> > The fact that, if what you say below is true, "iomem" does not work at
> > all, and no one complained from the Linux world so far, seems to me to 
> 
> iomem *does* work just fine for x86 PV guests, which is what it was
> added for. Apparently it was never extended to HVM guests.
> 
Ok, understood.

> > For one, the "Allow guest to access" there leaves a lot of room for
> > interpretation, I think.
> 
> Not if you think about it in terms of being a PV guest option, where the
> mapping just happens when the guest makes a PTE to map it.
> 
I see it now.

> Probably this option is currently misplaced in the man page, it should
> be PV specific.
> 
That seems a good thing to do.

> > To me, a legitimate use case is this: I want to run version X of my non
> > DT capable OS on version Z of Xen, on release K of board B. In such
> > configuration, the GPIO controller is at MFN 0x000abcd, and I want only
> > VM V to have direct access to it (board B dos not have an IOMMU).
> > 
> > I would also assume that one is in full control of the guest address
> 
> If "one" here is the user then I don't think so.
> 
Well, fact is "user", in this context, is who puts the embedded system
together into a product, rather than folks actually using such product,
so, I don't see that much unlikely.

> A given version of Xen will provide a particular memory layout to the
> guest. If you want to run non-single image OSes (i.e. things without
> device tree) on Xen then you will need to build a specific kernel binary
> for that version of Xen hardcoding the particular layout of that version
> of Xen. If you upgrade Xen then you will need to rebuild your guest to
> use the correct address layout.
> 
Which sounds really bad, but at the same time is right the case, in the
most of the embedded scenarios I've been in touch with.

> If the user doesn't want that, i.e. they want a single binary to run on
> multiple versions of Xen, then they had better implement device tree
> support in their kernel.
> 
I totally agree. :-)

> > I certainly don't claim to have the right answer but, in the described
> > scenario, either:
> >  1) the combination of iomem=[ MFN,NR@PFN ]", defaulting to 1:1 if  
> >     "@PFN is missing, and e820_host
> >  2) calling (the equivalent of) XEN_DOMCTL_memory_map from the guest 
> >     kernel
> > 
> > would be good solutions, to the point that I think we could even support
> > both. The main point being that, I think, even in the worst case, any
> > erroneous usage of either, would "just" destroy the guest, and that's
> > acceptable.
> 
> I don't think we want both and I'm leaning towards #1 right now, but
> with the e820_host thing being unnecessary in the first instance.
> 
Well, perfect then, that's what I argued for too, since the beginning of
this thread. :-)

> > If going for _only_ 2), then "iomem=[]" would just be there to ensure
> > the future mapping operation to be successful, i.e., for granting
> > mapping rights, as it's doing right now. It would be up to the guest
> > kernel to make sure the MFN it is trying to map are consistent with what
> > was specified in "iomem=[]". Given the use case we're talking about, I
> > don't think this is an unreasonable request, as far as we make the iomem
> > man entry more clearly stating this.
> 
> My worry with this one is that it makes might make it harder to DTRT in
> the future, e.g. by adding device tree nodes to represent things mapped
> with iomem=[], by committing us to a world where the guest makes these
> mappings and not the tools.
> 
Again, I completely agree.

> > As I was saying above, I think there is room for both, but I don't mind
> > picking up one. However, if we want to fix iomem=[] and go as far as
> > having it doing the mapping, then I think we all agree we need the
> > DOMCTL.
> > 
> > So, looks like the discussion resolves to something like:
> >  - do we need the DOMCTL for other purposes than iomem=[] ?
> >  - if no, what do we want to do with iomem=[] ?
> 
> Please come up with a design which answers this, I've given my opinions
> above but if you think some other design is better then argue for it.
> 
Not at all, I concur with you. I like it because a guest kernel, which
is compiled to find some device registers at a certain address, can just
go ahead and use them without any further modification. In fact,
specifying what these address are, is usually quite simple. It would
require rebuilding, but there are config/board files, etc... There are a
few that even have nice graphical frontends for this (and I think ERIKA
Enterprise has one too). Having to issue the physmap call would not be
terrible in this case, as we're rebuilding anyway, but it's certainly
more modification.

I also agree with you when you say that this leaves us in a better
position for future decisions.

Finally, it looks to me as a more consistent extension of current
iomem's behavior, in the pure x86 PV case.

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

[Ian Campbell]

On Fri, 2014-03-14 at 16:45 +0100, Dario Faggioli wrote:

> > > To me, a legitimate use case is this: I want to run version X of my non
> > > DT capable OS on version Z of Xen, on release K of board B. In such
> > > configuration, the GPIO controller is at MFN 0x000abcd, and I want only
> > > VM V to have direct access to it (board B dos not have an IOMMU).
> > > 
> > > I would also assume that one is in full control of the guest address
> > 
> > If "one" here is the user then I don't think so.
> > 
> Well, fact is "user", in this context, is who puts the embedded system
> together into a product, rather than folks actually using such product,
> so, I don't see that much unlikely.

I don't imagine even those folks are going to want to hack the
hypervisor to change the guest address space layout, so I don't think
you can assume they have full control, they will get whatever the layout
for that version of Xen is. They do control the hypervisor version so I
suppose they control in that sense.

If they *are* hacking the hypervisor to fiddle with the guest addres
space then they have no need for this toolstack integration feature,
they can (and likely will) just bodge it in the same place.

Ian.

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

[Dario Faggioli]

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

On ven, 2014-03-14 at 16:19 +0000, Ian Campbell wrote:
> On Fri, 2014-03-14 at 16:45 +0100, Dario Faggioli wrote:

> > > If "one" here is the user then I don't think so.
> > > 
> > Well, fact is "user", in this context, is who puts the embedded system
> > together into a product, rather than folks actually using such product,
> > so, I don't see that much unlikely.
> 
> I don't imagine even those folks are going to want to hack the
> hypervisor to change the guest address space layout, so I don't think
> you can assume they have full control, they will get whatever the layout
> for that version of Xen is. 
>
Don't want to speak for someone else, of course. For what I've seen, no,
they're not going to hack Xen, they're going to stick to a specific
version as long as possible.

For different reasons, that's not much different from what happens in
way less embedded contexts, I think. :-)

> They do control the hypervisor version so I
> suppose they control in that sense.
> 
Indeed that was the control I meant.

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

[Eric Trudeau]

> -----Original Message-----
> From: Dario Faggioli [mailto:dario.faggioli@citrix.com]
> Sent: Thursday, March 13, 2014 2:38 PM
> To: Ian Campbell
> Cc: Julien Grall; Arianna Avanzini; paolo.valente@unimore.it; Keir Fraser;
> stefano.stabellini@eu.citrix.com; Tim Deegan; Ian.Jackson@eu.citrix.com; xen-
> devel@lists.xen.org; julien.grall@citrix.com; Eric Trudeau; Jan Beulich;
> viktor.kleinik@globallogic.com
> Subject: Re: [Xen-devel] [RFC PATCH v2 3/3] tools, libxl: handle the iomem
> parameter with the memory_mapping hcall
> 
> On gio, 2014-03-13 at 17:32 +0000, Ian Campbell wrote:
> > On Thu, 2014-03-13 at 16:47 +0000, Julien Grall wrote:
> 
> > > > Sure, but I don't think I see any conflict between this and the approach
> > > > Ian proposed above. What am I missing?
> > >
> > > I believe, Ian was assuming that the user know the layout because this
> > > solution will be used to a very specific case (I think mostly when the
> > > device tree won't describe the hardware).
> >
> > Right, my assumption was that the kernel was also compiled for the exact
> > hardware layout as part of some sort of embedded/appliance situation...
> >
> Exactly, that may very well be the case. It may not, in Arianna's case,
> but it well can be true for others, or even for her, in future.
> 
> Therefore, I keep failing to see why to prevent this to be the case.
> 
> > > I'm wondering, if we can let the kernel calling the hypercall. He knows
> > > what is the memory layout of the VM.
> >
> > This would be somewhat analogous to what happens with an x86 PV guest.
> > It would have to be an physmap call or something since this domctl
> > wouldn't be accessible by the guest.
> >
> > That makes a lot of sense actually since this domctl seems to have been
> > intended for use by the x86 HVM device model (qemu).
> >
> I thought about that too. The reason why this was the taken approach is
> this xen-devel discussion:
> http://lists.xen.org/archives/html/xen-devel/2013-06/msg00870.html
> 
> in particular, this Julien's message:
> http://lists.xen.org/archives/html/xen-devel/2013-06/msg00902.html
> 
> Also, Eric and Viktor where asking for/working on something similar, so
> there perhaps would be some good in having this...
> 
> Eric, Viktor, can you comment why you need this call and how you use, or
> want to use it for?
> Would it be the same for you to have it in the form of a physmap call,
> and invoke it from within the guest kernel?

In our target scenarios, we are relying on the Virtualization Extensions in ARM for
security as well as for segregating Linux "machines" in order to share CPU resources
more effectively.
Therefore, we do not want the guest domain to decide the IPA->PA mapping, Dom0
is in charge of this through hypercalls to Xen.  We like the iomem in the config file,
and we use the irq parameter as well to route IRQs to guests, because it allows
Dom0 only to control how the physical devices are assigned to the DomU's. This
also keeps the guest OS drivers from having to change when they are running in a
virtual machine.

In our use cases, we want primarily 1:1 mapping for most memory because we have
no IOMMU and each DomU can do DMA and needs 1:1 contiguous memory for that.

In our development port of Xen, we do not have the DomU devices in the DT given
to Xen at boot.  Therefore, Dom0 does not see these devices and the Dom0 Linux
probe does not try to init the devices.  Our near-future plans are to add the devices
targeted for DomU into the Xen DT, but with status property set to disabled.  Dom0
will use some domain plan to create DTs for the DomUs by assigning the appropriate
devices to each and enabling the devices that were disabled in Dom0's DT.

This is probably more information that you wanted, but it may help in a better
understanding of our usage of Xen.

> 
> In Arianna's case, it think it would be more than fine to implement it
> that way, and call it from within the OS, isn't this the case, Arianna?
> 
> One thing I don't see right now is, in the in-kernel case, what we
> should do when finding the "iomem=[]" option in a config file.
> 
> Also, just trying to recap, for Arianna's sake, moving the
> implementation of the DOMCTL in common code (and implementing the
> missing bits to make it works properly, of course) is still something we
> want, right?
> 
> 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 v2 3/3] tools, libxl: handle the iomem parameter with the memory_mapping hcall

[Ian Campbell]

On Fri, 2014-03-14 at 18:39 +0000, Eric Trudeau wrote:
> In our target scenarios, we are relying on the Virtualization Extensions in ARM for
> security as well as for segregating Linux "machines" in order to share CPU resources
> more effectively.
> Therefore, we do not want the guest domain to decide the IPA->PA mapping, Dom0
> is in charge of this through hypercalls to Xen.

Note that in no case would the guest be able to map arbitrary PA, each
PA still needs to be whitelisted by the tools before it can be mapped to
the guest whether it is the tools or the guest which actually
establishes the mappings.

>   We like the iomem in the config file,
> and we use the irq parameter as well to route IRQs to guests, because it allows
> Dom0 only to control how the physical devices are assigned to the DomU's. This
> also keeps the guest OS drivers from having to change when they are running in a
> virtual machine.
> 
> In our use cases, we want primarily 1:1 mapping for most memory because we have
> no IOMMU and each DomU can do DMA and needs 1:1 contiguous memory for that.

Lack of an IOMMU and the presence of guest DMA means that things are not
going to be completely isolated though.

> In our development port of Xen, we do not have the DomU devices in the DT given
> to Xen at boot.  Therefore, Dom0 does not see these devices and the Dom0 Linux
> probe does not try to init the devices.  Our near-future plans are to add the devices
> targeted for DomU into the Xen DT, but with status property set to disabled.  Dom0
> will use some domain plan to create DTs for the DomUs by assigning the appropriate
> devices to each and enabling the devices that were disabled in Dom0's DT.
> 
> This is probably more information that you wanted, but it may help in a better
> understanding of our usage of Xen.

Thanks.

Ian.

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

[Jan Beulich]

>>> On 13.03.14 at 16:27, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Mon, 2014-03-10 at 09:25 +0100, Arianna Avanzini wrote:
>> Currently, the configuration-parsing code concerning the handling of the
>> iomem parameter only invokes the XEN_DOMCTL_iomem_permission hypercall.
>> This commit lets the XEN_DOMCTL_memory_mapping hypercall be invoked
>> after XEN_DOMCTL_iomem_permission when the iomem parameter is parsed
>> from a domU configuration file, so that the address range can be mapped
>> to the address space of the domU.
>> NOTE: the added code is still common to both x86 and ARM; it also
>>       implements a simple 1:1 mapping that could clash with the domU's
>>       existing memory layout if the range is already in use in the
>>       guest's address space.
> 
> In that case you need to CC the x86 maintainers (Jan, Keir, Tim) here.
> It doesn't seem to me that this is going to be the correct thing to do
> for either x86 PV or x86 HVM guests.
> 
> My gut feeling is that this should be ifdef'd or otherwise made
> conditional.

At the very least - it really looks more like a temporary hack than
a long term solution to me. Why would we ever want, for other
than experimental purposes, a 1:1 address relationship baked
into anything?

Jan

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

[Ian Campbell]

On Thu, 2014-03-13 at 15:43 +0000, Jan Beulich wrote:
> >>> On 13.03.14 at 16:27, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Mon, 2014-03-10 at 09:25 +0100, Arianna Avanzini wrote:
> >> Currently, the configuration-parsing code concerning the handling of the
> >> iomem parameter only invokes the XEN_DOMCTL_iomem_permission hypercall.
> >> This commit lets the XEN_DOMCTL_memory_mapping hypercall be invoked
> >> after XEN_DOMCTL_iomem_permission when the iomem parameter is parsed
> >> from a domU configuration file, so that the address range can be mapped
> >> to the address space of the domU.
> >> NOTE: the added code is still common to both x86 and ARM; it also
> >>       implements a simple 1:1 mapping that could clash with the domU's
> >>       existing memory layout if the range is already in use in the
> >>       guest's address space.
> > 
> > In that case you need to CC the x86 maintainers (Jan, Keir, Tim) here.
> > It doesn't seem to me that this is going to be the correct thing to do
> > for either x86 PV or x86 HVM guests.
> > 
> > My gut feeling is that this should be ifdef'd or otherwise made
> > conditional.
> 
> At the very least - it really looks more like a temporary hack than
> a long term solution to me. Why would we ever want, for other
> than experimental purposes, a 1:1 address relationship baked
> into anything?

Indeed, although that's slightly secondary to whether these call make
sense at all for x86 regardless of whether they are 1:1 or not.

Ian.

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

[Dario Faggioli]

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

On gio, 2014-03-13 at 15:43 +0000, Jan Beulich wrote:
> >>> On 13.03.14 at 16:27, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Mon, 2014-03-10 at 09:25 +0100, Arianna Avanzini wrote:
> >> NOTE: the added code is still common to both x86 and ARM; it also
> >>       implements a simple 1:1 mapping that could clash with the domU's
> >>       existing memory layout if the range is already in use in the
> >>       guest's address space.
> > 
> > In that case you need to CC the x86 maintainers (Jan, Keir, Tim) here.
> > It doesn't seem to me that this is going to be the correct thing to do
> > for either x86 PV or x86 HVM guests.
> > 
> > My gut feeling is that this should be ifdef'd or otherwise made
> > conditional.
> 
> At the very least - it really looks more like a temporary hack than
> a long term solution to me. Why would we ever want, for other
> than experimental purposes, a 1:1 address relationship baked
> into anything?
> 
We discussed a bit about this during v1's submission of this series.
Some pointers here:

http://lists.xenproject.org/archives/html/xen-devel/2014-03/msg00036.html
http://lists.xenproject.org/archives/html/xen-devel/2014-03/msg00054.html

Summarizing, the idea is allowing for some kind of "raw device
passthrough" for the cases where:
 - there is no IOMMU in the hw
 - the OS does not support DT or ACPI

Which is the case of Arianna's port and, I believe, it may be something
other people wanting to port small and special purpose/embedded OSes on
Xen would face too (perhaps Eric and Viktor can add something about
their own use case).

AFAIUI, once you settle on allowing it and bypassing DT parsing, then
the point becomes _where_ to put the mapping. 1:1 looked more the only
than the best option, although it is of course at risk of clashes with
other stuff put there by Xen.

For this reason, we decided that having both 1:1 mapping, and an
equivalent of x86's e820_host for ARM would be a good enough solution.
Of course, it's responsibility to the user/sysadmin to provide the
appropriate set of options... or get to keep the pieces, if they
don't. :-)

The agreement was that Arianna would keep on implementing this, with the
1:1 mapping. A follow-up work (from either her or someone else, e.g.,
Julien said he could be up for it at some point) would add the e820-ish
part.

Personally, I think I agree with Ian about still defaulting to 1:1, but
also allowing for a bit more of flexibility, should mapping at a
specific PFN ever become a thing. Especially, I don't see much harm in
this (either the flexible or the unflexible variant), except for people
abusing this possibility, but again, that's up to them (I guess it's the
good old "should we allow users to shoot in their foot?" thing.)

Hope this clarified things a bit...

Do you happen to see any alternative solution, in absence of proper
DT/ACPI support in the guest OS? If no, what would be the best solution
to keep this out of x86 way? Is #ifdef, as Ian's suggesting, fine?

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

[Jan Beulich]

>>> On 13.03.14 at 17:53, Dario Faggioli <dario.faggioli@citrix.com> wrote:
> Do you happen to see any alternative solution, in absence of proper
> DT/ACPI support in the guest OS? If no, what would be the best solution
> to keep this out of x86 way? Is #ifdef, as Ian's suggesting, fine?

Yes, but I'd strongly suggest using a white list approach rather than
a blacklist one here (which of course only matters if there'll ever be
a port to another architecture).

Jan