hw/riscv/sifive_u.c | 21 ++++++++++++++++++++- include/hw/riscv/sifive_u.h | 1 + 2 files changed, 21 insertions(+), 1 deletion(-)
At present the board serial number is hard-coded to 1, and passed
to OTP model during initialization. Firmware (FSBL, U-Boot) uses
the serial number to generate a unique MAC address for the on-chip
ethernet controller. When multiple QEMU 'sifive_u' instances are
created and connected to the same subnet, they all have the same
MAC address hence it creates a unusable network.
A new "serial" property is introduced to specify the board serial
number. When not given, the default serial number 1 is used.
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---
Changes in v2:
- Move setting OTP serial number property from riscv_sifive_u_soc_init()
to riscv_sifive_u_soc_realize(), to fix the 'check-qtest-riscv' error.
I am not really sure why doing so could fix the 'make check' error.
The v1 patch worked fine and nothing seems wrong.
hw/riscv/sifive_u.c | 21 ++++++++++++++++++++-
include/hw/riscv/sifive_u.h | 1 +
2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 0e12b3c..ca561d3 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -34,6 +34,7 @@
#include "qemu/log.h"
#include "qemu/error-report.h"
#include "qapi/error.h"
+#include "qapi/visitor.h"
#include "hw/boards.h"
#include "hw/loader.h"
#include "hw/sysbus.h"
@@ -434,7 +435,6 @@ static void riscv_sifive_u_soc_init(Object *obj)
TYPE_SIFIVE_U_PRCI);
sysbus_init_child_obj(obj, "otp", &s->otp, sizeof(s->otp),
TYPE_SIFIVE_U_OTP);
- qdev_prop_set_uint32(DEVICE(&s->otp), "serial", OTP_SERIAL);
sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem),
TYPE_CADENCE_GEM);
}
@@ -453,6 +453,18 @@ static void sifive_u_set_start_in_flash(Object *obj, bool value, Error **errp)
s->start_in_flash = value;
}
+static void sifive_u_get_serial(Object *obj, Visitor *v, const char *name,
+ void *opaque, Error **errp)
+{
+ visit_type_uint32(v, name, (uint32_t *)opaque, errp);
+}
+
+static void sifive_u_set_serial(Object *obj, Visitor *v, const char *name,
+ void *opaque, Error **errp)
+{
+ visit_type_uint32(v, name, (uint32_t *)opaque, errp);
+}
+
static void riscv_sifive_u_machine_instance_init(Object *obj)
{
SiFiveUState *s = RISCV_U_MACHINE(obj);
@@ -464,11 +476,17 @@ static void riscv_sifive_u_machine_instance_init(Object *obj)
"Set on to tell QEMU's ROM to jump to " \
"flash. Otherwise QEMU will jump to DRAM",
NULL);
+
+ s->serial = OTP_SERIAL;
+ object_property_add(obj, "serial", "uint32", sifive_u_get_serial,
+ sifive_u_set_serial, NULL, &s->serial, NULL);
+ object_property_set_description(obj, "serial", "Board serial number", NULL);
}
static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
{
MachineState *ms = MACHINE(qdev_get_machine());
+ SiFiveUState *us = RISCV_U_MACHINE(ms);
SiFiveUSoCState *s = RISCV_U_SOC(dev);
const struct MemmapEntry *memmap = sifive_u_memmap;
MemoryRegion *system_memory = get_system_memory();
@@ -554,6 +572,7 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
object_property_set_bool(OBJECT(&s->prci), true, "realized", &err);
sysbus_mmio_map(SYS_BUS_DEVICE(&s->prci), 0, memmap[SIFIVE_U_PRCI].base);
+ qdev_prop_set_uint32(DEVICE(&s->otp), "serial", us->serial);
object_property_set_bool(OBJECT(&s->otp), true, "realized", &err);
sysbus_mmio_map(SYS_BUS_DEVICE(&s->otp), 0, memmap[SIFIVE_U_OTP].base);
diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index 82667b5..7cf742e 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -59,6 +59,7 @@ typedef struct SiFiveUState {
int fdt_size;
bool start_in_flash;
+ uint32_t serial;
} SiFiveUState;
enum {
--
2.7.4
On Sun, Feb 16, 2020 at 9:55 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > At present the board serial number is hard-coded to 1, and passed > to OTP model during initialization. Firmware (FSBL, U-Boot) uses > the serial number to generate a unique MAC address for the on-chip > ethernet controller. When multiple QEMU 'sifive_u' instances are > created and connected to the same subnet, they all have the same > MAC address hence it creates a unusable network. > > A new "serial" property is introduced to specify the board serial > number. When not given, the default serial number 1 is used. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > --- > > Changes in v2: > - Move setting OTP serial number property from riscv_sifive_u_soc_init() > to riscv_sifive_u_soc_realize(), to fix the 'check-qtest-riscv' error. > I am not really sure why doing so could fix the 'make check' error. > The v1 patch worked fine and nothing seems wrong. > > hw/riscv/sifive_u.c | 21 ++++++++++++++++++++- > include/hw/riscv/sifive_u.h | 1 + > 2 files changed, 21 insertions(+), 1 deletion(-) > Ping? Regards, Bin
On Sun, 16 Feb 2020 05:55:17 PST (-0800), bmeng.cn@gmail.com wrote: > At present the board serial number is hard-coded to 1, and passed > to OTP model during initialization. Firmware (FSBL, U-Boot) uses > the serial number to generate a unique MAC address for the on-chip > ethernet controller. When multiple QEMU 'sifive_u' instances are > created and connected to the same subnet, they all have the same > MAC address hence it creates a unusable network. > > A new "serial" property is introduced to specify the board serial > number. When not given, the default serial number 1 is used. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > --- > > Changes in v2: > - Move setting OTP serial number property from riscv_sifive_u_soc_init() > to riscv_sifive_u_soc_realize(), to fix the 'check-qtest-riscv' error. > I am not really sure why doing so could fix the 'make check' error. > The v1 patch worked fine and nothing seems wrong. > > hw/riscv/sifive_u.c | 21 ++++++++++++++++++++- > include/hw/riscv/sifive_u.h | 1 + > 2 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c > index 0e12b3c..ca561d3 100644 > --- a/hw/riscv/sifive_u.c > +++ b/hw/riscv/sifive_u.c > @@ -34,6 +34,7 @@ > #include "qemu/log.h" > #include "qemu/error-report.h" > #include "qapi/error.h" > +#include "qapi/visitor.h" > #include "hw/boards.h" > #include "hw/loader.h" > #include "hw/sysbus.h" > @@ -434,7 +435,6 @@ static void riscv_sifive_u_soc_init(Object *obj) > TYPE_SIFIVE_U_PRCI); > sysbus_init_child_obj(obj, "otp", &s->otp, sizeof(s->otp), > TYPE_SIFIVE_U_OTP); > - qdev_prop_set_uint32(DEVICE(&s->otp), "serial", OTP_SERIAL); > sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem), > TYPE_CADENCE_GEM); > } > @@ -453,6 +453,18 @@ static void sifive_u_set_start_in_flash(Object *obj, bool value, Error **errp) > s->start_in_flash = value; > } > > +static void sifive_u_get_serial(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + visit_type_uint32(v, name, (uint32_t *)opaque, errp); > +} > + > +static void sifive_u_set_serial(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + visit_type_uint32(v, name, (uint32_t *)opaque, errp); > +} > + > static void riscv_sifive_u_machine_instance_init(Object *obj) > { > SiFiveUState *s = RISCV_U_MACHINE(obj); > @@ -464,11 +476,17 @@ static void riscv_sifive_u_machine_instance_init(Object *obj) > "Set on to tell QEMU's ROM to jump to " \ > "flash. Otherwise QEMU will jump to DRAM", > NULL); > + > + s->serial = OTP_SERIAL; > + object_property_add(obj, "serial", "uint32", sifive_u_get_serial, > + sifive_u_set_serial, NULL, &s->serial, NULL); > + object_property_set_description(obj, "serial", "Board serial number", NULL); > } > > static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) > { > MachineState *ms = MACHINE(qdev_get_machine()); > + SiFiveUState *us = RISCV_U_MACHINE(ms); > SiFiveUSoCState *s = RISCV_U_SOC(dev); > const struct MemmapEntry *memmap = sifive_u_memmap; > MemoryRegion *system_memory = get_system_memory(); > @@ -554,6 +572,7 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) > object_property_set_bool(OBJECT(&s->prci), true, "realized", &err); > sysbus_mmio_map(SYS_BUS_DEVICE(&s->prci), 0, memmap[SIFIVE_U_PRCI].base); > > + qdev_prop_set_uint32(DEVICE(&s->otp), "serial", us->serial); > object_property_set_bool(OBJECT(&s->otp), true, "realized", &err); > sysbus_mmio_map(SYS_BUS_DEVICE(&s->otp), 0, memmap[SIFIVE_U_OTP].base); > > diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h > index 82667b5..7cf742e 100644 > --- a/include/hw/riscv/sifive_u.h > +++ b/include/hw/riscv/sifive_u.h > @@ -59,6 +59,7 @@ typedef struct SiFiveUState { > int fdt_size; > > bool start_in_flash; > + uint32_t serial; > } SiFiveUState; > > enum { Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com> Thanks. This is in the queue for the soft freeze.
On Wed, Mar 4, 2020 at 3:00 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Sun, 16 Feb 2020 05:55:17 PST (-0800), bmeng.cn@gmail.com wrote: > > At present the board serial number is hard-coded to 1, and passed > > to OTP model during initialization. Firmware (FSBL, U-Boot) uses > > the serial number to generate a unique MAC address for the on-chip > > ethernet controller. When multiple QEMU 'sifive_u' instances are > > created and connected to the same subnet, they all have the same > > MAC address hence it creates a unusable network. > > > > A new "serial" property is introduced to specify the board serial > > number. When not given, the default serial number 1 is used. > > > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > > > --- > > > > Changes in v2: > > - Move setting OTP serial number property from riscv_sifive_u_soc_init() > > to riscv_sifive_u_soc_realize(), to fix the 'check-qtest-riscv' error. > > I am not really sure why doing so could fix the 'make check' error. > > The v1 patch worked fine and nothing seems wrong. > > > > hw/riscv/sifive_u.c | 21 ++++++++++++++++++++- > > include/hw/riscv/sifive_u.h | 1 + > > 2 files changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c > > index 0e12b3c..ca561d3 100644 > > --- a/hw/riscv/sifive_u.c > > +++ b/hw/riscv/sifive_u.c > > @@ -34,6 +34,7 @@ > > #include "qemu/log.h" > > #include "qemu/error-report.h" > > #include "qapi/error.h" > > +#include "qapi/visitor.h" > > #include "hw/boards.h" > > #include "hw/loader.h" > > #include "hw/sysbus.h" > > @@ -434,7 +435,6 @@ static void riscv_sifive_u_soc_init(Object *obj) > > TYPE_SIFIVE_U_PRCI); > > sysbus_init_child_obj(obj, "otp", &s->otp, sizeof(s->otp), > > TYPE_SIFIVE_U_OTP); > > - qdev_prop_set_uint32(DEVICE(&s->otp), "serial", OTP_SERIAL); > > sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem), > > TYPE_CADENCE_GEM); > > } > > @@ -453,6 +453,18 @@ static void sifive_u_set_start_in_flash(Object *obj, bool value, Error **errp) > > s->start_in_flash = value; > > } > > > > +static void sifive_u_get_serial(Object *obj, Visitor *v, const char *name, > > + void *opaque, Error **errp) > > +{ > > + visit_type_uint32(v, name, (uint32_t *)opaque, errp); > > +} > > + > > +static void sifive_u_set_serial(Object *obj, Visitor *v, const char *name, > > + void *opaque, Error **errp) > > +{ > > + visit_type_uint32(v, name, (uint32_t *)opaque, errp); > > +} > > + > > static void riscv_sifive_u_machine_instance_init(Object *obj) > > { > > SiFiveUState *s = RISCV_U_MACHINE(obj); > > @@ -464,11 +476,17 @@ static void riscv_sifive_u_machine_instance_init(Object *obj) > > "Set on to tell QEMU's ROM to jump to " \ > > "flash. Otherwise QEMU will jump to DRAM", > > NULL); > > + > > + s->serial = OTP_SERIAL; > > + object_property_add(obj, "serial", "uint32", sifive_u_get_serial, > > + sifive_u_set_serial, NULL, &s->serial, NULL); > > + object_property_set_description(obj, "serial", "Board serial number", NULL); > > } > > > > static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) > > { > > MachineState *ms = MACHINE(qdev_get_machine()); > > + SiFiveUState *us = RISCV_U_MACHINE(ms); > > SiFiveUSoCState *s = RISCV_U_SOC(dev); > > const struct MemmapEntry *memmap = sifive_u_memmap; > > MemoryRegion *system_memory = get_system_memory(); > > @@ -554,6 +572,7 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) > > object_property_set_bool(OBJECT(&s->prci), true, "realized", &err); > > sysbus_mmio_map(SYS_BUS_DEVICE(&s->prci), 0, memmap[SIFIVE_U_PRCI].base); > > > > + qdev_prop_set_uint32(DEVICE(&s->otp), "serial", us->serial); > > object_property_set_bool(OBJECT(&s->otp), true, "realized", &err); > > sysbus_mmio_map(SYS_BUS_DEVICE(&s->otp), 0, memmap[SIFIVE_U_OTP].base); > > > > diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h > > index 82667b5..7cf742e 100644 > > --- a/include/hw/riscv/sifive_u.h > > +++ b/include/hw/riscv/sifive_u.h > > @@ -59,6 +59,7 @@ typedef struct SiFiveUState { > > int fdt_size; > > > > bool start_in_flash; > > + uint32_t serial; > > } SiFiveUState; > > > > enum { > > Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com> > > Thanks. This is in the queue for the soft freeze. This patch isn't correct. The SoC gets the sifive_u machine state which is a bad idea. If the SoC is ever user on a different machine this will crash. I have sent a patch series that adds a machine and SoC property instead. Alistair >
On Sun, Feb 16, 2020 at 5:56 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > At present the board serial number is hard-coded to 1, and passed > to OTP model during initialization. Firmware (FSBL, U-Boot) uses > the serial number to generate a unique MAC address for the on-chip > ethernet controller. When multiple QEMU 'sifive_u' instances are > created and connected to the same subnet, they all have the same > MAC address hence it creates a unusable network. > > A new "serial" property is introduced to specify the board serial > number. When not given, the default serial number 1 is used. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > --- > > Changes in v2: > - Move setting OTP serial number property from riscv_sifive_u_soc_init() > to riscv_sifive_u_soc_realize(), to fix the 'check-qtest-riscv' error. > I am not really sure why doing so could fix the 'make check' error. > The v1 patch worked fine and nothing seems wrong. > > hw/riscv/sifive_u.c | 21 ++++++++++++++++++++- > include/hw/riscv/sifive_u.h | 1 + > 2 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c > index 0e12b3c..ca561d3 100644 > --- a/hw/riscv/sifive_u.c > +++ b/hw/riscv/sifive_u.c > @@ -34,6 +34,7 @@ > #include "qemu/log.h" > #include "qemu/error-report.h" > #include "qapi/error.h" > +#include "qapi/visitor.h" > #include "hw/boards.h" > #include "hw/loader.h" > #include "hw/sysbus.h" > @@ -434,7 +435,6 @@ static void riscv_sifive_u_soc_init(Object *obj) > TYPE_SIFIVE_U_PRCI); > sysbus_init_child_obj(obj, "otp", &s->otp, sizeof(s->otp), > TYPE_SIFIVE_U_OTP); > - qdev_prop_set_uint32(DEVICE(&s->otp), "serial", OTP_SERIAL); > sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem), > TYPE_CADENCE_GEM); > } > @@ -453,6 +453,18 @@ static void sifive_u_set_start_in_flash(Object *obj, bool value, Error **errp) > s->start_in_flash = value; > } > > +static void sifive_u_get_serial(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + visit_type_uint32(v, name, (uint32_t *)opaque, errp); > +} > + > +static void sifive_u_set_serial(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + visit_type_uint32(v, name, (uint32_t *)opaque, errp); This is a little confusing. Maybe it's worth adding a comment that opaque is s->serial? Either that or change opaque to be SiFiveUState *s and then access serial via the struct. > +} > + > static void riscv_sifive_u_machine_instance_init(Object *obj) > { > SiFiveUState *s = RISCV_U_MACHINE(obj); > @@ -464,11 +476,17 @@ static void riscv_sifive_u_machine_instance_init(Object *obj) > "Set on to tell QEMU's ROM to jump to " \ > "flash. Otherwise QEMU will jump to DRAM", > NULL); > + > + s->serial = OTP_SERIAL; > + object_property_add(obj, "serial", "uint32", sifive_u_get_serial, > + sifive_u_set_serial, NULL, &s->serial, NULL); > + object_property_set_description(obj, "serial", "Board serial number", NULL); > } > > static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) > { > MachineState *ms = MACHINE(qdev_get_machine()); > + SiFiveUState *us = RISCV_U_MACHINE(ms); I don't think the Soc should access the machine like this. What if we use this Soc on a different machine? There should be a SoC "serial" property that is set before realise as well. Alistair > SiFiveUSoCState *s = RISCV_U_SOC(dev); > const struct MemmapEntry *memmap = sifive_u_memmap; > MemoryRegion *system_memory = get_system_memory(); > @@ -554,6 +572,7 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) > object_property_set_bool(OBJECT(&s->prci), true, "realized", &err); > sysbus_mmio_map(SYS_BUS_DEVICE(&s->prci), 0, memmap[SIFIVE_U_PRCI].base); > > + qdev_prop_set_uint32(DEVICE(&s->otp), "serial", us->serial); > object_property_set_bool(OBJECT(&s->otp), true, "realized", &err); > sysbus_mmio_map(SYS_BUS_DEVICE(&s->otp), 0, memmap[SIFIVE_U_OTP].base); > > diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h > index 82667b5..7cf742e 100644 > --- a/include/hw/riscv/sifive_u.h > +++ b/include/hw/riscv/sifive_u.h > @@ -59,6 +59,7 @@ typedef struct SiFiveUState { > int fdt_size; > > bool start_in_flash; > + uint32_t serial; > } SiFiveUState; > > enum { > -- > 2.7.4 > >
Hi Alistair, On Tue, Feb 25, 2020 at 5:14 AM Alistair Francis <alistair23@gmail.com> wrote: > > On Sun, Feb 16, 2020 at 5:56 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > At present the board serial number is hard-coded to 1, and passed > > to OTP model during initialization. Firmware (FSBL, U-Boot) uses > > the serial number to generate a unique MAC address for the on-chip > > ethernet controller. When multiple QEMU 'sifive_u' instances are > > created and connected to the same subnet, they all have the same > > MAC address hence it creates a unusable network. > > > > A new "serial" property is introduced to specify the board serial > > number. When not given, the default serial number 1 is used. > > > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > > > --- > > > > Changes in v2: > > - Move setting OTP serial number property from riscv_sifive_u_soc_init() > > to riscv_sifive_u_soc_realize(), to fix the 'check-qtest-riscv' error. > > I am not really sure why doing so could fix the 'make check' error. > > The v1 patch worked fine and nothing seems wrong. > > > > hw/riscv/sifive_u.c | 21 ++++++++++++++++++++- > > include/hw/riscv/sifive_u.h | 1 + > > 2 files changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c > > index 0e12b3c..ca561d3 100644 > > --- a/hw/riscv/sifive_u.c > > +++ b/hw/riscv/sifive_u.c > > @@ -34,6 +34,7 @@ > > #include "qemu/log.h" > > #include "qemu/error-report.h" > > #include "qapi/error.h" > > +#include "qapi/visitor.h" > > #include "hw/boards.h" > > #include "hw/loader.h" > > #include "hw/sysbus.h" > > @@ -434,7 +435,6 @@ static void riscv_sifive_u_soc_init(Object *obj) > > TYPE_SIFIVE_U_PRCI); > > sysbus_init_child_obj(obj, "otp", &s->otp, sizeof(s->otp), > > TYPE_SIFIVE_U_OTP); > > - qdev_prop_set_uint32(DEVICE(&s->otp), "serial", OTP_SERIAL); > > sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem), > > TYPE_CADENCE_GEM); > > } > > @@ -453,6 +453,18 @@ static void sifive_u_set_start_in_flash(Object *obj, bool value, Error **errp) > > s->start_in_flash = value; > > } > > > > +static void sifive_u_get_serial(Object *obj, Visitor *v, const char *name, > > + void *opaque, Error **errp) > > +{ > > + visit_type_uint32(v, name, (uint32_t *)opaque, errp); > > +} > > + > > +static void sifive_u_set_serial(Object *obj, Visitor *v, const char *name, > > + void *opaque, Error **errp) > > +{ > > + visit_type_uint32(v, name, (uint32_t *)opaque, errp); > > This is a little confusing. Maybe it's worth adding a comment that > opaque is s->serial? Yes, I can add a comment. > > Either that or change opaque to be SiFiveUState *s and then access > serial via the struct. Do you mean something like this? Calling object_property_add() with opaque as NULL, not &s->serial: object_property_add(obj, "serial", "uint32", sifive_u_get_serial, sifive_u_set_serial, NULL, NULL, NULL); Then in the sifive_u_get_serial() or sifive_u_set_serial(), replace opaque with RISCV_U_MACHINE(obj)->serial. Wow, it looks we have designed so flexible APIs :) > > > +} > > + > > static void riscv_sifive_u_machine_instance_init(Object *obj) > > { > > SiFiveUState *s = RISCV_U_MACHINE(obj); > > @@ -464,11 +476,17 @@ static void riscv_sifive_u_machine_instance_init(Object *obj) > > "Set on to tell QEMU's ROM to jump to " \ > > "flash. Otherwise QEMU will jump to DRAM", > > NULL); > > + > > + s->serial = OTP_SERIAL; > > + object_property_add(obj, "serial", "uint32", sifive_u_get_serial, > > + sifive_u_set_serial, NULL, &s->serial, NULL); > > + object_property_set_description(obj, "serial", "Board serial number", NULL); > > } > > > > static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) > > { > > MachineState *ms = MACHINE(qdev_get_machine()); > > + SiFiveUState *us = RISCV_U_MACHINE(ms); > > I don't think the Soc should access the machine like this. What if we > use this Soc on a different machine? > Yes, agree. The v1 patch does this in the riscv_sifive_u_init(), but it could not pass the "make check". See the changelog I mentioned. Do you know how to fix the "make check" properly? The issue is quite strange. The v1 patch worked perfectly OK and I did not observe any crash during my normal use, but with "make check" QEMU RISC-V crashes with the v1 patch. > There should be a SoC "serial" property that is set before realise as well. > v1 patch: http://patchwork.ozlabs.org/patch/1196127/ Regards, Bin
On Mon, Feb 24, 2020 at 9:02 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi Alistair, > > On Tue, Feb 25, 2020 at 5:14 AM Alistair Francis <alistair23@gmail.com> wrote: > > > > On Sun, Feb 16, 2020 at 5:56 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > > > At present the board serial number is hard-coded to 1, and passed > > > to OTP model during initialization. Firmware (FSBL, U-Boot) uses > > > the serial number to generate a unique MAC address for the on-chip > > > ethernet controller. When multiple QEMU 'sifive_u' instances are > > > created and connected to the same subnet, they all have the same > > > MAC address hence it creates a unusable network. > > > > > > A new "serial" property is introduced to specify the board serial > > > number. When not given, the default serial number 1 is used. > > > > > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > > > > > --- > > > > > > Changes in v2: > > > - Move setting OTP serial number property from riscv_sifive_u_soc_init() > > > to riscv_sifive_u_soc_realize(), to fix the 'check-qtest-riscv' error. > > > I am not really sure why doing so could fix the 'make check' error. > > > The v1 patch worked fine and nothing seems wrong. > > > > > > hw/riscv/sifive_u.c | 21 ++++++++++++++++++++- > > > include/hw/riscv/sifive_u.h | 1 + > > > 2 files changed, 21 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c > > > index 0e12b3c..ca561d3 100644 > > > --- a/hw/riscv/sifive_u.c > > > +++ b/hw/riscv/sifive_u.c > > > @@ -34,6 +34,7 @@ > > > #include "qemu/log.h" > > > #include "qemu/error-report.h" > > > #include "qapi/error.h" > > > +#include "qapi/visitor.h" > > > #include "hw/boards.h" > > > #include "hw/loader.h" > > > #include "hw/sysbus.h" > > > @@ -434,7 +435,6 @@ static void riscv_sifive_u_soc_init(Object *obj) > > > TYPE_SIFIVE_U_PRCI); > > > sysbus_init_child_obj(obj, "otp", &s->otp, sizeof(s->otp), > > > TYPE_SIFIVE_U_OTP); > > > - qdev_prop_set_uint32(DEVICE(&s->otp), "serial", OTP_SERIAL); > > > sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem), > > > TYPE_CADENCE_GEM); > > > } > > > @@ -453,6 +453,18 @@ static void sifive_u_set_start_in_flash(Object *obj, bool value, Error **errp) > > > s->start_in_flash = value; > > > } > > > > > > +static void sifive_u_get_serial(Object *obj, Visitor *v, const char *name, > > > + void *opaque, Error **errp) > > > +{ > > > + visit_type_uint32(v, name, (uint32_t *)opaque, errp); > > > +} > > > + > > > +static void sifive_u_set_serial(Object *obj, Visitor *v, const char *name, > > > + void *opaque, Error **errp) > > > +{ > > > + visit_type_uint32(v, name, (uint32_t *)opaque, errp); > > > > This is a little confusing. Maybe it's worth adding a comment that > > opaque is s->serial? > > Yes, I can add a comment. > > > > > Either that or change opaque to be SiFiveUState *s and then access > > serial via the struct. > > Do you mean something like this? > > Calling object_property_add() with opaque as NULL, not &s->serial: > > object_property_add(obj, "serial", "uint32", sifive_u_get_serial, > sifive_u_set_serial, NULL, NULL, NULL); > > Then in the sifive_u_get_serial() or sifive_u_set_serial(), replace > opaque with RISCV_U_MACHINE(obj)->serial. > > Wow, it looks we have designed so flexible APIs :) > > > > > > +} > > > + > > > static void riscv_sifive_u_machine_instance_init(Object *obj) > > > { > > > SiFiveUState *s = RISCV_U_MACHINE(obj); > > > @@ -464,11 +476,17 @@ static void riscv_sifive_u_machine_instance_init(Object *obj) > > > "Set on to tell QEMU's ROM to jump to " \ > > > "flash. Otherwise QEMU will jump to DRAM", > > > NULL); > > > + > > > + s->serial = OTP_SERIAL; > > > + object_property_add(obj, "serial", "uint32", sifive_u_get_serial, > > > + sifive_u_set_serial, NULL, &s->serial, NULL); > > > + object_property_set_description(obj, "serial", "Board serial number", NULL); > > > } > > > > > > static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) > > > { > > > MachineState *ms = MACHINE(qdev_get_machine()); > > > + SiFiveUState *us = RISCV_U_MACHINE(ms); > > > > I don't think the Soc should access the machine like this. What if we > > use this Soc on a different machine? > > > > Yes, agree. The v1 patch does this in the riscv_sifive_u_init(), but > it could not pass the "make check". See the changelog I mentioned. Do > you know how to fix the "make check" properly? The issue is quite > strange. The v1 patch worked perfectly OK and I did not observe any > crash during my normal use, but with "make check" QEMU RISC-V crashes > with the v1 patch. I can reproduce the error and I have a fix. I'll send it as soon as I finish testing. Do you mind testing this branch: https://github.com/alistair23/qemu/tree/mainline/alistair/sifive_serial.next Alistair > > > There should be a SoC "serial" property that is set before realise as well. > > > > v1 patch: http://patchwork.ozlabs.org/patch/1196127/ > > Regards, > Bin
Hi Alistair, On Tue, Mar 3, 2020 at 8:07 AM Alistair Francis <alistair23@gmail.com> wrote: > > On Mon, Feb 24, 2020 at 9:02 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > Hi Alistair, > > > > On Tue, Feb 25, 2020 at 5:14 AM Alistair Francis <alistair23@gmail.com> wrote: > > > > > > On Sun, Feb 16, 2020 at 5:56 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > > > > > At present the board serial number is hard-coded to 1, and passed > > > > to OTP model during initialization. Firmware (FSBL, U-Boot) uses > > > > the serial number to generate a unique MAC address for the on-chip > > > > ethernet controller. When multiple QEMU 'sifive_u' instances are > > > > created and connected to the same subnet, they all have the same > > > > MAC address hence it creates a unusable network. > > > > > > > > A new "serial" property is introduced to specify the board serial > > > > number. When not given, the default serial number 1 is used. > > > > > > > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > > > > > > > --- > > > > > > > > Changes in v2: > > > > - Move setting OTP serial number property from riscv_sifive_u_soc_init() > > > > to riscv_sifive_u_soc_realize(), to fix the 'check-qtest-riscv' error. > > > > I am not really sure why doing so could fix the 'make check' error. > > > > The v1 patch worked fine and nothing seems wrong. > > > > > > > > hw/riscv/sifive_u.c | 21 ++++++++++++++++++++- > > > > include/hw/riscv/sifive_u.h | 1 + > > > > 2 files changed, 21 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c > > > > index 0e12b3c..ca561d3 100644 > > > > --- a/hw/riscv/sifive_u.c > > > > +++ b/hw/riscv/sifive_u.c > > > > @@ -34,6 +34,7 @@ > > > > #include "qemu/log.h" > > > > #include "qemu/error-report.h" > > > > #include "qapi/error.h" > > > > +#include "qapi/visitor.h" > > > > #include "hw/boards.h" > > > > #include "hw/loader.h" > > > > #include "hw/sysbus.h" > > > > @@ -434,7 +435,6 @@ static void riscv_sifive_u_soc_init(Object *obj) > > > > TYPE_SIFIVE_U_PRCI); > > > > sysbus_init_child_obj(obj, "otp", &s->otp, sizeof(s->otp), > > > > TYPE_SIFIVE_U_OTP); > > > > - qdev_prop_set_uint32(DEVICE(&s->otp), "serial", OTP_SERIAL); > > > > sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem), > > > > TYPE_CADENCE_GEM); > > > > } > > > > @@ -453,6 +453,18 @@ static void sifive_u_set_start_in_flash(Object *obj, bool value, Error **errp) > > > > s->start_in_flash = value; > > > > } > > > > > > > > +static void sifive_u_get_serial(Object *obj, Visitor *v, const char *name, > > > > + void *opaque, Error **errp) > > > > +{ > > > > + visit_type_uint32(v, name, (uint32_t *)opaque, errp); > > > > +} > > > > + > > > > +static void sifive_u_set_serial(Object *obj, Visitor *v, const char *name, > > > > + void *opaque, Error **errp) > > > > +{ > > > > + visit_type_uint32(v, name, (uint32_t *)opaque, errp); > > > > > > This is a little confusing. Maybe it's worth adding a comment that > > > opaque is s->serial? > > > > Yes, I can add a comment. > > > > > > > > Either that or change opaque to be SiFiveUState *s and then access > > > serial via the struct. > > > > Do you mean something like this? > > > > Calling object_property_add() with opaque as NULL, not &s->serial: > > > > object_property_add(obj, "serial", "uint32", sifive_u_get_serial, > > sifive_u_set_serial, NULL, NULL, NULL); > > > > Then in the sifive_u_get_serial() or sifive_u_set_serial(), replace > > opaque with RISCV_U_MACHINE(obj)->serial. > > > > Wow, it looks we have designed so flexible APIs :) > > > > > > > > > +} > > > > + > > > > static void riscv_sifive_u_machine_instance_init(Object *obj) > > > > { > > > > SiFiveUState *s = RISCV_U_MACHINE(obj); > > > > @@ -464,11 +476,17 @@ static void riscv_sifive_u_machine_instance_init(Object *obj) > > > > "Set on to tell QEMU's ROM to jump to " \ > > > > "flash. Otherwise QEMU will jump to DRAM", > > > > NULL); > > > > + > > > > + s->serial = OTP_SERIAL; > > > > + object_property_add(obj, "serial", "uint32", sifive_u_get_serial, > > > > + sifive_u_set_serial, NULL, &s->serial, NULL); > > > > + object_property_set_description(obj, "serial", "Board serial number", NULL); > > > > } > > > > > > > > static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) > > > > { > > > > MachineState *ms = MACHINE(qdev_get_machine()); > > > > + SiFiveUState *us = RISCV_U_MACHINE(ms); > > > > > > I don't think the Soc should access the machine like this. What if we > > > use this Soc on a different machine? > > > > > > > Yes, agree. The v1 patch does this in the riscv_sifive_u_init(), but > > it could not pass the "make check". See the changelog I mentioned. Do > > you know how to fix the "make check" properly? The issue is quite > > strange. The v1 patch worked perfectly OK and I did not observe any > > crash during my normal use, but with "make check" QEMU RISC-V crashes > > with the v1 patch. > > I can reproduce the error and I have a fix. I'll send it as soon as I > finish testing. > > Do you mind testing this branch: > https://github.com/alistair23/qemu/tree/mainline/alistair/sifive_serial.next > This seems to not adding "serial" as a property. $ ./riscv64-softmmu/qemu-system-riscv64 -nographic -M sifive_u,help sifive_u.start-in-flash=bool (Set on to tell QEMU's ROM to jump to flash. Otherwise QEMU will jump to DRAM) sifive_u.memory-backend=string (Set RAM backendValid value is ID of hostmem based backend) sifive_u.enforce-config-section=bool (Set on to enforce configuration section migration) sifive_u.initrd=string (Linux initial ramdisk file) sifive_u.mem-merge=bool (Enable/disable memory merge support) sifive_u.firmware=string (Firmware image) sifive_u.dtb=string (Linux kernel device tree file) sifive_u.suppress-vmdesc=bool (Set on to disable self-describing migration) sifive_u.usb=bool (Set on/off to enable/disable usb) sifive_u.kernel=string (Linux kernel image file) sifive_u.dt-compatible=string (Overrides the "compatible" property of the dt root node) sifive_u.memory-encryption=string (Set memory encryption object to use) sifive_u.dumpdtb=string (Dump current dtb to a file and quit) sifive_u.append=string (Linux kernel command line) sifive_u.dump-guest-core=bool (Include guest memory in a core dump) sifive_u.phandle-start=int (The first phandle ID we may generate dynamically) sifive_u.graphics=bool (Set on/off to enable/disable graphics emulation) $ ./riscv64-softmmu/qemu-system-riscv64 -nographic -M sifive_u,serial=2 qemu-system-riscv64: Insufficient permission to perform this operation Regards, Bin
© 2016 - 2024 Red Hat, Inc.