Re: [v3 PATCH 5/5] RISC-V: Add EFI stub support.

From: Atish Patra <atishp@atishpatra.org>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Atish Patra <Atish.Patra@wdc.com>,
	"linux-riscv@lists.infradead.org"
	<linux-riscv@lists.infradead.org>,
	"linux-efi@vger.kernel.org" <linux-efi@vger.kernel.org>,
	"palmer@dabbelt.com" <palmer@dabbelt.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"arnd@arndb.de" <arnd@arndb.de>, Anup Patel <Anup.Patel@wdc.com>
Subject: Re: [v3 PATCH 5/5] RISC-V: Add EFI stub support.
Date: Mon, 20 Apr 2020 22:06:07 -0700	[thread overview]
Message-ID: <CAOnJCUKpLg8EzyaQ59kGVx0Fwfb--T9M2GuBSAAPdoTZfxHSbQ@mail.gmail.com> (raw)
In-Reply-To: <CAMj1kXHpHnhOjYJaBvj5XhYnOzbH5MzBJePy7dDpd-S3s029CQ@mail.gmail.com>

On Mon, Apr 20, 2020 at 1:02 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 20 Apr 2020 at 09:59, Atish Patra <atishp@atishpatra.org> wrote:
> >
> > On Mon, Apr 20, 2020 at 12:04 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Mon, 20 Apr 2020 at 06:20, Atish Patra <Atish.Patra@wdc.com> wrote:
> > > >
> ...
> > > >
> > > > "If the preferred address is set as the base of DRAM, efi_bs_call is
> > > > bound to fail as well because the base of DRAM is reserved by the
> > > > firmware. So the efi memory allocator can't allocate at that address.
> > > > Technically, it will work but it is no different than passing
> > > > ULONG_MAX. So I thought ULONG_MAX will avoid the confusion.
> > > >
> > > > We try to allocate as low as possible so I am passing dram_base as the
> > > > minimum address anyways. As the firmware reserved the first few KBs,
> > > >
> > >
> > >
> > > OK, so the preferred address *is* the base of DRAM (assuming it is 2
> > > MB aligned). However, in the general case, you keep some firmware
> > > state there (couple of KB) and so you typically end up at DRAM base
> > > plus 2 MB?
> > >
> >
> > Yes.
> >
> > > So first question: why does the firmware put this stuff at the base of
> > > DRAM in the first place? Does it *have* to live there?
> > >
> >
> > The firmware includes the RISC-V specific supervisor binary interface (SBI)[[1]
> > implementation. As it is a RISC-V specific run time service provider,
> > it has to be resident in memory.
> > The arm equivalent of SBI would be PSCI.
> >
> > [1] https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.adoc
> >
>
> I am not asking why it has to be resident in memory. I am asking why
> it has to be at the *base* of memory.
>

Sorry. I misunderstood the question. I think it is kept at the start
of dram to accommodate
embedded systems with smaller memory. It was also easier to manage at
the beginning of
the memory because all other next stages in the boot process just
ignores first few KBs of
the memory. We also did not have a memory reservation scheme back then.

Having said that, this parameter is configurable for each platform in
OpenSBI. In future, this restriction
can be relaxed if a platform vendor wants. IIRC, linux kernel can't
use  the memory below the kernel start
address (where PAGE_OFFSET is mapped) because of generic mm code limitations.

Added Anup (In case he wants to add something to this)

> > > Then, if the base of DRAM is guaranteed to be occupied, why not make
> > > the preferred address base of DRAM + 2 MB ? (or 4 MB for the 32-bit
> > > case)
> >
> > I guess that will work too. I was inclined to use low_alloc_above so
> > that we ensure that the kernel
> > boots from the lowest possible address given the alignment
> > restriction. If the alignment restrictions are relaxed,
> > in future, we just have to change the macro.
> >
> > However, I think calling relocate_kernel with a preferred offset
> > (dram_base + KIMG_ALIGN) is
> > better because it will always fall back to low_alloc_above anyways. I
> > will send the patch.
>
> Indeed. In the common case, it will just do the allocate_pages()
> directly, which is preferred. It will fall back to
> efi_low_alloc_aboce() [and do the memory map parsing and traversal
> etc] only if needed.

-- 
Regards,
Atish