Re: [PATCH] riscv: fix the global name pfn_base confliction error

[Alex Ghiti]

Le 28/07/2021 à 09:33, Lee Ken a écrit :
> On Wed, Jul 28, 2021 at 3:19 PM Alex Ghiti <alex@xxxxxxxx> wrote:
> > Hi Kenneth,
> >
> > Le 28/07/2021 à 08:43, Kenneth Lee a écrit :
> > > From: Kenneth Lee <liguozhu@xxxxxxxxxxxxx>
> > >
> > > RISCV use a global variable pfn_base for page/pfn translation. But this
> > > is a common name and will be used elsewhere. In those case,
> > > the page-pfn macro which refer this name will refer to the local/input
> > > variable of those function (such as in vfio_pin_pages_remote). This make
> > > everything wrong.
> > >
> > > This patch change the name from pfn_base to riscv_global_pfn_base to fix
> > > this problem
> >
> > What about removing this variable entirely and using
> > PFN_DOWN(kernel_map.phys_addr) directly in ARCH_PFN_OFFSET definition?
> > That would remove code from mm/init.c, which is nice :)
> >
> > Thanks,
> >
> > Alex
> >
> > > Signed-off-by: Kenneth Lee <liguozhu@xxxxxxxxxxxxx>
> > > ---
> > >    arch/riscv/include/asm/page.h | 4 ++--
> > >    arch/riscv/mm/init.c          | 6 +++---
> > >    2 files changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> > > index cca8764aed83..8711e415f37c 100644
> > > --- a/arch/riscv/include/asm/page.h
> > > +++ b/arch/riscv/include/asm/page.h
> > > @@ -79,8 +79,8 @@ typedef struct page *pgtable_t;
> > >    #endif
> > >
> > >    #ifdef CONFIG_MMU
> > > -extern unsigned long pfn_base;
> > > -#define ARCH_PFN_OFFSET              (pfn_base)
> > > +extern unsigned long riscv_global_pfn_base;
> > > +#define ARCH_PFN_OFFSET              (riscv_global_pfn_base)
> > >    #else
> > >    #define ARCH_PFN_OFFSET             (PAGE_OFFSET >> PAGE_SHIFT)
> > >    #endif /* CONFIG_MMU */
> > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > index a14bf3910eec..2ce4e9a46ca0 100644
> > > --- a/arch/riscv/mm/init.c
> > > +++ b/arch/riscv/mm/init.c
> > > @@ -228,8 +228,8 @@ static struct pt_alloc_ops _pt_ops __initdata;
> > >    #define pt_ops _pt_ops
> > >    #endif
> > >
> > > -unsigned long pfn_base __ro_after_init;
> > > -EXPORT_SYMBOL(pfn_base);
> > > +unsigned long riscv_global_pfn_base __ro_after_init;
> > > +EXPORT_SYMBOL(riscv_global_pfn_base);
> > >
> > >    pgd_t swapper_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> > >    pgd_t trampoline_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> > > @@ -572,7 +572,7 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> > >        kernel_map.va_kernel_pa_offset = kernel_map.virt_addr - kernel_map.phys_addr;
> > >    #endif
> > >
> > > -     pfn_base = PFN_DOWN(kernel_map.phys_addr);
> > > +     riscv_global_pfn_base = PFN_DOWN(kernel_map.phys_addr);
> > >
> > >        /*
> > >         * Enforce boot alignment requirements of RV32 and
>
> Yes. It is a choice. But I think it is the choice of the maintainer of
> the module. He should have a full consideration on how to expose the
> interface. I'm now just working for VFIO enablement in RISCV. It is
> not wise to be trapped into this;)

I think there is a misunderstanding, I suggested to do the following to 
get rid of pfn_base entirely:

diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
index adf5b4671684..8afb3db61eb0 100644
--- a/arch/riscv/include/asm/page.h
+++ b/arch/riscv/include/asm/page.h
@@ -79,8 +79,7 @@ typedef struct page *pgtable_t;
 #endif

 #ifdef CONFIG_MMU
-extern unsigned long pfn_base;
-#define ARCH_PFN_OFFSET                (pfn_base)
+#define ARCH_PFN_OFFSET                (PFN_DOWN(kernel_map.phys_addr))
 #else
 #define ARCH_PFN_OFFSET                (PAGE_OFFSET >> PAGE_SHIFT)
 #endif /* CONFIG_MMU */
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index c9a6362d8c7f..c51f0da03a62 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -211,9 +211,6 @@ static void __init setup_bootmem(void)
 #ifdef CONFIG_MMU
 static struct pt_alloc_ops pt_ops __initdata;

-unsigned long pfn_base __ro_after_init;
-EXPORT_SYMBOL(pfn_base);
-
 pgd_t swapper_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
 pgd_t trampoline_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
 extern pte_t fixmap_pte[PTRS_PER_PTE];// __page_aligned_bss;
@@ -608,8 +605,6 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
        kernel_map.va_pa_offset = PAGE_OFFSET - kernel_map.phys_addr;
        kernel_map.va_kernel_pa_offset = kernel_map.virt_addr - 
kernel_map.phys_addr;

-       pfn_base = PFN_DOWN(kernel_map.phys_addr);
-
        /* Sanity check alignment and size */
        BUG_ON((PAGE_OFFSET % PGDIR_SIZE) != 0);
        BUG_ON((kernel_map.phys_addr % PMD_SIZE) != 0);