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

Message ID 1382605365-19244-2-git-send-email-julien.grall@linaro.org
State Superseded
Headers show

Commit Message

Julien Grall Oct. 24, 2013, 9:02 a.m.
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(-)

Comments

Ian Campbell Nov. 11, 2013, 1:23 p.m. | #1
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.
Julien Grall Nov. 12, 2013, 1:13 p.m. | #2
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.

Patch

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,