From: Oleksandr Tyshchenko <[email protected]>
The problem is that Arm's implementation of virt_addr_valid()
leads to memblock_is_map_memory() check, which will fail for
ZONE_DEVICE based addresses. But, the pfn_valid() check in turn
is able to cope with ZONE_DEVICE based memory.
You can find a good explanation of that problem at:
https://lore.kernel.org/lkml/[email protected]
Signed-off-by: Oleksandr Tyshchenko <[email protected]>
---
I am not quite sure whether it is a "correct" place and
the change itself, I just partially restored a behaviour before:
https://lore.kernel.org/lkml/[email protected]
So, the target of this patch is to get a feedback how to resolve
this properly if, of course, this really needs to be resolved
(I might miss important bits here).
It is worth mentioning that patch doesn't fix the current code base
(if I am not mistaken, no one calls virt_addr_valid() on Arm64 for
ZONE_DEVICE based addresses at the moment, so it seems that nothing
is broken), the fix is intended for the subsequent patch in this
series that will try to enable Xen's "unpopulated-alloc" usage
on Arm (it was enabled on x86 so far).
Please see:
[RFC PATCH 2/2] xen/unpopulated-alloc: Query hypervisor to provide
unallocated space
The subsequent patch will enable the code where virt_addr_valid()
is used in drivers/xen/unpopulated-alloc.c:fill_list() to check that
a virtual address returned by memremap_pages() is valid.
---
arch/arm64/include/asm/memory.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index 824a365..1a35a44 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -351,7 +351,7 @@ static inline void *phys_to_virt(phys_addr_t x)
#define virt_addr_valid(addr) ({ \
__typeof__(addr) __addr = __tag_reset(addr); \
- __is_lm_address(__addr) && pfn_is_map_memory(virt_to_pfn(__addr)); \
+ __is_lm_address(__addr) && pfn_valid(virt_to_pfn(__addr)); \
})
void dump_mem_limit(void);
--
2.7.4
Adding Mike and Anshuman,
On Wed, Jul 28, 2021 at 07:44:15PM +0300, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <[email protected]>
>
> The problem is that Arm's implementation of virt_addr_valid()
> leads to memblock_is_map_memory() check, which will fail for
> ZONE_DEVICE based addresses. But, the pfn_valid() check in turn
> is able to cope with ZONE_DEVICE based memory.
>
> You can find a good explanation of that problem at:
> https://lore.kernel.org/lkml/[email protected]
>
> Signed-off-by: Oleksandr Tyshchenko <[email protected]>
> ---
> I am not quite sure whether it is a "correct" place and
> the change itself, I just partially restored a behaviour before:
> https://lore.kernel.org/lkml/[email protected]
> So, the target of this patch is to get a feedback how to resolve
> this properly if, of course, this really needs to be resolved
> (I might miss important bits here).
>
> It is worth mentioning that patch doesn't fix the current code base
> (if I am not mistaken, no one calls virt_addr_valid() on Arm64 for
> ZONE_DEVICE based addresses at the moment, so it seems that nothing
> is broken), the fix is intended for the subsequent patch in this
> series that will try to enable Xen's "unpopulated-alloc" usage
> on Arm (it was enabled on x86 so far).
> Please see:
> [RFC PATCH 2/2] xen/unpopulated-alloc: Query hypervisor to provide
> unallocated space
>
> The subsequent patch will enable the code where virt_addr_valid()
> is used in drivers/xen/unpopulated-alloc.c:fill_list() to check that
> a virtual address returned by memremap_pages() is valid.
I wonder what the point of calling virt_addr_valid() in fill_list() is?
If memremap_pages() succeeded, the pages were mapped at the returned
vaddr, there's no need for an additional virt_addr_valid() check.
> ---
> arch/arm64/include/asm/memory.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index 824a365..1a35a44 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -351,7 +351,7 @@ static inline void *phys_to_virt(phys_addr_t x)
>
> #define virt_addr_valid(addr) ({ \
> __typeof__(addr) __addr = __tag_reset(addr); \
> - __is_lm_address(__addr) && pfn_is_map_memory(virt_to_pfn(__addr)); \
> + __is_lm_address(__addr) && pfn_valid(virt_to_pfn(__addr)); \
> })
pfn_valid() only guarantees the presence of a struct page but not
necessarily that the virtual address is accessible (valid). So this
change would break the NOMAP ranges case.
--
Catalin
Hi,
On Mon, Aug 02, 2021 at 01:19:48PM +0100, Catalin Marinas wrote:
> Adding Mike and Anshuman,
>
> On Wed, Jul 28, 2021 at 07:44:15PM +0300, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <[email protected]>
> >
> > The problem is that Arm's implementation of virt_addr_valid()
> > leads to memblock_is_map_memory() check, which will fail for
> > ZONE_DEVICE based addresses. But, the pfn_valid() check in turn
> > is able to cope with ZONE_DEVICE based memory.
> >
> > You can find a good explanation of that problem at:
> > https://lore.kernel.org/lkml/[email protected]
> >
> > Signed-off-by: Oleksandr Tyshchenko <[email protected]>
> > ---
> > I am not quite sure whether it is a "correct" place and
> > the change itself, I just partially restored a behaviour before:
> > https://lore.kernel.org/lkml/[email protected]
> > So, the target of this patch is to get a feedback how to resolve
> > this properly if, of course, this really needs to be resolved
> > (I might miss important bits here).
> >
> > It is worth mentioning that patch doesn't fix the current code base
> > (if I am not mistaken, no one calls virt_addr_valid() on Arm64 for
> > ZONE_DEVICE based addresses at the moment, so it seems that nothing
> > is broken), the fix is intended for the subsequent patch in this
> > series that will try to enable Xen's "unpopulated-alloc" usage
> > on Arm (it was enabled on x86 so far).
> > Please see:
> > [RFC PATCH 2/2] xen/unpopulated-alloc: Query hypervisor to provide
> > unallocated space
> >
> > The subsequent patch will enable the code where virt_addr_valid()
> > is used in drivers/xen/unpopulated-alloc.c:fill_list() to check that
> > a virtual address returned by memremap_pages() is valid.
> I wonder what the point of calling virt_addr_valid() in fill_list() is?
> If memremap_pages() succeeded, the pages were mapped at the returned
> vaddr, there's no need for an additional virt_addr_valid() check.
The virt_addr_valid() check in fill_list() looks bogus to me as well. If
memremap_pages() succeeds the range is guaranteed to have proper page
table.
I believe the first patch should be rather removal of the virt_addr_valid()
check in fill_list().
> > ---
> > arch/arm64/include/asm/memory.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > index 824a365..1a35a44 100644
> > --- a/arch/arm64/include/asm/memory.h
> > +++ b/arch/arm64/include/asm/memory.h
> > @@ -351,7 +351,7 @@ static inline void *phys_to_virt(phys_addr_t x)
> >
> > #define virt_addr_valid(addr) ({ \
> > __typeof__(addr) __addr = __tag_reset(addr); \
> > - __is_lm_address(__addr) && pfn_is_map_memory(virt_to_pfn(__addr)); \
> > + __is_lm_address(__addr) && pfn_valid(virt_to_pfn(__addr)); \
> > })
>
> pfn_valid() only guarantees the presence of a struct page but not
> necessarily that the virtual address is accessible (valid). So this
> change would break the NOMAP ranges case.
+1
--
Sincerely yours,
Mike.
Hello, all.
On 02.08.21 18:08, Mike Rapoport wrote:
> Hi,
>
> On Mon, Aug 02, 2021 at 01:19:48PM +0100, Catalin Marinas wrote:
>> Adding Mike and Anshuman,
>>
>> On Wed, Jul 28, 2021 at 07:44:15PM +0300, Oleksandr Tyshchenko wrote:
>>> From: Oleksandr Tyshchenko <[email protected]>
>>>
>>> The problem is that Arm's implementation of virt_addr_valid()
>>> leads to memblock_is_map_memory() check, which will fail for
>>> ZONE_DEVICE based addresses. But, the pfn_valid() check in turn
>>> is able to cope with ZONE_DEVICE based memory.
>>>
>>> You can find a good explanation of that problem at:
>>> https://lore.kernel.org/lkml/[email protected]
>>>
>>> Signed-off-by: Oleksandr Tyshchenko <[email protected]>
>>> ---
>>> I am not quite sure whether it is a "correct" place and
>>> the change itself, I just partially restored a behaviour before:
>>> https://lore.kernel.org/lkml/[email protected]
>>> So, the target of this patch is to get a feedback how to resolve
>>> this properly if, of course, this really needs to be resolved
>>> (I might miss important bits here).
>>>
>>> It is worth mentioning that patch doesn't fix the current code base
>>> (if I am not mistaken, no one calls virt_addr_valid() on Arm64 for
>>> ZONE_DEVICE based addresses at the moment, so it seems that nothing
>>> is broken), the fix is intended for the subsequent patch in this
>>> series that will try to enable Xen's "unpopulated-alloc" usage
>>> on Arm (it was enabled on x86 so far).
>>> Please see:
>>> [RFC PATCH 2/2] xen/unpopulated-alloc: Query hypervisor to provide
>>> unallocated space
>>>
>>> The subsequent patch will enable the code where virt_addr_valid()
>>> is used in drivers/xen/unpopulated-alloc.c:fill_list() to check that
>>> a virtual address returned by memremap_pages() is valid.
>
>> I wonder what the point of calling virt_addr_valid() in fill_list() is?
>> If memremap_pages() succeeded, the pages were mapped at the returned
>> vaddr, there's no need for an additional virt_addr_valid() check.
> The virt_addr_valid() check in fill_list() looks bogus to me as well. If
> memremap_pages() succeeds the range is guaranteed to have proper page
> table.
>
> I believe the first patch should be rather removal of the virt_addr_valid()
> check in fill_list().
Thank you for the clarification, I will send a patch to remove
virt_addr_valid()
check in fill_list() for the non-RFC version.
>
>>> ---
>>> arch/arm64/include/asm/memory.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
>>> index 824a365..1a35a44 100644
>>> --- a/arch/arm64/include/asm/memory.h
>>> +++ b/arch/arm64/include/asm/memory.h
>>> @@ -351,7 +351,7 @@ static inline void *phys_to_virt(phys_addr_t x)
>>>
>>> #define virt_addr_valid(addr) ({ \
>>> __typeof__(addr) __addr = __tag_reset(addr); \
>>> - __is_lm_address(__addr) && pfn_is_map_memory(virt_to_pfn(__addr)); \
>>> + __is_lm_address(__addr) && pfn_valid(virt_to_pfn(__addr)); \
>>> })
>> pfn_valid() only guarantees the presence of a struct page but not
>> necessarily that the virtual address is accessible (valid). So this
>> change would break the NOMAP ranges case.
> +1
Oh, I got it.
--
Regards,
Oleksandr Tyshchenko