Re: [PATCH] hw/riscv: split RAM into low and high memory

[Alistair Francis]

On Thu, Aug 3, 2023 at 10:47 AM Wu, Fei <fei2.wu@intel.com> wrote:
>
> On 8/1/2023 6:46 AM, Daniel Henrique Barboza wrote:
> >
> >
> > On 7/30/23 22:53, Fei Wu wrote:
> >> riscv virt platform's memory started at 0x80000000 and
> >> straddled the 4GiB boundary. Curiously enough, this choice
> >> of a memory layout will prevent from launching a VM with
> >> a bit more than 2000MiB and PCIe pass-thru on an x86 host, due
> >> to identity mapping requirements for the MSI doorbell on x86,
> >> and these (APIC/IOAPIC) live right below 4GiB.
> >>
> >> So just split the RAM range into two portions:
> >> - 1 GiB range from 0x80000000 to 0xc0000000.
> >> - The remainder at 0x100000000
> >>
> >> ...leaving a hole between the ranges.
> >
> > I am afraid this breaks some existing distro setups, like Ubuntu. After
> > this patch
> > this emulation stopped working:
> >
> > ~/work/qemu/build/qemu-system-riscv64 \
> >     -machine virt -nographic -m 8G -smp 8 \
> >     -kernel ./uboot-ubuntu/usr/lib/u-boot/qemu-riscv64_smode/uboot.elf \
> >     -drive file=snapshot.img,format=qcow2,if=virtio \
> >     -netdev bridge,id=bridge1,br=virbr0 -device
> > virtio-net-pci,netdev=bridge1
> >
> >
> > This is basically a guest created via the official Canonical tutorial:
> >
> > https://wiki.ubuntu.com/RISC-V/QEMU
> >
> > The error being thrown:
> >
> > =================
> >
> > Boot HART ID              : 4
> > Boot HART Domain          : root
> > Boot HART Priv Version    : v1.12
> > Boot HART Base ISA        : rv64imafdch
> > Boot HART ISA Extensions  : time,sstc
> > Boot HART PMP Count       : 16
> > Boot HART PMP Granularity : 4
> > Boot HART PMP Address Bits: 54
> > Boot HART MHPM Count      : 16
> > Boot HART MIDELEG         : 0x0000000000001666
> > Boot HART MEDELEG         : 0x0000000000f0b509
> >
> >
> > U-Boot 2022.07+dfsg-1ubuntu4.2 (Nov 24 2022 - 18:47:41 +0000)
> >
> > CPU:
> > rv64imafdch_zicbom_zicboz_zicsr_zifencei_zihintpause_zawrs_zfa_zca_zcd_zba_zbb_zbc_zbs_sstc_svadu
> > Model: riscv-virtio,qemu
> > DRAM:  Unhandled exception: Store/AMO access fault
> > EPC: 00000000802018b8 RA: 00000000802126a0 TVAL: 00000000ff733f90
> >
> > Code: b823 06b2 bc23 06b2 b023 08b2 b423 08b2 (b823 08b2)
> >
> >
> > resetting ...
> > System reset not supported on this platform
> > ### ERROR ### Please RESET the board ###
> > QEMU 8.0.90 monitor - type 'help' for more infor
> > =================
> >
> >
> > Based on the change made I can make an educated guess on what is going
> > wrong.
> > We have another board with a similar memory topology you're making here,
> > the
> > Microchip Polarfire (microchip_pfsoc.c). We were having some problems
> > with this
> > board while trying to consolidate the boot process between all boards in
> > hw/riscv/boot.c because of its non-continuous RAM bank. The full story
> > can be
> > read in the commit message of 4b402886ac89 ("hw/riscv: change
> > riscv_compute_fdt_addr()
> > semantics") but the short version can be seen in riscv_compute_fdt_addr()
> > from boot.c:
> >
> >  - if ram_start is less than 3072MiB, the FDT will be  put at the lowest
> > value
> > between 3072 MiB and the end of that RAM bank;
> >
> > - if ram_start is higher than 3072 MiB the FDT will be put at the end of
> > the
> > RAM bank.
> >
> > So, after this patch, since riscv_compute_fdt_addr() is being used with
> > the now
> > lower RAM bank, the fdt is being put in LOW_MEM - fdt_size for any setup
> > that has
> > more than 1Gb RAM, and this breaks assumptions made by uboot and Ubuntu
> > and possibly
> > others that are trying to retrieve the FDT from the gap that you created
> > between
> > low and hi mem in this patch.
> >
> > In fact, this same Ubuntu guest I mentioned above will boot if I put
> > only 1 Gb of RAM
> > (-m 1Gb). If I try with -m 1.1Gb I reproduce this error. This can be a
> > validation of
> > the guess I'm making here: Ubuntu is trying to fetch stuff (probably the
> > fdt) from
> > the gap between the memory areas.
> >
> > This change on top of this patch doesn't work either:
> >
> > $ git diff
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 8fbdc7220c..dfff48d849 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -1335,9 +1335,16 @@ static void virt_machine_done(Notifier *notifier,
> > void *data)
> >                                           kernel_start_addr, true, NULL);
> >      }
> >
> > -    fdt_load_addr = riscv_compute_fdt_addr(memmap[VIRT_DRAM].base,
> > +    if (machine->ram_size < memmap[VIRT_DRAM].size) {
> > +        fdt_load_addr = riscv_compute_fdt_addr(memmap[VIRT_DRAM].base,
> >                                             memmap[VIRT_DRAM].size,
> >                                             machine);
> > +    } else {
> > +        fdt_load_addr =
> > riscv_compute_fdt_addr(memmap[VIRT_DRAM_HIGH].base,
> > +                                           memmap[VIRT_DRAM_HIGH].size,
> > +                                           machine);
> > +    }
> > +
> >
> > This would put the fdt at the end of the HI RAM for guests with more
> > than 1Gb of RAM.
> > This change in fact makes the situation even worse, breaking setups that
> > were working
> > before with this patch.
> >
> > There's a chance that reducing the gap between the RAM banks can make
> > Ubuntu work
> > reliably again but now I'm a little cold feet with this change.
> >
> >
> > I think we'll need some kernel/Opensbi folks to weight in here to see if
> > there's a
> > failsafe memory setup that won't break distros out there and allow your
> > passthrough
> > to work.
> >
> Hi Daniel,
>
> Do you know who we should talk to? I think it's not uncommon to have
> seperated multi-range memory, the stack should support this configuration.

@Palmer Dabbelt @Anup Patel any ideas?

Alistair