[v2,06/10] xen/arm: Retrieve p2m type in get_page_from_gfn

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

Commit Message

Julien Grall Dec. 9, 2013, 3:34 a.m.
Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Use p2m_lookup as p2m_get_entry was removed
---
 xen/include/asm-arm/p2m.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Ian Campbell Dec. 9, 2013, 4:06 p.m. | #1
On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Use p2m_lookup as p2m_get_entry was removed
> ---
>  xen/include/asm-arm/p2m.h |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index f4bcd7d..b63204d 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -109,7 +109,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);
> +    paddr_t maddr = p2m_lookup(d, pfn_to_paddr(gfn), t);
> +    unsigned long mfn = maddr >> PAGE_SHIFT;

I think this resend happened before I replied to the original a second
time, (in <1386583029.13126.13.camel@kazak.uk.xensource.com>) so you
hadn't seen it.

TL;DR: INVALID_MADDR >> PAGE_SHIFT != INVALID_MFN on arm64.

Ian.
Julien Grall Dec. 9, 2013, 4:50 p.m. | #2
On 12/09/2013 04:06 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>     Changes in v2:
>>         - Use p2m_lookup as p2m_get_entry was removed
>> ---
>>  xen/include/asm-arm/p2m.h |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index f4bcd7d..b63204d 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -109,7 +109,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);
>> +    paddr_t maddr = p2m_lookup(d, pfn_to_paddr(gfn), t);
>> +    unsigned long mfn = maddr >> PAGE_SHIFT;
> 
> I think this resend happened before I replied to the original a second
> time, (in <1386583029.13126.13.camel@kazak.uk.xensource.com>) so you
> hadn't seen it.

Indeed, I will fix it.

> TL;DR: INVALID_MADDR >> PAGE_SHIFT != INVALID_MFN on arm64.

Does it mean that INVALID_MFN can be a valid mfn on arm64? If so, we
have some place in common code where this constant is used (see
common/domain.c for instance).
Ian Campbell Dec. 9, 2013, 4:58 p.m. | #3
On Mon, 2013-12-09 at 16:50 +0000, Julien Grall wrote:
> On 12/09/2013 04:06 PM, Ian Campbell wrote:
> > On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >>
> >> ---
> >>     Changes in v2:
> >>         - Use p2m_lookup as p2m_get_entry was removed
> >> ---
> >>  xen/include/asm-arm/p2m.h |    3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> >> index f4bcd7d..b63204d 100644
> >> --- a/xen/include/asm-arm/p2m.h
> >> +++ b/xen/include/asm-arm/p2m.h
> >> @@ -109,7 +109,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);
> >> +    paddr_t maddr = p2m_lookup(d, pfn_to_paddr(gfn), t);
> >> +    unsigned long mfn = maddr >> PAGE_SHIFT;
> > 
> > I think this resend happened before I replied to the original a second
> > time, (in <1386583029.13126.13.camel@kazak.uk.xensource.com>) so you
> > hadn't seen it.
> 
> Indeed, I will fix it.
> 
> > TL;DR: INVALID_MADDR >> PAGE_SHIFT != INVALID_MFN on arm64.
> 
> Does it mean that INVALID_MFN can be a valid mfn on arm64? If so, we
> have some place in common code where this constant is used (see
> common/domain.c for instance).

No, INVALID_MFN is 0xffffffffffffffff which is fine[0].

But given INVALID_MADDR of 0xffffffffffffffff,
(INVALID_MADDR>>PAGE_SHIFT) is 0x000fffffffffffff.

Ian.

[0] actually, the page at -1 could in theory be valid on any platform,
either arm32 or x86, bit a) it's unlikely and b) there isn't really any
better value to use as this sentinal. Lets not worry about it right now.
Julien Grall Dec. 10, 2013, 2:04 a.m. | #4
On 12/09/2013 04:58 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 16:50 +0000, Julien Grall wrote:
>> On 12/09/2013 04:06 PM, Ian Campbell wrote:
>>> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>>
>>>> ---
>>>>      Changes in v2:
>>>>          - Use p2m_lookup as p2m_get_entry was removed
>>>> ---
>>>>   xen/include/asm-arm/p2m.h |    3 ++-
>>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>>>> index f4bcd7d..b63204d 100644
>>>> --- a/xen/include/asm-arm/p2m.h
>>>> +++ b/xen/include/asm-arm/p2m.h
>>>> @@ -109,7 +109,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);
>>>> +    paddr_t maddr = p2m_lookup(d, pfn_to_paddr(gfn), t);
>>>> +    unsigned long mfn = maddr >> PAGE_SHIFT;
>>>
>>> I think this resend happened before I replied to the original a second
>>> time, (in <1386583029.13126.13.camel@kazak.uk.xensource.com>) so you
>>> hadn't seen it.
>>
>> Indeed, I will fix it.
>>
>>> TL;DR: INVALID_MADDR >> PAGE_SHIFT != INVALID_MFN on arm64.
>>
>> Does it mean that INVALID_MFN can be a valid mfn on arm64? If so, we
>> have some place in common code where this constant is used (see
>> common/domain.c for instance).
>
> No, INVALID_MFN is 0xffffffffffffffff which is fine[0].
>
> But given INVALID_MADDR of 0xffffffffffffffff,
> (INVALID_MADDR>>PAGE_SHIFT) is 0x000fffffffffffff.

oh right, I didn't pay attention for that. The patch is now fixed.

Patch

diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index f4bcd7d..b63204d 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -109,7 +109,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);
+    paddr_t maddr = p2m_lookup(d, pfn_to_paddr(gfn), t);
+    unsigned long mfn = maddr >> PAGE_SHIFT;
 
     if (!mfn_valid(mfn))
         return NULL;