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

Message ID 1386560047-17500-5-git-send-email-julien.grall@linaro.org
State Superseded
Headers show

Commit Message

Julien Grall Dec. 9, 2013, 3:34 a.m.
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(-)

Comments

Ian Campbell Dec. 9, 2013, 4:03 p.m. | #1
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.
Julien Grall Dec. 9, 2013, 4:37 p.m. | #2
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.
Ian Campbell Dec. 9, 2013, 4:53 p.m. | #3
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.
Julien Grall Dec. 10, 2013, 1:55 a.m. | #4
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).
Ian Campbell Dec. 10, 2013, 9:37 a.m. | #5
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.
Julien Grall Dec. 10, 2013, 1:50 p.m. | #6
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.
Ian Campbell Dec. 10, 2013, 1:58 p.m. | #7
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

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;