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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAAhSdy2SRxMEaJE6WsP87KeXw_J1X-6eYAMV7j0bhEGgNcLiyg@mail.gmail.com>
Date:   Tue, 21 May 2019 13:08:14 +0530
From:   Anup Patel <anup@...infault.org>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     Anup Patel <Anup.Patel@....com>,
        Palmer Dabbelt <palmer@...ive.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Mike Rapoport <rppt@...ux.ibm.com>,
        Atish Patra <Atish.Patra@....com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        "linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>
Subject: Re: [PATCH v4 2/2] RISC-V: Setup initial page tables in two stages

On Mon, May 20, 2019 at 5:13 PM Christoph Hellwig <hch@...radead.org> 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