Re: [PATCH v4 2/2] RISC-V: Setup initial page tables in two stages

[Anup Patel]

On Mon, May 20, 2019 at 5:13 PM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
>
> >  void __init parse_dtb(unsigned int hartid, void *dtb)
> >  {
> > -     if (early_init_dt_scan(__va(dtb)))
> > +     dtb = (void *)fix_to_virt(FIX_FDT) + ((uintptr_t)dtb & ~PAGE_MASK);
> > +     if (early_init_dt_scan(dtb))
>
> FYI, parse_dtb in mainline now lost the hartid argument and takes a
> phys_addr_t for the dtb address.

Okay, this patch is based on 5.1 kernel. I guess I will have to rebase
it anyway.

>
> That being said I find the above way to magic.  So we take the fixmap
> address and then only the offset from something passed as a pointer?
> This just looks very weird.  The way FIX_FDT is defined to add to my
> confusion, which might partially be due to not understanding fixmaps
> very well.  But it seems like at very least we should set up an
> actual kernel pointer for the dtb in setup_vm based on what that
> gets passed and stop passing any arguments to parse_dtb to keep
> that magic in one place.  And possibly add some comment.

I agree with your suggestion. I will setup early_dtb_ptr in setup_vm()
and use it here.

FYI, the fixmap virtual address range is not covered by linear va-to-pa
translation (i.e. __va() and __pa() cannot be used). The mapping
granularity of fixmap is always PAGE_SIZE hence add offset to
fix_to_virt(FIX_FDT).

>
> > +#if MAX_EARLY_MAPPING_SIZE < PGDIR_SIZE
>
> It seems MAX_EARLY_MAPPING_SIZE is defined to a fix constant,
> why do we need these conditionals?

Sure, I will remove the conditional. It's totally redundant. I forgot to
remove it previously.

Regards,
Anup