[6/8] xen/arm: Retrieve p2m type in get_page_from_gfn

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/include/asm-arm/p2m.h |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/include/asm-arm/p2m.h |    5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 3de69c4..54d1dab 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -105,9 +105,8 @@ static inline struct page_info *get_page_from_gfn(
>      struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
>  {
>      struct page_info *page;
> -    unsigned long mfn = gmfn_to_mfn(d, gfn);
> -
> -    ASSERT(t == NULL);
> +    paddr_t maddr = p2m_get_entry(d, pfn_to_paddr(gfn), t);
> +    unsigned long mfn = maddr >> PAGE_SHIFT;

Do we need to eg. convert p2m_invalid into returning NULL instead of
whatever maddr contains in that case?

In the case of p2m_mmio_direct we need to be careful because there is no
struct page. Ah.. here it is in the context:
>      if (!mfn_valid(mfn))
>          return NULL;

Although I wonder if we should convert mmio_direct into NULL even if the
MMIO region happens to have a struct page? (e.g. due to holes in the
frametable)

Ian.

On 12/05/2013 04:23 PM, Ian Campbell wrote:
> On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>   xen/include/asm-arm/p2m.h |    5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index 3de69c4..54d1dab 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -105,9 +105,8 @@ static inline struct page_info *get_page_from_gfn(
>>       struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
>>   {
>>       struct page_info *page;
>> -    unsigned long mfn = gmfn_to_mfn(d, gfn);
>> -
>> -    ASSERT(t == NULL);
>> +    paddr_t maddr = p2m_get_entry(d, pfn_to_paddr(gfn), t);
>> +    unsigned long mfn = maddr >> PAGE_SHIFT;
>
> Do we need to eg. convert p2m_invalid into returning NULL instead of
> whatever maddr contains in that case?
>
> In the case of p2m_mmio_direct we need to be careful because there is no
> struct page. Ah.. here it is in the context:
>>       if (!mfn_valid(mfn))
>>           return NULL;
>
> Although I wonder if we should convert mmio_direct into NULL even if the
> MMIO region happens to have a struct page? (e.g. due to holes in the
> frametable)

I will do the both in the next version.

On 12/05/2013 04:23 PM, Ian Campbell wrote:
> On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>   xen/include/asm-arm/p2m.h |    5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index 3de69c4..54d1dab 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -105,9 +105,8 @@ static inline struct page_info *get_page_from_gfn(
>>       struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
>>   {
>>       struct page_info *page;
>> -    unsigned long mfn = gmfn_to_mfn(d, gfn);
>> -
>> -    ASSERT(t == NULL);
>> +    paddr_t maddr = p2m_get_entry(d, pfn_to_paddr(gfn), t);
>> +    unsigned long mfn = maddr >> PAGE_SHIFT;
>
> Do we need to eg. convert p2m_invalid into returning NULL instead of
> whatever maddr contains in that case?

In that case maddr will contain INVALID_PADDR, which will turn in 
INVALID_MFN with the shift. So we don't need to check the p2m type.

>
> In the case of p2m_mmio_direct we need to be careful because there is no
> struct page. Ah.. here it is in the context:
>>       if (!mfn_valid(mfn))
>>           return NULL;
>
> Although I wonder if we should convert mmio_direct into NULL even if the
> MMIO region happens to have a struct page? (e.g. due to holes in the
> frametable)

Actually, I think it's fine. get_page will fail because the page won't 
belong to the domain. So the function will return NULL.

On Mon, 2013-12-09 at 02:36 +0000, Julien Grall wrote:
> 
> On 12/05/2013 04:23 PM, Ian Campbell wrote:
> > On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >> ---
> >>   xen/include/asm-arm/p2m.h |    5 ++---
> >>   1 file changed, 2 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> >> index 3de69c4..54d1dab 100644
> >> --- a/xen/include/asm-arm/p2m.h
> >> +++ b/xen/include/asm-arm/p2m.h
> >> @@ -105,9 +105,8 @@ static inline struct page_info *get_page_from_gfn(
> >>       struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
> >>   {
> >>       struct page_info *page;
> >> -    unsigned long mfn = gmfn_to_mfn(d, gfn);
> >> -
> >> -    ASSERT(t == NULL);
> >> +    paddr_t maddr = p2m_get_entry(d, pfn_to_paddr(gfn), t);
> >> +    unsigned long mfn = maddr >> PAGE_SHIFT;
> >
> > Do we need to eg. convert p2m_invalid into returning NULL instead of
> > whatever maddr contains in that case?
> 
> In that case maddr will contain INVALID_PADDR, which will turn in 
> INVALID_MFN with the shift. So we don't need to check the p2m type.

INVALID_PADDR is ~0ULL (==64-bits of ones) and INVALID_MFN is ~0UL,
which could be either 32- or 64-bits of ones depending on arm32 vs
arm64.

So on arm32 the shift and size truncation from paddr to mfn will give us
32-bits of ones (assuming that isn't undefined behaviour somehow). But
on 64-bit the shift will give us zeroes at the top 12 bits, which is not
== INVALID_MFN

> 
> >
> > In the case of p2m_mmio_direct we need to be careful because there is no
> > struct page. Ah.. here it is in the context:
> >>       if (!mfn_valid(mfn))
> >>           return NULL;
> >
> > Although I wonder if we should convert mmio_direct into NULL even if the
> > MMIO region happens to have a struct page? (e.g. due to holes in the
> > frametable)
> 
> Actually, I think it's fine. get_page will fail because the page won't 
> belong to the domain. So the function will return NULL.

I'd rather be explicit that rely on such things, if possible, though.

If nothing else it makes the codes logic easier to understand.

Ian.