diff mbox

[Xen-devel,09/22] xen/arm: p2m: Use a whitelist rather than blacklist in get_page_from_gfn

Message ID 1469031064-23344-10-git-send-email-julien.grall@arm.com
State Superseded
Headers show

Commit Message

Julien Grall July 20, 2016, 4:10 p.m. UTC
Currently, the check in get_page_from_gfn is using a blacklist. This is
very fragile because we may forgot to update the check when a new p2m
type is added.

To avoid any possible issue, use a whitelist. All type backed by a RAM
page can could potential be valid. The check is borrowed from x86.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/asm-arm/p2m.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Julien Grall July 27, 2016, 9:59 a.m. UTC | #1
On 26/07/16 23:44, Stefano Stabellini wrote:
> On Wed, 20 Jul 2016, Julien Grall wrote:
>> Currently, the check in get_page_from_gfn is using a blacklist. This is
>> very fragile because we may forgot to update the check when a new p2m
>> type is added.
>>
>> To avoid any possible issue, use a whitelist. All type backed by a RAM
>> page can could potential be valid. The check is borrowed from x86.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>  xen/include/asm-arm/p2m.h | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index 3091c04..78d37ab 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -104,9 +104,16 @@ typedef enum {
>>  #define P2M_RAM_TYPES (p2m_to_mask(p2m_ram_rw) |        \
>>                         p2m_to_mask(p2m_ram_ro))
>>
>> +/* Grant mapping types, which map to a real frame in another VM */
>> +#define P2M_GRANT_TYPES (p2m_to_mask(p2m_grant_map_rw) |  \
>> +                         p2m_to_mask(p2m_grant_map_ro))
>> +
>>  /* Useful predicates */
>>  #define p2m_is_ram(_t) (p2m_to_mask(_t) & P2M_RAM_TYPES)
>>  #define p2m_is_foreign(_t) (p2m_to_mask(_t) & p2m_to_mask(p2m_map_foreign))
>> +#define p2m_is_any_ram(_t) (p2m_to_mask(_t) &                   \
>> +                            (P2M_RAM_TYPES | P2M_GRANT_TYPES |  \
>> +                             p2m_to_mask(p2m_map_foreign)))
>>
>>  static inline
>>  void p2m_mem_access_emulate_check(struct vcpu *v,
>> @@ -224,7 +231,7 @@ static inline struct page_info *get_page_from_gfn(
>>      if (t)
>>          *t = p2mt;
>>
>> -    if ( p2mt == p2m_invalid || p2mt == p2m_mmio_direct )
>> +    if ( !p2m_is_any_ram(p2mt) )
>>          return NULL;
>
> What about the iommu mappings?

iommu mappings (p2m_iommu_map_*) are special mappings for the direct 
mapping workaround. They should not be used in get_page_from_gfn.

Regards,
Julien Grall July 27, 2016, 5:57 p.m. UTC | #2
Hi Stefano,

On 27/07/16 18:56, Stefano Stabellini wrote:
> On Wed, 27 Jul 2016, Julien Grall wrote:
>> On 26/07/16 23:44, Stefano Stabellini wrote:
>>> On Wed, 20 Jul 2016, Julien Grall wrote:
>>>> Currently, the check in get_page_from_gfn is using a blacklist. This is
>>>> very fragile because we may forgot to update the check when a new p2m
>>>> type is added.
>>>>
>>>> To avoid any possible issue, use a whitelist. All type backed by a RAM
>>>> page can could potential be valid. The check is borrowed from x86.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>>  xen/include/asm-arm/p2m.h | 9 ++++++++-
>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>>>> index 3091c04..78d37ab 100644
>>>> --- a/xen/include/asm-arm/p2m.h
>>>> +++ b/xen/include/asm-arm/p2m.h
>>>> @@ -104,9 +104,16 @@ typedef enum {
>>>>  #define P2M_RAM_TYPES (p2m_to_mask(p2m_ram_rw) |        \
>>>>                         p2m_to_mask(p2m_ram_ro))
>>>>
>>>> +/* Grant mapping types, which map to a real frame in another VM */
>>>> +#define P2M_GRANT_TYPES (p2m_to_mask(p2m_grant_map_rw) |  \
>>>> +                         p2m_to_mask(p2m_grant_map_ro))
>>>> +
>>>>  /* Useful predicates */
>>>>  #define p2m_is_ram(_t) (p2m_to_mask(_t) & P2M_RAM_TYPES)
>>>>  #define p2m_is_foreign(_t) (p2m_to_mask(_t) &
>>>> p2m_to_mask(p2m_map_foreign))
>>>> +#define p2m_is_any_ram(_t) (p2m_to_mask(_t) &                   \
>>>> +                            (P2M_RAM_TYPES | P2M_GRANT_TYPES |  \
>>>> +                             p2m_to_mask(p2m_map_foreign)))
>>>>
>>>>  static inline
>>>>  void p2m_mem_access_emulate_check(struct vcpu *v,
>>>> @@ -224,7 +231,7 @@ static inline struct page_info *get_page_from_gfn(
>>>>      if (t)
>>>>          *t = p2mt;
>>>>
>>>> -    if ( p2mt == p2m_invalid || p2mt == p2m_mmio_direct )
>>>> +    if ( !p2m_is_any_ram(p2mt) )
>>>>          return NULL;
>>>
>>> What about the iommu mappings?
>>
>> iommu mappings (p2m_iommu_map_*) are special mappings for the direct mapping
>> workaround. They should not be used in get_page_from_gfn.
>
> Make sense. I think they deserve to be mentioned in the commit message,
> as this patch changes behavior for them.

Good point. I will update the commit message.

Regards,
diff mbox

Patch

diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 3091c04..78d37ab 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -104,9 +104,16 @@  typedef enum {
 #define P2M_RAM_TYPES (p2m_to_mask(p2m_ram_rw) |        \
                        p2m_to_mask(p2m_ram_ro))
 
+/* Grant mapping types, which map to a real frame in another VM */
+#define P2M_GRANT_TYPES (p2m_to_mask(p2m_grant_map_rw) |  \
+                         p2m_to_mask(p2m_grant_map_ro))
+
 /* Useful predicates */
 #define p2m_is_ram(_t) (p2m_to_mask(_t) & P2M_RAM_TYPES)
 #define p2m_is_foreign(_t) (p2m_to_mask(_t) & p2m_to_mask(p2m_map_foreign))
+#define p2m_is_any_ram(_t) (p2m_to_mask(_t) &                   \
+                            (P2M_RAM_TYPES | P2M_GRANT_TYPES |  \
+                             p2m_to_mask(p2m_map_foreign)))
 
 static inline
 void p2m_mem_access_emulate_check(struct vcpu *v,
@@ -224,7 +231,7 @@  static inline struct page_info *get_page_from_gfn(
     if (t)
         *t = p2mt;
 
-    if ( p2mt == p2m_invalid || p2mt == p2m_mmio_direct )
+    if ( !p2m_is_any_ram(p2mt) )
         return NULL;
 
     if ( !mfn_valid(mfn) )