[PATCH v2 00/10] xen/arm: Handle correctly foreign mapping

[PATCH v2 00/10] xen/arm: Handle correctly foreign mapping

[Julien Grall]

Hello,

This patch series aims to fix "Failed to unmap" message in dom0 when a guest is
creating. Without this patch series, dom0 will leak memory each time a domain
is created. It should be considered as a blocker for Xen 4.4 release.

The series is based on pvh dom0 v6 patch series from Mukesh with patch #7
(http://lists.xenproject.org/archives/html/xen-devel/2013-12/msg01026.html)
replaced by the version V6.1
(http://lists.xenproject.org/archives/html/xen-devel/2013-12/msg01168.html)

    - Patch #1-2: prepare work for the others patches
    - Patch #3-6: add support for p2m type
    - Patch #7-9: handle correctly foreign mapping
    - Patch #10: it's not really part of this series. It adds support
    for read-only grant-mapping

All the patches can be found on this git:
    git clone -b map-foreign-v2 git://xenbits.xen.org/people/julieng/xen-unstable.git

For all the changes, see in each patch.

Sincerely yours,

Cc: George Dunlap <george.dunlap@eu.citrix.com>

Julien Grall (10):
  xen/arm: Introduce steps in domain_relinquish_resource
  xen/arm: move mfn_to_p2m_entry in arch/arm/p2m.c
  xen/arm: Implement p2m_type_t as an enum
  xen/arm: Store p2m type in each page of the guest
  xen/arm: p2m: Extend p2m_lookup parameters to retrieve the p2m type
  xen/arm: Retrieve p2m type in get_page_from_gfn
  xen/arm: Introduce relinquish_p2m_mapping to remove refcount every
    mapped page
  xen/arm: Implement xen_rem_foreign_from_p2m
  xen/arm: Set foreign page type to p2m_map_foreign
  xen/arm: grant-table: Support read-only mapping

 xen/arch/arm/domain.c        |   42 ++++++++++---
 xen/arch/arm/mm.c            |   52 +++++++++++-----
 xen/arch/arm/p2m.c           |  136 +++++++++++++++++++++++++++++++++++++-----
 xen/arch/arm/traps.c         |    6 +-
 xen/include/asm-arm/domain.h |    9 +++
 xen/include/asm-arm/p2m.h    |   65 ++++++++++++++++----
 xen/include/asm-arm/page.h   |   24 +-------
 7 files changed, 258 insertions(+), 76 deletions(-)

-- 
1.7.10.4

[PATCH v2 01/10] xen/arm: Introduce steps in domain_relinquish_resource

[Julien Grall]

In a later patch, a new step will be added. It will avoid to check every step
when the function was preempted.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Introduce the patch
---
 xen/arch/arm/domain.c        |   37 ++++++++++++++++++++++++++++++-------
 xen/include/asm-arm/domain.h |    8 ++++++++
 2 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 67c65c3..1590708 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -497,6 +497,8 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags)
 {
     int rc;
 
+    d->arch.relmem = RELMEM_not_started;
+
     /* Idle domains do not need this setup */
     if ( is_idle_domain(d) )
         return 0;
@@ -696,15 +698,36 @@ int domain_relinquish_resources(struct domain *d)
 {
     int ret = 0;
 
-    ret = relinquish_memory(d, &d->xenpage_list);
-    if ( ret )
-        return ret;
+    switch ( d->arch.relmem )
+    {
+    case RELMEM_not_started:
+        d->arch.relmem = RELMEM_xen;
+        /* Falltrough */
 
-    ret = relinquish_memory(d, &d->page_list);
-    if ( ret )
-        return ret;
+    case RELMEM_xen:
+        ret = relinquish_memory(d, &d->xenpage_list);
+        if ( ret )
+            return ret;
 
-    return ret;
+        d->arch.relmem = RELMEM_page;
+        /* Fallthrough */
+
+    case RELMEM_page:
+        ret = relinquish_memory(d, &d->page_list);
+        if ( ret )
+            return ret;
+
+        d->arch.relmem = RELMEM_done;
+        /* Fallthrough */
+
+    case RELMEM_done:
+        break;
+
+    default:
+        BUG();
+    }
+
+    return 0;
 }
 
 void arch_dump_domain_info(struct domain *d)
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 8ebee3e..922eda3 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -70,6 +70,14 @@ struct arch_domain
     struct hvm_domain hvm_domain;
     xen_pfn_t *grant_table_gpfn;
 
+    /* Continuable domain_relinquish_resources(). */
+    enum {
+        RELMEM_not_started,
+        RELMEM_xen,
+        RELMEM_page,
+        RELMEM_done,
+    } relmem;
+
     /* Virtual CPUID */
     uint32_t vpidr;
 
-- 
1.7.10.4

Re: [PATCH v2 01/10] xen/arm: Introduce steps in domain_relinquish_resource

[Ian Campbell]

On Mon, 2013-12-09 at 03:33 +0000, Julien Grall wrote:
> In a later patch, a new step will be added. It will avoid to check every step
> when the function was preempted.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH v2 02/10] xen/arm: move mfn_to_p2m_entry in arch/arm/p2m.c

[Julien Grall]

The function mfn_to_p2m_entry will be extended in a following patch to handle
p2m_type_t. It will break compilation because p2m_type_t is not defined
(interdependence between includes).
It's easier to move the function in arch/arm/p2m.c and it's not harmful as the
function is only used in this file.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/p2m.c         |   22 ++++++++++++++++++++++
 xen/include/asm-arm/page.h |   22 ----------------------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 1d5c841..8f8b47e 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -124,6 +124,28 @@ int p2m_pod_decrease_reservation(struct domain *d,
     return -ENOSYS;
 }
 
+static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr)
+{
+    paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
+    lpae_t e = (lpae_t) {
+        .p2m.xn = 0,
+        .p2m.af = 1,
+        .p2m.sh = LPAE_SH_OUTER,
+        .p2m.read = 1,
+        .p2m.write = 1,
+        .p2m.mattr = mattr,
+        .p2m.table = 1,
+        .p2m.valid = 1,
+    };
+
+    ASSERT(!(pa & ~PAGE_MASK));
+    ASSERT(!(pa & ~PADDR_MASK));
+
+    e.bits |= pa;
+
+    return e;
+}
+
 /* Allocate a new page table page and hook it in via the given entry */
 static int p2m_create_table(struct domain *d,
                             lpae_t *entry)
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index d468418..0625464 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -213,28 +213,6 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn)
     return e;
 }
 
-static inline lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr)
-{
-    paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
-    lpae_t e = (lpae_t) {
-        .p2m.xn = 0,
-        .p2m.af = 1,
-        .p2m.sh = LPAE_SH_OUTER,
-        .p2m.write = 1,
-        .p2m.read = 1,
-        .p2m.mattr = mattr,
-        .p2m.table = 1,
-        .p2m.valid = 1,
-    };
-
-    ASSERT(!(pa & ~PAGE_MASK));
-    ASSERT(!(pa & ~PADDR_MASK));
-
-    e.bits |= pa;
-
-    return e;
-}
-
 #if defined(CONFIG_ARM_32)
 # include <asm/arm32/page.h>
 #elif defined(CONFIG_ARM_64)
-- 
1.7.10.4

[PATCH v2 03/10] xen/arm: Implement p2m_type_t as an enum

[Julien Grall]

Until now, Xen doesn't know the type of the page (ram, foreign page, mmio,...).
Introduce p2m_type_t with basic types:
    - p2m_invalid: Nothing is mapped here
    - p2m_ram_rw: Normal read/write guest RAM
    - p2m_ram_ro: Read-only guest RAM
    - p2m_mmio_direct: Read/write mapping of device memory
    - p2m_map_foreign: RAM page from foreign guest
    - p2m_grant_map_rw: Read/write grant mapping
    - p2m_grant_map_ro: Read-only grant mapping

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Add comment for future improvement
        - Add p2m_max_real_type. Will be use later to check the size of
        the enum
        - Let the compiler choose the value for each name of the enum
        - Add grant mapping type
---
 xen/include/asm-arm/p2m.h |   21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 0c554a5..f621d15 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -20,6 +20,25 @@ struct p2m_domain {
     uint8_t vmid;
 };
 
+/* List of possible type for each page in the p2m
+ * The number of available bit per page in the p2m for this purpose  is 4 bits.
+ * So it's possible to only have 16 fields. If we run out of value in the
+ * future, it's possible to use higher value for pseudo-type and don't store
+ * them in the p2m.
+ */
+typedef enum {
+    p2m_invalid = 0,    /* Nothing mapped here */
+    p2m_ram_rw,         /* Normal read/write guest RAM */
+    p2m_ram_ro,         /* Read-only; writes are silently dropped */
+    p2m_mmio_direct,    /* Read/write mapping of genuine MMIO area */
+    p2m_map_foreign,    /* Ram pages from foreign domain */
+    p2m_grant_map_rw,   /* Read/write grant mapping */
+    p2m_grant_map_ro,   /* Read-only grant mapping */
+    p2m_max_real_type,  /* Types after this won't be store in the p2m */
+} p2m_type_t;
+
+#define p2m_is_foreign(_t)  ((_t) == p2m_map_foreign)
+
 /* Initialise vmid allocator */
 void p2m_vmid_allocator_init(void);
 
@@ -72,7 +91,6 @@ p2m_pod_decrease_reservation(struct domain *d,
                              unsigned int order);
 
 /* Look up a GFN and take a reference count on the backing page. */
-typedef int p2m_type_t;
 typedef unsigned int p2m_query_t;
 #define P2M_ALLOC    (1u<<0)   /* Populate PoD and paged-out entries */
 #define P2M_UNSHARE  (1u<<1)   /* Break CoW sharing */
@@ -108,7 +126,6 @@ static inline int get_page_and_type(struct page_info *page,
     return rc;
 }
 
-#define p2m_is_foreign(_t) (0 && (_t))
 static inline int xenmem_rem_foreign_from_p2m(struct domain *d, 
                                               unsigned long gpfn)
 {
-- 
1.7.10.4

Re: [PATCH v2 03/10] xen/arm: Implement p2m_type_t as an enum

[Ian Campbell]

On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> Until now, Xen doesn't know the type of the page (ram, foreign page, mmio,...).
> Introduce p2m_type_t with basic types:
>     - p2m_invalid: Nothing is mapped here
>     - p2m_ram_rw: Normal read/write guest RAM
>     - p2m_ram_ro: Read-only guest RAM
>     - p2m_mmio_direct: Read/write mapping of device memory
>     - p2m_map_foreign: RAM page from foreign guest
>     - p2m_grant_map_rw: Read/write grant mapping
>     - p2m_grant_map_ro: Read-only grant mapping
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Add comment for future improvement
>         - Add p2m_max_real_type. Will be use later to check the size of
>         the enum
>         - Let the compiler choose the value for each name of the enum
>         - Add grant mapping type
> ---
>  xen/include/asm-arm/p2m.h |   21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 0c554a5..f621d15 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -20,6 +20,25 @@ struct p2m_domain {
>      uint8_t vmid;
>  };
>  
> +/* List of possible type for each page in the p2m
> + * The number of available bit per page in the p2m for this purpose  is 4 bits.

s/  / /.

I think you might also have meant s/p2m/pte/ or "p2m entry"?

> + * So it's possible to only have 16 fields. If we run out of value in the
> + * future, it's possible to use higher value for pseudo-type and don't store
> + * them in the p2m.
> + */
> +typedef enum {
> +    p2m_invalid = 0,    /* Nothing mapped here */
> +    p2m_ram_rw,         /* Normal read/write guest RAM */
> +    p2m_ram_ro,         /* Read-only; writes are silently dropped */
> +    p2m_mmio_direct,    /* Read/write mapping of genuine MMIO area */
> +    p2m_map_foreign,    /* Ram pages from foreign domain */
> +    p2m_grant_map_rw,   /* Read/write grant mapping */
> +    p2m_grant_map_ro,   /* Read-only grant mapping */
> +    p2m_max_real_type,  /* Types after this won't be store in the p2m */

Did you intend to add a BUILD_BUG_ON referencing this last one?

In any case:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

> +} p2m_type_t;
> +
> +#define p2m_is_foreign(_t)  ((_t) == p2m_map_foreign)
> +
>  /* Initialise vmid allocator */
>  void p2m_vmid_allocator_init(void);
>  
> @@ -72,7 +91,6 @@ p2m_pod_decrease_reservation(struct domain *d,
>                               unsigned int order);
>  
>  /* Look up a GFN and take a reference count on the backing page. */
> -typedef int p2m_type_t;
>  typedef unsigned int p2m_query_t;
>  #define P2M_ALLOC    (1u<<0)   /* Populate PoD and paged-out entries */
>  #define P2M_UNSHARE  (1u<<1)   /* Break CoW sharing */
> @@ -108,7 +126,6 @@ static inline int get_page_and_type(struct page_info *page,
>      return rc;
>  }
>  
> -#define p2m_is_foreign(_t) (0 && (_t))
>  static inline int xenmem_rem_foreign_from_p2m(struct domain *d, 
>                                                unsigned long gpfn)
>  {

Re: [PATCH v2 03/10] xen/arm: Implement p2m_type_t as an enum

[Julien Grall]

On 12/09/2013 03:59 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>> Until now, Xen doesn't know the type of the page (ram, foreign page, mmio,...).
>> Introduce p2m_type_t with basic types:
>>     - p2m_invalid: Nothing is mapped here
>>     - p2m_ram_rw: Normal read/write guest RAM
>>     - p2m_ram_ro: Read-only guest RAM
>>     - p2m_mmio_direct: Read/write mapping of device memory
>>     - p2m_map_foreign: RAM page from foreign guest
>>     - p2m_grant_map_rw: Read/write grant mapping
>>     - p2m_grant_map_ro: Read-only grant mapping
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>     Changes in v2:
>>         - Add comment for future improvement
>>         - Add p2m_max_real_type. Will be use later to check the size of
>>         the enum
>>         - Let the compiler choose the value for each name of the enum
>>         - Add grant mapping type
>> ---
>>  xen/include/asm-arm/p2m.h |   21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index 0c554a5..f621d15 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -20,6 +20,25 @@ struct p2m_domain {
>>      uint8_t vmid;
>>  };
>>  
>> +/* List of possible type for each page in the p2m
>> + * The number of available bit per page in the p2m for this purpose  is 4 bits.
> 
> s/  / /.
> 
> I think you might also have meant s/p2m/pte/ or "p2m entry"?

Right, I will fix it.

> 
>> + * So it's possible to only have 16 fields. If we run out of value in the
>> + * future, it's possible to use higher value for pseudo-type and don't store
>> + * them in the p2m.
>> + */
>> +typedef enum {
>> +    p2m_invalid = 0,    /* Nothing mapped here */
>> +    p2m_ram_rw,         /* Normal read/write guest RAM */
>> +    p2m_ram_ro,         /* Read-only; writes are silently dropped */
>> +    p2m_mmio_direct,    /* Read/write mapping of genuine MMIO area */
>> +    p2m_map_foreign,    /* Ram pages from foreign domain */
>> +    p2m_grant_map_rw,   /* Read/write grant mapping */
>> +    p2m_grant_map_ro,   /* Read-only grant mapping */
>> +    p2m_max_real_type,  /* Types after this won't be store in the p2m */
> 
> Did you intend to add a BUILD_BUG_ON referencing this last one?

The BUILD_BUG_ON is added by patch #4.

> In any case:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks.

-- 
Julien Grall

Re: [PATCH v2 03/10] xen/arm: Implement p2m_type_t as an enum

[Ian Campbell]

On Mon, 2013-12-09 at 16:34 +0000, Julien Grall wrote:
> On 12/09/2013 03:59 PM, Ian Campbell wrote:
> > On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> >> +    p2m_max_real_type,  /* Types after this won't be store in the p2m */
> > 
> > Did you intend to add a BUILD_BUG_ON referencing this last one?
> 
> The BUILD_BUG_ON is added by patch #4.

So it is. Perhaps it should either be in this patch or you should add
max_real_type in that patch.

Ian.

Re: [PATCH v2 03/10] xen/arm: Implement p2m_type_t as an enum

[Julien Grall]

On 12/09/2013 04:54 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 16:34 +0000, Julien Grall wrote:
>> On 12/09/2013 03:59 PM, Ian Campbell wrote:
>>> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>>>> +    p2m_max_real_type,  /* Types after this won't be store in the p2m */
>>>
>>> Did you intend to add a BUILD_BUG_ON referencing this last one?
>>
>> The BUILD_BUG_ON is added by patch #4.
>
> So it is. Perhaps it should either be in this patch or you should add
> max_real_type in that patch.

I have moved the BUILD_BUG_ON in this patch.


-- 
Julien Grall

[PATCH v2 04/10] xen/arm: Store p2m type in each page of the guest

[Julien Grall]

Use the field 'avail' to store the type of the page. Rename it to 'p2mt' for
convenience.
The information stored in this field will be retrieved in a future patch to
hange the behaviour when the page is removed.

Also introduce guest_physmap_add_entry to map and set a specific p2m type for
a page.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Rename 'avail' field to 'p2mt' in the p2m structure
        - Add BUILD_BUG_ON to check if the enum value will fit in the field
        - Implement grant mapping type
---
 xen/arch/arm/p2m.c         |   58 ++++++++++++++++++++++++++++++++------------
 xen/include/asm-arm/p2m.h  |   18 +++++++++++---
 xen/include/asm-arm/page.h |    2 +-
 3 files changed, 58 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 8f8b47e..e43804c 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -124,7 +124,8 @@ int p2m_pod_decrease_reservation(struct domain *d,
     return -ENOSYS;
 }
 
-static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr)
+static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
+                               p2m_type_t t)
 {
     paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
     lpae_t e = (lpae_t) {
@@ -132,12 +133,34 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr)
         .p2m.af = 1,
         .p2m.sh = LPAE_SH_OUTER,
         .p2m.read = 1,
-        .p2m.write = 1,
         .p2m.mattr = mattr,
         .p2m.table = 1,
         .p2m.valid = 1,
+        .p2m.p2mt = t,
     };
 
+    BUILD_BUG_ON(p2m_max_real_type > (2 << 4));
+
+    switch (t)
+    {
+    case p2m_grant_map_rw:
+        e.p2m.xn = 1;
+        /* Fallthrough */
+    case p2m_ram_rw:
+    case p2m_mmio_direct:
+    case p2m_map_foreign:
+        e.p2m.write = 1;
+        break;
+
+    case p2m_grant_map_ro:
+        e.p2m.xn = 1;
+        /* Fallthrough */
+    case p2m_invalid:
+    case p2m_ram_ro:
+    default:
+        e.p2m.write = 0;
+    }
+
     ASSERT(!(pa & ~PAGE_MASK));
     ASSERT(!(pa & ~PADDR_MASK));
 
@@ -167,7 +190,7 @@ static int p2m_create_table(struct domain *d,
     clear_page(p);
     unmap_domain_page(p);
 
-    pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM);
+    pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_invalid);
 
     write_pte(entry, pte);
 
@@ -185,7 +208,8 @@ static int create_p2m_entries(struct domain *d,
                      paddr_t start_gpaddr,
                      paddr_t end_gpaddr,
                      paddr_t maddr,
-                     int mattr)
+                     int mattr,
+                     p2m_type_t t)
 {
     int rc, flush;
     struct p2m_domain *p2m = &d->arch.p2m;
@@ -260,14 +284,15 @@ static int create_p2m_entries(struct domain *d,
                         goto out;
                     }
 
-                    pte = mfn_to_p2m_entry(page_to_mfn(page), mattr);
+                    pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t);
 
                     write_pte(&third[third_table_offset(addr)], pte);
                 }
                 break;
             case INSERT:
                 {
-                    lpae_t pte = mfn_to_p2m_entry(maddr >> PAGE_SHIFT, mattr);
+                    lpae_t pte = mfn_to_p2m_entry(maddr >> PAGE_SHIFT,
+                                                  mattr, t);
                     write_pte(&third[third_table_offset(addr)], pte);
                     maddr += PAGE_SIZE;
                 }
@@ -302,7 +327,8 @@ int p2m_populate_ram(struct domain *d,
                      paddr_t start,
                      paddr_t end)
 {
-    return create_p2m_entries(d, ALLOCATE, start, end, 0, MATTR_MEM);
+    return create_p2m_entries(d, ALLOCATE, start, end,
+                              0, MATTR_MEM, p2m_ram_rw);
 }
 
 int map_mmio_regions(struct domain *d,
@@ -310,18 +336,20 @@ int map_mmio_regions(struct domain *d,
                      paddr_t end_gaddr,
                      paddr_t maddr)
 {
-    return create_p2m_entries(d, INSERT, start_gaddr, end_gaddr, maddr, MATTR_DEV);
+    return create_p2m_entries(d, INSERT, start_gaddr, end_gaddr,
+                              maddr, MATTR_DEV, p2m_mmio_direct);
 }
 
-int guest_physmap_add_page(struct domain *d,
-                           unsigned long gpfn,
-                           unsigned long mfn,
-                           unsigned int page_order)
+int guest_physmap_add_entry(struct domain *d,
+                            unsigned long gpfn,
+                            unsigned long mfn,
+                            unsigned long page_order,
+                            p2m_type_t t)
 {
     return create_p2m_entries(d, INSERT,
                               pfn_to_paddr(gpfn),
-                              pfn_to_paddr(gpfn + (1<<page_order)),
-                              pfn_to_paddr(mfn), MATTR_MEM);
+                              pfn_to_paddr(gpfn + (1 << page_order)),
+                              pfn_to_paddr(mfn), MATTR_MEM, t);
 }
 
 void guest_physmap_remove_page(struct domain *d,
@@ -331,7 +359,7 @@ void guest_physmap_remove_page(struct domain *d,
     create_p2m_entries(d, REMOVE,
                        pfn_to_paddr(gpfn),
                        pfn_to_paddr(gpfn + (1<<page_order)),
-                       pfn_to_paddr(mfn), MATTR_MEM);
+                       pfn_to_paddr(mfn), MATTR_MEM, p2m_invalid);
 }
 
 int p2m_alloc_table(struct domain *d)
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index f621d15..56bbf4d 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -68,11 +68,21 @@ int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end);
 int map_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,
+                            unsigned long mfn,
+                            unsigned long page_order,
+                            p2m_type_t t);
+
 /* Untyped version for RAM only, for compatibility */
-int guest_physmap_add_page(struct domain *d,
-                           unsigned long gfn,
-                           unsigned long mfn,
-                           unsigned int page_order);
+static inline int guest_physmap_add_page(struct domain *d,
+                                         unsigned long gfn,
+                                         unsigned long mfn,
+                                         unsigned int page_order)
+{
+    return guest_physmap_add_entry(d, gfn, mfn, page_order, p2m_ram_rw);
+}
+
 void guest_physmap_remove_page(struct domain *d,
                                unsigned long gpfn,
                                unsigned long mfn, unsigned int page_order);
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 0625464..a363a62 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -153,7 +153,7 @@ typedef struct {
     unsigned long contig:1;     /* In a block of 16 contiguous entries */
     unsigned long sbz2:1;
     unsigned long xn:1;         /* eXecute-Never */
-    unsigned long avail:4;      /* Ignored by hardware */
+    unsigned long p2mt:4;       /* Ignore by hardware. Used to store p2m types */
 
     unsigned long sbz1:5;
 } __attribute__((__packed__)) lpae_p2m_t;
-- 
1.7.10.4

Re: [PATCH v2 04/10] xen/arm: Store p2m type in each page of the guest

[Ian Campbell]

On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> Use the field 'avail' to store the type of the page. Rename it to 'p2mt' for
> convenience.

The p2m is redundant in most uses, and the t isn't very descriptive. I
think "type" would be fine.

> The information stored in this field will be retrieved in a future patch to
> hange the behaviour when the page is removed.

"change"

> Also introduce guest_physmap_add_entry to map and set a specific p2m type for
> a page.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Rename 'avail' field to 'p2mt' in the p2m structure
>         - Add BUILD_BUG_ON to check if the enum value will fit in the field
>         - Implement grant mapping type
> ---
>  xen/arch/arm/p2m.c         |   58 ++++++++++++++++++++++++++++++++------------
>  xen/include/asm-arm/p2m.h  |   18 +++++++++++---
>  xen/include/asm-arm/page.h |    2 +-
>  3 files changed, 58 insertions(+), 20 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 8f8b47e..e43804c 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -124,7 +124,8 @@ int p2m_pod_decrease_reservation(struct domain *d,
>      return -ENOSYS;
>  }
>  
> -static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr)
> +static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
> +                               p2m_type_t t)
>  {
>      paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
>      lpae_t e = (lpae_t) {
> @@ -132,12 +133,34 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr)
>          .p2m.af = 1,
>          .p2m.sh = LPAE_SH_OUTER,
>          .p2m.read = 1,
> -        .p2m.write = 1,
>          .p2m.mattr = mattr,
>          .p2m.table = 1,
>          .p2m.valid = 1,
> +        .p2m.p2mt = t,
>      };
>  
> +    BUILD_BUG_ON(p2m_max_real_type > (2 << 4));
> +
> +    switch (t)
> +    {
> +    case p2m_grant_map_rw:
> +        e.p2m.xn = 1;
> +        /* Fallthrough */
> +    case p2m_ram_rw:
> +    case p2m_mmio_direct:
> +    case p2m_map_foreign:
> +        e.p2m.write = 1;
> +        break;
> +
> +    case p2m_grant_map_ro:
> +        e.p2m.xn = 1;
> +        /* Fallthrough */
> +    case p2m_invalid:
> +    case p2m_ram_ro:
> +    default:

I think you've enumerated all the options, so it is better to leave out
the default -- then at least some compilers will complain if we add a
new type and forget to add it here.

The bulk of the patch looks good though, thanks.

Ian.

Re: [PATCH v2 04/10] xen/arm: Store p2m type in each page of the guest

[Julien Grall]

On 12/09/2013 04:03 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>> Use the field 'avail' to store the type of the page. Rename it to 'p2mt' for
>> convenience.
> 
> The p2m is redundant in most uses, and the t isn't very descriptive. I
> think "type" would be fine.
> 
>> The information stored in this field will be retrieved in a future patch to
>> hange the behaviour when the page is removed.
> 
> "change"
> 
>> Also introduce guest_physmap_add_entry to map and set a specific p2m type for
>> a page.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>     Changes in v2:
>>         - Rename 'avail' field to 'p2mt' in the p2m structure
>>         - Add BUILD_BUG_ON to check if the enum value will fit in the field
>>         - Implement grant mapping type
>> ---
>>  xen/arch/arm/p2m.c         |   58 ++++++++++++++++++++++++++++++++------------
>>  xen/include/asm-arm/p2m.h  |   18 +++++++++++---
>>  xen/include/asm-arm/page.h |    2 +-
>>  3 files changed, 58 insertions(+), 20 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 8f8b47e..e43804c 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -124,7 +124,8 @@ int p2m_pod_decrease_reservation(struct domain *d,
>>      return -ENOSYS;
>>  }
>>  
>> -static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr)
>> +static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
>> +                               p2m_type_t t)
>>  {
>>      paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
>>      lpae_t e = (lpae_t) {
>> @@ -132,12 +133,34 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr)
>>          .p2m.af = 1,
>>          .p2m.sh = LPAE_SH_OUTER,
>>          .p2m.read = 1,
>> -        .p2m.write = 1,
>>          .p2m.mattr = mattr,
>>          .p2m.table = 1,
>>          .p2m.valid = 1,
>> +        .p2m.p2mt = t,
>>      };
>>  
>> +    BUILD_BUG_ON(p2m_max_real_type > (2 << 4));
>> +
>> +    switch (t)
>> +    {
>> +    case p2m_grant_map_rw:
>> +        e.p2m.xn = 1;
>> +        /* Fallthrough */
>> +    case p2m_ram_rw:
>> +    case p2m_mmio_direct:
>> +    case p2m_map_foreign:
>> +        e.p2m.write = 1;
>> +        break;
>> +
>> +    case p2m_grant_map_ro:
>> +        e.p2m.xn = 1;
>> +        /* Fallthrough */
>> +    case p2m_invalid:
>> +    case p2m_ram_ro:
>> +    default:
> 
> I think you've enumerated all the options, so it is better to leave out
> the default -- then at least some compilers will complain if we add a
> new type and forget to add it here.
> 

Ok. I will remove it.


-- 
Julien Grall

Re: [PATCH v2 04/10] xen/arm: Store p2m type in each page of the guest

[Ian Campbell]

On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> +    BUILD_BUG_ON(p2m_max_real_type > (2 << 4));

2<<4 is 32.

I think you want either 1<<4, 2^4 or just 16 (my preference is 16). And
I think it should be >= since the valid values are 0..15 inclusive.

Ian.

Re: [PATCH v2 04/10] xen/arm: Store p2m type in each page of the guest

[Julien Grall]

On 12/09/2013 04:53 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>> +    BUILD_BUG_ON(p2m_max_real_type > (2 << 4));
>
> 2<<4 is 32.
>
> I think you want either 1<<4, 2^4 or just 16 (my preference is 16). And

Right.

> I think it should be >= since the valid values are 0..15 inclusive.

No, because p2m_max_real_type won't be store in the p2m, so we can have 
16 values [0..15] and p2m_max_real_type will be the 17th (ie 16).

-- 
Julien Grall

Re: [PATCH v2 04/10] xen/arm: Store p2m type in each page of the guest

[Ian Campbell]

On Tue, 2013-12-10 at 01:55 +0000, Julien Grall wrote:
> On 12/09/2013 04:53 PM, Ian Campbell wrote:
> > On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> >> +    BUILD_BUG_ON(p2m_max_real_type > (2 << 4));
> >
> > 2<<4 is 32.
> >
> > I think you want either 1<<4, 2^4 or just 16 (my preference is 16). And
> 
> Right.
> 
> > I think it should be >= since the valid values are 0..15 inclusive.
> 
> No, because p2m_max_real_type won't be store in the p2m, so we can have 
> 16 values [0..15] and p2m_max_real_type will be the 17th (ie 16).

If p2m_max_real_type will be the 17th (ie 16) then the check needs to be
p2m_max_real_type >= 16.

Ian.

Re: [PATCH v2 04/10] xen/arm: Store p2m type in each page of the guest

[Julien Grall]

On 12/10/2013 09:37 AM, Ian Campbell wrote:
> On Tue, 2013-12-10 at 01:55 +0000, Julien Grall wrote:
>> On 12/09/2013 04:53 PM, Ian Campbell wrote:
>>> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>>>> +    BUILD_BUG_ON(p2m_max_real_type > (2 << 4));
>>>
>>> 2<<4 is 32.
>>>
>>> I think you want either 1<<4, 2^4 or just 16 (my preference is 16). And
>>
>> Right.
>>
>>> I think it should be >= since the valid values are 0..15 inclusive.
>>
>> No, because p2m_max_real_type won't be store in the p2m, so we can have
>> 16 values [0..15] and p2m_max_real_type will be the 17th (ie 16).
>
> If p2m_max_real_type will be the 17th (ie 16) then the check needs to be
> p2m_max_real_type >= 16.

No because p2m_max_real_type = 16 is valid.
Which is invalid is foo = 16 and p2m_max_real_type = 17.

-- 
Julien Grall

Re: [PATCH v2 04/10] xen/arm: Store p2m type in each page of the guest

[Ian Campbell]

On Tue, 2013-12-10 at 13:50 +0000, Julien Grall wrote:
> 
> On 12/10/2013 09:37 AM, Ian Campbell wrote:
> > On Tue, 2013-12-10 at 01:55 +0000, Julien Grall wrote:
> >> On 12/09/2013 04:53 PM, Ian Campbell wrote:
> >>> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> >>>> +    BUILD_BUG_ON(p2m_max_real_type > (2 << 4));
> >>>
> >>> 2<<4 is 32.
> >>>
> >>> I think you want either 1<<4, 2^4 or just 16 (my preference is 16). And
> >>
> >> Right.
> >>
> >>> I think it should be >= since the valid values are 0..15 inclusive.
> >>
> >> No, because p2m_max_real_type won't be store in the p2m, so we can have
> >> 16 values [0..15] and p2m_max_real_type will be the 17th (ie 16).
> >
> > If p2m_max_real_type will be the 17th (ie 16) then the check needs to be
> > p2m_max_real_type >= 16.
> 
> No because p2m_max_real_type = 16 is valid.
> Which is invalid is foo = 16 and p2m_max_real_type = 17.

Oh right, because it's actually the one after the last valid one.

OK.

Ian.

[PATCH v2 05/10] xen/arm: p2m: Extend p2m_lookup parameters to retrieve the p2m type

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - This patch was "Add p2m_get_entry" on the previous version
        - Don't add a new function but only extend p2m_lookup
---
 xen/arch/arm/p2m.c        |   13 +++++++++++--
 xen/arch/arm/traps.c      |    6 +++---
 xen/include/asm-arm/p2m.h |    2 +-
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index e43804c..f0bbaca 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -71,11 +71,17 @@ static lpae_t *p2m_map_first(struct p2m_domain *p2m, paddr_t addr)
  * There are no processor functions to do a stage 2 only lookup therefore we
  * do a a software walk.
  */
-paddr_t p2m_lookup(struct domain *d, paddr_t paddr)
+paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
     lpae_t pte, *first = NULL, *second = NULL, *third = NULL;
     paddr_t maddr = INVALID_PADDR;
+    p2m_type_t _t;
+
+    /* Allow t to be NULL */
+    t = t ?: &_t;
+
+    *t = p2m_invalid;
 
     spin_lock(&p2m->lock);
 
@@ -99,7 +105,10 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr)
 
 done:
     if ( pte.p2m.valid )
+    {
         maddr = (pte.bits & PADDR_MASK & PAGE_MASK) | (paddr & ~PAGE_MASK);
+        *t = pte.p2m.p2mt;
+    }
 
     if (third) unmap_domain_page(third);
     if (second) unmap_domain_page(second);
@@ -502,7 +511,7 @@ err:
 
 unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn)
 {
-    paddr_t p = p2m_lookup(d, pfn_to_paddr(gpfn));
+    paddr_t p = p2m_lookup(d, pfn_to_paddr(gpfn), NULL);
     return p >> PAGE_SHIFT;
 }
 
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 458128e..0a811e7 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1267,7 +1267,7 @@ void dump_guest_s1_walk(struct domain *d, vaddr_t addr)
     printk("dom%d VA 0x%08"PRIvaddr"\n", d->domain_id, addr);
     printk("    TTBCR: 0x%08"PRIregister"\n", ttbcr);
     printk("    TTBR0: 0x%016"PRIx64" = 0x%"PRIpaddr"\n",
-           ttbr0, p2m_lookup(d, ttbr0 & PAGE_MASK));
+           ttbr0, p2m_lookup(d, ttbr0 & PAGE_MASK, NULL));
 
     if ( ttbcr & TTBCR_EAE )
     {
@@ -1280,7 +1280,7 @@ void dump_guest_s1_walk(struct domain *d, vaddr_t addr)
         return;
     }
 
-    paddr = p2m_lookup(d, ttbr0 & PAGE_MASK);
+    paddr = p2m_lookup(d, ttbr0 & PAGE_MASK, NULL);
     if ( paddr == INVALID_PADDR )
     {
         printk("Failed TTBR0 maddr lookup\n");
@@ -1295,7 +1295,7 @@ void dump_guest_s1_walk(struct domain *d, vaddr_t addr)
          !(first[offset] & 0x2) )
         goto done;
 
-    paddr = p2m_lookup(d, first[offset] & PAGE_MASK);
+    paddr = p2m_lookup(d, first[offset] & PAGE_MASK, NULL);
 
     if ( paddr == INVALID_PADDR )
     {
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 56bbf4d..f4bcd7d 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -58,7 +58,7 @@ int p2m_alloc_table(struct domain *d);
 void p2m_load_VTTBR(struct domain *d);
 
 /* Look up the MFN corresponding to a domain's PFN. */
-paddr_t p2m_lookup(struct domain *d, paddr_t gpfn);
+paddr_t p2m_lookup(struct domain *d, paddr_t gpfn, p2m_type_t *t);
 
 /* Setup p2m RAM mapping for domain d from start-end. */
 int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end);
-- 
1.7.10.4

Re: [PATCH v2 05/10] xen/arm: p2m: Extend p2m_lookup parameters to retrieve the p2m type

[Ian Campbell]

On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH v2 06/10] xen/arm: Retrieve p2m type in get_page_from_gfn

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Use p2m_lookup as p2m_get_entry was removed
---
 xen/include/asm-arm/p2m.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index f4bcd7d..b63204d 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -109,7 +109,8 @@ static inline struct page_info *get_page_from_gfn(
     struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
 {
     struct page_info *page;
-    unsigned long mfn = gmfn_to_mfn(d, gfn);
+    paddr_t maddr = p2m_lookup(d, pfn_to_paddr(gfn), t);
+    unsigned long mfn = maddr >> PAGE_SHIFT;
 
     if (!mfn_valid(mfn))
         return NULL;
-- 
1.7.10.4

Re: [PATCH v2 06/10] xen/arm: Retrieve p2m type in get_page_from_gfn

[Ian Campbell]

On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Use p2m_lookup as p2m_get_entry was removed
> ---
>  xen/include/asm-arm/p2m.h |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index f4bcd7d..b63204d 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -109,7 +109,8 @@ static inline struct page_info *get_page_from_gfn(
>      struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
>  {
>      struct page_info *page;
> -    unsigned long mfn = gmfn_to_mfn(d, gfn);
> +    paddr_t maddr = p2m_lookup(d, pfn_to_paddr(gfn), t);
> +    unsigned long mfn = maddr >> PAGE_SHIFT;

I think this resend happened before I replied to the original a second
time, (in <1386583029.13126.13.camel@kazak.uk.xensource.com>) so you
hadn't seen it.

TL;DR: INVALID_MADDR >> PAGE_SHIFT != INVALID_MFN on arm64.

Ian.

Re: [PATCH v2 06/10] xen/arm: Retrieve p2m type in get_page_from_gfn

[Julien Grall]

On 12/09/2013 04:06 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>     Changes in v2:
>>         - Use p2m_lookup as p2m_get_entry was removed
>> ---
>>  xen/include/asm-arm/p2m.h |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index f4bcd7d..b63204d 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -109,7 +109,8 @@ static inline struct page_info *get_page_from_gfn(
>>      struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
>>  {
>>      struct page_info *page;
>> -    unsigned long mfn = gmfn_to_mfn(d, gfn);
>> +    paddr_t maddr = p2m_lookup(d, pfn_to_paddr(gfn), t);
>> +    unsigned long mfn = maddr >> PAGE_SHIFT;
> 
> I think this resend happened before I replied to the original a second
> time, (in <1386583029.13126.13.camel@kazak.uk.xensource.com>) so you
> hadn't seen it.

Indeed, I will fix it.

> TL;DR: INVALID_MADDR >> PAGE_SHIFT != INVALID_MFN on arm64.

Does it mean that INVALID_MFN can be a valid mfn on arm64? If so, we
have some place in common code where this constant is used (see
common/domain.c for instance).

-- 
Julien Grall

Re: [PATCH v2 06/10] xen/arm: Retrieve p2m type in get_page_from_gfn

[Ian Campbell]

On Mon, 2013-12-09 at 16:50 +0000, Julien Grall wrote:
> On 12/09/2013 04:06 PM, Ian Campbell wrote:
> > On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >>
> >> ---
> >>     Changes in v2:
> >>         - Use p2m_lookup as p2m_get_entry was removed
> >> ---
> >>  xen/include/asm-arm/p2m.h |    3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> >> index f4bcd7d..b63204d 100644
> >> --- a/xen/include/asm-arm/p2m.h
> >> +++ b/xen/include/asm-arm/p2m.h
> >> @@ -109,7 +109,8 @@ static inline struct page_info *get_page_from_gfn(
> >>      struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
> >>  {
> >>      struct page_info *page;
> >> -    unsigned long mfn = gmfn_to_mfn(d, gfn);
> >> +    paddr_t maddr = p2m_lookup(d, pfn_to_paddr(gfn), t);
> >> +    unsigned long mfn = maddr >> PAGE_SHIFT;
> > 
> > I think this resend happened before I replied to the original a second
> > time, (in <1386583029.13126.13.camel@kazak.uk.xensource.com>) so you
> > hadn't seen it.
> 
> Indeed, I will fix it.
> 
> > TL;DR: INVALID_MADDR >> PAGE_SHIFT != INVALID_MFN on arm64.
> 
> Does it mean that INVALID_MFN can be a valid mfn on arm64? If so, we
> have some place in common code where this constant is used (see
> common/domain.c for instance).

No, INVALID_MFN is 0xffffffffffffffff which is fine[0].

But given INVALID_MADDR of 0xffffffffffffffff,
(INVALID_MADDR>>PAGE_SHIFT) is 0x000fffffffffffff.

Ian.

[0] actually, the page at -1 could in theory be valid on any platform,
either arm32 or x86, bit a) it's unlikely and b) there isn't really any
better value to use as this sentinal. Lets not worry about it right now.

Re: [PATCH v2 06/10] xen/arm: Retrieve p2m type in get_page_from_gfn

[Julien Grall]

On 12/09/2013 04:58 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 16:50 +0000, Julien Grall wrote:
>> On 12/09/2013 04:06 PM, Ian Campbell wrote:
>>> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>>
>>>> ---
>>>>      Changes in v2:
>>>>          - Use p2m_lookup as p2m_get_entry was removed
>>>> ---
>>>>   xen/include/asm-arm/p2m.h |    3 ++-
>>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>>>> index f4bcd7d..b63204d 100644
>>>> --- a/xen/include/asm-arm/p2m.h
>>>> +++ b/xen/include/asm-arm/p2m.h
>>>> @@ -109,7 +109,8 @@ static inline struct page_info *get_page_from_gfn(
>>>>       struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
>>>>   {
>>>>       struct page_info *page;
>>>> -    unsigned long mfn = gmfn_to_mfn(d, gfn);
>>>> +    paddr_t maddr = p2m_lookup(d, pfn_to_paddr(gfn), t);
>>>> +    unsigned long mfn = maddr >> PAGE_SHIFT;
>>>
>>> I think this resend happened before I replied to the original a second
>>> time, (in <1386583029.13126.13.camel@kazak.uk.xensource.com>) so you
>>> hadn't seen it.
>>
>> Indeed, I will fix it.
>>
>>> TL;DR: INVALID_MADDR >> PAGE_SHIFT != INVALID_MFN on arm64.
>>
>> Does it mean that INVALID_MFN can be a valid mfn on arm64? If so, we
>> have some place in common code where this constant is used (see
>> common/domain.c for instance).
>
> No, INVALID_MFN is 0xffffffffffffffff which is fine[0].
>
> But given INVALID_MADDR of 0xffffffffffffffff,
> (INVALID_MADDR>>PAGE_SHIFT) is 0x000fffffffffffff.

oh right, I didn't pay attention for that. The patch is now fixed.

-- 
Julien Grall

[PATCH v2 07/10] xen/arm: Introduce relinquish_p2m_mapping to remove refcount every mapped page

[Julien Grall]

This function will be called when the domain relinquishes its memory.
It removes refcount on every mapped page to a valid MFN.

Currently, Xen doesn't take refcount on every new mapping but only for foreign
mapping. Restrict the function only on foreign mapping.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Introduce the patch
---
 xen/arch/arm/domain.c        |    5 +++++
 xen/arch/arm/p2m.c           |   47 ++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/domain.h |    1 +
 xen/include/asm-arm/p2m.h    |   15 ++++++++++++++
 4 files changed, 68 insertions(+)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 1590708..e7c2f67 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -717,6 +717,11 @@ int domain_relinquish_resources(struct domain *d)
         if ( ret )
             return ret;
 
+    case RELMEM_mapping:
+        ret = relinquish_p2m_mapping(d);
+        if ( ret )
+            return ret;
+
         d->arch.relmem = RELMEM_done;
         /* Fallthrough */
 
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index f0bbaca..dbd6a06 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -6,6 +6,7 @@
 #include <xen/bitops.h>
 #include <asm/flushtlb.h>
 #include <asm/gic.h>
+#include <asm/event.h>
 
 /* First level P2M is 2 consecutive pages */
 #define P2M_FIRST_ORDER 1
@@ -320,6 +321,16 @@ static int create_p2m_entries(struct domain *d,
             flush_tlb_all_local();
     }
 
+    if ( (t == p2m_ram_rw) || (t == p2m_ram_ro) || (t == p2m_map_foreign))
+    {
+        unsigned long sgfn = paddr_to_pfn(start_gpaddr);
+        unsigned long egfn = paddr_to_pfn(end_gpaddr);
+
+        p2m->max_mapped_gfn = MAX(p2m->max_mapped_gfn, egfn);
+        /* Use next_gfn_to_relinquish to store the lowest gfn mapped */
+        p2m->next_gfn_to_relinquish = MIN(p2m->next_gfn_to_relinquish, sgfn);
+    }
+
     rc = 0;
 
 out:
@@ -503,12 +514,48 @@ int p2m_init(struct domain *d)
 
     p2m->first_level = NULL;
 
+    p2m->max_mapped_gfn = 0;
+    p2m->next_gfn_to_relinquish = ULONG_MAX;
+
 err:
     spin_unlock(&p2m->lock);
 
     return rc;
 }
 
+int relinquish_p2m_mapping(struct domain *d)
+{
+    struct p2m_domain *p2m = &d->arch.p2m;
+    unsigned long gfn, count = 0;
+    int rc = 0;
+
+    for ( gfn = p2m->next_gfn_to_relinquish;
+          gfn < p2m->max_mapped_gfn; gfn++ )
+    {
+        p2m_type_t t;
+        paddr_t p = p2m_lookup(d, gfn, &t);
+        unsigned long mfn = p >> PAGE_SHIFT;
+
+        if ( mfn_valid(mfn) && p2m_is_foreign(t) )
+        {
+            put_page(mfn_to_page(mfn));
+            guest_physmap_remove_page(d, gfn, mfn, 0);
+        }
+
+        count++;
+
+        /* Preempt every 2MiB. Arbitrary */
+        if ( (count == 512) && hypercall_preempt_check() )
+        {
+            p2m->next_gfn_to_relinquish = gfn + 1;
+            rc = -EAGAIN;
+            break;
+        }
+    }
+
+    return rc;
+}
+
 unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn)
 {
     paddr_t p = p2m_lookup(d, pfn_to_paddr(gpfn), NULL);
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 922eda3..4a4c018 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -75,6 +75,7 @@ struct arch_domain
         RELMEM_not_started,
         RELMEM_xen,
         RELMEM_page,
+        RELMEM_mapping,
         RELMEM_done,
     } relmem;
 
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index b63204d..b0d3aea 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -18,6 +18,15 @@ struct p2m_domain {
 
     /* Current VMID in use */
     uint8_t vmid;
+
+    /* Highest guest frame that's ever been mapped in the p2m
+     * Take only into account ram and foreign mapping
+     */
+    unsigned long max_mapped_gfn;
+
+    /* When releasing mapped gfn's in a preemptible manner, recall where
+     * to resume the search */
+    unsigned long next_gfn_to_relinquish;
 };
 
 /* List of possible type for each page in the p2m
@@ -48,6 +57,12 @@ int p2m_init(struct domain *d);
 /* Return all the p2m resources to Xen. */
 void p2m_teardown(struct domain *d);
 
+/* Remove mapping refcount on each mapping page in the p2m
+ *
+ * TODO: For the moment only foreign mapping is handled
+ */
+int relinquish_p2m_mapping(struct domain *d);
+
 /* Allocate a new p2m table for a domain.
  *
  * Returns 0 for success or -errno.
-- 
1.7.10.4

Re: [PATCH v2 07/10] xen/arm: Introduce relinquish_p2m_mapping to remove refcount every mapped page

[Ian Campbell]

On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> This function will be called when the domain relinquishes its memory.
> It removes refcount on every mapped page to a valid MFN.
> 
> Currently, Xen doesn't take refcount on every new mapping but only for foreign
> mapping. Restrict the function only on foreign mapping.

Skimming the remainder of the patch's titles and recalling a previous
conversation the intention is not to extend this for 4.4, correct?

> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Introduce the patch
> ---
>  xen/arch/arm/domain.c        |    5 +++++
>  xen/arch/arm/p2m.c           |   47 ++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/domain.h |    1 +
>  xen/include/asm-arm/p2m.h    |   15 ++++++++++++++
>  4 files changed, 68 insertions(+)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 1590708..e7c2f67 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -717,6 +717,11 @@ int domain_relinquish_resources(struct domain *d)
>          if ( ret )
>              return ret;
>  
> +    case RELMEM_mapping:

Something somewhere should be setting d->arch.relmem = RELMEM_mapping at
the appropriate time. (immediately above I think?)

You also want a "Fallthrough" comment just above.

> +        ret = relinquish_p2m_mapping(d);
> +        if ( ret )
> +            return ret;
> +
>          d->arch.relmem = RELMEM_done;
>          /* Fallthrough */
>  
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index f0bbaca..dbd6a06 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -6,6 +6,7 @@
>  #include <xen/bitops.h>
>  #include <asm/flushtlb.h>
>  #include <asm/gic.h>
> +#include <asm/event.h>
>  
>  /* First level P2M is 2 consecutive pages */
>  #define P2M_FIRST_ORDER 1
> @@ -320,6 +321,16 @@ static int create_p2m_entries(struct domain *d,
>              flush_tlb_all_local();
>      }
>  
> +    if ( (t == p2m_ram_rw) || (t == p2m_ram_ro) || (t == p2m_map_foreign))
> +    {
> +        unsigned long sgfn = paddr_to_pfn(start_gpaddr);
> +        unsigned long egfn = paddr_to_pfn(end_gpaddr);
> +
> +        p2m->max_mapped_gfn = MAX(p2m->max_mapped_gfn, egfn);
> +        /* Use next_gfn_to_relinquish to store the lowest gfn mapped */
> +        p2m->next_gfn_to_relinquish = MIN(p2m->next_gfn_to_relinquish, sgfn);
> +    }
> +
>      rc = 0;
>  
>  out:
> @@ -503,12 +514,48 @@ int p2m_init(struct domain *d)
>  
>      p2m->first_level = NULL;
>  
> +    p2m->max_mapped_gfn = 0;
> +    p2m->next_gfn_to_relinquish = ULONG_MAX;
> +
>  err:
>      spin_unlock(&p2m->lock);
>  
>      return rc;
>  }
>  
> +int relinquish_p2m_mapping(struct domain *d)
> +{
> +    struct p2m_domain *p2m = &d->arch.p2m;
> +    unsigned long gfn, count = 0;
> +    int rc = 0;
> +
> +    for ( gfn = p2m->next_gfn_to_relinquish;
> +          gfn < p2m->max_mapped_gfn; gfn++ )

I know that Tim has been keen to get rid of these sorts of loops on x86,
and with good reason I think.

> +    {
> +        p2m_type_t t;
> +        paddr_t p = p2m_lookup(d, gfn, &t);

This does the full walk for each address, even though 2/3 of the levels
are more than likely identical to the previous gfn.

It would be better to do a full walk, which sadly will look a lot like
p2m_lookup, no avoiding that I think.

You can still resume the walk based on next_gfn_to_relinquish and bound
it on max_mapped_gfn, although I don't think it is strictly necessary.

> +        unsigned long mfn = p >> PAGE_SHIFT;
> +
> +        if ( mfn_valid(mfn) && p2m_is_foreign(t) )

I think it would be worth reiterating in a comment that we only take a
ref for foreign mappings right now.
> +        {
> +            put_page(mfn_to_page(mfn));
> +            guest_physmap_remove_page(d, gfn, mfn, 0);

You should unmap it and then put it I think.

Is this going to do yet another lookup/walk?

The REMOVE case of create_p2m_entries is so trivial you could open code
it here, or if you wanted to you could refactor it into a helper.

I am wondering if the conditional put page ought to be at the point of
removal (i.e. in the helper) rather than here. (I think Tim made a
similar comment on the x86 version of the remove_from_physmap pvh
patches, you probably need to match the generic change which that
implies)

BTW, if you do the clear (but not the put_page) for every entry then the
present bit (or pte.bits == 0) might be a useful proxy for
next_lN_entry? I suppose even walking, say, 4GB of mostly empty P2M is
not going to be super cheap.

> +        }
> +
> +        count++;
> +
> +        /* Preempt every 2MiB. Arbitrary */
> +        if ( (count == 512) && hypercall_preempt_check() )
> +        {
> +            p2m->next_gfn_to_relinquish = gfn + 1;
> +            rc = -EAGAIN;

I think I'm just failing to find it, but where is the call to
hypercall_create_continuation? I suppose it is somewhere way up the
stack?

I'm not sure the count == 512 is needed -- hypercall_preempt_check
should be sufficient?

> +            break;
> +        }
> +    }
> +
> +    return rc;
> +}
> +
>  unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn)
>  {
>      paddr_t p = p2m_lookup(d, pfn_to_paddr(gpfn), NULL);
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 922eda3..4a4c018 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -75,6 +75,7 @@ struct arch_domain
>          RELMEM_not_started,
>          RELMEM_xen,
>          RELMEM_page,
> +        RELMEM_mapping,
>          RELMEM_done,
>      } relmem;
>  
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index b63204d..b0d3aea 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -18,6 +18,15 @@ struct p2m_domain {
>  
>      /* Current VMID in use */
>      uint8_t vmid;
> +
> +    /* Highest guest frame that's ever been mapped in the p2m
> +     * Take only into account ram and foreign mapping
> +     */
> +    unsigned long max_mapped_gfn;
> +
> +    /* When releasing mapped gfn's in a preemptible manner, recall where
> +     * to resume the search */
> +    unsigned long next_gfn_to_relinquish;
>  };
>  
>  /* List of possible type for each page in the p2m
> @@ -48,6 +57,12 @@ int p2m_init(struct domain *d);
>  /* Return all the p2m resources to Xen. */
>  void p2m_teardown(struct domain *d);
>  
> +/* Remove mapping refcount on each mapping page in the p2m
> + *
> + * TODO: For the moment only foreign mapping is handled
> + */
> +int relinquish_p2m_mapping(struct domain *d);
> +
>  /* Allocate a new p2m table for a domain.
>   *
>   * Returns 0 for success or -errno.

Re: [PATCH v2 07/10] xen/arm: Introduce relinquish_p2m_mapping to remove refcount every mapped page

[Julien Grall]

On 12/09/2013 04:28 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>> This function will be called when the domain relinquishes its memory.
>> It removes refcount on every mapped page to a valid MFN.
>>
>> Currently, Xen doesn't take refcount on every new mapping but only for foreign
>> mapping. Restrict the function only on foreign mapping.
>
> Skimming the remainder of the patch's titles and recalling a previous
> conversation the intention is not to extend this for 4.4, correct?

Right, it's too big for Xen 4.4.

>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>      Changes in v2:
>>          - Introduce the patch
>> ---
>>   xen/arch/arm/domain.c        |    5 +++++
>>   xen/arch/arm/p2m.c           |   47 ++++++++++++++++++++++++++++++++++++++++++
>>   xen/include/asm-arm/domain.h |    1 +
>>   xen/include/asm-arm/p2m.h    |   15 ++++++++++++++
>>   4 files changed, 68 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 1590708..e7c2f67 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -717,6 +717,11 @@ int domain_relinquish_resources(struct domain *d)
>>           if ( ret )
>>               return ret;
>>
>> +    case RELMEM_mapping:
>
> Something somewhere should be setting d->arch.relmem = RELMEM_mapping at
> the appropriate time. (immediately above I think?)
>
> You also want a "Fallthrough" comment just above.

Oops, I will update the patch for the next version.

>> +        ret = relinquish_p2m_mapping(d);
>> +        if ( ret )
>> +            return ret;
>> +
>>           d->arch.relmem = RELMEM_done;
>>           /* Fallthrough */
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index f0bbaca..dbd6a06 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -6,6 +6,7 @@
>>   #include <xen/bitops.h>
>>   #include <asm/flushtlb.h>
>>   #include <asm/gic.h>
>> +#include <asm/event.h>
>>
>>   /* First level P2M is 2 consecutive pages */
>>   #define P2M_FIRST_ORDER 1
>> @@ -320,6 +321,16 @@ static int create_p2m_entries(struct domain *d,
>>               flush_tlb_all_local();
>>       }
>>
>> +    if ( (t == p2m_ram_rw) || (t == p2m_ram_ro) || (t == p2m_map_foreign))
>> +    {
>> +        unsigned long sgfn = paddr_to_pfn(start_gpaddr);
>> +        unsigned long egfn = paddr_to_pfn(end_gpaddr);
>> +
>> +        p2m->max_mapped_gfn = MAX(p2m->max_mapped_gfn, egfn);
>> +        /* Use next_gfn_to_relinquish to store the lowest gfn mapped */
>> +        p2m->next_gfn_to_relinquish = MIN(p2m->next_gfn_to_relinquish, sgfn);
>> +    }
>> +
>>       rc = 0;
>>
>>   out:
>> @@ -503,12 +514,48 @@ int p2m_init(struct domain *d)
>>
>>       p2m->first_level = NULL;
>>
>> +    p2m->max_mapped_gfn = 0;
>> +    p2m->next_gfn_to_relinquish = ULONG_MAX;
>> +
>>   err:
>>       spin_unlock(&p2m->lock);
>>
>>       return rc;
>>   }
>>
>> +int relinquish_p2m_mapping(struct domain *d)
>> +{
>> +    struct p2m_domain *p2m = &d->arch.p2m;
>> +    unsigned long gfn, count = 0;
>> +    int rc = 0;
>> +
>> +    for ( gfn = p2m->next_gfn_to_relinquish;
>> +          gfn < p2m->max_mapped_gfn; gfn++ )
>
> I know that Tim has been keen to get rid of these sorts of loops on x86,
> and with good reason I think.
>
>> +    {
>> +        p2m_type_t t;
>> +        paddr_t p = p2m_lookup(d, gfn, &t);
>
> This does the full walk for each address, even though 2/3 of the levels
> are more than likely identical to the previous gfn.
>
> It would be better to do a full walk, which sadly will look a lot like
> p2m_lookup, no avoiding that I think.
>
> You can still resume the walk based on next_gfn_to_relinquish and bound
> it on max_mapped_gfn, although I don't think it is strictly necessary.
>> +        unsigned long mfn = p >> PAGE_SHIFT;
>> +
>> +        if ( mfn_valid(mfn) && p2m_is_foreign(t) )
>
> I think it would be worth reiterating in a comment that we only take a
> ref for foreign mappings right now.

Will do.

>> +        {
>> +            put_page(mfn_to_page(mfn));
>> +            guest_physmap_remove_page(d, gfn, mfn, 0);
>
> You should unmap it and then put it I think.
>
> Is this going to do yet another lookup/walk?
>
> The REMOVE case of create_p2m_entries is so trivial you could open code
> it here, or if you wanted to you could refactor it into a helper.
>
> I am wondering if the conditional put page ought to be at the point of
> removal (i.e. in the helper) rather than here. (I think Tim made a
> similar comment on the x86 version of the remove_from_physmap pvh
> patches, you probably need to match the generic change which that
> implies)
>
> BTW, if you do the clear (but not the put_page) for every entry then the
> present bit (or pte.bits == 0) might be a useful proxy for
> next_lN_entry? I suppose even walking, say, 4GB of mostly empty P2M is
> not going to be super cheap.

Following the discution we had IRL, I will try to extend 
create_p2m_entries by adding RELINQUISH option.

>> +        }
>> +
>> +        count++;
>> +
>> +        /* Preempt every 2MiB. Arbitrary */
>> +        if ( (count == 512) && hypercall_preempt_check() )
>> +        {
>> +            p2m->next_gfn_to_relinquish = gfn + 1;
>> +            rc = -EAGAIN;
>
> I think I'm just failing to find it, but where is the call to
> hypercall_create_continuation? I suppose it is somewhere way up the
> stack?

Actually rc = -EAGAIN will be catched by xc_domain_destroy in libxc. The 
function will call again the hypercall if needed.

I'm not sure why... do you have any idea why x86 also uses this trick?

> I'm not sure the count == 512 is needed -- hypercall_preempt_check
> should be sufficient?

On ARM hypercall_preempt_check is a bit complex. Checking every loop 
seems a bit overkill.

-- 
Julien Grall

Re: [PATCH v2 07/10] xen/arm: Introduce relinquish_p2m_mapping to remove refcount every mapped page

[Julien Grall]

On 12/10/2013 01:31 AM, Julien Grall wrote:
>
>
> On 12/09/2013 04:28 PM, Ian Campbell wrote:
>> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>>> This function will be called when the domain relinquishes its memory.
>>> It removes refcount on every mapped page to a valid MFN.
>>>
>>> Currently, Xen doesn't take refcount on every new mapping but only
>>> for foreign
>>> mapping. Restrict the function only on foreign mapping.
>>
>> Skimming the remainder of the patch's titles and recalling a previous
>> conversation the intention is not to extend this for 4.4, correct?
>
> Right, it's too big for Xen 4.4.
>
>>>
>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>
>>> ---
>>>      Changes in v2:
>>>          - Introduce the patch
>>> ---
>>>   xen/arch/arm/domain.c        |    5 +++++
>>>   xen/arch/arm/p2m.c           |   47
>>> ++++++++++++++++++++++++++++++++++++++++++
>>>   xen/include/asm-arm/domain.h |    1 +
>>>   xen/include/asm-arm/p2m.h    |   15 ++++++++++++++
>>>   4 files changed, 68 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>>> index 1590708..e7c2f67 100644
>>> --- a/xen/arch/arm/domain.c
>>> +++ b/xen/arch/arm/domain.c
>>> @@ -717,6 +717,11 @@ int domain_relinquish_resources(struct domain *d)
>>>           if ( ret )
>>>               return ret;
>>>
>>> +    case RELMEM_mapping:
>>
>> Something somewhere should be setting d->arch.relmem = RELMEM_mapping at
>> the appropriate time. (immediately above I think?)
>>
>> You also want a "Fallthrough" comment just above.
>
> Oops, I will update the patch for the next version.
>
>>> +        ret = relinquish_p2m_mapping(d);
>>> +        if ( ret )
>>> +            return ret;
>>> +
>>>           d->arch.relmem = RELMEM_done;
>>>           /* Fallthrough */
>>>
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index f0bbaca..dbd6a06 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -6,6 +6,7 @@
>>>   #include <xen/bitops.h>
>>>   #include <asm/flushtlb.h>
>>>   #include <asm/gic.h>
>>> +#include <asm/event.h>
>>>
>>>   /* First level P2M is 2 consecutive pages */
>>>   #define P2M_FIRST_ORDER 1
>>> @@ -320,6 +321,16 @@ static int create_p2m_entries(struct domain *d,
>>>               flush_tlb_all_local();
>>>       }
>>>
>>> +    if ( (t == p2m_ram_rw) || (t == p2m_ram_ro) || (t ==
>>> p2m_map_foreign))
>>> +    {
>>> +        unsigned long sgfn = paddr_to_pfn(start_gpaddr);
>>> +        unsigned long egfn = paddr_to_pfn(end_gpaddr);
>>> +
>>> +        p2m->max_mapped_gfn = MAX(p2m->max_mapped_gfn, egfn);
>>> +        /* Use next_gfn_to_relinquish to store the lowest gfn mapped */
>>> +        p2m->next_gfn_to_relinquish =
>>> MIN(p2m->next_gfn_to_relinquish, sgfn);
>>> +    }
>>> +
>>>       rc = 0;
>>>
>>>   out:
>>> @@ -503,12 +514,48 @@ int p2m_init(struct domain *d)
>>>
>>>       p2m->first_level = NULL;
>>>
>>> +    p2m->max_mapped_gfn = 0;
>>> +    p2m->next_gfn_to_relinquish = ULONG_MAX;
>>> +
>>>   err:
>>>       spin_unlock(&p2m->lock);
>>>
>>>       return rc;
>>>   }
>>>
>>> +int relinquish_p2m_mapping(struct domain *d)
>>> +{
>>> +    struct p2m_domain *p2m = &d->arch.p2m;
>>> +    unsigned long gfn, count = 0;
>>> +    int rc = 0;
>>> +
>>> +    for ( gfn = p2m->next_gfn_to_relinquish;
>>> +          gfn < p2m->max_mapped_gfn; gfn++ )
>>
>> I know that Tim has been keen to get rid of these sorts of loops on x86,
>> and with good reason I think.
>>
>>> +    {
>>> +        p2m_type_t t;
>>> +        paddr_t p = p2m_lookup(d, gfn, &t);
>>
>> This does the full walk for each address, even though 2/3 of the levels
>> are more than likely identical to the previous gfn.
>>
>> It would be better to do a full walk, which sadly will look a lot like
>> p2m_lookup, no avoiding that I think.
>>
>> You can still resume the walk based on next_gfn_to_relinquish and bound
>> it on max_mapped_gfn, although I don't think it is strictly necessary.
>>> +        unsigned long mfn = p >> PAGE_SHIFT;
>>> +
>>> +        if ( mfn_valid(mfn) && p2m_is_foreign(t) )
>>
>> I think it would be worth reiterating in a comment that we only take a
>> ref for foreign mappings right now.
>
> Will do.
>
>>> +        {
>>> +            put_page(mfn_to_page(mfn));
>>> +            guest_physmap_remove_page(d, gfn, mfn, 0);
>>
>> You should unmap it and then put it I think.
>>
>> Is this going to do yet another lookup/walk?
>>
>> The REMOVE case of create_p2m_entries is so trivial you could open code
>> it here, or if you wanted to you could refactor it into a helper.
>>
>> I am wondering if the conditional put page ought to be at the point of
>> removal (i.e. in the helper) rather than here. (I think Tim made a
>> similar comment on the x86 version of the remove_from_physmap pvh
>> patches, you probably need to match the generic change which that
>> implies)
>>
>> BTW, if you do the clear (but not the put_page) for every entry then the
>> present bit (or pte.bits == 0) might be a useful proxy for
>> next_lN_entry? I suppose even walking, say, 4GB of mostly empty P2M is
>> not going to be super cheap.
>
> Following the discution we had IRL, I will try to extend
> create_p2m_entries by adding RELINQUISH option.
>
>>> +        }
>>> +
>>> +        count++;
>>> +
>>> +        /* Preempt every 2MiB. Arbitrary */
>>> +        if ( (count == 512) && hypercall_preempt_check() )
>>> +        {
>>> +            p2m->next_gfn_to_relinquish = gfn + 1;
>>> +            rc = -EAGAIN;
>>
>> I think I'm just failing to find it, but where is the call to
>> hypercall_create_continuation? I suppose it is somewhere way up the
>> stack?
>
> Actually rc = -EAGAIN will be catched by xc_domain_destroy in libxc. The
> function will call again the hypercall if needed.
>
> I'm not sure why... do you have any idea why x86 also uses this trick?
>
>> I'm not sure the count == 512 is needed -- hypercall_preempt_check
>> should be sufficient?
>
> On ARM hypercall_preempt_check is a bit complex. Checking every loop
> seems a bit overkill.
>

So I gave a try to remove the (count == 512) and it's very very slow 
(more than 10 times slower). So I will keep it.

-- 
Julien Grall

Re: [PATCH v2 07/10] xen/arm: Introduce relinquish_p2m_mapping to remove refcount every mapped page

[Ian Campbell]

On Tue, 2013-12-10 at 01:31 +0000, Julien Grall wrote:
> 
> On 12/09/2013 04:28 PM, Ian Campbell wrote:
> > On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> >> This function will be called when the domain relinquishes its memory.
> >> It removes refcount on every mapped page to a valid MFN.
> >>
> >> Currently, Xen doesn't take refcount on every new mapping but only for foreign
> >> mapping. Restrict the function only on foreign mapping.
> >
> > Skimming the remainder of the patch's titles and recalling a previous
> > conversation the intention is not to extend this for 4.4, correct?
> 
> Right, it's too big for Xen 4.4.
> 
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >>
> >> ---
> >>      Changes in v2:
> >>          - Introduce the patch
> >> ---
> >>   xen/arch/arm/domain.c        |    5 +++++
> >>   xen/arch/arm/p2m.c           |   47 ++++++++++++++++++++++++++++++++++++++++++
> >>   xen/include/asm-arm/domain.h |    1 +
> >>   xen/include/asm-arm/p2m.h    |   15 ++++++++++++++
> >>   4 files changed, 68 insertions(+)
> >>
> >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> >> index 1590708..e7c2f67 100644
> >> --- a/xen/arch/arm/domain.c
> >> +++ b/xen/arch/arm/domain.c
> >> @@ -717,6 +717,11 @@ int domain_relinquish_resources(struct domain *d)
> >>           if ( ret )
> >>               return ret;
> >>
> >> +    case RELMEM_mapping:
> >
> > Something somewhere should be setting d->arch.relmem = RELMEM_mapping at
> > the appropriate time. (immediately above I think?)
> >
> > You also want a "Fallthrough" comment just above.
> 
> Oops, I will update the patch for the next version.
> 
> >> +        ret = relinquish_p2m_mapping(d);
> >> +        if ( ret )
> >> +            return ret;
> >> +
> >>           d->arch.relmem = RELMEM_done;
> >>           /* Fallthrough */
> >>
> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> >> index f0bbaca..dbd6a06 100644
> >> --- a/xen/arch/arm/p2m.c
> >> +++ b/xen/arch/arm/p2m.c
> >> @@ -6,6 +6,7 @@
> >>   #include <xen/bitops.h>
> >>   #include <asm/flushtlb.h>
> >>   #include <asm/gic.h>
> >> +#include <asm/event.h>
> >>
> >>   /* First level P2M is 2 consecutive pages */
> >>   #define P2M_FIRST_ORDER 1
> >> @@ -320,6 +321,16 @@ static int create_p2m_entries(struct domain *d,
> >>               flush_tlb_all_local();
> >>       }
> >>
> >> +    if ( (t == p2m_ram_rw) || (t == p2m_ram_ro) || (t == p2m_map_foreign))
> >> +    {
> >> +        unsigned long sgfn = paddr_to_pfn(start_gpaddr);
> >> +        unsigned long egfn = paddr_to_pfn(end_gpaddr);
> >> +
> >> +        p2m->max_mapped_gfn = MAX(p2m->max_mapped_gfn, egfn);
> >> +        /* Use next_gfn_to_relinquish to store the lowest gfn mapped */
> >> +        p2m->next_gfn_to_relinquish = MIN(p2m->next_gfn_to_relinquish, sgfn);
> >> +    }
> >> +
> >>       rc = 0;
> >>
> >>   out:
> >> @@ -503,12 +514,48 @@ int p2m_init(struct domain *d)
> >>
> >>       p2m->first_level = NULL;
> >>
> >> +    p2m->max_mapped_gfn = 0;
> >> +    p2m->next_gfn_to_relinquish = ULONG_MAX;
> >> +
> >>   err:
> >>       spin_unlock(&p2m->lock);
> >>
> >>       return rc;
> >>   }
> >>
> >> +int relinquish_p2m_mapping(struct domain *d)
> >> +{
> >> +    struct p2m_domain *p2m = &d->arch.p2m;
> >> +    unsigned long gfn, count = 0;
> >> +    int rc = 0;
> >> +
> >> +    for ( gfn = p2m->next_gfn_to_relinquish;
> >> +          gfn < p2m->max_mapped_gfn; gfn++ )
> >
> > I know that Tim has been keen to get rid of these sorts of loops on x86,
> > and with good reason I think.
> >
> >> +    {
> >> +        p2m_type_t t;
> >> +        paddr_t p = p2m_lookup(d, gfn, &t);
> >
> > This does the full walk for each address, even though 2/3 of the levels
> > are more than likely identical to the previous gfn.
> >
> > It would be better to do a full walk, which sadly will look a lot like
> > p2m_lookup, no avoiding that I think.
> >
> > You can still resume the walk based on next_gfn_to_relinquish and bound
> > it on max_mapped_gfn, although I don't think it is strictly necessary.
> >> +        unsigned long mfn = p >> PAGE_SHIFT;
> >> +
> >> +        if ( mfn_valid(mfn) && p2m_is_foreign(t) )
> >
> > I think it would be worth reiterating in a comment that we only take a
> > ref for foreign mappings right now.
> 
> Will do.
> 
> >> +        {
> >> +            put_page(mfn_to_page(mfn));
> >> +            guest_physmap_remove_page(d, gfn, mfn, 0);
> >
> > You should unmap it and then put it I think.
> >
> > Is this going to do yet another lookup/walk?
> >
> > The REMOVE case of create_p2m_entries is so trivial you could open code
> > it here, or if you wanted to you could refactor it into a helper.
> >
> > I am wondering if the conditional put page ought to be at the point of
> > removal (i.e. in the helper) rather than here. (I think Tim made a
> > similar comment on the x86 version of the remove_from_physmap pvh
> > patches, you probably need to match the generic change which that
> > implies)
> >
> > BTW, if you do the clear (but not the put_page) for every entry then the
> > present bit (or pte.bits == 0) might be a useful proxy for
> > next_lN_entry? I suppose even walking, say, 4GB of mostly empty P2M is
> > not going to be super cheap.
> 
> Following the discution we had IRL, I will try to extend 
> create_p2m_entries by adding RELINQUISH option.

Looking at it again this morning the main change in behaviour will be to
continue on non-present entries instead of either filling them in or
aborting. It'll be interesting to see how this new mode of operation
fits into the function...

> >> +        }
> >> +
> >> +        count++;
> >> +
> >> +        /* Preempt every 2MiB. Arbitrary */
> >> +        if ( (count == 512) && hypercall_preempt_check() )
> >> +        {
> >> +            p2m->next_gfn_to_relinquish = gfn + 1;
> >> +            rc = -EAGAIN;
> >
> > I think I'm just failing to find it, but where is the call to
> > hypercall_create_continuation? I suppose it is somewhere way up the
> > stack?
> 
> Actually rc = -EAGAIN will be catched by xc_domain_destroy in libxc. The 
> function will call again the hypercall if needed.

Ah, I only followed the call chain on the hypervisor side.

> I'm not sure why... do you have any idea why x86 also uses this trick?

I suspect that this operation is so long running that it is beneficial
to return all the way to userspace context in order to allow other
processes to be scheduled. Avoiding the kernel's softlockup timer etc.

A hypercall continuation only give the opportunity for the guest kernel
to process interrupts IIRC.

> > I'm not sure the count == 512 is needed -- hypercall_preempt_check
> > should be sufficient?
> 
> On ARM hypercall_preempt_check is a bit complex. Checking every loop 
> seems a bit overkill.

OK. I see now that existing code has a mixture, every 512 is fine.
Perhaps using LPAE_ENTRIES is a little bit less arbitrary (although it
remains almost completely so...)

Ian.

[PATCH v2 08/10] xen/arm: Implement xen_rem_foreign_from_p2m

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Introduce the patch
---
 xen/arch/arm/mm.c         |   16 ++++++++++++++++
 xen/include/asm-arm/p2m.h |    6 +-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 960c872..ba51f6e 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -968,6 +968,22 @@ void share_xen_page_with_privileged_guests(
     share_xen_page_with_guest(page, dom_xen, readonly);
 }
 
+int xenmem_rem_foreign_from_p2m(struct domain *d, unsigned long gpfn)
+{
+    unsigned long mfn = gmfn_to_mfn(d, gpfn);
+    if ( !mfn_valid(mfn) )
+    {
+        gdprintk(XENLOG_WARNING, "Invalid mfn for gpfn:%lx domid:%d\n",
+                 gpfn, d->domain_id);
+        return -EINVAL;
+    }
+
+    guest_physmap_remove_page(d, gpfn, mfn, 0);
+    put_page(mfn_to_page(mfn));
+
+    return 0;
+}
+
 static int xenmem_add_to_physmap_one(
     struct domain *d,
     uint16_t space,
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index b0d3aea..cdd80d8 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -152,11 +152,7 @@ static inline int get_page_and_type(struct page_info *page,
     return rc;
 }
 
-static inline int xenmem_rem_foreign_from_p2m(struct domain *d, 
-                                              unsigned long gpfn)
-{
-    return -ENOSYS;
-}
+int xenmem_rem_foreign_from_p2m(struct domain *d, unsigned long gpfn);
 
 #endif /* _XEN_P2M_H */
 
-- 
1.7.10.4

Re: [PATCH v2 08/10] xen/arm: Implement xen_rem_foreign_from_p2m

[Ian Campbell]

On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Introduce the patch
> ---
>  xen/arch/arm/mm.c         |   16 ++++++++++++++++
>  xen/include/asm-arm/p2m.h |    6 +-----
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 960c872..ba51f6e 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -968,6 +968,22 @@ void share_xen_page_with_privileged_guests(
>      share_xen_page_with_guest(page, dom_xen, readonly);
>  }
>  
> +int xenmem_rem_foreign_from_p2m(struct domain *d, unsigned long gpfn)
> +{
> +    unsigned long mfn = gmfn_to_mfn(d, gpfn);
> +    if ( !mfn_valid(mfn) )
> +    {
> +        gdprintk(XENLOG_WARNING, "Invalid mfn for gpfn:%lx domid:%d\n",
> +                 gpfn, d->domain_id);

Is this guest triggerable? Or does it indicate a mistake when we made
the mapping?

> +        return -EINVAL;
> +    }
> +
> +    guest_physmap_remove_page(d, gpfn, mfn, 0);
> +    put_page(mfn_to_page(mfn));

Some of this could become a common helper with create_p2m_entries and
the relinquish stuff in the previous patch.

> +
> +    return 0;
> +}
> +
>  static int xenmem_add_to_physmap_one(
>      struct domain *d,
>      uint16_t space,
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index b0d3aea..cdd80d8 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -152,11 +152,7 @@ static inline int get_page_and_type(struct page_info *page,
>      return rc;
>  }
>  
> -static inline int xenmem_rem_foreign_from_p2m(struct domain *d, 
> -                                              unsigned long gpfn)
> -{
> -    return -ENOSYS;
> -}
> +int xenmem_rem_foreign_from_p2m(struct domain *d, unsigned long gpfn);
>  
>  #endif /* _XEN_P2M_H */
>  

Re: [PATCH v2 08/10] xen/arm: Implement xen_rem_foreign_from_p2m

[Julien Grall]

On 12/09/2013 04:31 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>     Changes in v2:
>>         - Introduce the patch
>> ---
>>  xen/arch/arm/mm.c         |   16 ++++++++++++++++
>>  xen/include/asm-arm/p2m.h |    6 +-----
>>  2 files changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 960c872..ba51f6e 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -968,6 +968,22 @@ void share_xen_page_with_privileged_guests(
>>      share_xen_page_with_guest(page, dom_xen, readonly);
>>  }
>>  
>> +int xenmem_rem_foreign_from_p2m(struct domain *d, unsigned long gpfn)
>> +{
>> +    unsigned long mfn = gmfn_to_mfn(d, gpfn);
>> +    if ( !mfn_valid(mfn) )
>> +    {
>> +        gdprintk(XENLOG_WARNING, "Invalid mfn for gpfn:%lx domid:%d\n",
>> +                 gpfn, d->domain_id);
> 
> Is this guest triggerable? Or does it indicate a mistake when we made
> the mapping?

It indicates a mistake when we made the mapping. Perhaps an ASSERT is
better here.

> 
>> +        return -EINVAL;
>> +    }
>> +
>> +    guest_physmap_remove_page(d, gpfn, mfn, 0);
>> +    put_page(mfn_to_page(mfn));
> 
> Some of this could become a common helper with create_p2m_entries and
> the relinquish stuff in the previous patch.

The best solution is to move put_page in guest_physmap_remove_page. But
as we didn't plan to handle refcount for each mapping in Xen 4.4,
perhaps we can delay it?

-- 
Julien Grall

Re: [PATCH v2 08/10] xen/arm: Implement xen_rem_foreign_from_p2m

[Ian Campbell]

On Mon, 2013-12-09 at 17:08 +0000, Julien Grall wrote:
> On 12/09/2013 04:31 PM, Ian Campbell wrote:
> > On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >>
> >> ---
> >>     Changes in v2:
> >>         - Introduce the patch
> >> ---
> >>  xen/arch/arm/mm.c         |   16 ++++++++++++++++
> >>  xen/include/asm-arm/p2m.h |    6 +-----
> >>  2 files changed, 17 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> >> index 960c872..ba51f6e 100644
> >> --- a/xen/arch/arm/mm.c
> >> +++ b/xen/arch/arm/mm.c
> >> @@ -968,6 +968,22 @@ void share_xen_page_with_privileged_guests(
> >>      share_xen_page_with_guest(page, dom_xen, readonly);
> >>  }
> >>  
> >> +int xenmem_rem_foreign_from_p2m(struct domain *d, unsigned long gpfn)
> >> +{
> >> +    unsigned long mfn = gmfn_to_mfn(d, gpfn);
> >> +    if ( !mfn_valid(mfn) )
> >> +    {
> >> +        gdprintk(XENLOG_WARNING, "Invalid mfn for gpfn:%lx domid:%d\n",
> >> +                 gpfn, d->domain_id);
> > 
> > Is this guest triggerable? Or does it indicate a mistake when we made
> > the mapping?
> 
> It indicates a mistake when we made the mapping. Perhaps an ASSERT is
> better here.

In that case either works for me.

> > 
> >> +        return -EINVAL;
> >> +    }
> >> +
> >> +    guest_physmap_remove_page(d, gpfn, mfn, 0);
> >> +    put_page(mfn_to_page(mfn));
> > 
> > Some of this could become a common helper with create_p2m_entries and
> > the relinquish stuff in the previous patch.
> 
> The best solution is to move put_page in guest_physmap_remove_page. But
> as we didn't plan to handle refcount for each mapping in Xen 4.4,
> perhaps we can delay it?

You can move it and still have it be conditional I think, since g_p_r_p
can see the type in the pte, can't it?

Ian.

Re: [PATCH v2 08/10] xen/arm: Implement xen_rem_foreign_from_p2m

[Julien Grall]

On 12/09/2013 05:18 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 17:08 +0000, Julien Grall wrote:
>> On 12/09/2013 04:31 PM, Ian Campbell wrote:
>>> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:

[..]

>
>>>
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    guest_physmap_remove_page(d, gpfn, mfn, 0);
>>>> +    put_page(mfn_to_page(mfn));
>>>
>>> Some of this could become a common helper with create_p2m_entries and
>>> the relinquish stuff in the previous patch.
>>
>> The best solution is to move put_page in guest_physmap_remove_page. But
>> as we didn't plan to handle refcount for each mapping in Xen 4.4,
>> perhaps we can delay it?
>
> You can move it and still have it be conditional I think, since g_p_r_p
> can see the type in the pte, can't it?

I plan to move the put_page in create_p2m_entries and check pte type.

-- 
Julien Grall

[PATCH v2 09/10] xen/arm: Set foreign page type to p2m_map_foreign

[Julien Grall]

Xen needs to know that the current page belongs to another domain. Also take
the refcount to this page.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Even if gcc is buggy (see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501)
        define p2m type per mapspace to let the compiler warns about unitialized
        values.
---
 xen/arch/arm/mm.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index ba51f6e..b08ffa0 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -993,6 +993,7 @@ static int xenmem_add_to_physmap_one(
 {
     unsigned long mfn = 0;
     int rc;
+    p2m_type_t t;
 
     switch ( space )
     {
@@ -1025,18 +1026,22 @@ static int xenmem_add_to_physmap_one(
         
         d->arch.grant_table_gpfn[idx] = gpfn;
 
+        t = p2m_ram_rw;
+
         spin_unlock(&d->grant_table->lock);
         break;
     case XENMAPSPACE_shared_info:
-        if ( idx == 0 )
-            mfn = virt_to_mfn(d->shared_info);
-        else
+        if ( idx != 0 )
             return -EINVAL;
+
+        mfn = virt_to_mfn(d->shared_info);
+        t = p2m_ram_rw;
+
         break;
     case XENMAPSPACE_gmfn_foreign:
     {
-        paddr_t maddr;
         struct domain *od;
+        struct page_info *page;
         od = rcu_lock_domain_by_any_id(foreign_domid);
         if ( od == NULL )
             return -ESRCH;
@@ -1048,15 +1053,18 @@ static int xenmem_add_to_physmap_one(
             return rc;
         }
 
-        maddr = p2m_lookup(od, pfn_to_paddr(idx));
-        if ( maddr == INVALID_PADDR )
+        /* Take refcount to the foreign domain page.
+         * Refcount will be release in XENMEM_remove_from_physmap */
+        page = get_page_from_gfn(od, idx, NULL, P2M_ALLOC);
+        if ( !page )
         {
             dump_p2m_lookup(od, pfn_to_paddr(idx));
             rcu_unlock_domain(od);
             return -EINVAL;
         }
 
-        mfn = maddr >> PAGE_SHIFT;
+        mfn = page_to_mfn(page);
+        t = p2m_map_foreign;
 
         rcu_unlock_domain(od);
         break;
@@ -1067,7 +1075,7 @@ static int xenmem_add_to_physmap_one(
     }
 
     /* Map at new location. */
-    rc = guest_physmap_add_page(d, gpfn, mfn, 0);
+    rc = guest_physmap_add_entry(d, gpfn, mfn, 0, t);
 
     return rc;
 }
-- 
1.7.10.4

Re: [PATCH v2 09/10] xen/arm: Set foreign page type to p2m_map_foreign

[Ian Campbell]

On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> Xen needs to know that the current page belongs to another domain. Also take
> the refcount to this page.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Even if gcc is buggy (see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501)
>         define p2m type per mapspace to let the compiler warns about unitialized
>         values.
> ---
>  xen/arch/arm/mm.c |   24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index ba51f6e..b08ffa0 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -993,6 +993,7 @@ static int xenmem_add_to_physmap_one(
>  {
>      unsigned long mfn = 0;
>      int rc;
> +    p2m_type_t t;
>  
>      switch ( space )
>      {
> @@ -1025,18 +1026,22 @@ static int xenmem_add_to_physmap_one(
>          
>          d->arch.grant_table_gpfn[idx] = gpfn;
>  
> +        t = p2m_ram_rw;
> +
>          spin_unlock(&d->grant_table->lock);
>          break;
>      case XENMAPSPACE_shared_info:
> -        if ( idx == 0 )
> -            mfn = virt_to_mfn(d->shared_info);
> -        else
> +        if ( idx != 0 )
>              return -EINVAL;
> +
> +        mfn = virt_to_mfn(d->shared_info);
> +        t = p2m_ram_rw;
> +
>          break;
>      case XENMAPSPACE_gmfn_foreign:
>      {
> -        paddr_t maddr;
>          struct domain *od;
> +        struct page_info *page;
>          od = rcu_lock_domain_by_any_id(foreign_domid);
>          if ( od == NULL )
>              return -ESRCH;
> @@ -1048,15 +1053,18 @@ static int xenmem_add_to_physmap_one(
>              return rc;
>          }
>  
> -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
> -        if ( maddr == INVALID_PADDR )
> +        /* Take refcount to the foreign domain page.

"refcount on...". Or "Take a reference to..."

> +         * Refcount will be release in XENMEM_remove_from_physmap */

and a few other places...

Like with the unmap it might be best to push this reference manipulation
down into the eventual function which makes the mapping. That would keep
this stuff much more localised.

> +        page = get_page_from_gfn(od, idx, NULL, P2M_ALLOC);

This made me wonder what happens if the guest drops its mapping while
the foreign one is around. At least the page won't be freed until the
foreign mapping goes away, but it won't be what the tools think it is.

I think this is probably OK, or at least I'm not sure right now what we
could do about it.

> +        if ( !page )
>          {
>              dump_p2m_lookup(od, pfn_to_paddr(idx));
>              rcu_unlock_domain(od);
>              return -EINVAL;
>          }
>  
> -        mfn = maddr >> PAGE_SHIFT;
> +        mfn = page_to_mfn(page);
> +        t = p2m_map_foreign;
>  
>          rcu_unlock_domain(od);
>          break;
> @@ -1067,7 +1075,7 @@ static int xenmem_add_to_physmap_one(
>      }
>  
>      /* Map at new location. */
> -    rc = guest_physmap_add_page(d, gpfn, mfn, 0);
> +    rc = guest_physmap_add_entry(d, gpfn, mfn, 0, t);
>  
>      return rc;
>  }

Re: [PATCH v2 09/10] xen/arm: Set foreign page type to p2m_map_foreign

[Julien Grall]

On 12/09/2013 04:40 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>> Xen needs to know that the current page belongs to another domain. Also take
>> the refcount to this page.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>      Changes in v2:
>>          - Even if gcc is buggy (see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501)
>>          define p2m type per mapspace to let the compiler warns about unitialized
>>          values.
>> ---
>>   xen/arch/arm/mm.c |   24 ++++++++++++++++--------
>>   1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index ba51f6e..b08ffa0 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -993,6 +993,7 @@ static int xenmem_add_to_physmap_one(
>>   {
>>       unsigned long mfn = 0;
>>       int rc;
>> +    p2m_type_t t;
>>
>>       switch ( space )
>>       {
>> @@ -1025,18 +1026,22 @@ static int xenmem_add_to_physmap_one(
>>
>>           d->arch.grant_table_gpfn[idx] = gpfn;
>>
>> +        t = p2m_ram_rw;
>> +
>>           spin_unlock(&d->grant_table->lock);
>>           break;
>>       case XENMAPSPACE_shared_info:
>> -        if ( idx == 0 )
>> -            mfn = virt_to_mfn(d->shared_info);
>> -        else
>> +        if ( idx != 0 )
>>               return -EINVAL;
>> +
>> +        mfn = virt_to_mfn(d->shared_info);
>> +        t = p2m_ram_rw;
>> +
>>           break;
>>       case XENMAPSPACE_gmfn_foreign:
>>       {
>> -        paddr_t maddr;
>>           struct domain *od;
>> +        struct page_info *page;
>>           od = rcu_lock_domain_by_any_id(foreign_domid);
>>           if ( od == NULL )
>>               return -ESRCH;
>> @@ -1048,15 +1053,18 @@ static int xenmem_add_to_physmap_one(
>>               return rc;
>>           }
>>
>> -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
>> -        if ( maddr == INVALID_PADDR )
>> +        /* Take refcount to the foreign domain page.
>
> "refcount on...". Or "Take a reference to..."
>
>> +         * Refcount will be release in XENMEM_remove_from_physmap */
>
> and a few other places...
>
> Like with the unmap it might be best to push this reference manipulation
> down into the eventual function which makes the mapping. That would keep
> this stuff much more localised.
>
>> +        page = get_page_from_gfn(od, idx, NULL, P2M_ALLOC);
>
> This made me wonder what happens if the guest drops its mapping while
> the foreign one is around. At least the page won't be freed until the
> foreign mapping goes away, but it won't be what the tools think it is.

In this case, the domain will get wrong data and can crash. It won't 
corrupted the domain who mapped the foreign mapping.

-- 
Julien Grall

Re: [PATCH v2 09/10] xen/arm: Set foreign page type to p2m_map_foreign

[Julien Grall]

On 12/09/2013 04:40 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>> Xen needs to know that the current page belongs to another domain. Also take
>> the refcount to this page.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>      Changes in v2:
>>          - Even if gcc is buggy (see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501)
>>          define p2m type per mapspace to let the compiler warns about unitialized
>>          values.
>> ---
>>   xen/arch/arm/mm.c |   24 ++++++++++++++++--------
>>   1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index ba51f6e..b08ffa0 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -993,6 +993,7 @@ static int xenmem_add_to_physmap_one(
>>   {
>>       unsigned long mfn = 0;
>>       int rc;
>> +    p2m_type_t t;
>>
>>       switch ( space )
>>       {
>> @@ -1025,18 +1026,22 @@ static int xenmem_add_to_physmap_one(
>>
>>           d->arch.grant_table_gpfn[idx] = gpfn;
>>
>> +        t = p2m_ram_rw;
>> +
>>           spin_unlock(&d->grant_table->lock);
>>           break;
>>       case XENMAPSPACE_shared_info:
>> -        if ( idx == 0 )
>> -            mfn = virt_to_mfn(d->shared_info);
>> -        else
>> +        if ( idx != 0 )
>>               return -EINVAL;
>> +
>> +        mfn = virt_to_mfn(d->shared_info);
>> +        t = p2m_ram_rw;
>> +
>>           break;
>>       case XENMAPSPACE_gmfn_foreign:
>>       {
>> -        paddr_t maddr;
>>           struct domain *od;
>> +        struct page_info *page;
>>           od = rcu_lock_domain_by_any_id(foreign_domid);
>>           if ( od == NULL )
>>               return -ESRCH;
>> @@ -1048,15 +1053,18 @@ static int xenmem_add_to_physmap_one(
>>               return rc;
>>           }
>>
>> -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
>> -        if ( maddr == INVALID_PADDR )
>> +        /* Take refcount to the foreign domain page.
>
> "refcount on...". Or "Take a reference to..."
>
>> +         * Refcount will be release in XENMEM_remove_from_physmap */
>
> and a few other places...
>
> Like with the unmap it might be best to push this reference manipulation
> down into the eventual function which makes the mapping. That would keep
> this stuff much more localised.

Forget to answer to this part:

We need to take the reference with foreign domain in parameter, 
otherwise we don't really know if the foreign domain has unlikely 
removed the mapping between the hypercall was called and now.

-- 
Julien Grall

Re: [PATCH v2 09/10] xen/arm: Set foreign page type to p2m_map_foreign

[Ian Campbell]

On Tue, 2013-12-10 at 01:51 +0000, Julien Grall wrote:
> 
> On 12/09/2013 04:40 PM, Ian Campbell wrote:
> > On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> >> Xen needs to know that the current page belongs to another domain. Also take
> >> the refcount to this page.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >>
> >> ---
> >>      Changes in v2:
> >>          - Even if gcc is buggy (see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501)
> >>          define p2m type per mapspace to let the compiler warns about unitialized
> >>          values.
> >> ---
> >>   xen/arch/arm/mm.c |   24 ++++++++++++++++--------
> >>   1 file changed, 16 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> >> index ba51f6e..b08ffa0 100644
> >> --- a/xen/arch/arm/mm.c
> >> +++ b/xen/arch/arm/mm.c
> >> @@ -993,6 +993,7 @@ static int xenmem_add_to_physmap_one(
> >>   {
> >>       unsigned long mfn = 0;
> >>       int rc;
> >> +    p2m_type_t t;
> >>
> >>       switch ( space )
> >>       {
> >> @@ -1025,18 +1026,22 @@ static int xenmem_add_to_physmap_one(
> >>
> >>           d->arch.grant_table_gpfn[idx] = gpfn;
> >>
> >> +        t = p2m_ram_rw;
> >> +
> >>           spin_unlock(&d->grant_table->lock);
> >>           break;
> >>       case XENMAPSPACE_shared_info:
> >> -        if ( idx == 0 )
> >> -            mfn = virt_to_mfn(d->shared_info);
> >> -        else
> >> +        if ( idx != 0 )
> >>               return -EINVAL;
> >> +
> >> +        mfn = virt_to_mfn(d->shared_info);
> >> +        t = p2m_ram_rw;
> >> +
> >>           break;
> >>       case XENMAPSPACE_gmfn_foreign:
> >>       {
> >> -        paddr_t maddr;
> >>           struct domain *od;
> >> +        struct page_info *page;
> >>           od = rcu_lock_domain_by_any_id(foreign_domid);
> >>           if ( od == NULL )
> >>               return -ESRCH;
> >> @@ -1048,15 +1053,18 @@ static int xenmem_add_to_physmap_one(
> >>               return rc;
> >>           }
> >>
> >> -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
> >> -        if ( maddr == INVALID_PADDR )
> >> +        /* Take refcount to the foreign domain page.
> >
> > "refcount on...". Or "Take a reference to..."
> >
> >> +         * Refcount will be release in XENMEM_remove_from_physmap */
> >
> > and a few other places...
> >
> > Like with the unmap it might be best to push this reference manipulation
> > down into the eventual function which makes the mapping. That would keep
> > this stuff much more localised.
> 
> Forget to answer to this part:
> 
> We need to take the reference with foreign domain in parameter, 
> otherwise we don't really know if the foreign domain has unlikely 
> removed the mapping between the hypercall was called and now.

Are we not holding the p2m lock at this point?

We probably do need to do the final check and unmap with the lock held.
Either by moving the unmap within the same lock as the lookup or perhaps
with some sort of cmpxchg mechanism where we pass in the pte value we
think we are tearing down. Just checking the type may not be sufficient
if the guest tears down one foreign mapping and replaces it with
another.

Ian.

Re: [PATCH v2 09/10] xen/arm: Set foreign page type to p2m_map_foreign

[Julien Grall]

On 12/10/2013 09:37 AM, Ian Campbell wrote:
> On Tue, 2013-12-10 at 01:51 +0000, Julien Grall wrote:
>>
>> On 12/09/2013 04:40 PM, Ian Campbell wrote:
>>> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>>>> Xen needs to know that the current page belongs to another domain. Also take
>>>> the refcount to this page.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>>
>>>> ---
>>>>       Changes in v2:
>>>>           - Even if gcc is buggy (see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501)
>>>>           define p2m type per mapspace to let the compiler warns about unitialized
>>>>           values.
>>>> ---
>>>>    xen/arch/arm/mm.c |   24 ++++++++++++++++--------
>>>>    1 file changed, 16 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>>>> index ba51f6e..b08ffa0 100644
>>>> --- a/xen/arch/arm/mm.c
>>>> +++ b/xen/arch/arm/mm.c
>>>> @@ -993,6 +993,7 @@ static int xenmem_add_to_physmap_one(
>>>>    {
>>>>        unsigned long mfn = 0;
>>>>        int rc;
>>>> +    p2m_type_t t;
>>>>
>>>>        switch ( space )
>>>>        {
>>>> @@ -1025,18 +1026,22 @@ static int xenmem_add_to_physmap_one(
>>>>
>>>>            d->arch.grant_table_gpfn[idx] = gpfn;
>>>>
>>>> +        t = p2m_ram_rw;
>>>> +
>>>>            spin_unlock(&d->grant_table->lock);
>>>>            break;
>>>>        case XENMAPSPACE_shared_info:
>>>> -        if ( idx == 0 )
>>>> -            mfn = virt_to_mfn(d->shared_info);
>>>> -        else
>>>> +        if ( idx != 0 )
>>>>                return -EINVAL;
>>>> +
>>>> +        mfn = virt_to_mfn(d->shared_info);
>>>> +        t = p2m_ram_rw;
>>>> +
>>>>            break;
>>>>        case XENMAPSPACE_gmfn_foreign:
>>>>        {
>>>> -        paddr_t maddr;
>>>>            struct domain *od;
>>>> +        struct page_info *page;
>>>>            od = rcu_lock_domain_by_any_id(foreign_domid);
>>>>            if ( od == NULL )
>>>>                return -ESRCH;
>>>> @@ -1048,15 +1053,18 @@ static int xenmem_add_to_physmap_one(
>>>>                return rc;
>>>>            }
>>>>
>>>> -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
>>>> -        if ( maddr == INVALID_PADDR )
>>>> +        /* Take refcount to the foreign domain page.
>>>
>>> "refcount on...". Or "Take a reference to..."
>>>
>>>> +         * Refcount will be release in XENMEM_remove_from_physmap */
>>>
>>> and a few other places...
>>>
>>> Like with the unmap it might be best to push this reference manipulation
>>> down into the eventual function which makes the mapping. That would keep
>>> this stuff much more localised.
>>
>> Forget to answer to this part:
>>
>> We need to take the reference with foreign domain in parameter,
>> otherwise we don't really know if the foreign domain has unlikely
>> removed the mapping between the hypercall was called and now.
>
> Are we not holding the p2m lock at this point?

p2m lock of which domain? For now, neither of foreign and current domain 
lock are taken in this function.

> We probably do need to do the final check and unmap with the lock held.
> Either by moving the unmap within the same lock as the lookup or perhaps
> with some sort of cmpxchg mechanism where we pass in the pte value we
> think we are tearing down. Just checking the type may not be sufficient
> if the guest tears down one foreign mapping and replaces it with
> another.

I'm not sure to follow you, are you talking about remove the foreign 
mapping? If so, I have reworking all the removing code to include the 
put_page (see V3).

-- 
Julien Grall

Re: [PATCH v2 09/10] xen/arm: Set foreign page type to p2m_map_foreign

[Ian Campbell]

On Tue, 2013-12-10 at 14:44 +0000, Julien Grall wrote:
> 
> On 12/10/2013 09:37 AM, Ian Campbell wrote:
> > On Tue, 2013-12-10 at 01:51 +0000, Julien Grall wrote:
> >>
> >> On 12/09/2013 04:40 PM, Ian Campbell wrote:
> >>> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> >>>> Xen needs to know that the current page belongs to another domain. Also take
> >>>> the refcount to this page.
> >>>>
> >>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >>>>
> >>>> ---
> >>>>       Changes in v2:
> >>>>           - Even if gcc is buggy (see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501)
> >>>>           define p2m type per mapspace to let the compiler warns about unitialized
> >>>>           values.
> >>>> ---
> >>>>    xen/arch/arm/mm.c |   24 ++++++++++++++++--------
> >>>>    1 file changed, 16 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> >>>> index ba51f6e..b08ffa0 100644
> >>>> --- a/xen/arch/arm/mm.c
> >>>> +++ b/xen/arch/arm/mm.c
> >>>> @@ -993,6 +993,7 @@ static int xenmem_add_to_physmap_one(
> >>>>    {
> >>>>        unsigned long mfn = 0;
> >>>>        int rc;
> >>>> +    p2m_type_t t;
> >>>>
> >>>>        switch ( space )
> >>>>        {
> >>>> @@ -1025,18 +1026,22 @@ static int xenmem_add_to_physmap_one(
> >>>>
> >>>>            d->arch.grant_table_gpfn[idx] = gpfn;
> >>>>
> >>>> +        t = p2m_ram_rw;
> >>>> +
> >>>>            spin_unlock(&d->grant_table->lock);
> >>>>            break;
> >>>>        case XENMAPSPACE_shared_info:
> >>>> -        if ( idx == 0 )
> >>>> -            mfn = virt_to_mfn(d->shared_info);
> >>>> -        else
> >>>> +        if ( idx != 0 )
> >>>>                return -EINVAL;
> >>>> +
> >>>> +        mfn = virt_to_mfn(d->shared_info);
> >>>> +        t = p2m_ram_rw;
> >>>> +
> >>>>            break;
> >>>>        case XENMAPSPACE_gmfn_foreign:
> >>>>        {
> >>>> -        paddr_t maddr;
> >>>>            struct domain *od;
> >>>> +        struct page_info *page;
> >>>>            od = rcu_lock_domain_by_any_id(foreign_domid);
> >>>>            if ( od == NULL )
> >>>>                return -ESRCH;
> >>>> @@ -1048,15 +1053,18 @@ static int xenmem_add_to_physmap_one(
> >>>>                return rc;
> >>>>            }
> >>>>
> >>>> -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
> >>>> -        if ( maddr == INVALID_PADDR )
> >>>> +        /* Take refcount to the foreign domain page.
> >>>
> >>> "refcount on...". Or "Take a reference to..."
> >>>
> >>>> +         * Refcount will be release in XENMEM_remove_from_physmap */
> >>>
> >>> and a few other places...
> >>>
> >>> Like with the unmap it might be best to push this reference manipulation
> >>> down into the eventual function which makes the mapping. That would keep
> >>> this stuff much more localised.
> >>
> >> Forget to answer to this part:
> >>
> >> We need to take the reference with foreign domain in parameter,
> >> otherwise we don't really know if the foreign domain has unlikely
> >> removed the mapping between the hypercall was called and now.
> >
> > Are we not holding the p2m lock at this point?
> 
> p2m lock of which domain? For now, neither of foreign and current domain 
> lock are taken in this function.

Possibly both? We need to make sure that the foreign entry we are
referencing doesn't get pulled out from under us, and also protect
against simultaneous modification of the one we are adding two.

> > We probably do need to do the final check and unmap with the lock held.
> > Either by moving the unmap within the same lock as the lookup or perhaps
> > with some sort of cmpxchg mechanism where we pass in the pte value we
> > think we are tearing down. Just checking the type may not be sufficient
> > if the guest tears down one foreign mapping and replaces it with
> > another.
> 
> I'm not sure to follow you, are you talking about remove the foreign 
> mapping? If so, I have reworking all the removing code to include the 
> put_page (see V3).

This will probably help, by moving it within the locked region. On
teardown we only need to care about the p2m of the entry we are
removing, not the foreign one.

Ian.

[PATCH v2 10/10] xen/arm: grant-table: Support read-only mapping

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Use p2m grant type to map grant-table mapping
---
 xen/arch/arm/mm.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index b08ffa0..4dedfe7 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1297,19 +1297,17 @@ int create_grant_host_mapping(unsigned long addr, unsigned long frame,
                               unsigned int flags, unsigned int cache_flags)
 {
     int rc;
+    p2m_type_t t = p2m_grant_map_rw;
 
     if ( cache_flags  || (flags & ~GNTMAP_readonly) != GNTMAP_host_map )
         return GNTST_general_error;
 
-    /* XXX: read only mappings */
     if ( flags & GNTMAP_readonly )
-    {
-        gdprintk(XENLOG_WARNING, "read only mappings not implemented yet\n");
-        return GNTST_general_error;
-    }
+        t = p2m_grant_map_ro;
+
+    rc = guest_physmap_add_entry(current->domain, addr >> PAGE_SHIFT,
+                                 frame, 0, t);
 
-    rc = guest_physmap_add_page(current->domain,
-                                 addr >> PAGE_SHIFT, frame, 0);
     if ( rc )
         return GNTST_general_error;
     else
-- 
1.7.10.4

Re: [PATCH v2 10/10] xen/arm: grant-table: Support read-only mapping

[Ian Campbell]

On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian.

Re: [PATCH v2 00/10] xen/arm: Handle correctly foreign mapping

[George Dunlap]

On 12/09/2013 03:33 AM, Julien Grall wrote:
> Hello,
>
> This patch series aims to fix "Failed to unmap" message in dom0 when a guest is
> creating. Without this patch series, dom0 will leak memory each time a domain
> is created. It should be considered as a blocker for Xen 4.4 release.

Leaking physical address space (which is I think what happens here) 
certainly sounds like a bug to me.

It's a bit unfortunate that all of this work (both PVH dom0 and this fix 
for ARM) is still being sorted out at this stage in the game, but I 
agree that both are blockers.

Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>

  -George