Re: [PATCH 5.11 225/254] arm64/mm: define arch_get_mappable_range()

[Greg Kroah-Hartman]

On Mon, Mar 29, 2021 at 03:49:19PM +0200, Ard Biesheuvel wrote:
> (+ Pavel)
> 
> On Mon, 29 Mar 2021 at 15:42, Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Mon, Mar 29, 2021 at 03:08:52PM +0200, Ard Biesheuvel wrote:
> > > On Mon, 29 Mar 2021 at 12:12, Greg Kroah-Hartman
> > > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Mon, Mar 29, 2021 at 03:05:25PM +0530, Naresh Kamboju wrote:
> > > > > On Mon, 29 Mar 2021 at 14:10, Greg Kroah-Hartman
> > > > > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > From: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> > > > > >
> > > > > > [ Upstream commit 03aaf83fba6e5af08b5dd174c72edee9b7d9ed9b ]
> > > > > >
> > > > > > This overrides arch_get_mappable_range() on arm64 platform which will be
> > > > > > used with recently added generic framework.  It drops
> > > > > > inside_linear_region() and subsequent check in arch_add_memory() which are
> > > > > > no longer required.  It also adds a VM_BUG_ON() check that would ensure
> > > > > > that mhp_range_allowed() has already been called.
> > > > > >
> > > > > > Link: https://lkml.kernel.org/r/1612149902-7867-3-git-send-email-anshuman.khandual@xxxxxxx
> > > > > > Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> > > > > > Reviewed-by: David Hildenbrand <david@xxxxxxxxxx>
> > > > > > Reviewed-by: Catalin Marinas <catalin.marinas@xxxxxxx>
> > > > > > Cc: Will Deacon <will@xxxxxxxxxx>
> > > > > > Cc: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > > > > > Cc: Mark Rutland <mark.rutland@xxxxxxx>
> > > > > > Cc: Heiko Carstens <hca@xxxxxxxxxxxxx>
> > > > > > Cc: Jason Wang <jasowang@xxxxxxxxxx>
> > > > > > Cc: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > > > > > Cc: "Michael S. Tsirkin" <mst@xxxxxxxxxx>
> > > > > > Cc: Michal Hocko <mhocko@xxxxxxxxxx>
> > > > > > Cc: Oscar Salvador <osalvador@xxxxxxx>
> > > > > > Cc: Pankaj Gupta <pankaj.gupta@xxxxxxxxxxxxxxx>
> > > > > > Cc: Pankaj Gupta <pankaj.gupta.linux@xxxxxxxxx>
> > > > > > Cc: teawater <teawaterz@xxxxxxxxxxxxxxxxx>
> > > > > > Cc: Vasily Gorbik <gor@xxxxxxxxxxxxx>
> > > > > > Cc: Wei Yang <richard.weiyang@xxxxxxxxxxxxxxxxx>
> > > > > > Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > > > > > Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> > > > > > Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> > > > > > ---
> > > > > >  arch/arm64/mm/mmu.c | 15 +++++++--------
> > > > > >  1 file changed, 7 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> > > > > > index 6f0648777d34..92b3be127796 100644
> > > > > > --- a/arch/arm64/mm/mmu.c
> > > > > > +++ b/arch/arm64/mm/mmu.c
> > > > > > @@ -1443,16 +1443,19 @@ static void __remove_pgd_mapping(pgd_t *pgdir, unsigned long start, u64 size)
> > > > > >         free_empty_tables(start, end, PAGE_OFFSET, PAGE_END);
> > > > > >  }
> > > > > >
> > > > > > -static bool inside_linear_region(u64 start, u64 size)
> > > > > > +struct range arch_get_mappable_range(void)
> > > > > >  {
> > > > > > +       struct range mhp_range;
> > > > > > +
> > > > > >         /*
> > > > > >          * Linear mapping region is the range [PAGE_OFFSET..(PAGE_END - 1)]
> > > > > >          * accommodating both its ends but excluding PAGE_END. Max physical
> > > > > >          * range which can be mapped inside this linear mapping range, must
> > > > > >          * also be derived from its end points.
> > > > > >          */
> > > > > > -       return start >= __pa(_PAGE_OFFSET(vabits_actual)) &&
> > > > > > -              (start + size - 1) <= __pa(PAGE_END - 1);
> > > > > > +       mhp_range.start = __pa(_PAGE_OFFSET(vabits_actual));
> > > > > > +       mhp_range.end =  __pa(PAGE_END - 1);
> > > > > > +       return mhp_range;
> > > > > >  }
> > > > > >
> > > > > >  int arch_add_memory(int nid, u64 start, u64 size,
> > > > > > @@ -1460,11 +1463,7 @@ int arch_add_memory(int nid, u64 start, u64 size,
> > > > > >  {
> > > > > >         int ret, flags = 0;
> > > > > >
> > > > > > -       if (!inside_linear_region(start, size)) {
> > > > > > -               pr_err("[%llx %llx] is outside linear mapping region\n", start, start + size);
> > > > > > -               return -EINVAL;
> > > > > > -       }
> > > > > > -
> > > > > > +       VM_BUG_ON(!mhp_range_allowed(start, size, true));
> > > > > >         if (rodata_full || debug_pagealloc_enabled())
> > > > > >                 flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> > > > >
> > > > > The stable rc 5.10 and 5.11 builds failed for arm64 architecture
> > > > > due to below warnings / errors,
> > > > >
> > > > > > Anshuman Khandual <anshuman.khandual@xxxxxxx>
> > > > > >     arm64/mm: define arch_get_mappable_range()
> > > > >
> > > > >
> > > > >   arch/arm64/mm/mmu.c: In function 'arch_add_memory':
> > > > >   arch/arm64/mm/mmu.c:1483:13: error: implicit declaration of function
> > > > > 'mhp_range_allowed'; did you mean 'cpu_map_prog_allowed'?
> > > > > [-Werror=implicit-function-declaration]
> > > > >     VM_BUG_ON(!mhp_range_allowed(start, size, true));
> > > > >                ^
> > > > >   include/linux/build_bug.h:30:63: note: in definition of macro
> > > > > 'BUILD_BUG_ON_INVALID'
> > > > >    #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))
> > > > >                                                                  ^
> > > > >   arch/arm64/mm/mmu.c:1483:2: note: in expansion of macro 'VM_BUG_ON'
> > > > >     VM_BUG_ON(!mhp_range_allowed(start, size, true));
> > > > >     ^~~~~~~~~
> > > > >
> > > > > Build link,
> > > > > https://ci.linaro.org/view/lkft/job/openembedded-lkft-linux-stable-rc-5.11/DISTRO=lkft,MACHINE=juno,label=docker-buster-lkft/41/consoleText
> > > > > https://ci.linaro.org/view/lkft/job/openembedded-lkft-linux-stable-rc-5.10/DISTRO=lkft,MACHINE=dragonboard-410c,label=docker-buster-lkft/120/consoleFull
> > > >
> > > > thanks, will go drop this, and the patch that was after it in the
> > > > series, from both trees and will push out a -rc2.
> > > >
> > >
> > > Why were these picked up in the first place? I don't see any fixes or
> > > cc:stable tags, and the commit log clearly describes that the change
> > > is preparatory work for enabling arm64 support into a recently
> > > introduced generic framework.
> >
> > This was needed for a follow-on patch in the series that fixed an issue.
> > Specifically it was commit ee7febce0519 ("arm64: mm: correct the inside
> > linear map range during hotplug check")
> >
> 
> Yeah, but during the discussion of that patch [0], we pointed out that
> it needed to be rebased because of these new changes. So trying to
> backport this rebased version is obviously not the right approach:
> Pavel's original patch would be much more suitable for that.
> 
> Could we have annotated this patch in a better way to make this more obvious?

Yes, given that there was no annotation on the patch at all to let us
know this :)

You can say things like "do not apply to stable trees" or "needs total
rework for older kernels" or other fun such things that when we read
them, we know to ask for help.  As it is, the patch provided nothing so
we guessed and got it wrong...

thanks,

greg k-h