Re: [PATCH v2 2/2] hw/riscv: sifive_u: Provide a reliable way for bootloader to detect whether it is running in QEMU

[Palmer Dabbelt]

On Thu, 09 Jul 2020 15:09:18 PDT (-0700), alistair23@gmail.com wrote:
> On Thu, Jul 9, 2020 at 3:07 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > The reset vector codes are subject to change, e.g.: with recent
> > fw_dynamic type image support, it breaks oreboot again.
>
> This is a recurring problem, I have another patch for Oreboot to fix
> the latest breakage.
>
> > Add a subregion in the MROM, with the size of machine RAM stored,
> > so that we can provide a reliable way for bootloader to detect
> > whether it is running in QEMU.
>
> I don't really like this though. I would prefer that we don't
> encourage guest software to behave differently on QEMU. I don't think
> other upstream boards do this.

I agree.  If you want an explicitly virtual board, use the virt board.  Users
of sifive_u are presumably trying to do their best to test against what the
hardware does without actually using the hardware.  Otherwise there should be
no reason to use the sifive_u board, as it's just sticking a layer of
complexity in the middle of everything.

> Besides Oreboot setting up the clocks are there any other users of this?

IIRC we have a scheme for handling the clock setup in QEMU where we accept
pretty much any control write and then just return reads that say the PLLs have
locked.  I'd be in favor of improving the scheme to improve compatibility with
the actual hardware, but adding some way for programs to skip the clocks
because they know they're in QEMU seems like the wrong way to go.

> Alistair
>
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> >
> > ---
> >
> > Changes in v2:
> > - correctly populate the value in little-endian
> >
> >  hw/riscv/sifive_u.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > index 3413369..79519d4 100644
> > --- a/hw/riscv/sifive_u.c
> > +++ b/hw/riscv/sifive_u.c
> > @@ -88,6 +88,7 @@ static const struct MemmapEntry {
> >
> >  #define OTP_SERIAL          1
> >  #define GEM_REVISION        0x10070109
> > +#define MROM_RAMSIZE_OFFSET 0xf8
> >
> >  static void create_fdt(SiFiveUState *s, const struct MemmapEntry *memmap,
> >      uint64_t mem_size, const char *cmdline)
> > @@ -496,6 +497,26 @@ static void sifive_u_machine_init(MachineState *machine)
> >      riscv_rom_copy_firmware_info(memmap[SIFIVE_U_MROM].base,
> >                                   memmap[SIFIVE_U_MROM].size,
> >                                   sizeof(reset_vec), kernel_entry);
> > +
> > +    /*
> > +     * Tell guest the machine ram size at MROM_RAMSIZE_OFFSET.
> > +     * On real hardware, the 64-bit value from MROM_RAMSIZE_OFFSET is zero.
> > +     * QEMU aware bootloader (e.g.: oreboot, U-Boot) can check value stored
> > +     * here to determine whether it is running in QEMU.
> > +     */
> > +
> > +    uint32_t ram_size[2] = {
> > +        machine->ram_size,
> > +        ((uint64_t)(machine->ram_size)) >> 32
> > +    };
> > +
> > +    /* copy in the ram size in little_endian byte order */
> > +    for (i = 0; i < ARRAY_SIZE(ram_size); i++) {
> > +        ram_size[i] = cpu_to_le32(ram_size[i]);
> > +    }
> > +    rom_add_blob_fixed_as("mrom.ram_size", ram_size, sizeof(ram_size),
> > +                          memmap[SIFIVE_U_MROM].base + MROM_RAMSIZE_OFFSET,
> > +                          &address_space_memory);
> >  }
> >
> >  static bool sifive_u_machine_get_start_in_flash(Object *obj, Error **errp)
> > --
> > 2.7.4