Re: [PATCH v2 2/2] hw/riscv: sifive_u: Provide a reliable way for bootloader to detect whether it is running in QEMU
On Sun, Jul 12, 2020 at 6:16 PM Bin Meng wrote: > > Hi Alistair, > > On Sun, Jul 12, 2020 at 12:04 AM Alistair Francis > wrote: > > > > On Thu, Jul 9, 2020 at 5:50 PM Bin Meng wrote: > > > > > > Hi Palmer, > > > > > > On Fri, Jul 10, 2020 at 8:45 AM Palmer Dabbelt > > > wrote: > > > > > > > > On Thu, 09 Jul 2020 15:09:18 PDT (-0700), alistai...@gmail.com wrote: > > > > > On Thu, Jul 9, 2020 at 3:07 AM Bin Meng wrote: > > > > >> > > > > >> From: Bin Meng > > > > >> > > > > >> 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. > > > > > > Understood. Then let's drop this patch. > > > > > > > > > > > > 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. > > > > > > > > > > Yep, that's my question to Oreboot too. > > > > > > U-Boot SPL can boot with QEMU and no problem was seen with clock > > > settings in PRCI model in QEMU. > > > > I don't think it's an unsolvable problem. There is just little work on > > Oreboot to run on QEMU. I can dig into it a bit and see if I can find > > a better fix on the Oreboot side. > > > > Can we remove the QEMU detect logic completely in Oreboot? Except the > QSPI controller QEMU should be able to run Oreboot since it runs > U-Boot SPL. That is the eventual goal. Alistair > > Regards, > Bin
Re: [PATCH v2 2/2] hw/riscv: sifive_u: Provide a reliable way for bootloader to detect whether it is running in QEMU
Hi Alistair, On Sun, Jul 12, 2020 at 12:04 AM Alistair Francis wrote: > > On Thu, Jul 9, 2020 at 5:50 PM Bin Meng wrote: > > > > Hi Palmer, > > > > On Fri, Jul 10, 2020 at 8:45 AM Palmer Dabbelt > > wrote: > > > > > > On Thu, 09 Jul 2020 15:09:18 PDT (-0700), alistai...@gmail.com wrote: > > > > On Thu, Jul 9, 2020 at 3:07 AM Bin Meng wrote: > > > >> > > > >> From: Bin Meng > > > >> > > > >> 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. > > > > Understood. Then let's drop this patch. > > > > > > > > > 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. > > > > > > > Yep, that's my question to Oreboot too. > > > > U-Boot SPL can boot with QEMU and no problem was seen with clock > > settings in PRCI model in QEMU. > > I don't think it's an unsolvable problem. There is just little work on > Oreboot to run on QEMU. I can dig into it a bit and see if I can find > a better fix on the Oreboot side. > Can we remove the QEMU detect logic completely in Oreboot? Except the QSPI controller QEMU should be able to run Oreboot since it runs U-Boot SPL. Regards, Bin
Re: [PATCH v2 2/2] hw/riscv: sifive_u: Provide a reliable way for bootloader to detect whether it is running in QEMU
Hi Alistair, On Sun, Jul 12, 2020 at 12:03 AM Alistair Francis wrote: > > On Thu, Jul 9, 2020 at 5:48 PM Bin Meng wrote: > > > > Hi Alistair, > > > > On Fri, Jul 10, 2020 at 6:19 AM Alistair Francis > > wrote: > > > > > > On Thu, Jul 9, 2020 at 3:07 AM Bin Meng wrote: > > > > > > > > From: Bin Meng > > > > > > > > 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. > > > > > > > Can Oreboot be updated to remove the QEMU detection? > > In general I think it should be. > > Right now it's not critical to do. I think from a QEMU perspective we > have finished changing the "ROM" code so after this release we can > update Oreboot and then it should settle down again. > > > > > > > > > > > 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. > > > > > > Besides Oreboot setting up the clocks are there any other users of this? > > > > I don't really have any specific reason, except for testing U-Boot SPL > > by relaxing the requirement of hardcoding the memory to 8G "-m 8G" as > > I indicated in the commit message below: > > Yeah, I think that's just something we will have to deal with. If the > guest expects 8GB and doesn't check the device tree passed to it then > the user has to create 8GB of memory. > Note there are two reasons that why "-m 8G" is used to test U-Boot SPL: 1. U-Boot DDR driver is hardcoding memory to 8G, which I had a fix locally and will send U-Boot upstream soon. 2. U-Boot SPL has to use its own device tree because we don't want to update QEMU to include all the DDR register settings in the device tree which is very big. Why I wanted to add this patch here is that I wanted to dynamically patch U-Boot SPL DT to use the actual ram size that QEMU instantiates. This way we can avoid editing U-Boot SPL DT to set the actual memory size. Regards, Bin
Re: [PATCH v2 2/2] hw/riscv: sifive_u: Provide a reliable way for bootloader to detect whether it is running in QEMU
On Thu, Jul 9, 2020 at 5:50 PM Bin Meng wrote: > > Hi Palmer, > > On Fri, Jul 10, 2020 at 8:45 AM Palmer Dabbelt > wrote: > > > > On Thu, 09 Jul 2020 15:09:18 PDT (-0700), alistai...@gmail.com wrote: > > > On Thu, Jul 9, 2020 at 3:07 AM Bin Meng wrote: > > >> > > >> From: Bin Meng > > >> > > >> 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. > > Understood. Then let's drop this patch. > > > > > > 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. > > > > Yep, that's my question to Oreboot too. > > U-Boot SPL can boot with QEMU and no problem was seen with clock > settings in PRCI model in QEMU. I don't think it's an unsolvable problem. There is just little work on Oreboot to run on QEMU. I can dig into it a bit and see if I can find a better fix on the Oreboot side. Alistair > > Regards, > Bin
Re: [PATCH v2 2/2] hw/riscv: sifive_u: Provide a reliable way for bootloader to detect whether it is running in QEMU
On Thu, Jul 9, 2020 at 5:48 PM Bin Meng wrote: > > Hi Alistair, > > On Fri, Jul 10, 2020 at 6:19 AM Alistair Francis wrote: > > > > On Thu, Jul 9, 2020 at 3:07 AM Bin Meng wrote: > > > > > > From: Bin Meng > > > > > > 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. > > > > Can Oreboot be updated to remove the QEMU detection? In general I think it should be. Right now it's not critical to do. I think from a QEMU perspective we have finished changing the "ROM" code so after this release we can update Oreboot and then it should settle down again. > > > > > > > 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. > > > > Besides Oreboot setting up the clocks are there any other users of this? > > I don't really have any specific reason, except for testing U-Boot SPL > by relaxing the requirement of hardcoding the memory to 8G "-m 8G" as > I indicated in the commit message below: Yeah, I think that's just something we will have to deal with. If the guest expects 8GB and doesn't check the device tree passed to it then the user has to create 8GB of memory. Alistair > > commit 3eaea6eb4e534f7b87c6eca808149bb671976800 > Author: Bin Meng > Date: Mon Jun 15 17:50:41 2020 -0700 > > hw/riscv: sifive_u: Add a dummy DDR memory controller device > > It is enough to simply map the SiFive FU540 DDR memory controller > into the MMIO space using create_unimplemented_device(), to make > the upstream U-Boot v2020.07 DDR memory initialization codes happy. > > Note we do not generate device tree fragment for the DDR memory > controller. Since the controller data in device tree consumes a > very large space (see fu540-hifive-unleashed-a00-ddr.dtsi in the > U-Boot source), and it is only needed by U-Boot SPL but not any > operating system, we choose not to generate the fragment here. > This also means when testing with U-Boot SPL, the device tree has > to come from U-Boot SPL itself, but not the one generated by QEMU > on the fly. The memory has to be set to 8GiB to match the real > HiFive Unleashed board when invoking QEMU (-m 8G). > > Cc'ing Pragnesh and Sagar as they wanted to test U-Boot SPL with QEMU > and talked to me the other day. > > Regards, > Bin
Re: [PATCH v2 2/2] hw/riscv: sifive_u: Provide a reliable way for bootloader to detect whether it is running in QEMU
Hi Palmer, On Fri, Jul 10, 2020 at 8:45 AM Palmer Dabbelt wrote: > > On Thu, 09 Jul 2020 15:09:18 PDT (-0700), alistai...@gmail.com wrote: > > On Thu, Jul 9, 2020 at 3:07 AM Bin Meng wrote: > >> > >> From: Bin Meng > >> > >> 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. Understood. Then let's drop this patch. > > > 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. > Yep, that's my question to Oreboot too. U-Boot SPL can boot with QEMU and no problem was seen with clock settings in PRCI model in QEMU. Regards, Bin
Re: [PATCH v2 2/2] hw/riscv: sifive_u: Provide a reliable way for bootloader to detect whether it is running in QEMU
Hi Alistair, On Fri, Jul 10, 2020 at 6:19 AM Alistair Francis wrote: > > On Thu, Jul 9, 2020 at 3:07 AM Bin Meng wrote: > > > > From: Bin Meng > > > > 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. > Can Oreboot be updated to remove the QEMU detection? > > > > 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. > > Besides Oreboot setting up the clocks are there any other users of this? I don't really have any specific reason, except for testing U-Boot SPL by relaxing the requirement of hardcoding the memory to 8G "-m 8G" as I indicated in the commit message below: commit 3eaea6eb4e534f7b87c6eca808149bb671976800 Author: Bin Meng Date: Mon Jun 15 17:50:41 2020 -0700 hw/riscv: sifive_u: Add a dummy DDR memory controller device It is enough to simply map the SiFive FU540 DDR memory controller into the MMIO space using create_unimplemented_device(), to make the upstream U-Boot v2020.07 DDR memory initialization codes happy. Note we do not generate device tree fragment for the DDR memory controller. Since the controller data in device tree consumes a very large space (see fu540-hifive-unleashed-a00-ddr.dtsi in the U-Boot source), and it is only needed by U-Boot SPL but not any operating system, we choose not to generate the fragment here. This also means when testing with U-Boot SPL, the device tree has to come from U-Boot SPL itself, but not the one generated by QEMU on the fly. The memory has to be set to 8GiB to match the real HiFive Unleashed board when invoking QEMU (-m 8G). Cc'ing Pragnesh and Sagar as they wanted to test U-Boot SPL with QEMU and talked to me the other day. Regards, Bin
Re: [PATCH v2 2/2] hw/riscv: sifive_u: Provide a reliable way for bootloader to detect whether it is running in QEMU
On Thu, 09 Jul 2020 15:09:18 PDT (-0700), alistai...@gmail.com wrote:
On Thu, Jul 9, 2020 at 3:07 AM Bin Meng wrote:
From: Bin Meng
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
---
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_REVISION0x10070109
+#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
Re: [PATCH v2 2/2] hw/riscv: sifive_u: Provide a reliable way for bootloader to detect whether it is running in QEMU
On Thu, Jul 9, 2020 at 3:07 AM Bin Meng wrote:
>
> From: Bin Meng
>
> 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.
Besides Oreboot setting up the clocks are there any other users of this?
Alistair
>
> Signed-off-by: Bin Meng
>
> ---
>
> 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_REVISION0x10070109
> +#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
>
>
