[PATCH 0/3] ARM: Support read-only mapping in the grant table

[PATCH 0/3] ARM: Support read-only mapping in the grant table

[Julien Grall]

Hi,

This small patch series implements read-only mapping for the grant table.

The read-only mapping is used by block-front when persistent grants is not used.
It's the case when a freebsd domU is booting on Xen on ARM.

The first two patches extend the p2m interface to support read-only mapping.
The third patch add support to read-only mapping in the grant table.

Cheers,

Julien Grall (3):
  xen/arm: p2m: extend create_p2m_entries to support read-only mapping
  xen/arm: p2m: add guest_physmap_add_page_rw
  xen/arm: grant-table: Support read-only mapping

 xen/arch/arm/mm.c          | 12 ++++--------
 xen/arch/arm/p2m.c         | 30 ++++++++++++++++++++++--------
 xen/include/asm-arm/p2m.h  |  5 +++++
 xen/include/asm-arm/page.h |  5 +++--
 4 files changed, 34 insertions(+), 18 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] xen/arm: p2m: extend create_p2m_entries to support read-only mapping

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/p2m.c         | 19 +++++++++++--------
 xen/include/asm-arm/page.h |  5 +++--
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 2d09fef..fdbb07b 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -111,7 +111,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, 1);
 
     write_pte(entry, pte);
 
@@ -129,7 +129,8 @@ static int create_p2m_entries(struct domain *d,
                      paddr_t start_gpaddr,
                      paddr_t end_gpaddr,
                      paddr_t maddr,
-                     int mattr)
+                     int mattr,
+                     bool_t rw)
 {
     int rc, flush;
     struct p2m_domain *p2m = &d->arch.p2m;
@@ -201,14 +202,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, rw);
 
                     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, rw);
                     write_pte(&third[third_table_offset(addr)], pte);
                     maddr += PAGE_SIZE;
                 }
@@ -243,7 +245,7 @@ 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, 1);
 }
 
 int map_mmio_regions(struct domain *d,
@@ -251,7 +253,8 @@ 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, 1);
 }
 
 int guest_physmap_add_page(struct domain *d,
@@ -261,7 +264,7 @@ int guest_physmap_add_page(struct domain *d,
 {
     return create_p2m_entries(d, INSERT, gpfn << PAGE_SHIFT,
                               (gpfn + (1<<page_order)) << PAGE_SHIFT,
-                              mfn << PAGE_SHIFT, MATTR_MEM);
+                              mfn << PAGE_SHIFT, MATTR_MEM, 1);
 }
 
 void guest_physmap_remove_page(struct domain *d,
@@ -270,7 +273,7 @@ void guest_physmap_remove_page(struct domain *d,
 {
     create_p2m_entries(d, REMOVE, gpfn << PAGE_SHIFT,
                        (gpfn + (1<<page_order)) << PAGE_SHIFT,
-                       mfn << PAGE_SHIFT, MATTR_MEM);
+                       mfn << PAGE_SHIFT, MATTR_MEM, 0);
 }
 
 int p2m_alloc_table(struct domain *d)
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 3d0f8a9..3e9dda7 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -213,14 +213,15 @@ 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)
+static inline lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
+                                      bool_t rw)
 {
     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.write = !!rw,
         .p2m.read = 1,
         .p2m.mattr = mattr,
         .p2m.table = 1,
-- 
1.8.3.1

Re: [PATCH 1/3] xen/arm: p2m: extend create_p2m_entries to support read-only mapping

[Ian Campbell]

On Thu, 2013-10-24 at 10:02 +0100, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/p2m.c         | 19 +++++++++++--------
>  xen/include/asm-arm/page.h |  5 +++--
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 2d09fef..fdbb07b 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -111,7 +111,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, 1);

If this is a bool_t then true/false might be more appropriate? (and all
the other such too)

I have a weak preference for avoiding such boolean parameters, since it
is never obvious what a given 1/0/true/false is actually is without
having to look at the prototype. At least there is only 1 such param
here (now).

We don't need full p2m typing yet but perhaps we can make a start by
defining "typdef enum { p2m_ram_rw = N; p2m_ram_ro = M } p2m_type_t" for
this?

> @@ -213,14 +213,15 @@ 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)
> +static inline lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
> +                                      bool_t rw)
>  {
>      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.write = !!rw,

I don't think the !! is strictly necessary for either a bool_t nor a
single bit bitfield.

Ian.

Re: [PATCH 1/3] xen/arm: p2m: extend create_p2m_entries to support read-only mapping

[Julien Grall]

On 11/11/2013 01:23 PM, Ian Campbell wrote:
> On Thu, 2013-10-24 at 10:02 +0100, Julien Grall wrote:
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>   xen/arch/arm/p2m.c         | 19 +++++++++++--------
>>   xen/include/asm-arm/page.h |  5 +++--
>>   2 files changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 2d09fef..fdbb07b 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -111,7 +111,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, 1);
>
> If this is a bool_t then true/false might be more appropriate? (and all
> the other such too)
>
> I have a weak preference for avoiding such boolean parameters, since it
> is never obvious what a given 1/0/true/false is actually is without
> having to look at the prototype. At least there is only 1 such param
> here (now).
>
> We don't need full p2m typing yet but perhaps we can make a start by
> defining "typdef enum { p2m_ram_rw = N; p2m_ram_ro = M } p2m_type_t" for
> this?

Sounds good. I will update the patch series with that.

>> @@ -213,14 +213,15 @@ 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)
>> +static inline lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
>> +                                      bool_t rw)
>>   {
>>       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.write = !!rw,
>
> I don't think the !! is strictly necessary for either a bool_t nor a
> single bit bitfield.

I will use p2m_type_t here and check if the type is equal to p2m_ram_rw.

-- 
Julien Grall

[PATCH 2/3] xen/arm: p2m: add guest_physmap_add_page_rw

[Julien Grall]

This function allows Xen to map memory read/write or read-only to the
guest memory.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/p2m.c        | 11 +++++++++++
 xen/include/asm-arm/p2m.h |  5 +++++
 2 files changed, 16 insertions(+)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index fdbb07b..0c515e1 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -267,6 +267,17 @@ int guest_physmap_add_page(struct domain *d,
                               mfn << PAGE_SHIFT, MATTR_MEM, 1);
 }
 
+int guest_physmap_add_page_rw(struct domain *d,
+                              unsigned long gpfn,
+                              unsigned long mfn,
+                              unsigned int page_order,
+                              bool_t rw)
+{
+    return create_p2m_entries(d, INSERT, gpfn << PAGE_SHIFT,
+                              (gpfn + (1 << page_order)) << PAGE_SHIFT,
+                              mfn << PAGE_SHIFT, MATTR_MEM, 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/p2m.h b/xen/include/asm-arm/p2m.h
index c660820..250f916 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -54,6 +54,11 @@ int guest_physmap_add_page(struct domain *d,
                            unsigned long gfn,
                            unsigned long mfn,
                            unsigned int page_order);
+int guest_physmap_add_page_rw(struct domain *d,
+                              unsigned long gfn,
+                              unsigned long mfn,
+                              unsigned int page_order,
+                              bool_t rw);
 void guest_physmap_remove_page(struct domain *d,
                                unsigned long gpfn,
                                unsigned long mfn, unsigned int page_order);
-- 
1.8.3.1

Re: [PATCH 2/3] xen/arm: p2m: add guest_physmap_add_page_rw

[Ian Campbell]

On Thu, 2013-10-24 at 10:02 +0100, Julien Grall wrote:
> This function allows Xen to map memory read/write or read-only to the
> guest memory.

x86's equivalent seems to be guest_physmap_add_entry() which is used by
guest_physmap_add_page. Unless there is a reason to diverge it would be
nice to keep the arch API similar I think.

x86 also passes a p2m type as I suggested in my previous reply.

Ian.

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

[Julien Grall]

Use guest_physmap_add_page_rw to map the page in the guest with a
specific read-write attribute.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/mm.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index eaeb0c3..e45c6aa 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1262,19 +1262,15 @@ int create_grant_host_mapping(unsigned long addr, unsigned long frame,
                               unsigned int flags, unsigned int cache_flags)
 {
     int rc;
+    bool_t 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;
-    }
+    rw = !(flags & GNTMAP_readonly);
 
-    rc = guest_physmap_add_page(current->domain,
-                                 addr >> PAGE_SHIFT, frame, 0);
+    rc = guest_physmap_add_page_rw(current->domain,
+                                   addr >> PAGE_SHIFT, frame, 0, rw);
     if ( rc )
         return GNTST_general_error;
     else
-- 
1.8.3.1