* [PATCH v1 0/6] RISC-V: Add more machine memory
@ 2019-09-19 22:24 Alistair Francis
2019-09-19 22:24 ` [PATCH v1 1/6] riscv/sifive_u: Add L2-LIM cache memory Alistair Francis
` (6 more replies)
0 siblings, 7 replies; 30+ messages in thread
From: Alistair Francis @ 2019-09-19 22:24 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis
This series aims to improve the use of QEMU for developing boot code. It
does a few things:
- sifive_u machine:
- Adds a chunk of memory in the Flash area. This allows boot loaders
to use this memory. I can't find details on the QSPI flash used on
the real board, so this is the best bet at the moment.
- Adds a chunk of memory in the L2-LIM area. This is actualy the L2
cache and should shrink as the L2 cache is enalbed. Unfortunatley I
don't see a nice way to shrink this memory.
- Adds a property that allows users to specify if QEMU should jump to
flash or DRAM after the ROM code.
- virt machine:
- Add the pflash_cfi01 flash device. This is based on the ARM virt
board implementation
- Adjusts QEMU to jump to the flash if a user has speciefied any
pflash.
Both machines have been tested with oreboot, but this should also help
the coreboot developers.
Alistair Francis (6):
riscv/sifive_u: Add L2-LIM cache memory
riscv/sifive_u: Add QSPI memory region
riscv/sifive_u: Manually define the machine
riscv/sifive_u: Add the start-in-flash property
riscv/virt: Add the PFlash CFI01 device
riscv/virt: Jump to pflash if specified
hw/riscv/Kconfig | 1 +
hw/riscv/sifive_u.c | 77 +++++++++++++++++++++++++++++--
hw/riscv/virt.c | 91 ++++++++++++++++++++++++++++++++++++-
include/hw/riscv/sifive_u.h | 11 ++++-
include/hw/riscv/virt.h | 3 ++
5 files changed, 177 insertions(+), 6 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v1 1/6] riscv/sifive_u: Add L2-LIM cache memory
2019-09-19 22:24 [PATCH v1 0/6] RISC-V: Add more machine memory Alistair Francis
@ 2019-09-19 22:24 ` Alistair Francis
2019-09-20 5:15 ` Bin Meng
2019-09-19 22:24 ` [PATCH v1 2/6] riscv/sifive_u: Add QSPI memory region Alistair Francis
` (5 subsequent siblings)
6 siblings, 1 reply; 30+ messages in thread
From: Alistair Francis @ 2019-09-19 22:24 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis
On reset only a single L2 cache way is enabled, the others are exposed
as memory that can be used by early boot firmware. This L2 region is
generally disabled using the WayEnable register at a later stage in the
boot process. To allow firmware to target QEMU and the HiFive Unleashed
let's add the L2 LIM (LooselyIntegrated Memory).
Ideally we would want to adjust the size of this chunk of memory as the
L2 Cache Controller WayEnable register is incremented. Unfortunately I
don't see a nice way to handle reducing or blocking out the L2 LIM while
still allowing it be re returned to all enabled from a reset.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
hw/riscv/sifive_u.c | 15 +++++++++++++++
include/hw/riscv/sifive_u.h | 1 +
2 files changed, 16 insertions(+)
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 9f8e84bf2e..de6e197882 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -65,6 +65,7 @@ static const struct MemmapEntry {
[SIFIVE_U_DEBUG] = { 0x0, 0x100 },
[SIFIVE_U_MROM] = { 0x1000, 0x11000 },
[SIFIVE_U_CLINT] = { 0x2000000, 0x10000 },
+ [SIFIVE_U_L2LIM] = { 0x8000000, 0x1e00000 },
[SIFIVE_U_PLIC] = { 0xc000000, 0x4000000 },
[SIFIVE_U_PRCI] = { 0x10000000, 0x1000 },
[SIFIVE_U_UART0] = { 0x10010000, 0x1000 },
@@ -431,6 +432,7 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
const struct MemmapEntry *memmap = sifive_u_memmap;
MemoryRegion *system_memory = get_system_memory();
MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
+ MemoryRegion *l2lim_mem = g_new(MemoryRegion, 1);
qemu_irq plic_gpios[SIFIVE_U_PLIC_NUM_SOURCES];
char *plic_hart_config;
size_t plic_hart_config_len;
@@ -459,6 +461,19 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
memory_region_add_subregion(system_memory, memmap[SIFIVE_U_MROM].base,
mask_rom);
+ /* Add L2-LIM at reset size.
+ * This should be reduced in size as the L2 Cache Controller WayEnable
+ * register is incremented. Unfortunately I don't see a nice (or any) way
+ * to handle reducing or blocking out the L2 LIM while still allowing it
+ * be re returned to all enabled after a reset. For the time being, just
+ * leave it enabled all the time. This won't break anything, but will be
+ * too generous to misbehaving guests.
+ */
+ memory_region_init_ram(l2lim_mem, NULL, "riscv.sifive.u.l2lim",
+ memmap[SIFIVE_U_L2LIM].size, &error_fatal);
+ memory_region_add_subregion(system_memory, memmap[SIFIVE_U_L2LIM].base,
+ l2lim_mem);
+
/* create PLIC hart topology configuration string */
plic_hart_config_len = (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1) *
ms->smp.cpus;
diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index e4df298c23..50e3620c02 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -58,6 +58,7 @@ enum {
SIFIVE_U_DEBUG,
SIFIVE_U_MROM,
SIFIVE_U_CLINT,
+ SIFIVE_U_L2LIM,
SIFIVE_U_PLIC,
SIFIVE_U_PRCI,
SIFIVE_U_UART0,
--
2.23.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v1 2/6] riscv/sifive_u: Add QSPI memory region
2019-09-19 22:24 [PATCH v1 0/6] RISC-V: Add more machine memory Alistair Francis
2019-09-19 22:24 ` [PATCH v1 1/6] riscv/sifive_u: Add L2-LIM cache memory Alistair Francis
@ 2019-09-19 22:24 ` Alistair Francis
2019-09-20 5:15 ` Bin Meng
2019-09-19 22:25 ` [PATCH v1 3/6] riscv/sifive_u: Manually define the machine Alistair Francis
` (4 subsequent siblings)
6 siblings, 1 reply; 30+ messages in thread
From: Alistair Francis @ 2019-09-19 22:24 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis
There doesn't seem to be details on what QSPI the HiFive Unleashed uses.
To allow boot firmware developers to use QEMU to target the Unleashed
let's add a chunk of memory to represent the QSPI. This can be targeted
using QEMU's -device loader command line option.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
hw/riscv/sifive_u.c | 8 ++++++++
include/hw/riscv/sifive_u.h | 1 +
2 files changed, 9 insertions(+)
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index de6e197882..9c5d791320 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -71,6 +71,7 @@ static const struct MemmapEntry {
[SIFIVE_U_UART0] = { 0x10010000, 0x1000 },
[SIFIVE_U_UART1] = { 0x10011000, 0x1000 },
[SIFIVE_U_OTP] = { 0x10070000, 0x1000 },
+ [SIFIVE_U_FLASH0] = { 0x20000000, 0x2000000 },
[SIFIVE_U_DRAM] = { 0x80000000, 0x0 },
[SIFIVE_U_GEM] = { 0x10090000, 0x2000 },
[SIFIVE_U_GEM_MGMT] = { 0x100a0000, 0x1000 },
@@ -313,6 +314,7 @@ static void riscv_sifive_u_init(MachineState *machine)
SiFiveUState *s = g_new0(SiFiveUState, 1);
MemoryRegion *system_memory = get_system_memory();
MemoryRegion *main_mem = g_new(MemoryRegion, 1);
+ MemoryRegion *flash0 = g_new(MemoryRegion, 1);
int i;
/* Initialize SoC */
@@ -328,6 +330,12 @@ static void riscv_sifive_u_init(MachineState *machine)
memory_region_add_subregion(system_memory, memmap[SIFIVE_U_DRAM].base,
main_mem);
+ /* register QSPI0 Flash */
+ memory_region_init_ram(flash0, NULL, "riscv.sifive.u.flash0",
+ memmap[SIFIVE_U_FLASH0].size, &error_fatal);
+ memory_region_add_subregion(system_memory, memmap[SIFIVE_U_FLASH0].base,
+ flash0);
+
/* create device tree */
create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index 50e3620c02..2a08e2a5db 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -64,6 +64,7 @@ enum {
SIFIVE_U_UART0,
SIFIVE_U_UART1,
SIFIVE_U_OTP,
+ SIFIVE_U_FLASH0,
SIFIVE_U_DRAM,
SIFIVE_U_GEM,
SIFIVE_U_GEM_MGMT
--
2.23.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v1 3/6] riscv/sifive_u: Manually define the machine
2019-09-19 22:24 [PATCH v1 0/6] RISC-V: Add more machine memory Alistair Francis
2019-09-19 22:24 ` [PATCH v1 1/6] riscv/sifive_u: Add L2-LIM cache memory Alistair Francis
2019-09-19 22:24 ` [PATCH v1 2/6] riscv/sifive_u: Add QSPI memory region Alistair Francis
@ 2019-09-19 22:25 ` Alistair Francis
2019-09-20 5:15 ` Bin Meng
2019-09-19 22:25 ` [PATCH v1 4/6] riscv/sifive_u: Add the start-in-flash property Alistair Francis
` (3 subsequent siblings)
6 siblings, 1 reply; 30+ messages in thread
From: Alistair Francis @ 2019-09-19 22:25 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis
Instead of using the DEFINE_MACHINE() macro to define the machine let's
do it manually. This allows us to specify machine properties.
This patch is no functional change.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
hw/riscv/sifive_u.c | 27 +++++++++++++++++++++++----
include/hw/riscv/sifive_u.h | 7 ++++++-
2 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 9c5d791320..c3949fb316 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -310,8 +310,7 @@ static void create_fdt(SiFiveUState *s, const struct MemmapEntry *memmap,
static void riscv_sifive_u_init(MachineState *machine)
{
const struct MemmapEntry *memmap = sifive_u_memmap;
-
- SiFiveUState *s = g_new0(SiFiveUState, 1);
+ SiFiveUState *s = RISCV_U_MACHINE(machine);
MemoryRegion *system_memory = get_system_memory();
MemoryRegion *main_mem = g_new(MemoryRegion, 1);
MemoryRegion *flash0 = g_new(MemoryRegion, 1);
@@ -545,8 +544,15 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
}
-static void riscv_sifive_u_machine_init(MachineClass *mc)
+static void riscv_sifive_u_machine_instance_init(Object *obj)
+{
+
+}
+
+static void riscv_sifive_u_machine_class_init(ObjectClass *oc, void *data)
{
+ MachineClass *mc = MACHINE_CLASS(oc);
+
mc->desc = "RISC-V Board compatible with SiFive U SDK";
mc->init = riscv_sifive_u_init;
mc->max_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + SIFIVE_U_COMPUTE_CPU_COUNT;
@@ -554,7 +560,20 @@ static void riscv_sifive_u_machine_init(MachineClass *mc)
mc->default_cpus = mc->min_cpus;
}
-DEFINE_MACHINE("sifive_u", riscv_sifive_u_machine_init)
+static const TypeInfo riscv_sifive_u_machine_init_typeinfo = {
+ .name = MACHINE_TYPE_NAME("sifive_u"),
+ .parent = TYPE_MACHINE,
+ .class_init = riscv_sifive_u_machine_class_init,
+ .instance_init = riscv_sifive_u_machine_instance_init,
+ .instance_size = sizeof(SiFiveUState),
+};
+
+static void riscv_sifive_u_machine_init_register_types(void)
+{
+ type_register_static(&riscv_sifive_u_machine_init_typeinfo);
+}
+
+type_init(riscv_sifive_u_machine_init_register_types)
static void riscv_sifive_u_soc_class_init(ObjectClass *oc, void *data)
{
diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index 2a08e2a5db..a921079fbe 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -44,12 +44,17 @@ typedef struct SiFiveUSoCState {
CadenceGEMState gem;
} SiFiveUSoCState;
+#define TYPE_RISCV_U_MACHINE MACHINE_TYPE_NAME("sifive_u")
+#define RISCV_U_MACHINE(obj) \
+ OBJECT_CHECK(SiFiveUState, (obj), TYPE_RISCV_U_MACHINE)
+
typedef struct SiFiveUState {
/*< private >*/
- SysBusDevice parent_obj;
+ MachineState parent_obj;
/*< public >*/
SiFiveUSoCState soc;
+
void *fdt;
int fdt_size;
} SiFiveUState;
--
2.23.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v1 4/6] riscv/sifive_u: Add the start-in-flash property
2019-09-19 22:24 [PATCH v1 0/6] RISC-V: Add more machine memory Alistair Francis
` (2 preceding siblings ...)
2019-09-19 22:25 ` [PATCH v1 3/6] riscv/sifive_u: Manually define the machine Alistair Francis
@ 2019-09-19 22:25 ` Alistair Francis
2019-09-20 5:15 ` Bin Meng
2019-09-19 22:25 ` [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device Alistair Francis
` (2 subsequent siblings)
6 siblings, 1 reply; 30+ messages in thread
From: Alistair Francis @ 2019-09-19 22:25 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis
Add a property that when set to true QEMU will jump from the ROM code to
the start of flash memory instead of DRAM which is the default
behaviour.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
hw/riscv/sifive_u.c | 27 +++++++++++++++++++++++++++
include/hw/riscv/sifive_u.h | 2 ++
2 files changed, 29 insertions(+)
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index c3949fb316..b7cd3631cd 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -373,6 +373,10 @@ static void riscv_sifive_u_init(MachineState *machine)
/* dtb: */
};
+ if (s->start_in_flash) {
+ reset_vec[6] = memmap[SIFIVE_U_FLASH0].base; /* start: .dword FLASH0_BASE */
+ }
+
/* copy in the reset vector in little_endian byte order */
for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
reset_vec[i] = cpu_to_le32(reset_vec[i]);
@@ -544,8 +548,31 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
}
+static bool virt_get_start_in_flash(Object *obj, Error **errp)
+{
+ SiFiveUState *s = RISCV_U_MACHINE(obj);
+
+ return s->start_in_flash;
+}
+
+static void virt_set_start_in_flash(Object *obj, bool value, Error **errp)
+{
+ SiFiveUState *s = RISCV_U_MACHINE(obj);
+
+ s->start_in_flash = value;
+}
+
static void riscv_sifive_u_machine_instance_init(Object *obj)
{
+ SiFiveUState *s = RISCV_U_MACHINE(obj);
+
+ s->start_in_flash = false;
+ object_property_add_bool(obj, "start-in-flash", virt_get_start_in_flash,
+ virt_set_start_in_flash, NULL);
+ object_property_set_description(obj, "start-in-flash",
+ "Set on to tell QEMU's ROM to jump to " \
+ "flash. Otherwise QEMU will jump to DRAM",
+ NULL);
}
diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index a921079fbe..2656b43c58 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -57,6 +57,8 @@ typedef struct SiFiveUState {
void *fdt;
int fdt_size;
+
+ bool start_in_flash;
} SiFiveUState;
enum {
--
2.23.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device
2019-09-19 22:24 [PATCH v1 0/6] RISC-V: Add more machine memory Alistair Francis
` (3 preceding siblings ...)
2019-09-19 22:25 ` [PATCH v1 4/6] riscv/sifive_u: Add the start-in-flash property Alistair Francis
@ 2019-09-19 22:25 ` Alistair Francis
2019-09-20 5:15 ` Bin Meng
2019-09-19 22:25 ` [PATCH v1 6/6] riscv/virt: Jump to pflash if specified Alistair Francis
2019-09-20 22:40 ` [PATCH v1 0/6] RISC-V: Add more machine memory Palmer Dabbelt
6 siblings, 1 reply; 30+ messages in thread
From: Alistair Francis @ 2019-09-19 22:25 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis
Add the CFI01 PFlash to the RISC-V virt board. This is the same PFlash
from the ARM Virt board and the implementation is based on the ARM Virt
board. This allows users to specify flash files from the command line.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
hw/riscv/Kconfig | 1 +
hw/riscv/virt.c | 81 +++++++++++++++++++++++++++++++++++++++++
include/hw/riscv/virt.h | 3 ++
3 files changed, 85 insertions(+)
diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
index fb19b2df3a..b12660b9f8 100644
--- a/hw/riscv/Kconfig
+++ b/hw/riscv/Kconfig
@@ -36,4 +36,5 @@ config RISCV_VIRT
select SERIAL
select VIRTIO_MMIO
select PCI_EXPRESS_GENERIC_BRIDGE
+ select PFLASH_CFI01
select SIFIVE
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index d36f5625ec..ca002ecea7 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -26,6 +26,7 @@
#include "hw/boards.h"
#include "hw/loader.h"
#include "hw/sysbus.h"
+#include "hw/qdev-properties.h"
#include "hw/char/serial.h"
#include "target/riscv/cpu.h"
#include "hw/riscv/riscv_hart.h"
@@ -61,12 +62,72 @@ static const struct MemmapEntry {
[VIRT_PLIC] = { 0xc000000, 0x4000000 },
[VIRT_UART0] = { 0x10000000, 0x100 },
[VIRT_VIRTIO] = { 0x10001000, 0x1000 },
+ [VIRT_FLASH] = { 0x20000000, 0x2000000 },
[VIRT_DRAM] = { 0x80000000, 0x0 },
[VIRT_PCIE_MMIO] = { 0x40000000, 0x40000000 },
[VIRT_PCIE_PIO] = { 0x03000000, 0x00010000 },
[VIRT_PCIE_ECAM] = { 0x30000000, 0x10000000 },
};
+#define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
+
+static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
+ const char *name,
+ const char *alias_prop_name)
+{
+ /*
+ * Create a single flash device. We use the same parameters as
+ * the flash devices on the ARM virt board.
+ */
+ DeviceState *dev = qdev_create(NULL, TYPE_PFLASH_CFI01);
+
+ qdev_prop_set_uint64(dev, "sector-length", VIRT_FLASH_SECTOR_SIZE);
+ qdev_prop_set_uint8(dev, "width", 4);
+ qdev_prop_set_uint8(dev, "device-width", 2);
+ qdev_prop_set_bit(dev, "big-endian", false);
+ qdev_prop_set_uint16(dev, "id0", 0x89);
+ qdev_prop_set_uint16(dev, "id1", 0x18);
+ qdev_prop_set_uint16(dev, "id2", 0x00);
+ qdev_prop_set_uint16(dev, "id3", 0x00);
+ qdev_prop_set_string(dev, "name", name);
+
+ return PFLASH_CFI01(dev);
+}
+
+static void virt_flash_create(RISCVVirtState *s)
+{
+ s->flash[0] = virt_flash_create1(s, "virt.flash0", "pflash0");
+ s->flash[1] = virt_flash_create1(s, "virt.flash1", "pflash1");
+}
+
+static void virt_flash_map1(PFlashCFI01 *flash,
+ hwaddr base, hwaddr size,
+ MemoryRegion *sysmem)
+{
+ DeviceState *dev = DEVICE(flash);
+
+ assert(size % VIRT_FLASH_SECTOR_SIZE == 0);
+ assert(size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
+ qdev_prop_set_uint32(dev, "num-blocks", size / VIRT_FLASH_SECTOR_SIZE);
+ qdev_init_nofail(dev);
+
+ memory_region_add_subregion(sysmem, base,
+ sysbus_mmio_get_region(SYS_BUS_DEVICE(dev),
+ 0));
+}
+
+static void virt_flash_map(RISCVVirtState *s,
+ MemoryRegion *sysmem)
+{
+ hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
+ hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
+
+ virt_flash_map1(s->flash[0], flashbase, flashsize,
+ sysmem);
+ virt_flash_map1(s->flash[1], flashbase + flashsize, flashsize,
+ sysmem);
+}
+
static void create_pcie_irq_map(void *fdt, char *nodename,
uint32_t plic_phandle)
{
@@ -121,6 +182,8 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
char *nodename;
uint32_t plic_phandle, phandle = 1;
int i;
+ hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
+ hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
fdt = s->fdt = create_device_tree(&s->fdt_size);
if (!fdt) {
@@ -316,6 +379,15 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
}
g_free(nodename);
+
+ nodename = g_strdup_printf("/flash@%" PRIx64, flashbase);
+ qemu_fdt_add_subnode(s->fdt, nodename);
+ qemu_fdt_setprop_string(s->fdt, nodename, "compatible", "cfi-flash");
+ qemu_fdt_setprop_sized_cells(s->fdt, nodename, "reg",
+ 2, flashbase, 2, flashsize,
+ 2, flashbase + flashsize, 2, flashsize);
+ qemu_fdt_setprop_cell(s->fdt, nodename, "bank-width", 4);
+ g_free(nodename);
}
@@ -496,6 +568,15 @@ static void riscv_virt_board_init(MachineState *machine)
0, qdev_get_gpio_in(DEVICE(s->plic), UART0_IRQ), 399193,
serial_hd(0), DEVICE_LITTLE_ENDIAN);
+ virt_flash_create(s);
+
+ /* Map legacy -drive if=pflash to machine properties */
+ for (i = 0; i < ARRAY_SIZE(s->flash); i++) {
+ pflash_cfi01_legacy_drive(s->flash[i],
+ drive_get(IF_PFLASH, 0, i));
+ }
+ virt_flash_map(s, system_memory);
+
g_free(plic_hart_config);
}
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index 6e5fbe5d3b..2ca8bd3512 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -21,6 +21,7 @@
#include "hw/riscv/riscv_hart.h"
#include "hw/sysbus.h"
+#include "hw/block/flash.h"
typedef struct {
/*< private >*/
@@ -29,6 +30,7 @@ typedef struct {
/*< public >*/
RISCVHartArrayState soc;
DeviceState *plic;
+ PFlashCFI01 *flash[2];
void *fdt;
int fdt_size;
} RISCVVirtState;
@@ -41,6 +43,7 @@ enum {
VIRT_PLIC,
VIRT_UART0,
VIRT_VIRTIO,
+ VIRT_FLASH,
VIRT_DRAM,
VIRT_PCIE_MMIO,
VIRT_PCIE_PIO,
--
2.23.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v1 6/6] riscv/virt: Jump to pflash if specified
2019-09-19 22:24 [PATCH v1 0/6] RISC-V: Add more machine memory Alistair Francis
` (4 preceding siblings ...)
2019-09-19 22:25 ` [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device Alistair Francis
@ 2019-09-19 22:25 ` Alistair Francis
2019-09-20 5:15 ` Bin Meng
2019-09-20 22:40 ` [PATCH v1 0/6] RISC-V: Add more machine memory Palmer Dabbelt
6 siblings, 1 reply; 30+ messages in thread
From: Alistair Francis @ 2019-09-19 22:25 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis
If the user supplied pflash to QEMU then change the reset code to jump
to the pflash base address instead of the DRAM base address.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
hw/riscv/virt.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index ca002ecea7..ed25cc6761 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -441,6 +441,7 @@ static void riscv_virt_board_init(MachineState *machine)
MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
char *plic_hart_config;
size_t plic_hart_config_len;
+ target_ulong start_addr = memmap[VIRT_DRAM].base;
int i;
unsigned int smp_cpus = machine->smp.cpus;
@@ -487,6 +488,13 @@ static void riscv_virt_board_init(MachineState *machine)
}
}
+ if (drive_get(IF_PFLASH, 0, 0)) {
+ /* Pflash was supplied, let's overwrite the address we jump to after
+ * reset to the base of the flash.
+ */
+ start_addr = virt_memmap[VIRT_FLASH].base;
+ }
+
/* reset vector */
uint32_t reset_vec[8] = {
0x00000297, /* 1: auipc t0, %pcrel_hi(dtb) */
@@ -499,7 +507,7 @@ static void riscv_virt_board_init(MachineState *machine)
#endif
0x00028067, /* jr t0 */
0x00000000,
- memmap[VIRT_DRAM].base, /* start: .dword memmap[VIRT_DRAM].base */
+ start_addr, /* start: .dword */
0x00000000,
/* dtb: */
};
--
2.23.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v1 6/6] riscv/virt: Jump to pflash if specified
2019-09-19 22:25 ` [PATCH v1 6/6] riscv/virt: Jump to pflash if specified Alistair Francis
@ 2019-09-20 5:15 ` Bin Meng
2019-09-23 9:09 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 30+ messages in thread
From: Bin Meng @ 2019-09-20 5:15 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, Palmer Dabbelt, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Fri, Sep 20, 2019 at 6:35 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> If the user supplied pflash to QEMU then change the reset code to jump
> to the pflash base address instead of the DRAM base address.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> hw/riscv/virt.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index ca002ecea7..ed25cc6761 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -441,6 +441,7 @@ static void riscv_virt_board_init(MachineState *machine)
> MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> char *plic_hart_config;
> size_t plic_hart_config_len;
> + target_ulong start_addr = memmap[VIRT_DRAM].base;
> int i;
> unsigned int smp_cpus = machine->smp.cpus;
>
> @@ -487,6 +488,13 @@ static void riscv_virt_board_init(MachineState *machine)
> }
> }
>
> + if (drive_get(IF_PFLASH, 0, 0)) {
> + /* Pflash was supplied, let's overwrite the address we jump to after
nits: wrong multi-line comment format
> + * reset to the base of the flash.
> + */
> + start_addr = virt_memmap[VIRT_FLASH].base;
> + }
> +
> /* reset vector */
> uint32_t reset_vec[8] = {
> 0x00000297, /* 1: auipc t0, %pcrel_hi(dtb) */
> @@ -499,7 +507,7 @@ static void riscv_virt_board_init(MachineState *machine)
> #endif
> 0x00028067, /* jr t0 */
> 0x00000000,
> - memmap[VIRT_DRAM].base, /* start: .dword memmap[VIRT_DRAM].base */
> + start_addr, /* start: .dword */
> 0x00000000,
> /* dtb: */
> };
> --
Otherwise,
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device
2019-09-19 22:25 ` [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device Alistair Francis
@ 2019-09-20 5:15 ` Bin Meng
2019-09-20 22:12 ` Alistair Francis
0 siblings, 1 reply; 30+ messages in thread
From: Bin Meng @ 2019-09-20 5:15 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, Palmer Dabbelt, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Fri, Sep 20, 2019 at 6:36 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> Add the CFI01 PFlash to the RISC-V virt board. This is the same PFlash
> from the ARM Virt board and the implementation is based on the ARM Virt
> board. This allows users to specify flash files from the command line.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> hw/riscv/Kconfig | 1 +
> hw/riscv/virt.c | 81 +++++++++++++++++++++++++++++++++++++++++
> include/hw/riscv/virt.h | 3 ++
> 3 files changed, 85 insertions(+)
>
> diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> index fb19b2df3a..b12660b9f8 100644
> --- a/hw/riscv/Kconfig
> +++ b/hw/riscv/Kconfig
> @@ -36,4 +36,5 @@ config RISCV_VIRT
> select SERIAL
> select VIRTIO_MMIO
> select PCI_EXPRESS_GENERIC_BRIDGE
> + select PFLASH_CFI01
> select SIFIVE
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index d36f5625ec..ca002ecea7 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -26,6 +26,7 @@
> #include "hw/boards.h"
> #include "hw/loader.h"
> #include "hw/sysbus.h"
> +#include "hw/qdev-properties.h"
> #include "hw/char/serial.h"
> #include "target/riscv/cpu.h"
> #include "hw/riscv/riscv_hart.h"
> @@ -61,12 +62,72 @@ static const struct MemmapEntry {
> [VIRT_PLIC] = { 0xc000000, 0x4000000 },
> [VIRT_UART0] = { 0x10000000, 0x100 },
> [VIRT_VIRTIO] = { 0x10001000, 0x1000 },
> + [VIRT_FLASH] = { 0x20000000, 0x2000000 },
> [VIRT_DRAM] = { 0x80000000, 0x0 },
> [VIRT_PCIE_MMIO] = { 0x40000000, 0x40000000 },
> [VIRT_PCIE_PIO] = { 0x03000000, 0x00010000 },
> [VIRT_PCIE_ECAM] = { 0x30000000, 0x10000000 },
> };
>
> +#define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
> +
> +static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
> + const char *name,
> + const char *alias_prop_name)
> +{
> + /*
> + * Create a single flash device. We use the same parameters as
> + * the flash devices on the ARM virt board.
> + */
> + DeviceState *dev = qdev_create(NULL, TYPE_PFLASH_CFI01);
> +
> + qdev_prop_set_uint64(dev, "sector-length", VIRT_FLASH_SECTOR_SIZE);
> + qdev_prop_set_uint8(dev, "width", 4);
> + qdev_prop_set_uint8(dev, "device-width", 2);
> + qdev_prop_set_bit(dev, "big-endian", false);
> + qdev_prop_set_uint16(dev, "id0", 0x89);
> + qdev_prop_set_uint16(dev, "id1", 0x18);
> + qdev_prop_set_uint16(dev, "id2", 0x00);
> + qdev_prop_set_uint16(dev, "id3", 0x00);
> + qdev_prop_set_string(dev, "name", name);
alias_prop_name is unused? ARM virt has 2 more calls in the same function here.
> +
> + return PFLASH_CFI01(dev);
> +}
> +
> +static void virt_flash_create(RISCVVirtState *s)
> +{
> + s->flash[0] = virt_flash_create1(s, "virt.flash0", "pflash0");
> + s->flash[1] = virt_flash_create1(s, "virt.flash1", "pflash1");
I don't think we should mirror what is used on ARM virt board to
create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
they need distinguish secure and non-secure. For sifive_u, only one is
enough.
> +}
> +
> +static void virt_flash_map1(PFlashCFI01 *flash,
> + hwaddr base, hwaddr size,
> + MemoryRegion *sysmem)
> +{
> + DeviceState *dev = DEVICE(flash);
> +
> + assert(size % VIRT_FLASH_SECTOR_SIZE == 0);
> + assert(size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
> + qdev_prop_set_uint32(dev, "num-blocks", size / VIRT_FLASH_SECTOR_SIZE);
> + qdev_init_nofail(dev);
> +
> + memory_region_add_subregion(sysmem, base,
> + sysbus_mmio_get_region(SYS_BUS_DEVICE(dev),
> + 0));
> +}
> +
> +static void virt_flash_map(RISCVVirtState *s,
> + MemoryRegion *sysmem)
> +{
> + hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
> + hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
> +
> + virt_flash_map1(s->flash[0], flashbase, flashsize,
> + sysmem);
> + virt_flash_map1(s->flash[1], flashbase + flashsize, flashsize,
> + sysmem);
> +}
> +
> static void create_pcie_irq_map(void *fdt, char *nodename,
> uint32_t plic_phandle)
> {
> @@ -121,6 +182,8 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
> char *nodename;
> uint32_t plic_phandle, phandle = 1;
> int i;
> + hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
> + hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
>
> fdt = s->fdt = create_device_tree(&s->fdt_size);
> if (!fdt) {
> @@ -316,6 +379,15 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
> qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
> }
> g_free(nodename);
> +
> + nodename = g_strdup_printf("/flash@%" PRIx64, flashbase);
> + qemu_fdt_add_subnode(s->fdt, nodename);
> + qemu_fdt_setprop_string(s->fdt, nodename, "compatible", "cfi-flash");
> + qemu_fdt_setprop_sized_cells(s->fdt, nodename, "reg",
> + 2, flashbase, 2, flashsize,
> + 2, flashbase + flashsize, 2, flashsize);
> + qemu_fdt_setprop_cell(s->fdt, nodename, "bank-width", 4);
> + g_free(nodename);
> }
>
>
> @@ -496,6 +568,15 @@ static void riscv_virt_board_init(MachineState *machine)
> 0, qdev_get_gpio_in(DEVICE(s->plic), UART0_IRQ), 399193,
> serial_hd(0), DEVICE_LITTLE_ENDIAN);
>
> + virt_flash_create(s);
> +
> + /* Map legacy -drive if=pflash to machine properties */
> + for (i = 0; i < ARRAY_SIZE(s->flash); i++) {
> + pflash_cfi01_legacy_drive(s->flash[i],
> + drive_get(IF_PFLASH, 0, i));
> + }
> + virt_flash_map(s, system_memory);
> +
> g_free(plic_hart_config);
> }
>
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index 6e5fbe5d3b..2ca8bd3512 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -21,6 +21,7 @@
>
> #include "hw/riscv/riscv_hart.h"
> #include "hw/sysbus.h"
> +#include "hw/block/flash.h"
>
> typedef struct {
> /*< private >*/
> @@ -29,6 +30,7 @@ typedef struct {
> /*< public >*/
> RISCVHartArrayState soc;
> DeviceState *plic;
> + PFlashCFI01 *flash[2];
> void *fdt;
> int fdt_size;
> } RISCVVirtState;
> @@ -41,6 +43,7 @@ enum {
> VIRT_PLIC,
> VIRT_UART0,
> VIRT_VIRTIO,
> + VIRT_FLASH,
> VIRT_DRAM,
> VIRT_PCIE_MMIO,
> VIRT_PCIE_PIO,
> --
Regards,
Bin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 4/6] riscv/sifive_u: Add the start-in-flash property
2019-09-19 22:25 ` [PATCH v1 4/6] riscv/sifive_u: Add the start-in-flash property Alistair Francis
@ 2019-09-20 5:15 ` Bin Meng
2019-09-20 22:07 ` Alistair Francis
0 siblings, 1 reply; 30+ messages in thread
From: Bin Meng @ 2019-09-20 5:15 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, Palmer Dabbelt, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Fri, Sep 20, 2019 at 6:32 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> Add a property that when set to true QEMU will jump from the ROM code to
> the start of flash memory instead of DRAM which is the default
> behaviour.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> hw/riscv/sifive_u.c | 27 +++++++++++++++++++++++++++
> include/hw/riscv/sifive_u.h | 2 ++
> 2 files changed, 29 insertions(+)
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index c3949fb316..b7cd3631cd 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -373,6 +373,10 @@ static void riscv_sifive_u_init(MachineState *machine)
> /* dtb: */
> };
>
> + if (s->start_in_flash) {
> + reset_vec[6] = memmap[SIFIVE_U_FLASH0].base; /* start: .dword FLASH0_BASE */
> + }
> +
> /* copy in the reset vector in little_endian byte order */
> for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> reset_vec[i] = cpu_to_le32(reset_vec[i]);
> @@ -544,8 +548,31 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
> memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
> }
>
> +static bool virt_get_start_in_flash(Object *obj, Error **errp)
> +{
> + SiFiveUState *s = RISCV_U_MACHINE(obj);
> +
> + return s->start_in_flash;
> +}
> +
> +static void virt_set_start_in_flash(Object *obj, bool value, Error **errp)
> +{
> + SiFiveUState *s = RISCV_U_MACHINE(obj);
> +
> + s->start_in_flash = value;
> +}
> +
> static void riscv_sifive_u_machine_instance_init(Object *obj)
> {
> + SiFiveUState *s = RISCV_U_MACHINE(obj);
> +
> + s->start_in_flash = false;
> + object_property_add_bool(obj, "start-in-flash", virt_get_start_in_flash,
> + virt_set_start_in_flash, NULL);
> + object_property_set_description(obj, "start-in-flash",
> + "Set on to tell QEMU's ROM to jump to " \
> + "flash. Otherwise QEMU will jump to DRAM",
> + NULL);
>
> }
>
> diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> index a921079fbe..2656b43c58 100644
> --- a/include/hw/riscv/sifive_u.h
> +++ b/include/hw/riscv/sifive_u.h
> @@ -57,6 +57,8 @@ typedef struct SiFiveUState {
>
> void *fdt;
> int fdt_size;
> +
> + bool start_in_flash;
> } SiFiveUState;
>
> enum {
This patch chose a different way from the one used in patch "[v1,6/6]
riscv/virt: Jump to pflash if specified":
- this patch uses reset_vec[6] while patch [6/6] defines a variable start_addr
- this patch adds a "start-in-flash" property to the machine, while
patch [6/6] tests against drive IF_PFLASH
We should be consistent and I would prefer to use the patch [6/6] way.
On Unleashed an SPI flash is mounted so we cannot add a PFlash to
sifive_u machine like what was done on virt machine, so we should test
IF_MTD instead. Thoughts?
Regards,
Bin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 3/6] riscv/sifive_u: Manually define the machine
2019-09-19 22:25 ` [PATCH v1 3/6] riscv/sifive_u: Manually define the machine Alistair Francis
@ 2019-09-20 5:15 ` Bin Meng
0 siblings, 0 replies; 30+ messages in thread
From: Bin Meng @ 2019-09-20 5:15 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, Palmer Dabbelt, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Fri, Sep 20, 2019 at 6:34 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> Instead of using the DEFINE_MACHINE() macro to define the machine let's
> do it manually. This allows us to specify machine properties.
>
> This patch is no functional change.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> hw/riscv/sifive_u.c | 27 +++++++++++++++++++++++----
> include/hw/riscv/sifive_u.h | 7 ++++++-
> 2 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 9c5d791320..c3949fb316 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -310,8 +310,7 @@ static void create_fdt(SiFiveUState *s, const struct MemmapEntry *memmap,
> static void riscv_sifive_u_init(MachineState *machine)
> {
> const struct MemmapEntry *memmap = sifive_u_memmap;
> -
> - SiFiveUState *s = g_new0(SiFiveUState, 1);
> + SiFiveUState *s = RISCV_U_MACHINE(machine);
> MemoryRegion *system_memory = get_system_memory();
> MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> MemoryRegion *flash0 = g_new(MemoryRegion, 1);
> @@ -545,8 +544,15 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
> memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
> }
>
> -static void riscv_sifive_u_machine_init(MachineClass *mc)
> +static void riscv_sifive_u_machine_instance_init(Object *obj)
> +{
> +
nits: remove this blank line
> +}
> +
> +static void riscv_sifive_u_machine_class_init(ObjectClass *oc, void *data)
> {
> + MachineClass *mc = MACHINE_CLASS(oc);
> +
> mc->desc = "RISC-V Board compatible with SiFive U SDK";
> mc->init = riscv_sifive_u_init;
> mc->max_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + SIFIVE_U_COMPUTE_CPU_COUNT;
> @@ -554,7 +560,20 @@ static void riscv_sifive_u_machine_init(MachineClass *mc)
> mc->default_cpus = mc->min_cpus;
> }
>
> -DEFINE_MACHINE("sifive_u", riscv_sifive_u_machine_init)
> +static const TypeInfo riscv_sifive_u_machine_init_typeinfo = {
nits: riscv_sifive_u_machine_typeinfo (no _init for consistency with others)
> + .name = MACHINE_TYPE_NAME("sifive_u"),
> + .parent = TYPE_MACHINE,
> + .class_init = riscv_sifive_u_machine_class_init,
> + .instance_init = riscv_sifive_u_machine_instance_init,
> + .instance_size = sizeof(SiFiveUState),
> +};
> +
> +static void riscv_sifive_u_machine_init_register_types(void)
> +{
> + type_register_static(&riscv_sifive_u_machine_init_typeinfo);
> +}
> +
> +type_init(riscv_sifive_u_machine_init_register_types)
nits: I would move the machine declaration to after the sifive_u SoC
declaration in this file.
>
> static void riscv_sifive_u_soc_class_init(ObjectClass *oc, void *data)
> {
> diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> index 2a08e2a5db..a921079fbe 100644
> --- a/include/hw/riscv/sifive_u.h
> +++ b/include/hw/riscv/sifive_u.h
> @@ -44,12 +44,17 @@ typedef struct SiFiveUSoCState {
> CadenceGEMState gem;
> } SiFiveUSoCState;
>
> +#define TYPE_RISCV_U_MACHINE MACHINE_TYPE_NAME("sifive_u")
> +#define RISCV_U_MACHINE(obj) \
> + OBJECT_CHECK(SiFiveUState, (obj), TYPE_RISCV_U_MACHINE)
> +
> typedef struct SiFiveUState {
> /*< private >*/
> - SysBusDevice parent_obj;
> + MachineState parent_obj;
>
> /*< public >*/
> SiFiveUSoCState soc;
> +
> void *fdt;
> int fdt_size;
> } SiFiveUState;
> --
Regards,
Bin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 2/6] riscv/sifive_u: Add QSPI memory region
2019-09-19 22:24 ` [PATCH v1 2/6] riscv/sifive_u: Add QSPI memory region Alistair Francis
@ 2019-09-20 5:15 ` Bin Meng
0 siblings, 0 replies; 30+ messages in thread
From: Bin Meng @ 2019-09-20 5:15 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, Palmer Dabbelt, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Fri, Sep 20, 2019 at 6:32 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> There doesn't seem to be details on what QSPI the HiFive Unleashed uses.
IMHO, this sentence should be removed as there are details available.
See the hifive-unleashed-a00.dts.
&qspi0 {
status = "okay";
flash@0 {
compatible = "issi,is25wp256", "jedec,spi-nor";
..
> To allow boot firmware developers to use QEMU to target the Unleashed
> let's add a chunk of memory to represent the QSPI. This can be targeted
nits: to represent the QSPI0 memory-mapped flash
> using QEMU's -device loader command line option.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> hw/riscv/sifive_u.c | 8 ++++++++
> include/hw/riscv/sifive_u.h | 1 +
> 2 files changed, 9 insertions(+)
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index de6e197882..9c5d791320 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -71,6 +71,7 @@ static const struct MemmapEntry {
> [SIFIVE_U_UART0] = { 0x10010000, 0x1000 },
> [SIFIVE_U_UART1] = { 0x10011000, 0x1000 },
> [SIFIVE_U_OTP] = { 0x10070000, 0x1000 },
> + [SIFIVE_U_FLASH0] = { 0x20000000, 0x2000000 },
We should map 256MiB per the manual.
> [SIFIVE_U_DRAM] = { 0x80000000, 0x0 },
> [SIFIVE_U_GEM] = { 0x10090000, 0x2000 },
> [SIFIVE_U_GEM_MGMT] = { 0x100a0000, 0x1000 },
> @@ -313,6 +314,7 @@ static void riscv_sifive_u_init(MachineState *machine)
> SiFiveUState *s = g_new0(SiFiveUState, 1);
> MemoryRegion *system_memory = get_system_memory();
> MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> + MemoryRegion *flash0 = g_new(MemoryRegion, 1);
> int i;
>
> /* Initialize SoC */
> @@ -328,6 +330,12 @@ static void riscv_sifive_u_init(MachineState *machine)
> memory_region_add_subregion(system_memory, memmap[SIFIVE_U_DRAM].base,
> main_mem);
>
> + /* register QSPI0 Flash */
> + memory_region_init_ram(flash0, NULL, "riscv.sifive.u.flash0",
> + memmap[SIFIVE_U_FLASH0].size, &error_fatal);
> + memory_region_add_subregion(system_memory, memmap[SIFIVE_U_FLASH0].base,
> + flash0);
> +
> /* create device tree */
> create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
>
> diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> index 50e3620c02..2a08e2a5db 100644
> --- a/include/hw/riscv/sifive_u.h
> +++ b/include/hw/riscv/sifive_u.h
> @@ -64,6 +64,7 @@ enum {
> SIFIVE_U_UART0,
> SIFIVE_U_UART1,
> SIFIVE_U_OTP,
> + SIFIVE_U_FLASH0,
> SIFIVE_U_DRAM,
> SIFIVE_U_GEM,
> SIFIVE_U_GEM_MGMT
> --
Regards,
Bin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 1/6] riscv/sifive_u: Add L2-LIM cache memory
2019-09-19 22:24 ` [PATCH v1 1/6] riscv/sifive_u: Add L2-LIM cache memory Alistair Francis
@ 2019-09-20 5:15 ` Bin Meng
0 siblings, 0 replies; 30+ messages in thread
From: Bin Meng @ 2019-09-20 5:15 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, Palmer Dabbelt, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Fri, Sep 20, 2019 at 6:32 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> On reset only a single L2 cache way is enabled, the others are exposed
> as memory that can be used by early boot firmware. This L2 region is
> generally disabled using the WayEnable register at a later stage in the
> boot process. To allow firmware to target QEMU and the HiFive Unleashed
> let's add the L2 LIM (LooselyIntegrated Memory).
>
> Ideally we would want to adjust the size of this chunk of memory as the
> L2 Cache Controller WayEnable register is incremented. Unfortunately I
> don't see a nice way to handle reducing or blocking out the L2 LIM while
> still allowing it be re returned to all enabled from a reset.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> hw/riscv/sifive_u.c | 15 +++++++++++++++
> include/hw/riscv/sifive_u.h | 1 +
> 2 files changed, 16 insertions(+)
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 9f8e84bf2e..de6e197882 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -65,6 +65,7 @@ static const struct MemmapEntry {
> [SIFIVE_U_DEBUG] = { 0x0, 0x100 },
> [SIFIVE_U_MROM] = { 0x1000, 0x11000 },
> [SIFIVE_U_CLINT] = { 0x2000000, 0x10000 },
> + [SIFIVE_U_L2LIM] = { 0x8000000, 0x1e00000 },
The size should be 0x2000000.
> [SIFIVE_U_PLIC] = { 0xc000000, 0x4000000 },
> [SIFIVE_U_PRCI] = { 0x10000000, 0x1000 },
> [SIFIVE_U_UART0] = { 0x10010000, 0x1000 },
> @@ -431,6 +432,7 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
> const struct MemmapEntry *memmap = sifive_u_memmap;
> MemoryRegion *system_memory = get_system_memory();
> MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> + MemoryRegion *l2lim_mem = g_new(MemoryRegion, 1);
> qemu_irq plic_gpios[SIFIVE_U_PLIC_NUM_SOURCES];
> char *plic_hart_config;
> size_t plic_hart_config_len;
> @@ -459,6 +461,19 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
> memory_region_add_subregion(system_memory, memmap[SIFIVE_U_MROM].base,
> mask_rom);
>
> + /* Add L2-LIM at reset size.
nits: wrong multi-line comment format
> + * This should be reduced in size as the L2 Cache Controller WayEnable
> + * register is incremented. Unfortunately I don't see a nice (or any) way
> + * to handle reducing or blocking out the L2 LIM while still allowing it
> + * be re returned to all enabled after a reset. For the time being, just
> + * leave it enabled all the time. This won't break anything, but will be
> + * too generous to misbehaving guests.
> + */
> + memory_region_init_ram(l2lim_mem, NULL, "riscv.sifive.u.l2lim",
> + memmap[SIFIVE_U_L2LIM].size, &error_fatal);
> + memory_region_add_subregion(system_memory, memmap[SIFIVE_U_L2LIM].base,
> + l2lim_mem);
> +
> /* create PLIC hart topology configuration string */
> plic_hart_config_len = (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1) *
> ms->smp.cpus;
> diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> index e4df298c23..50e3620c02 100644
> --- a/include/hw/riscv/sifive_u.h
> +++ b/include/hw/riscv/sifive_u.h
> @@ -58,6 +58,7 @@ enum {
> SIFIVE_U_DEBUG,
> SIFIVE_U_MROM,
> SIFIVE_U_CLINT,
> + SIFIVE_U_L2LIM,
> SIFIVE_U_PLIC,
> SIFIVE_U_PRCI,
> SIFIVE_U_UART0,
> --
Regards,
Bin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 4/6] riscv/sifive_u: Add the start-in-flash property
2019-09-20 5:15 ` Bin Meng
@ 2019-09-20 22:07 ` Alistair Francis
2019-09-22 2:19 ` Bin Meng
0 siblings, 1 reply; 30+ messages in thread
From: Alistair Francis @ 2019-09-20 22:07 UTC (permalink / raw)
To: Bin Meng
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers
On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, Sep 20, 2019 at 6:32 AM Alistair Francis
> <alistair.francis@wdc.com> wrote:
> >
> > Add a property that when set to true QEMU will jump from the ROM code to
> > the start of flash memory instead of DRAM which is the default
> > behaviour.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > hw/riscv/sifive_u.c | 27 +++++++++++++++++++++++++++
> > include/hw/riscv/sifive_u.h | 2 ++
> > 2 files changed, 29 insertions(+)
> >
> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > index c3949fb316..b7cd3631cd 100644
> > --- a/hw/riscv/sifive_u.c
> > +++ b/hw/riscv/sifive_u.c
> > @@ -373,6 +373,10 @@ static void riscv_sifive_u_init(MachineState *machine)
> > /* dtb: */
> > };
> >
> > + if (s->start_in_flash) {
> > + reset_vec[6] = memmap[SIFIVE_U_FLASH0].base; /* start: .dword FLASH0_BASE */
> > + }
> > +
> > /* copy in the reset vector in little_endian byte order */
> > for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > @@ -544,8 +548,31 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
> > memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
> > }
> >
> > +static bool virt_get_start_in_flash(Object *obj, Error **errp)
> > +{
> > + SiFiveUState *s = RISCV_U_MACHINE(obj);
> > +
> > + return s->start_in_flash;
> > +}
> > +
> > +static void virt_set_start_in_flash(Object *obj, bool value, Error **errp)
> > +{
> > + SiFiveUState *s = RISCV_U_MACHINE(obj);
> > +
> > + s->start_in_flash = value;
> > +}
> > +
> > static void riscv_sifive_u_machine_instance_init(Object *obj)
> > {
> > + SiFiveUState *s = RISCV_U_MACHINE(obj);
> > +
> > + s->start_in_flash = false;
> > + object_property_add_bool(obj, "start-in-flash", virt_get_start_in_flash,
> > + virt_set_start_in_flash, NULL);
> > + object_property_set_description(obj, "start-in-flash",
> > + "Set on to tell QEMU's ROM to jump to " \
> > + "flash. Otherwise QEMU will jump to DRAM",
> > + NULL);
> >
> > }
> >
> > diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> > index a921079fbe..2656b43c58 100644
> > --- a/include/hw/riscv/sifive_u.h
> > +++ b/include/hw/riscv/sifive_u.h
> > @@ -57,6 +57,8 @@ typedef struct SiFiveUState {
> >
> > void *fdt;
> > int fdt_size;
> > +
> > + bool start_in_flash;
> > } SiFiveUState;
> >
> > enum {
>
> This patch chose a different way from the one used in patch "[v1,6/6]
> riscv/virt: Jump to pflash if specified":
>
> - this patch uses reset_vec[6] while patch [6/6] defines a variable start_addr
> - this patch adds a "start-in-flash" property to the machine, while
> patch [6/6] tests against drive IF_PFLASH
Yes, we do it differently for the sifive_u board as the sifive_u board
doesn't use pflash so there is no way to know if the user has loaded
anything into the SPI memory.
>
> We should be consistent and I would prefer to use the patch [6/6] way.
> On Unleashed an SPI flash is mounted so we cannot add a PFlash to
> sifive_u machine like what was done on virt machine, so we should test
> IF_MTD instead. Thoughts?
How would we test that?
Right now I am loading the binary in SPI with the -device loader
option. The machine can't really know what is/isn't loaded there.
It's not ideal, but I don't see a nicer way.
Alistair
>
> Regards,
> Bin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device
2019-09-20 5:15 ` Bin Meng
@ 2019-09-20 22:12 ` Alistair Francis
2019-09-22 2:15 ` Bin Meng
2019-09-23 21:46 ` Peter Maydell
0 siblings, 2 replies; 30+ messages in thread
From: Alistair Francis @ 2019-09-20 22:12 UTC (permalink / raw)
To: Bin Meng
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers
On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, Sep 20, 2019 at 6:36 AM Alistair Francis
> <alistair.francis@wdc.com> wrote:
> >
> > Add the CFI01 PFlash to the RISC-V virt board. This is the same PFlash
> > from the ARM Virt board and the implementation is based on the ARM Virt
> > board. This allows users to specify flash files from the command line.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > hw/riscv/Kconfig | 1 +
> > hw/riscv/virt.c | 81 +++++++++++++++++++++++++++++++++++++++++
> > include/hw/riscv/virt.h | 3 ++
> > 3 files changed, 85 insertions(+)
> >
> > diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> > index fb19b2df3a..b12660b9f8 100644
> > --- a/hw/riscv/Kconfig
> > +++ b/hw/riscv/Kconfig
> > @@ -36,4 +36,5 @@ config RISCV_VIRT
> > select SERIAL
> > select VIRTIO_MMIO
> > select PCI_EXPRESS_GENERIC_BRIDGE
> > + select PFLASH_CFI01
> > select SIFIVE
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index d36f5625ec..ca002ecea7 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -26,6 +26,7 @@
> > #include "hw/boards.h"
> > #include "hw/loader.h"
> > #include "hw/sysbus.h"
> > +#include "hw/qdev-properties.h"
> > #include "hw/char/serial.h"
> > #include "target/riscv/cpu.h"
> > #include "hw/riscv/riscv_hart.h"
> > @@ -61,12 +62,72 @@ static const struct MemmapEntry {
> > [VIRT_PLIC] = { 0xc000000, 0x4000000 },
> > [VIRT_UART0] = { 0x10000000, 0x100 },
> > [VIRT_VIRTIO] = { 0x10001000, 0x1000 },
> > + [VIRT_FLASH] = { 0x20000000, 0x2000000 },
> > [VIRT_DRAM] = { 0x80000000, 0x0 },
> > [VIRT_PCIE_MMIO] = { 0x40000000, 0x40000000 },
> > [VIRT_PCIE_PIO] = { 0x03000000, 0x00010000 },
> > [VIRT_PCIE_ECAM] = { 0x30000000, 0x10000000 },
> > };
> >
> > +#define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
> > +
> > +static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
> > + const char *name,
> > + const char *alias_prop_name)
> > +{
> > + /*
> > + * Create a single flash device. We use the same parameters as
> > + * the flash devices on the ARM virt board.
> > + */
> > + DeviceState *dev = qdev_create(NULL, TYPE_PFLASH_CFI01);
> > +
> > + qdev_prop_set_uint64(dev, "sector-length", VIRT_FLASH_SECTOR_SIZE);
> > + qdev_prop_set_uint8(dev, "width", 4);
> > + qdev_prop_set_uint8(dev, "device-width", 2);
> > + qdev_prop_set_bit(dev, "big-endian", false);
> > + qdev_prop_set_uint16(dev, "id0", 0x89);
> > + qdev_prop_set_uint16(dev, "id1", 0x18);
> > + qdev_prop_set_uint16(dev, "id2", 0x00);
> > + qdev_prop_set_uint16(dev, "id3", 0x00);
> > + qdev_prop_set_string(dev, "name", name);
>
> alias_prop_name is unused? ARM virt has 2 more calls in the same function here.
Yep, you are right. I have removed this.
>
> > +
> > + return PFLASH_CFI01(dev);
> > +}
> > +
> > +static void virt_flash_create(RISCVVirtState *s)
> > +{
> > + s->flash[0] = virt_flash_create1(s, "virt.flash0", "pflash0");
> > + s->flash[1] = virt_flash_create1(s, "virt.flash1", "pflash1");
>
> I don't think we should mirror what is used on ARM virt board to
> create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
> they need distinguish secure and non-secure. For sifive_u, only one is
> enough.
I went back and forward about 1 or 2. Two seems more usable as maybe
someone wants to include two pflash files? The Xilinx machine also has
two so I'm kind of used to 2, but I'm not really fussed.
Unless anyone else wants two I will change it to 1.
Alistair
>
> > +}
> > +
> > +static void virt_flash_map1(PFlashCFI01 *flash,
> > + hwaddr base, hwaddr size,
> > + MemoryRegion *sysmem)
> > +{
> > + DeviceState *dev = DEVICE(flash);
> > +
> > + assert(size % VIRT_FLASH_SECTOR_SIZE == 0);
> > + assert(size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
> > + qdev_prop_set_uint32(dev, "num-blocks", size / VIRT_FLASH_SECTOR_SIZE);
> > + qdev_init_nofail(dev);
> > +
> > + memory_region_add_subregion(sysmem, base,
> > + sysbus_mmio_get_region(SYS_BUS_DEVICE(dev),
> > + 0));
> > +}
> > +
> > +static void virt_flash_map(RISCVVirtState *s,
> > + MemoryRegion *sysmem)
> > +{
> > + hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
> > + hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
> > +
> > + virt_flash_map1(s->flash[0], flashbase, flashsize,
> > + sysmem);
> > + virt_flash_map1(s->flash[1], flashbase + flashsize, flashsize,
> > + sysmem);
> > +}
> > +
> > static void create_pcie_irq_map(void *fdt, char *nodename,
> > uint32_t plic_phandle)
> > {
> > @@ -121,6 +182,8 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
> > char *nodename;
> > uint32_t plic_phandle, phandle = 1;
> > int i;
> > + hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
> > + hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
> >
> > fdt = s->fdt = create_device_tree(&s->fdt_size);
> > if (!fdt) {
> > @@ -316,6 +379,15 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
> > qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
> > }
> > g_free(nodename);
> > +
> > + nodename = g_strdup_printf("/flash@%" PRIx64, flashbase);
> > + qemu_fdt_add_subnode(s->fdt, nodename);
> > + qemu_fdt_setprop_string(s->fdt, nodename, "compatible", "cfi-flash");
> > + qemu_fdt_setprop_sized_cells(s->fdt, nodename, "reg",
> > + 2, flashbase, 2, flashsize,
> > + 2, flashbase + flashsize, 2, flashsize);
> > + qemu_fdt_setprop_cell(s->fdt, nodename, "bank-width", 4);
> > + g_free(nodename);
> > }
> >
> >
> > @@ -496,6 +568,15 @@ static void riscv_virt_board_init(MachineState *machine)
> > 0, qdev_get_gpio_in(DEVICE(s->plic), UART0_IRQ), 399193,
> > serial_hd(0), DEVICE_LITTLE_ENDIAN);
> >
> > + virt_flash_create(s);
> > +
> > + /* Map legacy -drive if=pflash to machine properties */
> > + for (i = 0; i < ARRAY_SIZE(s->flash); i++) {
> > + pflash_cfi01_legacy_drive(s->flash[i],
> > + drive_get(IF_PFLASH, 0, i));
> > + }
> > + virt_flash_map(s, system_memory);
> > +
> > g_free(plic_hart_config);
> > }
> >
> > diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> > index 6e5fbe5d3b..2ca8bd3512 100644
> > --- a/include/hw/riscv/virt.h
> > +++ b/include/hw/riscv/virt.h
> > @@ -21,6 +21,7 @@
> >
> > #include "hw/riscv/riscv_hart.h"
> > #include "hw/sysbus.h"
> > +#include "hw/block/flash.h"
> >
> > typedef struct {
> > /*< private >*/
> > @@ -29,6 +30,7 @@ typedef struct {
> > /*< public >*/
> > RISCVHartArrayState soc;
> > DeviceState *plic;
> > + PFlashCFI01 *flash[2];
> > void *fdt;
> > int fdt_size;
> > } RISCVVirtState;
> > @@ -41,6 +43,7 @@ enum {
> > VIRT_PLIC,
> > VIRT_UART0,
> > VIRT_VIRTIO,
> > + VIRT_FLASH,
> > VIRT_DRAM,
> > VIRT_PCIE_MMIO,
> > VIRT_PCIE_PIO,
> > --
>
> Regards,
> Bin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 0/6] RISC-V: Add more machine memory
2019-09-19 22:24 [PATCH v1 0/6] RISC-V: Add more machine memory Alistair Francis
` (5 preceding siblings ...)
2019-09-19 22:25 ` [PATCH v1 6/6] riscv/virt: Jump to pflash if specified Alistair Francis
@ 2019-09-20 22:40 ` Palmer Dabbelt
6 siblings, 0 replies; 30+ messages in thread
From: Palmer Dabbelt @ 2019-09-20 22:40 UTC (permalink / raw)
To: Alistair Francis; +Cc: Alistair Francis, qemu-riscv, qemu-devel, alistair23
On Thu, 19 Sep 2019 15:24:51 PDT (-0700), Alistair Francis wrote:
> This series aims to improve the use of QEMU for developing boot code. It
> does a few things:
>
> - sifive_u machine:
> - Adds a chunk of memory in the Flash area. This allows boot loaders
> to use this memory. I can't find details on the QSPI flash used on
> the real board, so this is the best bet at the moment.
IIRC it's a is25wp256.
> - Adds a chunk of memory in the L2-LIM area. This is actualy the L2
> cache and should shrink as the L2 cache is enalbed. Unfortunatley I
> don't see a nice way to shrink this memory.
> - Adds a property that allows users to specify if QEMU should jump to
> flash or DRAM after the ROM code.
>
> - virt machine:
> - Add the pflash_cfi01 flash device. This is based on the ARM virt
> board implementation
> - Adjusts QEMU to jump to the flash if a user has speciefied any
> pflash.
>
> Both machines have been tested with oreboot, but this should also help
> the coreboot developers.
>
> Alistair Francis (6):
> riscv/sifive_u: Add L2-LIM cache memory
> riscv/sifive_u: Add QSPI memory region
> riscv/sifive_u: Manually define the machine
> riscv/sifive_u: Add the start-in-flash property
> riscv/virt: Add the PFlash CFI01 device
> riscv/virt: Jump to pflash if specified
>
> hw/riscv/Kconfig | 1 +
> hw/riscv/sifive_u.c | 77 +++++++++++++++++++++++++++++--
> hw/riscv/virt.c | 91 ++++++++++++++++++++++++++++++++++++-
> include/hw/riscv/sifive_u.h | 11 ++++-
> include/hw/riscv/virt.h | 3 ++
> 5 files changed, 177 insertions(+), 6 deletions(-)
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device
2019-09-20 22:12 ` Alistair Francis
@ 2019-09-22 2:15 ` Bin Meng
2019-09-23 20:08 ` Alistair Francis
2019-09-23 21:46 ` Peter Maydell
1 sibling, 1 reply; 30+ messages in thread
From: Bin Meng @ 2019-09-22 2:15 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers
On Sat, Sep 21, 2019 at 6:16 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Fri, Sep 20, 2019 at 6:36 AM Alistair Francis
> > <alistair.francis@wdc.com> wrote:
> > >
> > > Add the CFI01 PFlash to the RISC-V virt board. This is the same PFlash
> > > from the ARM Virt board and the implementation is based on the ARM Virt
> > > board. This allows users to specify flash files from the command line.
> > >
> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > ---
> > > hw/riscv/Kconfig | 1 +
> > > hw/riscv/virt.c | 81 +++++++++++++++++++++++++++++++++++++++++
> > > include/hw/riscv/virt.h | 3 ++
> > > 3 files changed, 85 insertions(+)
> > >
> > > diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> > > index fb19b2df3a..b12660b9f8 100644
> > > --- a/hw/riscv/Kconfig
> > > +++ b/hw/riscv/Kconfig
> > > @@ -36,4 +36,5 @@ config RISCV_VIRT
> > > select SERIAL
> > > select VIRTIO_MMIO
> > > select PCI_EXPRESS_GENERIC_BRIDGE
> > > + select PFLASH_CFI01
> > > select SIFIVE
> > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > > index d36f5625ec..ca002ecea7 100644
> > > --- a/hw/riscv/virt.c
> > > +++ b/hw/riscv/virt.c
> > > @@ -26,6 +26,7 @@
> > > #include "hw/boards.h"
> > > #include "hw/loader.h"
> > > #include "hw/sysbus.h"
> > > +#include "hw/qdev-properties.h"
> > > #include "hw/char/serial.h"
> > > #include "target/riscv/cpu.h"
> > > #include "hw/riscv/riscv_hart.h"
> > > @@ -61,12 +62,72 @@ static const struct MemmapEntry {
> > > [VIRT_PLIC] = { 0xc000000, 0x4000000 },
> > > [VIRT_UART0] = { 0x10000000, 0x100 },
> > > [VIRT_VIRTIO] = { 0x10001000, 0x1000 },
> > > + [VIRT_FLASH] = { 0x20000000, 0x2000000 },
> > > [VIRT_DRAM] = { 0x80000000, 0x0 },
> > > [VIRT_PCIE_MMIO] = { 0x40000000, 0x40000000 },
> > > [VIRT_PCIE_PIO] = { 0x03000000, 0x00010000 },
> > > [VIRT_PCIE_ECAM] = { 0x30000000, 0x10000000 },
> > > };
> > >
> > > +#define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
> > > +
> > > +static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
> > > + const char *name,
> > > + const char *alias_prop_name)
> > > +{
> > > + /*
> > > + * Create a single flash device. We use the same parameters as
> > > + * the flash devices on the ARM virt board.
> > > + */
> > > + DeviceState *dev = qdev_create(NULL, TYPE_PFLASH_CFI01);
> > > +
> > > + qdev_prop_set_uint64(dev, "sector-length", VIRT_FLASH_SECTOR_SIZE);
> > > + qdev_prop_set_uint8(dev, "width", 4);
> > > + qdev_prop_set_uint8(dev, "device-width", 2);
> > > + qdev_prop_set_bit(dev, "big-endian", false);
> > > + qdev_prop_set_uint16(dev, "id0", 0x89);
> > > + qdev_prop_set_uint16(dev, "id1", 0x18);
> > > + qdev_prop_set_uint16(dev, "id2", 0x00);
> > > + qdev_prop_set_uint16(dev, "id3", 0x00);
> > > + qdev_prop_set_string(dev, "name", name);
> >
> > alias_prop_name is unused? ARM virt has 2 more calls in the same function here.
>
> Yep, you are right. I have removed this.
Any reason of removing this?
>
> >
> > > +
> > > + return PFLASH_CFI01(dev);
> > > +}
> > > +
> > > +static void virt_flash_create(RISCVVirtState *s)
> > > +{
> > > + s->flash[0] = virt_flash_create1(s, "virt.flash0", "pflash0");
> > > + s->flash[1] = virt_flash_create1(s, "virt.flash1", "pflash1");
> >
> > I don't think we should mirror what is used on ARM virt board to
> > create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
> > they need distinguish secure and non-secure. For sifive_u, only one is
> > enough.
>
> I went back and forward about 1 or 2. Two seems more usable as maybe
> someone wants to include two pflash files? The Xilinx machine also has
> two so I'm kind of used to 2, but I'm not really fussed.
>
> Unless anyone else wants two I will change it to 1.
Regards,
Bin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 4/6] riscv/sifive_u: Add the start-in-flash property
2019-09-20 22:07 ` Alistair Francis
@ 2019-09-22 2:19 ` Bin Meng
2019-09-23 17:51 ` Alistair Francis
0 siblings, 1 reply; 30+ messages in thread
From: Bin Meng @ 2019-09-22 2:19 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers
On Sat, Sep 21, 2019 at 6:12 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Fri, Sep 20, 2019 at 6:32 AM Alistair Francis
> > <alistair.francis@wdc.com> wrote:
> > >
> > > Add a property that when set to true QEMU will jump from the ROM code to
> > > the start of flash memory instead of DRAM which is the default
> > > behaviour.
> > >
> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > ---
> > > hw/riscv/sifive_u.c | 27 +++++++++++++++++++++++++++
> > > include/hw/riscv/sifive_u.h | 2 ++
> > > 2 files changed, 29 insertions(+)
> > >
> > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > > index c3949fb316..b7cd3631cd 100644
> > > --- a/hw/riscv/sifive_u.c
> > > +++ b/hw/riscv/sifive_u.c
> > > @@ -373,6 +373,10 @@ static void riscv_sifive_u_init(MachineState *machine)
> > > /* dtb: */
> > > };
> > >
> > > + if (s->start_in_flash) {
> > > + reset_vec[6] = memmap[SIFIVE_U_FLASH0].base; /* start: .dword FLASH0_BASE */
> > > + }
> > > +
> > > /* copy in the reset vector in little_endian byte order */
> > > for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > > reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > > @@ -544,8 +548,31 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
> > > memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
> > > }
> > >
> > > +static bool virt_get_start_in_flash(Object *obj, Error **errp)
> > > +{
> > > + SiFiveUState *s = RISCV_U_MACHINE(obj);
> > > +
> > > + return s->start_in_flash;
> > > +}
> > > +
> > > +static void virt_set_start_in_flash(Object *obj, bool value, Error **errp)
> > > +{
> > > + SiFiveUState *s = RISCV_U_MACHINE(obj);
> > > +
> > > + s->start_in_flash = value;
> > > +}
> > > +
> > > static void riscv_sifive_u_machine_instance_init(Object *obj)
> > > {
> > > + SiFiveUState *s = RISCV_U_MACHINE(obj);
> > > +
> > > + s->start_in_flash = false;
> > > + object_property_add_bool(obj, "start-in-flash", virt_get_start_in_flash,
> > > + virt_set_start_in_flash, NULL);
> > > + object_property_set_description(obj, "start-in-flash",
> > > + "Set on to tell QEMU's ROM to jump to " \
> > > + "flash. Otherwise QEMU will jump to DRAM",
> > > + NULL);
> > >
> > > }
> > >
> > > diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> > > index a921079fbe..2656b43c58 100644
> > > --- a/include/hw/riscv/sifive_u.h
> > > +++ b/include/hw/riscv/sifive_u.h
> > > @@ -57,6 +57,8 @@ typedef struct SiFiveUState {
> > >
> > > void *fdt;
> > > int fdt_size;
> > > +
> > > + bool start_in_flash;
> > > } SiFiveUState;
> > >
> > > enum {
> >
> > This patch chose a different way from the one used in patch "[v1,6/6]
> > riscv/virt: Jump to pflash if specified":
> >
> > - this patch uses reset_vec[6] while patch [6/6] defines a variable start_addr
> > - this patch adds a "start-in-flash" property to the machine, while
> > patch [6/6] tests against drive IF_PFLASH
>
> Yes, we do it differently for the sifive_u board as the sifive_u board
> doesn't use pflash so there is no way to know if the user has loaded
> anything into the SPI memory.
>
OK.
> >
> > We should be consistent and I would prefer to use the patch [6/6] way.
> > On Unleashed an SPI flash is mounted so we cannot add a PFlash to
> > sifive_u machine like what was done on virt machine, so we should test
> > IF_MTD instead. Thoughts?
>
> How would we test that?
>
> Right now I am loading the binary in SPI with the -device loader
> option. The machine can't really know what is/isn't loaded there.
>
> It's not ideal, but I don't see a nicer way.
I think we need write a SiFive SPI model to support this in a clean
way. Ideally we should simulate the hardware boot workflow as
documented in the FU540 manual chapter 6 "Boot Process".
Regards,
Bin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 6/6] riscv/virt: Jump to pflash if specified
2019-09-20 5:15 ` Bin Meng
@ 2019-09-23 9:09 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-09-23 9:09 UTC (permalink / raw)
To: Bin Meng, Alistair Francis
Cc: Alistair Francis, Palmer Dabbelt, open list:RISC-V,
qemu-devel@nongnu.org Developers
On 9/20/19 7:15 AM, Bin Meng wrote:
> On Fri, Sep 20, 2019 at 6:35 AM Alistair Francis
> <alistair.francis@wdc.com> wrote:
>>
>> If the user supplied pflash to QEMU then change the reset code to jump
>> to the pflash base address instead of the DRAM base address.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>> ---
>> hw/riscv/virt.c | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index ca002ecea7..ed25cc6761 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -441,6 +441,7 @@ static void riscv_virt_board_init(MachineState *machine)
>> MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>> char *plic_hart_config;
>> size_t plic_hart_config_len;
>> + target_ulong start_addr = memmap[VIRT_DRAM].base;
>> int i;
>> unsigned int smp_cpus = machine->smp.cpus;
>>
>> @@ -487,6 +488,13 @@ static void riscv_virt_board_init(MachineState *machine)
>> }
>> }
>>
>> + if (drive_get(IF_PFLASH, 0, 0)) {
>> + /* Pflash was supplied, let's overwrite the address we jump to after
>
> nits: wrong multi-line comment format
>
>> + * reset to the base of the flash.
>> + */
>> + start_addr = virt_memmap[VIRT_FLASH].base;
>> + }
>> +
>> /* reset vector */
>> uint32_t reset_vec[8] = {
>> 0x00000297, /* 1: auipc t0, %pcrel_hi(dtb) */
>> @@ -499,7 +507,7 @@ static void riscv_virt_board_init(MachineState *machine)
>> #endif
>> 0x00028067, /* jr t0 */
>> 0x00000000,
>> - memmap[VIRT_DRAM].base, /* start: .dword memmap[VIRT_DRAM].base */
>> + start_addr, /* start: .dword */
>> 0x00000000,
>> /* dtb: */
>> };
>> --
>
> Otherwise,
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 4/6] riscv/sifive_u: Add the start-in-flash property
2019-09-22 2:19 ` Bin Meng
@ 2019-09-23 17:51 ` Alistair Francis
2019-09-24 0:57 ` Bin Meng
0 siblings, 1 reply; 30+ messages in thread
From: Alistair Francis @ 2019-09-23 17:51 UTC (permalink / raw)
To: Bin Meng
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers, Chih-Min Chao
On Sat, Sep 21, 2019 at 7:19 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sat, Sep 21, 2019 at 6:12 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > On Fri, Sep 20, 2019 at 6:32 AM Alistair Francis
> > > <alistair.francis@wdc.com> wrote:
> > > >
> > > > Add a property that when set to true QEMU will jump from the ROM code to
> > > > the start of flash memory instead of DRAM which is the default
> > > > behaviour.
> > > >
> > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > ---
> > > > hw/riscv/sifive_u.c | 27 +++++++++++++++++++++++++++
> > > > include/hw/riscv/sifive_u.h | 2 ++
> > > > 2 files changed, 29 insertions(+)
> > > >
> > > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > > > index c3949fb316..b7cd3631cd 100644
> > > > --- a/hw/riscv/sifive_u.c
> > > > +++ b/hw/riscv/sifive_u.c
> > > > @@ -373,6 +373,10 @@ static void riscv_sifive_u_init(MachineState *machine)
> > > > /* dtb: */
> > > > };
> > > >
> > > > + if (s->start_in_flash) {
> > > > + reset_vec[6] = memmap[SIFIVE_U_FLASH0].base; /* start: .dword FLASH0_BASE */
> > > > + }
> > > > +
> > > > /* copy in the reset vector in little_endian byte order */
> > > > for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > > > reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > > > @@ -544,8 +548,31 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
> > > > memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
> > > > }
> > > >
> > > > +static bool virt_get_start_in_flash(Object *obj, Error **errp)
> > > > +{
> > > > + SiFiveUState *s = RISCV_U_MACHINE(obj);
> > > > +
> > > > + return s->start_in_flash;
> > > > +}
> > > > +
> > > > +static void virt_set_start_in_flash(Object *obj, bool value, Error **errp)
> > > > +{
> > > > + SiFiveUState *s = RISCV_U_MACHINE(obj);
> > > > +
> > > > + s->start_in_flash = value;
> > > > +}
> > > > +
> > > > static void riscv_sifive_u_machine_instance_init(Object *obj)
> > > > {
> > > > + SiFiveUState *s = RISCV_U_MACHINE(obj);
> > > > +
> > > > + s->start_in_flash = false;
> > > > + object_property_add_bool(obj, "start-in-flash", virt_get_start_in_flash,
> > > > + virt_set_start_in_flash, NULL);
> > > > + object_property_set_description(obj, "start-in-flash",
> > > > + "Set on to tell QEMU's ROM to jump to " \
> > > > + "flash. Otherwise QEMU will jump to DRAM",
> > > > + NULL);
> > > >
> > > > }
> > > >
> > > > diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> > > > index a921079fbe..2656b43c58 100644
> > > > --- a/include/hw/riscv/sifive_u.h
> > > > +++ b/include/hw/riscv/sifive_u.h
> > > > @@ -57,6 +57,8 @@ typedef struct SiFiveUState {
> > > >
> > > > void *fdt;
> > > > int fdt_size;
> > > > +
> > > > + bool start_in_flash;
> > > > } SiFiveUState;
> > > >
> > > > enum {
> > >
> > > This patch chose a different way from the one used in patch "[v1,6/6]
> > > riscv/virt: Jump to pflash if specified":
> > >
> > > - this patch uses reset_vec[6] while patch [6/6] defines a variable start_addr
> > > - this patch adds a "start-in-flash" property to the machine, while
> > > patch [6/6] tests against drive IF_PFLASH
> >
> > Yes, we do it differently for the sifive_u board as the sifive_u board
> > doesn't use pflash so there is no way to know if the user has loaded
> > anything into the SPI memory.
> >
>
> OK.
>
> > >
> > > We should be consistent and I would prefer to use the patch [6/6] way.
> > > On Unleashed an SPI flash is mounted so we cannot add a PFlash to
> > > sifive_u machine like what was done on virt machine, so we should test
> > > IF_MTD instead. Thoughts?
> >
> > How would we test that?
> >
> > Right now I am loading the binary in SPI with the -device loader
> > option. The machine can't really know what is/isn't loaded there.
> >
> > It's not ideal, but I don't see a nicer way.
>
> I think we need write a SiFive SPI model to support this in a clean
> way. Ideally we should simulate the hardware boot workflow as
> documented in the FU540 manual chapter 6 "Boot Process".
I really didn't want to do this. For me it's low priority and there
are enough other things to work on rather then adding SiFive device
models. Maybe someone who works at SiFive would be able to do this?
My hope with this series is that we could unblock firmware developers
(oreboot and coreboot) while the SPI model is written.
Alistair
>
> Regards,
> Bin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device
2019-09-22 2:15 ` Bin Meng
@ 2019-09-23 20:08 ` Alistair Francis
0 siblings, 0 replies; 30+ messages in thread
From: Alistair Francis @ 2019-09-23 20:08 UTC (permalink / raw)
To: Bin Meng
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers
On Sat, Sep 21, 2019 at 7:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sat, Sep 21, 2019 at 6:16 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > On Fri, Sep 20, 2019 at 6:36 AM Alistair Francis
> > > <alistair.francis@wdc.com> wrote:
> > > >
> > > > Add the CFI01 PFlash to the RISC-V virt board. This is the same PFlash
> > > > from the ARM Virt board and the implementation is based on the ARM Virt
> > > > board. This allows users to specify flash files from the command line.
> > > >
> > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > ---
> > > > hw/riscv/Kconfig | 1 +
> > > > hw/riscv/virt.c | 81 +++++++++++++++++++++++++++++++++++++++++
> > > > include/hw/riscv/virt.h | 3 ++
> > > > 3 files changed, 85 insertions(+)
> > > >
> > > > diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> > > > index fb19b2df3a..b12660b9f8 100644
> > > > --- a/hw/riscv/Kconfig
> > > > +++ b/hw/riscv/Kconfig
> > > > @@ -36,4 +36,5 @@ config RISCV_VIRT
> > > > select SERIAL
> > > > select VIRTIO_MMIO
> > > > select PCI_EXPRESS_GENERIC_BRIDGE
> > > > + select PFLASH_CFI01
> > > > select SIFIVE
> > > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > > > index d36f5625ec..ca002ecea7 100644
> > > > --- a/hw/riscv/virt.c
> > > > +++ b/hw/riscv/virt.c
> > > > @@ -26,6 +26,7 @@
> > > > #include "hw/boards.h"
> > > > #include "hw/loader.h"
> > > > #include "hw/sysbus.h"
> > > > +#include "hw/qdev-properties.h"
> > > > #include "hw/char/serial.h"
> > > > #include "target/riscv/cpu.h"
> > > > #include "hw/riscv/riscv_hart.h"
> > > > @@ -61,12 +62,72 @@ static const struct MemmapEntry {
> > > > [VIRT_PLIC] = { 0xc000000, 0x4000000 },
> > > > [VIRT_UART0] = { 0x10000000, 0x100 },
> > > > [VIRT_VIRTIO] = { 0x10001000, 0x1000 },
> > > > + [VIRT_FLASH] = { 0x20000000, 0x2000000 },
> > > > [VIRT_DRAM] = { 0x80000000, 0x0 },
> > > > [VIRT_PCIE_MMIO] = { 0x40000000, 0x40000000 },
> > > > [VIRT_PCIE_PIO] = { 0x03000000, 0x00010000 },
> > > > [VIRT_PCIE_ECAM] = { 0x30000000, 0x10000000 },
> > > > };
> > > >
> > > > +#define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
> > > > +
> > > > +static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
> > > > + const char *name,
> > > > + const char *alias_prop_name)
> > > > +{
> > > > + /*
> > > > + * Create a single flash device. We use the same parameters as
> > > > + * the flash devices on the ARM virt board.
> > > > + */
> > > > + DeviceState *dev = qdev_create(NULL, TYPE_PFLASH_CFI01);
> > > > +
> > > > + qdev_prop_set_uint64(dev, "sector-length", VIRT_FLASH_SECTOR_SIZE);
> > > > + qdev_prop_set_uint8(dev, "width", 4);
> > > > + qdev_prop_set_uint8(dev, "device-width", 2);
> > > > + qdev_prop_set_bit(dev, "big-endian", false);
> > > > + qdev_prop_set_uint16(dev, "id0", 0x89);
> > > > + qdev_prop_set_uint16(dev, "id1", 0x18);
> > > > + qdev_prop_set_uint16(dev, "id2", 0x00);
> > > > + qdev_prop_set_uint16(dev, "id3", 0x00);
> > > > + qdev_prop_set_string(dev, "name", name);
> > >
> > > alias_prop_name is unused? ARM virt has 2 more calls in the same function here.
> >
> > Yep, you are right. I have removed this.
>
> Any reason of removing this?
The way the virt machine was structured didn't work with the options.
I have fixed that in v2 of the series.
Alistair
>
> >
> > >
> > > > +
> > > > + return PFLASH_CFI01(dev);
> > > > +}
> > > > +
> > > > +static void virt_flash_create(RISCVVirtState *s)
> > > > +{
> > > > + s->flash[0] = virt_flash_create1(s, "virt.flash0", "pflash0");
> > > > + s->flash[1] = virt_flash_create1(s, "virt.flash1", "pflash1");
> > >
> > > I don't think we should mirror what is used on ARM virt board to
> > > create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
> > > they need distinguish secure and non-secure. For sifive_u, only one is
> > > enough.
> >
> > I went back and forward about 1 or 2. Two seems more usable as maybe
> > someone wants to include two pflash files? The Xilinx machine also has
> > two so I'm kind of used to 2, but I'm not really fussed.
> >
> > Unless anyone else wants two I will change it to 1.
>
> Regards,
> Bin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device
2019-09-20 22:12 ` Alistair Francis
2019-09-22 2:15 ` Bin Meng
@ 2019-09-23 21:46 ` Peter Maydell
2019-09-24 9:32 ` Philippe Mathieu-Daudé
2019-09-25 0:54 ` Alistair Francis
1 sibling, 2 replies; 30+ messages in thread
From: Peter Maydell @ 2019-09-23 21:46 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, Palmer Dabbelt,
qemu-devel@nongnu.org Developers, Markus Armbruster,
Alistair Francis, Bin Meng
On Fri, 20 Sep 2019 at 23:23, Alistair Francis <alistair23@gmail.com> wrote:
> On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > I don't think we should mirror what is used on ARM virt board to
> > create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
> > they need distinguish secure and non-secure. For sifive_u, only one is
> > enough.
>
> I went back and forward about 1 or 2. Two seems more usable as maybe
> someone wants to include two pflash files? The Xilinx machine also has
> two so I'm kind of used to 2, but I'm not really fussed.
One of the reasons for having 2 on the Arm board (we do this
even if we're not supporting secure vs non-secure) is that
then you can use one for a fixed read-only BIOS image (backed
by a file on the host filesystem shared between all VMs), and
one backed by a read-write per-VM file providing permanent
storage for BIOS environment variables. Notably UEFI likes to
work this way, but the idea applies in theory to other
boot loader or BIOSes I guess.
I would suggest also checking with Markus that your code
for instantiating the flash devices follows the current
recommendations so the backing storage can be configured
via -blockdev. (This is a fairly recent change from June or
so; current-in-master virt and sbsa boards provide an example
of doing the right thing, I think.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 4/6] riscv/sifive_u: Add the start-in-flash property
2019-09-23 17:51 ` Alistair Francis
@ 2019-09-24 0:57 ` Bin Meng
0 siblings, 0 replies; 30+ messages in thread
From: Bin Meng @ 2019-09-24 0:57 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers, Chih-Min Chao
On Tue, Sep 24, 2019 at 1:51 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Sat, Sep 21, 2019 at 7:19 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Sat, Sep 21, 2019 at 6:12 AM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > On Fri, Sep 20, 2019 at 6:32 AM Alistair Francis
> > > > <alistair.francis@wdc.com> wrote:
> > > > >
> > > > > Add a property that when set to true QEMU will jump from the ROM code to
> > > > > the start of flash memory instead of DRAM which is the default
> > > > > behaviour.
> > > > >
> > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > ---
> > > > > hw/riscv/sifive_u.c | 27 +++++++++++++++++++++++++++
> > > > > include/hw/riscv/sifive_u.h | 2 ++
> > > > > 2 files changed, 29 insertions(+)
> > > > >
> > > > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > > > > index c3949fb316..b7cd3631cd 100644
> > > > > --- a/hw/riscv/sifive_u.c
> > > > > +++ b/hw/riscv/sifive_u.c
> > > > > @@ -373,6 +373,10 @@ static void riscv_sifive_u_init(MachineState *machine)
> > > > > /* dtb: */
> > > > > };
> > > > >
> > > > > + if (s->start_in_flash) {
> > > > > + reset_vec[6] = memmap[SIFIVE_U_FLASH0].base; /* start: .dword FLASH0_BASE */
> > > > > + }
> > > > > +
> > > > > /* copy in the reset vector in little_endian byte order */
> > > > > for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > > > > reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > > > > @@ -544,8 +548,31 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
> > > > > memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
> > > > > }
> > > > >
> > > > > +static bool virt_get_start_in_flash(Object *obj, Error **errp)
> > > > > +{
> > > > > + SiFiveUState *s = RISCV_U_MACHINE(obj);
> > > > > +
> > > > > + return s->start_in_flash;
> > > > > +}
> > > > > +
> > > > > +static void virt_set_start_in_flash(Object *obj, bool value, Error **errp)
> > > > > +{
> > > > > + SiFiveUState *s = RISCV_U_MACHINE(obj);
> > > > > +
> > > > > + s->start_in_flash = value;
> > > > > +}
> > > > > +
> > > > > static void riscv_sifive_u_machine_instance_init(Object *obj)
> > > > > {
> > > > > + SiFiveUState *s = RISCV_U_MACHINE(obj);
> > > > > +
> > > > > + s->start_in_flash = false;
> > > > > + object_property_add_bool(obj, "start-in-flash", virt_get_start_in_flash,
> > > > > + virt_set_start_in_flash, NULL);
> > > > > + object_property_set_description(obj, "start-in-flash",
> > > > > + "Set on to tell QEMU's ROM to jump to " \
> > > > > + "flash. Otherwise QEMU will jump to DRAM",
> > > > > + NULL);
> > > > >
> > > > > }
> > > > >
> > > > > diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> > > > > index a921079fbe..2656b43c58 100644
> > > > > --- a/include/hw/riscv/sifive_u.h
> > > > > +++ b/include/hw/riscv/sifive_u.h
> > > > > @@ -57,6 +57,8 @@ typedef struct SiFiveUState {
> > > > >
> > > > > void *fdt;
> > > > > int fdt_size;
> > > > > +
> > > > > + bool start_in_flash;
> > > > > } SiFiveUState;
> > > > >
> > > > > enum {
> > > >
> > > > This patch chose a different way from the one used in patch "[v1,6/6]
> > > > riscv/virt: Jump to pflash if specified":
> > > >
> > > > - this patch uses reset_vec[6] while patch [6/6] defines a variable start_addr
> > > > - this patch adds a "start-in-flash" property to the machine, while
> > > > patch [6/6] tests against drive IF_PFLASH
> > >
> > > Yes, we do it differently for the sifive_u board as the sifive_u board
> > > doesn't use pflash so there is no way to know if the user has loaded
> > > anything into the SPI memory.
> > >
> >
> > OK.
> >
> > > >
> > > > We should be consistent and I would prefer to use the patch [6/6] way.
> > > > On Unleashed an SPI flash is mounted so we cannot add a PFlash to
> > > > sifive_u machine like what was done on virt machine, so we should test
> > > > IF_MTD instead. Thoughts?
> > >
> > > How would we test that?
> > >
> > > Right now I am loading the binary in SPI with the -device loader
> > > option. The machine can't really know what is/isn't loaded there.
> > >
> > > It's not ideal, but I don't see a nicer way.
> >
> > I think we need write a SiFive SPI model to support this in a clean
> > way. Ideally we should simulate the hardware boot workflow as
> > documented in the FU540 manual chapter 6 "Boot Process".
>
> I really didn't want to do this. For me it's low priority and there
> are enough other things to work on rather then adding SiFive device
> models. Maybe someone who works at SiFive would be able to do this?
>
> My hope with this series is that we could unblock firmware developers
> (oreboot and coreboot) while the SPI model is written.
>
OK, so let's set the expectation that this is a temporary flash
solution and will be changed in the future when the SiFive SPI model
is ready.
Regards,
Bin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device
2019-09-23 21:46 ` Peter Maydell
@ 2019-09-24 9:32 ` Philippe Mathieu-Daudé
2019-09-24 17:12 ` Laszlo Ersek
2019-09-25 0:55 ` Alistair Francis
2019-09-25 0:54 ` Alistair Francis
1 sibling, 2 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-09-24 9:32 UTC (permalink / raw)
To: Peter Maydell, Alistair Francis, Laszlo Ersek
Cc: open list:RISC-V, Palmer Dabbelt,
qemu-devel@nongnu.org Developers, Markus Armbruster,
Alistair Francis, Bin Meng
On 9/23/19 11:46 PM, Peter Maydell wrote:
> On Fri, 20 Sep 2019 at 23:23, Alistair Francis <alistair23@gmail.com> wrote:
>> On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>> I don't think we should mirror what is used on ARM virt board to
>>> create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
>>> they need distinguish secure and non-secure. For sifive_u, only one is
>>> enough.
>>
>> I went back and forward about 1 or 2. Two seems more usable as maybe
>> someone wants to include two pflash files? The Xilinx machine also has
>> two so I'm kind of used to 2, but I'm not really fussed.
The Xilinx machine has 2 because it matches the hardware.
> One of the reasons for having 2 on the Arm board (we do this
> even if we're not supporting secure vs non-secure) is that
> then you can use one for a fixed read-only BIOS image (backed
> by a file on the host filesystem shared between all VMs), and
> one backed by a read-write per-VM file providing permanent
> storage for BIOS environment variables. Notably UEFI likes to
> work this way, but the idea applies in theory to other
> boot loader or BIOSes I guess.
IIRC Laszlo's explanation, the only reason it is that way is because the
pflash_cfi01 model still doesn't implement sector locking. At the OVMF
emerged from EDK2, to have a safe ROM vs DATA storage it was decided to
use 2 different flashes.
When I understood when this config layout started, I suggested Laszlo to
use a real ROM to store the OVMF CODE since it is pointless to do
firmware upgrade in virtualized environment, but he said it was too late
to change the design.
If you don't plan to run UEFI "Capsule Update" on your Virt board, I
suggest using memory_region_init_rom() with your firmware CODE, and a
parallel/SPI flash for the data VARStore.
> I would suggest also checking with Markus that your code
> for instantiating the flash devices follows the current
> recommendations so the backing storage can be configured
> via -blockdev. (This is a fairly recent change from June or
> so; current-in-master virt and sbsa boards provide an example
> of doing the right thing, I think.)
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device
2019-09-24 9:32 ` Philippe Mathieu-Daudé
@ 2019-09-24 17:12 ` Laszlo Ersek
2019-09-25 0:55 ` Alistair Francis
1 sibling, 0 replies; 30+ messages in thread
From: Laszlo Ersek @ 2019-09-24 17:12 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Peter Maydell, Alistair Francis
Cc: open list:RISC-V, Palmer Dabbelt,
qemu-devel@nongnu.org Developers, Markus Armbruster,
Alistair Francis, Bin Meng
On 09/24/19 11:32, Philippe Mathieu-Daudé wrote:
> On 9/23/19 11:46 PM, Peter Maydell wrote:
>> On Fri, 20 Sep 2019 at 23:23, Alistair Francis <alistair23@gmail.com> wrote:
>>> On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>>> I don't think we should mirror what is used on ARM virt board to
>>>> create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
>>>> they need distinguish secure and non-secure. For sifive_u, only one is
>>>> enough.
>>>
>>> I went back and forward about 1 or 2. Two seems more usable as maybe
>>> someone wants to include two pflash files? The Xilinx machine also has
>>> two so I'm kind of used to 2, but I'm not really fussed.
>
> The Xilinx machine has 2 because it matches the hardware.
>
>> One of the reasons for having 2 on the Arm board (we do this
>> even if we're not supporting secure vs non-secure) is that
>> then you can use one for a fixed read-only BIOS image (backed
>> by a file on the host filesystem shared between all VMs), and
>> one backed by a read-write per-VM file providing permanent
>> storage for BIOS environment variables. Notably UEFI likes to
>> work this way, but the idea applies in theory to other
>> boot loader or BIOSes I guess.
>
> IIRC Laszlo's explanation, the only reason it is that way is because the
> pflash_cfi01 model still doesn't implement sector locking. At the OVMF
> emerged from EDK2, to have a safe ROM vs DATA storage it was decided to
> use 2 different flashes.
> When I understood when this config layout started, I suggested Laszlo to
> use a real ROM to store the OVMF CODE since it is pointless to do
> firmware upgrade in virtualized environment, but he said it was too late
> to change the design.
Right, at that point I couldn't see how "-bios" *plus* "-pflash" could
have worked. In chronological order (for OVMF anyway), there was -bios,
then -pflash (R/W), then (with some QEMU changes) two -pflash flags (R/O
+ R/W, which OVMF wouldn't tell apart from the single R/W -pflash).
Thanks
Laszlo
>
> If you don't plan to run UEFI "Capsule Update" on your Virt board, I
> suggest using memory_region_init_rom() with your firmware CODE, and a
> parallel/SPI flash for the data VARStore.
>
>> I would suggest also checking with Markus that your code
>> for instantiating the flash devices follows the current
>> recommendations so the backing storage can be configured
>> via -blockdev. (This is a fairly recent change from June or
>> so; current-in-master virt and sbsa boards provide an example
>> of doing the right thing, I think.)
>>
>> thanks
>> -- PMM
>>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device
2019-09-23 21:46 ` Peter Maydell
2019-09-24 9:32 ` Philippe Mathieu-Daudé
@ 2019-09-25 0:54 ` Alistair Francis
2019-09-25 9:00 ` Markus Armbruster
1 sibling, 1 reply; 30+ messages in thread
From: Alistair Francis @ 2019-09-25 0:54 UTC (permalink / raw)
To: Peter Maydell
Cc: open list:RISC-V, Palmer Dabbelt,
qemu-devel@nongnu.org Developers, Markus Armbruster,
Alistair Francis, Bin Meng
On Mon, Sep 23, 2019 at 2:46 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 20 Sep 2019 at 23:23, Alistair Francis <alistair23@gmail.com> wrote:
> > On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > I don't think we should mirror what is used on ARM virt board to
> > > create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
> > > they need distinguish secure and non-secure. For sifive_u, only one is
> > > enough.
> >
> > I went back and forward about 1 or 2. Two seems more usable as maybe
> > someone wants to include two pflash files? The Xilinx machine also has
> > two so I'm kind of used to 2, but I'm not really fussed.
>
> One of the reasons for having 2 on the Arm board (we do this
> even if we're not supporting secure vs non-secure) is that
> then you can use one for a fixed read-only BIOS image (backed
> by a file on the host filesystem shared between all VMs), and
> one backed by a read-write per-VM file providing permanent
> storage for BIOS environment variables. Notably UEFI likes to
> work this way, but the idea applies in theory to other
> boot loader or BIOSes I guess.
This seems like a good reason to have two and there isn't really a
disadvantage so I have kept it with two.
>
> I would suggest also checking with Markus that your code
> for instantiating the flash devices follows the current
> recommendations so the backing storage can be configured
> via -blockdev. (This is a fairly recent change from June or
> so; current-in-master virt and sbsa boards provide an example
> of doing the right thing, I think.)
I have updated the code to more closely match the ARM virt machine, so
I think I'm doing it correctly.
Alistair
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device
2019-09-24 9:32 ` Philippe Mathieu-Daudé
2019-09-24 17:12 ` Laszlo Ersek
@ 2019-09-25 0:55 ` Alistair Francis
2019-09-25 11:15 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 30+ messages in thread
From: Alistair Francis @ 2019-09-25 0:55 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, open list:RISC-V, Palmer Dabbelt,
Markus Armbruster, qemu-devel@nongnu.org Developers,
Alistair Francis, Bin Meng, Laszlo Ersek
On Tue, Sep 24, 2019 at 2:32 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> On 9/23/19 11:46 PM, Peter Maydell wrote:
> > On Fri, 20 Sep 2019 at 23:23, Alistair Francis <alistair23@gmail.com> wrote:
> >> On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>> I don't think we should mirror what is used on ARM virt board to
> >>> create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
> >>> they need distinguish secure and non-secure. For sifive_u, only one is
> >>> enough.
> >>
> >> I went back and forward about 1 or 2. Two seems more usable as maybe
> >> someone wants to include two pflash files? The Xilinx machine also has
> >> two so I'm kind of used to 2, but I'm not really fussed.
>
> The Xilinx machine has 2 because it matches the hardware.
>
> > One of the reasons for having 2 on the Arm board (we do this
> > even if we're not supporting secure vs non-secure) is that
> > then you can use one for a fixed read-only BIOS image (backed
> > by a file on the host filesystem shared between all VMs), and
> > one backed by a read-write per-VM file providing permanent
> > storage for BIOS environment variables. Notably UEFI likes to
> > work this way, but the idea applies in theory to other
> > boot loader or BIOSes I guess.
>
> IIRC Laszlo's explanation, the only reason it is that way is because the
> pflash_cfi01 model still doesn't implement sector locking. At the OVMF
> emerged from EDK2, to have a safe ROM vs DATA storage it was decided to
> use 2 different flashes.
> When I understood when this config layout started, I suggested Laszlo to
> use a real ROM to store the OVMF CODE since it is pointless to do
> firmware upgrade in virtualized environment, but he said it was too late
> to change the design.
>
> If you don't plan to run UEFI "Capsule Update" on your Virt board, I
> suggest using memory_region_init_rom() with your firmware CODE, and a
> parallel/SPI flash for the data VARStore.
We might run that one day, who knows :)
Alistair
>
> > I would suggest also checking with Markus that your code
> > for instantiating the flash devices follows the current
> > recommendations so the backing storage can be configured
> > via -blockdev. (This is a fairly recent change from June or
> > so; current-in-master virt and sbsa boards provide an example
> > of doing the right thing, I think.)
> >
> > thanks
> > -- PMM
> >
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device
2019-09-25 0:54 ` Alistair Francis
@ 2019-09-25 9:00 ` Markus Armbruster
2019-09-27 21:49 ` Alistair Francis
0 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2019-09-25 9:00 UTC (permalink / raw)
To: Alistair Francis
Cc: Peter Maydell, open list:RISC-V, Palmer Dabbelt,
qemu-devel@nongnu.org Developers, Alistair Francis, Bin Meng
Alistair Francis <alistair23@gmail.com> writes:
> On Mon, Sep 23, 2019 at 2:46 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Fri, 20 Sep 2019 at 23:23, Alistair Francis <alistair23@gmail.com> wrote:
>> > On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>> > > I don't think we should mirror what is used on ARM virt board to
>> > > create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
>> > > they need distinguish secure and non-secure. For sifive_u, only one is
>> > > enough.
>> >
>> > I went back and forward about 1 or 2. Two seems more usable as maybe
>> > someone wants to include two pflash files? The Xilinx machine also has
>> > two so I'm kind of used to 2, but I'm not really fussed.
>>
>> One of the reasons for having 2 on the Arm board (we do this
>> even if we're not supporting secure vs non-secure) is that
>> then you can use one for a fixed read-only BIOS image (backed
>> by a file on the host filesystem shared between all VMs), and
>> one backed by a read-write per-VM file providing permanent
>> storage for BIOS environment variables. Notably UEFI likes to
>> work this way, but the idea applies in theory to other
>> boot loader or BIOSes I guess.
>
> This seems like a good reason to have two and there isn't really a
> disadvantage so I have kept it with two.
Good.
Implementing sector locking would be even better. I'm not asking you to
do that work.
>> I would suggest also checking with Markus that your code
>> for instantiating the flash devices follows the current
>> recommendations so the backing storage can be configured
>> via -blockdev. (This is a fairly recent change from June or
>> so; current-in-master virt and sbsa boards provide an example
>> of doing the right thing, I think.)
>
> I have updated the code to more closely match the ARM virt machine, so
> I think I'm doing it correctly.
You might want to consider omitting legacy configuration options -drive
if=pflash and -bios for a simpler interface.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device
2019-09-25 0:55 ` Alistair Francis
@ 2019-09-25 11:15 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-09-25 11:15 UTC (permalink / raw)
To: Alistair Francis
Cc: Peter Maydell, open list:RISC-V, Palmer Dabbelt,
Markus Armbruster, qemu-devel@nongnu.org Developers,
Alistair Francis, Bin Meng, Laszlo Ersek
On 9/25/19 2:55 AM, Alistair Francis wrote:
> On Tue, Sep 24, 2019 at 2:32 AM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
>>
>> On 9/23/19 11:46 PM, Peter Maydell wrote:
>>> On Fri, 20 Sep 2019 at 23:23, Alistair Francis <alistair23@gmail.com> wrote:
>>>> On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>> I don't think we should mirror what is used on ARM virt board to
>>>>> create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
>>>>> they need distinguish secure and non-secure. For sifive_u, only one is
>>>>> enough.
>>>>
>>>> I went back and forward about 1 or 2. Two seems more usable as maybe
>>>> someone wants to include two pflash files? The Xilinx machine also has
>>>> two so I'm kind of used to 2, but I'm not really fussed.
>>
>> The Xilinx machine has 2 because it matches the hardware.
>>
>>> One of the reasons for having 2 on the Arm board (we do this
>>> even if we're not supporting secure vs non-secure) is that
>>> then you can use one for a fixed read-only BIOS image (backed
>>> by a file on the host filesystem shared between all VMs), and
>>> one backed by a read-write per-VM file providing permanent
>>> storage for BIOS environment variables. Notably UEFI likes to
>>> work this way, but the idea applies in theory to other
>>> boot loader or BIOSes I guess.
>>
>> IIRC Laszlo's explanation, the only reason it is that way is because the
>> pflash_cfi01 model still doesn't implement sector locking. At the OVMF
>> emerged from EDK2, to have a safe ROM vs DATA storage it was decided to
>> use 2 different flashes.
>> When I understood when this config layout started, I suggested Laszlo to
>> use a real ROM to store the OVMF CODE since it is pointless to do
>> firmware upgrade in virtualized environment, but he said it was too late
>> to change the design.
>>
>> If you don't plan to run UEFI "Capsule Update" on your Virt board, I
>> suggest using memory_region_init_rom() with your firmware CODE, and a
>> parallel/SPI flash for the data VARStore.
>
> We might run that one day, who knows :)
You certainly want to run EDK2, I'm following RISCV progress on the list.
What doesn't make sense on a virtualized platform is the "Capsule
Update" feature IMO. Where it makes sense is on the SiFive E/U boards
models.
>>> I would suggest also checking with Markus that your code
>>> for instantiating the flash devices follows the current
>>> recommendations so the backing storage can be configured
>>> via -blockdev. (This is a fairly recent change from June or
>>> so; current-in-master virt and sbsa boards provide an example
>>> of doing the right thing, I think.)
>>>
>>> thanks
>>> -- PMM
>>>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device
2019-09-25 9:00 ` Markus Armbruster
@ 2019-09-27 21:49 ` Alistair Francis
0 siblings, 0 replies; 30+ messages in thread
From: Alistair Francis @ 2019-09-27 21:49 UTC (permalink / raw)
To: Markus Armbruster
Cc: Peter Maydell, open list:RISC-V, Palmer Dabbelt,
qemu-devel@nongnu.org Developers, Alistair Francis, Bin Meng
On Wed, Sep 25, 2019 at 2:00 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Alistair Francis <alistair23@gmail.com> writes:
>
> > On Mon, Sep 23, 2019 at 2:46 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >>
> >> On Fri, 20 Sep 2019 at 23:23, Alistair Francis <alistair23@gmail.com> wrote:
> >> > On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >> > > I don't think we should mirror what is used on ARM virt board to
> >> > > create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
> >> > > they need distinguish secure and non-secure. For sifive_u, only one is
> >> > > enough.
> >> >
> >> > I went back and forward about 1 or 2. Two seems more usable as maybe
> >> > someone wants to include two pflash files? The Xilinx machine also has
> >> > two so I'm kind of used to 2, but I'm not really fussed.
> >>
> >> One of the reasons for having 2 on the Arm board (we do this
> >> even if we're not supporting secure vs non-secure) is that
> >> then you can use one for a fixed read-only BIOS image (backed
> >> by a file on the host filesystem shared between all VMs), and
> >> one backed by a read-write per-VM file providing permanent
> >> storage for BIOS environment variables. Notably UEFI likes to
> >> work this way, but the idea applies in theory to other
> >> boot loader or BIOSes I guess.
> >
> > This seems like a good reason to have two and there isn't really a
> > disadvantage so I have kept it with two.
>
> Good.
>
> Implementing sector locking would be even better. I'm not asking you to
> do that work.
>
> >> I would suggest also checking with Markus that your code
> >> for instantiating the flash devices follows the current
> >> recommendations so the backing storage can be configured
> >> via -blockdev. (This is a fairly recent change from June or
> >> so; current-in-master virt and sbsa boards provide an example
> >> of doing the right thing, I think.)
> >
> > I have updated the code to more closely match the ARM virt machine, so
> > I think I'm doing it correctly.
>
> You might want to consider omitting legacy configuration options -drive
> if=pflash and -bios for a simpler interface.
We just moved to -bios and it's been really helpful.
Doesn't -pflash use -drive if=pflash? How else should these be attached?
Alistair
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2019-09-27 21:56 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-19 22:24 [PATCH v1 0/6] RISC-V: Add more machine memory Alistair Francis
2019-09-19 22:24 ` [PATCH v1 1/6] riscv/sifive_u: Add L2-LIM cache memory Alistair Francis
2019-09-20 5:15 ` Bin Meng
2019-09-19 22:24 ` [PATCH v1 2/6] riscv/sifive_u: Add QSPI memory region Alistair Francis
2019-09-20 5:15 ` Bin Meng
2019-09-19 22:25 ` [PATCH v1 3/6] riscv/sifive_u: Manually define the machine Alistair Francis
2019-09-20 5:15 ` Bin Meng
2019-09-19 22:25 ` [PATCH v1 4/6] riscv/sifive_u: Add the start-in-flash property Alistair Francis
2019-09-20 5:15 ` Bin Meng
2019-09-20 22:07 ` Alistair Francis
2019-09-22 2:19 ` Bin Meng
2019-09-23 17:51 ` Alistair Francis
2019-09-24 0:57 ` Bin Meng
2019-09-19 22:25 ` [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device Alistair Francis
2019-09-20 5:15 ` Bin Meng
2019-09-20 22:12 ` Alistair Francis
2019-09-22 2:15 ` Bin Meng
2019-09-23 20:08 ` Alistair Francis
2019-09-23 21:46 ` Peter Maydell
2019-09-24 9:32 ` Philippe Mathieu-Daudé
2019-09-24 17:12 ` Laszlo Ersek
2019-09-25 0:55 ` Alistair Francis
2019-09-25 11:15 ` Philippe Mathieu-Daudé
2019-09-25 0:54 ` Alistair Francis
2019-09-25 9:00 ` Markus Armbruster
2019-09-27 21:49 ` Alistair Francis
2019-09-19 22:25 ` [PATCH v1 6/6] riscv/virt: Jump to pflash if specified Alistair Francis
2019-09-20 5:15 ` Bin Meng
2019-09-23 9:09 ` Philippe Mathieu-Daudé
2019-09-20 22:40 ` [PATCH v1 0/6] RISC-V: Add more machine memory Palmer Dabbelt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).