[Xen-devel,RFC,14/16] xen/arm: p2m: Extend p2m_get_entry to return the value of bit[0] (valid bit)

Message ID 20181008183352.16291-15-julien.grall@arm.com
State Accepted
Commit 0bc1a7b3005c91695ddebf7841ef81d12e0e435e
Headers show
Series
  • xen/arm: Implement Set/Way operations
Related show

Commit Message

Julien Grall Oct. 8, 2018, 6:33 p.m.
With the recent changes, a P2M entry may be populated but may as not
valid. In some situation, it would be useful to know whether the entry
has been marked available to guest in order to perform a specific
action. So extend p2m_get_entry to return the value of bit[0] (valid bit).

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mem_access.c |  6 +++---
 xen/arch/arm/p2m.c        | 20 ++++++++++++++++----
 xen/include/asm-arm/p2m.h |  3 ++-
 3 files changed, 21 insertions(+), 8 deletions(-)

Comments

Stefano Stabellini Nov. 2, 2018, 11:44 p.m. | #1
On Mon, 8 Oct 2018, Julien Grall wrote:
> With the recent changes, a P2M entry may be populated but may as not
> valid. In some situation, it would be useful to know whether the entry
> has been marked available to guest in order to perform a specific
> action. So extend p2m_get_entry to return the value of bit[0] (valid bit).
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/mem_access.c |  6 +++---
>  xen/arch/arm/p2m.c        | 20 ++++++++++++++++----
>  xen/include/asm-arm/p2m.h |  3 ++-
>  3 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
> index 9239bdf323..f434510b2a 100644
> --- a/xen/arch/arm/mem_access.c
> +++ b/xen/arch/arm/mem_access.c
> @@ -70,7 +70,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
>           * No setting was found in the Radix tree. Check if the
>           * entry exists in the page-tables.
>           */
> -        mfn_t mfn = p2m_get_entry(p2m, gfn, NULL, NULL, NULL);
> +        mfn_t mfn = p2m_get_entry(p2m, gfn, NULL, NULL, NULL, NULL);
>  
>          if ( mfn_eq(mfn, INVALID_MFN) )
>              return -ESRCH;
> @@ -199,7 +199,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
>       * We had a mem_access permission limiting the access, but the page type
>       * could also be limiting, so we need to check that as well.
>       */
> -    mfn = p2m_get_entry(p2m, gfn, &t, NULL, NULL);
> +    mfn = p2m_get_entry(p2m, gfn, &t, NULL, NULL, NULL);
>      if ( mfn_eq(mfn, INVALID_MFN) )
>          goto err;
>  
> @@ -405,7 +405,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>            gfn = gfn_next_boundary(gfn, order) )
>      {
>          p2m_type_t t;
> -        mfn_t mfn = p2m_get_entry(p2m, gfn, &t, NULL, &order);
> +        mfn_t mfn = p2m_get_entry(p2m, gfn, &t, NULL, &order, NULL);
>  
>  
>          if ( !mfn_eq(mfn, INVALID_MFN) )
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 12b459924b..df6b48d73b 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -306,10 +306,14 @@ static int p2m_next_level(struct p2m_domain *p2m, bool read_only,
>   *
>   * If the entry is not present, INVALID_MFN will be returned and the
>   * page_order will be set according to the order of the invalid range.
> + *
> + * valid will contain the value of bit[0] (e.g valid bit) of the
> + * entry.
>   */
>  mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>                      p2m_type_t *t, p2m_access_t *a,
> -                    unsigned int *page_order)
> +                    unsigned int *page_order,
> +                    bool *valid)
>  {
>      paddr_t addr = gfn_to_gaddr(gfn);
>      unsigned int level = 0;
> @@ -317,6 +321,7 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>      int rc;
>      mfn_t mfn = INVALID_MFN;
>      p2m_type_t _t;
> +    bool _valid;
>  
>      /* Convenience aliases */
>      const unsigned int offsets[4] = {
> @@ -334,6 +339,10 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>  
>      *t = p2m_invalid;
>  
> +    /* Allow valid to be NULL */
> +    valid = valid?: &_valid;
> +    *valid = false;

Why not a simple:

  if ( valid )
    *valid = false;

especially given that you do the same if ( valid ) check below.
In fact, it doesn' look like we need _valid?


>      /* XXX: Check if the mapping is lower than the mapped gfn */
>  
>      /* This gfn is higher than the highest the p2m map currently holds */
> @@ -379,6 +388,9 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>           * to the GFN.
>           */
>          mfn = mfn_add(mfn, gfn_x(gfn) & ((1UL << level_orders[level]) - 1));
> +
> +        if ( valid )
> +            *valid = lpae_is_valid(entry);
>      }
>  
>  out_unmap:
> @@ -397,7 +409,7 @@ mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>  
>      p2m_read_lock(p2m);
> -    mfn = p2m_get_entry(p2m, gfn, t, NULL, NULL);
> +    mfn = p2m_get_entry(p2m, gfn, t, NULL, NULL, NULL);
>      p2m_read_unlock(p2m);
>  
>      return mfn;
> @@ -1464,7 +1476,7 @@ int relinquish_p2m_mapping(struct domain *d)
>      for ( ; gfn_x(start) < gfn_x(end);
>            start = gfn_next_boundary(start, order) )
>      {
> -        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order);
> +        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order, NULL);
>  
>          count++;
>          /*
> @@ -1527,7 +1539,7 @@ int p2m_cache_flush_range(struct domain *d, gfn_t start, gfn_t end)
>  
>      for ( ; gfn_x(start) < gfn_x(end); start = next_gfn )
>      {
> -        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order);
> +        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order, NULL);
>  
>          next_gfn = gfn_next_boundary(start, order);
>  
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index d7afa2bbe8..92213dd1ab 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -211,7 +211,8 @@ mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
>   */
>  mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>                      p2m_type_t *t, p2m_access_t *a,
> -                    unsigned int *page_order);
> +                    unsigned int *page_order,
> +                    bool *valid);
>  
>  /*
>   * Direct set a p2m entry: only for use by the P2M code.
> -- 
> 2.11.0
>
Julien Grall Nov. 5, 2018, 11:56 a.m. | #2
Hi Stefano,

On 02/11/2018 23:44, Stefano Stabellini wrote:
> On Mon, 8 Oct 2018, Julien Grall wrote:
>> With the recent changes, a P2M entry may be populated but may as not
>> valid. In some situation, it would be useful to know whether the entry
>> has been marked available to guest in order to perform a specific
>> action. So extend p2m_get_entry to return the value of bit[0] (valid bit).
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/mem_access.c |  6 +++---
>>   xen/arch/arm/p2m.c        | 20 ++++++++++++++++----
>>   xen/include/asm-arm/p2m.h |  3 ++-
>>   3 files changed, 21 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
>> index 9239bdf323..f434510b2a 100644
>> --- a/xen/arch/arm/mem_access.c
>> +++ b/xen/arch/arm/mem_access.c
>> @@ -70,7 +70,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
>>            * No setting was found in the Radix tree. Check if the
>>            * entry exists in the page-tables.
>>            */
>> -        mfn_t mfn = p2m_get_entry(p2m, gfn, NULL, NULL, NULL);
>> +        mfn_t mfn = p2m_get_entry(p2m, gfn, NULL, NULL, NULL, NULL);
>>   
>>           if ( mfn_eq(mfn, INVALID_MFN) )
>>               return -ESRCH;
>> @@ -199,7 +199,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
>>        * We had a mem_access permission limiting the access, but the page type
>>        * could also be limiting, so we need to check that as well.
>>        */
>> -    mfn = p2m_get_entry(p2m, gfn, &t, NULL, NULL);
>> +    mfn = p2m_get_entry(p2m, gfn, &t, NULL, NULL, NULL);
>>       if ( mfn_eq(mfn, INVALID_MFN) )
>>           goto err;
>>   
>> @@ -405,7 +405,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>>             gfn = gfn_next_boundary(gfn, order) )
>>       {
>>           p2m_type_t t;
>> -        mfn_t mfn = p2m_get_entry(p2m, gfn, &t, NULL, &order);
>> +        mfn_t mfn = p2m_get_entry(p2m, gfn, &t, NULL, &order, NULL);
>>   
>>   
>>           if ( !mfn_eq(mfn, INVALID_MFN) )
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 12b459924b..df6b48d73b 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -306,10 +306,14 @@ static int p2m_next_level(struct p2m_domain *p2m, bool read_only,
>>    *
>>    * If the entry is not present, INVALID_MFN will be returned and the
>>    * page_order will be set according to the order of the invalid range.
>> + *
>> + * valid will contain the value of bit[0] (e.g valid bit) of the
>> + * entry.
>>    */
>>   mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>>                       p2m_type_t *t, p2m_access_t *a,
>> -                    unsigned int *page_order)
>> +                    unsigned int *page_order,
>> +                    bool *valid)
>>   {
>>       paddr_t addr = gfn_to_gaddr(gfn);
>>       unsigned int level = 0;
>> @@ -317,6 +321,7 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>>       int rc;
>>       mfn_t mfn = INVALID_MFN;
>>       p2m_type_t _t;
>> +    bool _valid;
>>   
>>       /* Convenience aliases */
>>       const unsigned int offsets[4] = {
>> @@ -334,6 +339,10 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>>   
>>       *t = p2m_invalid;
>>   
>> +    /* Allow valid to be NULL */
>> +    valid = valid?: &_valid;
>> +    *valid = false;
> 
> Why not a simple:
> 
>    if ( valid )
>      *valid = false;
> 
> especially given that you do the same if ( valid ) check below.
> In fact, it doesn' look like we need _valid?

I thought I dropped the if ( valid ) below. I would actually prefer to keep 
_valid and avoid using if ( ... ) everywhere.

This makes the code slightly easier to follow.

> 
> 
>>       /* XXX: Check if the mapping is lower than the mapped gfn */
>>   
>>       /* This gfn is higher than the highest the p2m map currently holds */
>> @@ -379,6 +388,9 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>>            * to the GFN.
>>            */
>>           mfn = mfn_add(mfn, gfn_x(gfn) & ((1UL << level_orders[level]) - 1));
>> +
>> +        if ( valid )
>> +            *valid = lpae_is_valid(entry);
>>       }
>>   
>>   out_unmap:
>> @@ -397,7 +409,7 @@ mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>>       struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>   
>>       p2m_read_lock(p2m);
>> -    mfn = p2m_get_entry(p2m, gfn, t, NULL, NULL);
>> +    mfn = p2m_get_entry(p2m, gfn, t, NULL, NULL, NULL);
>>       p2m_read_unlock(p2m);
>>   
>>       return mfn;
>> @@ -1464,7 +1476,7 @@ int relinquish_p2m_mapping(struct domain *d)
>>       for ( ; gfn_x(start) < gfn_x(end);
>>             start = gfn_next_boundary(start, order) )
>>       {
>> -        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order);
>> +        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order, NULL);
>>   
>>           count++;
>>           /*
>> @@ -1527,7 +1539,7 @@ int p2m_cache_flush_range(struct domain *d, gfn_t start, gfn_t end)
>>   
>>       for ( ; gfn_x(start) < gfn_x(end); start = next_gfn )
>>       {
>> -        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order);
>> +        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order, NULL);
>>   
>>           next_gfn = gfn_next_boundary(start, order);
>>   
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index d7afa2bbe8..92213dd1ab 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -211,7 +211,8 @@ mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
>>    */
>>   mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>>                       p2m_type_t *t, p2m_access_t *a,
>> -                    unsigned int *page_order);
>> +                    unsigned int *page_order,
>> +                    bool *valid);
>>   
>>   /*
>>    * Direct set a p2m entry: only for use by the P2M code.
>> -- 
>> 2.11.0
>>

Cheers,
Stefano Stabellini Nov. 5, 2018, 5:31 p.m. | #3
On Mon, 5 Nov 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 02/11/2018 23:44, Stefano Stabellini wrote:
> > On Mon, 8 Oct 2018, Julien Grall wrote:
> > > With the recent changes, a P2M entry may be populated but may as not
> > > valid. In some situation, it would be useful to know whether the entry
> > > has been marked available to guest in order to perform a specific
> > > action. So extend p2m_get_entry to return the value of bit[0] (valid bit).
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > ---
> > >   xen/arch/arm/mem_access.c |  6 +++---
> > >   xen/arch/arm/p2m.c        | 20 ++++++++++++++++----
> > >   xen/include/asm-arm/p2m.h |  3 ++-
> > >   3 files changed, 21 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
> > > index 9239bdf323..f434510b2a 100644
> > > --- a/xen/arch/arm/mem_access.c
> > > +++ b/xen/arch/arm/mem_access.c
> > > @@ -70,7 +70,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t
> > > gfn,
> > >            * No setting was found in the Radix tree. Check if the
> > >            * entry exists in the page-tables.
> > >            */
> > > -        mfn_t mfn = p2m_get_entry(p2m, gfn, NULL, NULL, NULL);
> > > +        mfn_t mfn = p2m_get_entry(p2m, gfn, NULL, NULL, NULL, NULL);
> > >             if ( mfn_eq(mfn, INVALID_MFN) )
> > >               return -ESRCH;
> > > @@ -199,7 +199,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva,
> > > unsigned long flag,
> > >        * We had a mem_access permission limiting the access, but the page
> > > type
> > >        * could also be limiting, so we need to check that as well.
> > >        */
> > > -    mfn = p2m_get_entry(p2m, gfn, &t, NULL, NULL);
> > > +    mfn = p2m_get_entry(p2m, gfn, &t, NULL, NULL, NULL);
> > >       if ( mfn_eq(mfn, INVALID_MFN) )
> > >           goto err;
> > >   @@ -405,7 +405,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn,
> > > uint32_t nr,
> > >             gfn = gfn_next_boundary(gfn, order) )
> > >       {
> > >           p2m_type_t t;
> > > -        mfn_t mfn = p2m_get_entry(p2m, gfn, &t, NULL, &order);
> > > +        mfn_t mfn = p2m_get_entry(p2m, gfn, &t, NULL, &order, NULL);
> > >               if ( !mfn_eq(mfn, INVALID_MFN) )
> > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > index 12b459924b..df6b48d73b 100644
> > > --- a/xen/arch/arm/p2m.c
> > > +++ b/xen/arch/arm/p2m.c
> > > @@ -306,10 +306,14 @@ static int p2m_next_level(struct p2m_domain *p2m,
> > > bool read_only,
> > >    *
> > >    * If the entry is not present, INVALID_MFN will be returned and the
> > >    * page_order will be set according to the order of the invalid range.
> > > + *
> > > + * valid will contain the value of bit[0] (e.g valid bit) of the
> > > + * entry.
> > >    */
> > >   mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
> > >                       p2m_type_t *t, p2m_access_t *a,
> > > -                    unsigned int *page_order)
> > > +                    unsigned int *page_order,
> > > +                    bool *valid)
> > >   {
> > >       paddr_t addr = gfn_to_gaddr(gfn);
> > >       unsigned int level = 0;
> > > @@ -317,6 +321,7 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
> > >       int rc;
> > >       mfn_t mfn = INVALID_MFN;
> > >       p2m_type_t _t;
> > > +    bool _valid;
> > >         /* Convenience aliases */
> > >       const unsigned int offsets[4] = {
> > > @@ -334,6 +339,10 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t
> > > gfn,
> > >         *t = p2m_invalid;
> > >   +    /* Allow valid to be NULL */
> > > +    valid = valid?: &_valid;
> > > +    *valid = false;
> > 
> > Why not a simple:
> > 
> >    if ( valid )
> >      *valid = false;
> > 
> > especially given that you do the same if ( valid ) check below.
> > In fact, it doesn' look like we need _valid?
> 
> I thought I dropped the if ( valid ) below. I would actually prefer to keep
> _valid and avoid using if ( ... ) everywhere.
> 
> This makes the code slightly easier to follow.

_valid is a good trick, but I don't think is worth doing it in this
change: it is easy to follow anyway. Up to you, I am fine either way.


> > 
> > 
> > >       /* XXX: Check if the mapping is lower than the mapped gfn */
> > >         /* This gfn is higher than the highest the p2m map currently holds
> > > */
> > > @@ -379,6 +388,9 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
> > >            * to the GFN.
> > >            */
> > >           mfn = mfn_add(mfn, gfn_x(gfn) & ((1UL << level_orders[level]) -
> > > 1));
> > > +
> > > +        if ( valid )
> > > +            *valid = lpae_is_valid(entry);
> > >       }
> > >     out_unmap:
> > > @@ -397,7 +409,7 @@ mfn_t p2m_lookup(struct domain *d, gfn_t gfn,
> > > p2m_type_t *t)
> > >       struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > >         p2m_read_lock(p2m);
> > > -    mfn = p2m_get_entry(p2m, gfn, t, NULL, NULL);
> > > +    mfn = p2m_get_entry(p2m, gfn, t, NULL, NULL, NULL);
> > >       p2m_read_unlock(p2m);
> > >         return mfn;
> > > @@ -1464,7 +1476,7 @@ int relinquish_p2m_mapping(struct domain *d)
> > >       for ( ; gfn_x(start) < gfn_x(end);
> > >             start = gfn_next_boundary(start, order) )
> > >       {
> > > -        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order);
> > > +        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order, NULL);
> > >             count++;
> > >           /*
> > > @@ -1527,7 +1539,7 @@ int p2m_cache_flush_range(struct domain *d, gfn_t
> > > start, gfn_t end)
> > >         for ( ; gfn_x(start) < gfn_x(end); start = next_gfn )
> > >       {
> > > -        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order);
> > > +        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order, NULL);
> > >             next_gfn = gfn_next_boundary(start, order);
> > >   diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> > > index d7afa2bbe8..92213dd1ab 100644
> > > --- a/xen/include/asm-arm/p2m.h
> > > +++ b/xen/include/asm-arm/p2m.h
> > > @@ -211,7 +211,8 @@ mfn_t p2m_lookup(struct domain *d, gfn_t gfn,
> > > p2m_type_t *t);
> > >    */
> > >   mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
> > >                       p2m_type_t *t, p2m_access_t *a,
> > > -                    unsigned int *page_order);
> > > +                    unsigned int *page_order,
> > > +                    bool *valid);
> > >     /*
> > >    * Direct set a p2m entry: only for use by the P2M code.
> > > -- 
> > > 2.11.0
> > > 
> 
> Cheers,
> 
> -- 
> Julien Grall
>
Julien Grall Nov. 5, 2018, 5:32 p.m. | #4
Hi Stefano,

On 05/11/2018 17:31, Stefano Stabellini wrote:
> On Mon, 5 Nov 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 02/11/2018 23:44, Stefano Stabellini wrote:
>>> On Mon, 8 Oct 2018, Julien Grall wrote:
>>>> With the recent changes, a P2M entry may be populated but may as not
>>>> valid. In some situation, it would be useful to know whether the entry
>>>> has been marked available to guest in order to perform a specific
>>>> action. So extend p2m_get_entry to return the value of bit[0] (valid bit).
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>>    xen/arch/arm/mem_access.c |  6 +++---
>>>>    xen/arch/arm/p2m.c        | 20 ++++++++++++++++----
>>>>    xen/include/asm-arm/p2m.h |  3 ++-
>>>>    3 files changed, 21 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
>>>> index 9239bdf323..f434510b2a 100644
>>>> --- a/xen/arch/arm/mem_access.c
>>>> +++ b/xen/arch/arm/mem_access.c
>>>> @@ -70,7 +70,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t
>>>> gfn,
>>>>             * No setting was found in the Radix tree. Check if the
>>>>             * entry exists in the page-tables.
>>>>             */
>>>> -        mfn_t mfn = p2m_get_entry(p2m, gfn, NULL, NULL, NULL);
>>>> +        mfn_t mfn = p2m_get_entry(p2m, gfn, NULL, NULL, NULL, NULL);
>>>>              if ( mfn_eq(mfn, INVALID_MFN) )
>>>>                return -ESRCH;
>>>> @@ -199,7 +199,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva,
>>>> unsigned long flag,
>>>>         * We had a mem_access permission limiting the access, but the page
>>>> type
>>>>         * could also be limiting, so we need to check that as well.
>>>>         */
>>>> -    mfn = p2m_get_entry(p2m, gfn, &t, NULL, NULL);
>>>> +    mfn = p2m_get_entry(p2m, gfn, &t, NULL, NULL, NULL);
>>>>        if ( mfn_eq(mfn, INVALID_MFN) )
>>>>            goto err;
>>>>    @@ -405,7 +405,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn,
>>>> uint32_t nr,
>>>>              gfn = gfn_next_boundary(gfn, order) )
>>>>        {
>>>>            p2m_type_t t;
>>>> -        mfn_t mfn = p2m_get_entry(p2m, gfn, &t, NULL, &order);
>>>> +        mfn_t mfn = p2m_get_entry(p2m, gfn, &t, NULL, &order, NULL);
>>>>                if ( !mfn_eq(mfn, INVALID_MFN) )
>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>> index 12b459924b..df6b48d73b 100644
>>>> --- a/xen/arch/arm/p2m.c
>>>> +++ b/xen/arch/arm/p2m.c
>>>> @@ -306,10 +306,14 @@ static int p2m_next_level(struct p2m_domain *p2m,
>>>> bool read_only,
>>>>     *
>>>>     * If the entry is not present, INVALID_MFN will be returned and the
>>>>     * page_order will be set according to the order of the invalid range.
>>>> + *
>>>> + * valid will contain the value of bit[0] (e.g valid bit) of the
>>>> + * entry.
>>>>     */
>>>>    mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>>>>                        p2m_type_t *t, p2m_access_t *a,
>>>> -                    unsigned int *page_order)
>>>> +                    unsigned int *page_order,
>>>> +                    bool *valid)
>>>>    {
>>>>        paddr_t addr = gfn_to_gaddr(gfn);
>>>>        unsigned int level = 0;
>>>> @@ -317,6 +321,7 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>>>>        int rc;
>>>>        mfn_t mfn = INVALID_MFN;
>>>>        p2m_type_t _t;
>>>> +    bool _valid;
>>>>          /* Convenience aliases */
>>>>        const unsigned int offsets[4] = {
>>>> @@ -334,6 +339,10 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t
>>>> gfn,
>>>>          *t = p2m_invalid;
>>>>    +    /* Allow valid to be NULL */
>>>> +    valid = valid?: &_valid;
>>>> +    *valid = false;
>>>
>>> Why not a simple:
>>>
>>>     if ( valid )
>>>       *valid = false;
>>>
>>> especially given that you do the same if ( valid ) check below.
>>> In fact, it doesn' look like we need _valid?
>>
>> I thought I dropped the if ( valid ) below. I would actually prefer to keep
>> _valid and avoid using if ( ... ) everywhere.
>>
>> This makes the code slightly easier to follow.
> 
> _valid is a good trick, but I don't think is worth doing it in this
> change: it is easy to follow anyway. Up to you, I am fine either way.

I will use if ( valid ) in the next version.

Cheers,

Patch

diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
index 9239bdf323..f434510b2a 100644
--- a/xen/arch/arm/mem_access.c
+++ b/xen/arch/arm/mem_access.c
@@ -70,7 +70,7 @@  static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
          * No setting was found in the Radix tree. Check if the
          * entry exists in the page-tables.
          */
-        mfn_t mfn = p2m_get_entry(p2m, gfn, NULL, NULL, NULL);
+        mfn_t mfn = p2m_get_entry(p2m, gfn, NULL, NULL, NULL, NULL);
 
         if ( mfn_eq(mfn, INVALID_MFN) )
             return -ESRCH;
@@ -199,7 +199,7 @@  p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
      * We had a mem_access permission limiting the access, but the page type
      * could also be limiting, so we need to check that as well.
      */
-    mfn = p2m_get_entry(p2m, gfn, &t, NULL, NULL);
+    mfn = p2m_get_entry(p2m, gfn, &t, NULL, NULL, NULL);
     if ( mfn_eq(mfn, INVALID_MFN) )
         goto err;
 
@@ -405,7 +405,7 @@  long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
           gfn = gfn_next_boundary(gfn, order) )
     {
         p2m_type_t t;
-        mfn_t mfn = p2m_get_entry(p2m, gfn, &t, NULL, &order);
+        mfn_t mfn = p2m_get_entry(p2m, gfn, &t, NULL, &order, NULL);
 
 
         if ( !mfn_eq(mfn, INVALID_MFN) )
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 12b459924b..df6b48d73b 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -306,10 +306,14 @@  static int p2m_next_level(struct p2m_domain *p2m, bool read_only,
  *
  * If the entry is not present, INVALID_MFN will be returned and the
  * page_order will be set according to the order of the invalid range.
+ *
+ * valid will contain the value of bit[0] (e.g valid bit) of the
+ * entry.
  */
 mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
                     p2m_type_t *t, p2m_access_t *a,
-                    unsigned int *page_order)
+                    unsigned int *page_order,
+                    bool *valid)
 {
     paddr_t addr = gfn_to_gaddr(gfn);
     unsigned int level = 0;
@@ -317,6 +321,7 @@  mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
     int rc;
     mfn_t mfn = INVALID_MFN;
     p2m_type_t _t;
+    bool _valid;
 
     /* Convenience aliases */
     const unsigned int offsets[4] = {
@@ -334,6 +339,10 @@  mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
 
     *t = p2m_invalid;
 
+    /* Allow valid to be NULL */
+    valid = valid?: &_valid;
+    *valid = false;
+
     /* XXX: Check if the mapping is lower than the mapped gfn */
 
     /* This gfn is higher than the highest the p2m map currently holds */
@@ -379,6 +388,9 @@  mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
          * to the GFN.
          */
         mfn = mfn_add(mfn, gfn_x(gfn) & ((1UL << level_orders[level]) - 1));
+
+        if ( valid )
+            *valid = lpae_is_valid(entry);
     }
 
 out_unmap:
@@ -397,7 +409,7 @@  mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
 
     p2m_read_lock(p2m);
-    mfn = p2m_get_entry(p2m, gfn, t, NULL, NULL);
+    mfn = p2m_get_entry(p2m, gfn, t, NULL, NULL, NULL);
     p2m_read_unlock(p2m);
 
     return mfn;
@@ -1464,7 +1476,7 @@  int relinquish_p2m_mapping(struct domain *d)
     for ( ; gfn_x(start) < gfn_x(end);
           start = gfn_next_boundary(start, order) )
     {
-        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order);
+        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order, NULL);
 
         count++;
         /*
@@ -1527,7 +1539,7 @@  int p2m_cache_flush_range(struct domain *d, gfn_t start, gfn_t end)
 
     for ( ; gfn_x(start) < gfn_x(end); start = next_gfn )
     {
-        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order);
+        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order, NULL);
 
         next_gfn = gfn_next_boundary(start, order);
 
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index d7afa2bbe8..92213dd1ab 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -211,7 +211,8 @@  mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
  */
 mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
                     p2m_type_t *t, p2m_access_t *a,
-                    unsigned int *page_order);
+                    unsigned int *page_order,
+                    bool *valid);
 
 /*
  * Direct set a p2m entry: only for use by the P2M code.