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

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

Commit Message

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

Comments

Ian Campbell Dec. 9, 2013, 3:59 p.m. | #1
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)
>  {
Julien Grall Dec. 9, 2013, 4:34 p.m. | #2
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.
Ian Campbell Dec. 9, 2013, 4:54 p.m. | #3
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.
Julien Grall Dec. 10, 2013, 2:02 a.m. | #4
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.

Patch

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