* [PATCH v1 1/3] riscv/sifive_u: Fix up file ordering
2020-03-04 1:29 [PATCH v1 0/3] hw/riscv: Add a serial property to the sifive_u machine Alistair Francis
@ 2020-03-04 1:29 ` Alistair Francis
2020-03-04 14:09 ` Bin Meng
2020-03-04 1:29 ` [PATCH v1 2/3] riscv/sifive_u: Add a serial property to the sifive_u SoC Alistair Francis
2020-03-04 1:29 ` [PATCH v1 3/3] riscv/sifive_u: Add a serial property to the sifive_u machine Alistair Francis
2 siblings, 1 reply; 12+ messages in thread
From: Alistair Francis @ 2020-03-04 1:29 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair.francis, palmer, alistair23
Split the file into clear machine and SoC sections.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
hw/riscv/sifive_u.c | 107 ++++++++++++++++++++++----------------------
1 file changed, 54 insertions(+), 53 deletions(-)
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 156a003642..9a0145b5b4 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -399,6 +399,60 @@ static void riscv_sifive_u_init(MachineState *machine)
&address_space_memory);
}
+static bool sifive_u_get_start_in_flash(Object *obj, Error **errp)
+{
+ SiFiveUState *s = RISCV_U_MACHINE(obj);
+
+ return s->start_in_flash;
+}
+
+static void sifive_u_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", sifive_u_get_start_in_flash,
+ sifive_u_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);
+}
+
+
+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;
+ mc->min_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + 1;
+ mc->default_cpus = mc->min_cpus;
+}
+
+static const TypeInfo riscv_sifive_u_machine_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_typeinfo);
+}
+
+type_init(riscv_sifive_u_machine_init_register_types)
+
static void riscv_sifive_u_soc_init(Object *obj)
{
MachineState *ms = MACHINE(qdev_get_machine());
@@ -439,33 +493,6 @@ static void riscv_sifive_u_soc_init(Object *obj)
TYPE_CADENCE_GEM);
}
-static bool sifive_u_get_start_in_flash(Object *obj, Error **errp)
-{
- SiFiveUState *s = RISCV_U_MACHINE(obj);
-
- return s->start_in_flash;
-}
-
-static void sifive_u_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", sifive_u_get_start_in_flash,
- sifive_u_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);
-}
-
static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
{
MachineState *ms = MACHINE(qdev_get_machine());
@@ -603,29 +630,3 @@ static void riscv_sifive_u_soc_register_types(void)
}
type_init(riscv_sifive_u_soc_register_types)
-
-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;
- mc->min_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + 1;
- mc->default_cpus = mc->min_cpus;
-}
-
-static const TypeInfo riscv_sifive_u_machine_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_typeinfo);
-}
-
-type_init(riscv_sifive_u_machine_init_register_types)
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v1 1/3] riscv/sifive_u: Fix up file ordering
2020-03-04 1:29 ` [PATCH v1 1/3] riscv/sifive_u: Fix up file ordering Alistair Francis
@ 2020-03-04 14:09 ` Bin Meng
2020-03-04 23:54 ` Alistair Francis
0 siblings, 1 reply; 12+ messages in thread
From: Bin Meng @ 2020-03-04 14:09 UTC (permalink / raw)
To: Alistair Francis
Cc: Palmer Dabbelt, open list:RISC-V,
qemu-devel@nongnu.org Developers, Alistair Francis
On Wed, Mar 4, 2020 at 9:37 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> Split the file into clear machine and SoC sections.
>
Yep, I found functions in this file are a little bit confusing as well ..
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> hw/riscv/sifive_u.c | 107 ++++++++++++++++++++++----------------------
> 1 file changed, 54 insertions(+), 53 deletions(-)
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 156a003642..9a0145b5b4 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -399,6 +399,60 @@ static void riscv_sifive_u_init(MachineState *machine)
So while we are here, could we rename this to something like:
sifive_u_machine_init()? ie: drop the riscv_ prefix.
> &address_space_memory);
> }
>
> +static bool sifive_u_get_start_in_flash(Object *obj, Error **errp)
and sifive_u_machine_get_start_in_flash()? and other functions too?
> +{
> + SiFiveUState *s = RISCV_U_MACHINE(obj);
> +
> + return s->start_in_flash;
> +}
> +
> +static void sifive_u_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", sifive_u_get_start_in_flash,
> + sifive_u_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);
> +}
> +
> +
> +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;
> + mc->min_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + 1;
> + mc->default_cpus = mc->min_cpus;
> +}
> +
> +static const TypeInfo riscv_sifive_u_machine_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_typeinfo);
> +}
> +
> +type_init(riscv_sifive_u_machine_init_register_types)
> +
> static void riscv_sifive_u_soc_init(Object *obj)
Similarly this can be renamed to: sifive_u_soc_init(), and other soc functions.
> {
> MachineState *ms = MACHINE(qdev_get_machine());
> @@ -439,33 +493,6 @@ static void riscv_sifive_u_soc_init(Object *obj)
> TYPE_CADENCE_GEM);
> }
>
> -static bool sifive_u_get_start_in_flash(Object *obj, Error **errp)
> -{
> - SiFiveUState *s = RISCV_U_MACHINE(obj);
> -
> - return s->start_in_flash;
> -}
> -
> -static void sifive_u_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", sifive_u_get_start_in_flash,
> - sifive_u_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);
> -}
> -
> static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
> {
> MachineState *ms = MACHINE(qdev_get_machine());
> @@ -603,29 +630,3 @@ static void riscv_sifive_u_soc_register_types(void)
> }
>
> type_init(riscv_sifive_u_soc_register_types)
> -
> -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;
> - mc->min_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + 1;
> - mc->default_cpus = mc->min_cpus;
> -}
> -
> -static const TypeInfo riscv_sifive_u_machine_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_typeinfo);
> -}
> -
> -type_init(riscv_sifive_u_machine_init_register_types)
> --
Regards,
Bin
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v1 1/3] riscv/sifive_u: Fix up file ordering
2020-03-04 14:09 ` Bin Meng
@ 2020-03-04 23:54 ` Alistair Francis
0 siblings, 0 replies; 12+ messages in thread
From: Alistair Francis @ 2020-03-04 23:54 UTC (permalink / raw)
To: Bin Meng
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers
On Wed, Mar 4, 2020 at 6:10 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Wed, Mar 4, 2020 at 9:37 AM Alistair Francis
> <alistair.francis@wdc.com> wrote:
> >
> > Split the file into clear machine and SoC sections.
> >
>
> Yep, I found functions in this file are a little bit confusing as well ..
>
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > hw/riscv/sifive_u.c | 107 ++++++++++++++++++++++----------------------
> > 1 file changed, 54 insertions(+), 53 deletions(-)
> >
> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > index 156a003642..9a0145b5b4 100644
> > --- a/hw/riscv/sifive_u.c
> > +++ b/hw/riscv/sifive_u.c
> > @@ -399,6 +399,60 @@ static void riscv_sifive_u_init(MachineState *machine)
>
> So while we are here, could we rename this to something like:
> sifive_u_machine_init()? ie: drop the riscv_ prefix.
>
> > &address_space_memory);
> > }
> >
> > +static bool sifive_u_get_start_in_flash(Object *obj, Error **errp)
>
> and sifive_u_machine_get_start_in_flash()? and other functions too?
>
> > +{
> > + SiFiveUState *s = RISCV_U_MACHINE(obj);
> > +
> > + return s->start_in_flash;
> > +}
> > +
> > +static void sifive_u_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", sifive_u_get_start_in_flash,
> > + sifive_u_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);
> > +}
> > +
> > +
> > +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;
> > + mc->min_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + 1;
> > + mc->default_cpus = mc->min_cpus;
> > +}
> > +
> > +static const TypeInfo riscv_sifive_u_machine_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_typeinfo);
> > +}
> > +
> > +type_init(riscv_sifive_u_machine_init_register_types)
> > +
> > static void riscv_sifive_u_soc_init(Object *obj)
>
> Similarly this can be renamed to: sifive_u_soc_init(), and other soc functions.
I missed this in v2, fixed in the next version.
Alistair
>
> > {
> > MachineState *ms = MACHINE(qdev_get_machine());
> > @@ -439,33 +493,6 @@ static void riscv_sifive_u_soc_init(Object *obj)
> > TYPE_CADENCE_GEM);
> > }
> >
> > -static bool sifive_u_get_start_in_flash(Object *obj, Error **errp)
> > -{
> > - SiFiveUState *s = RISCV_U_MACHINE(obj);
> > -
> > - return s->start_in_flash;
> > -}
> > -
> > -static void sifive_u_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", sifive_u_get_start_in_flash,
> > - sifive_u_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);
> > -}
> > -
> > static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
> > {
> > MachineState *ms = MACHINE(qdev_get_machine());
> > @@ -603,29 +630,3 @@ static void riscv_sifive_u_soc_register_types(void)
> > }
> >
> > type_init(riscv_sifive_u_soc_register_types)
> > -
> > -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;
> > - mc->min_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + 1;
> > - mc->default_cpus = mc->min_cpus;
> > -}
> > -
> > -static const TypeInfo riscv_sifive_u_machine_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_typeinfo);
> > -}
> > -
> > -type_init(riscv_sifive_u_machine_init_register_types)
> > --
>
> Regards,
> Bin
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v1 2/3] riscv/sifive_u: Add a serial property to the sifive_u SoC
2020-03-04 1:29 [PATCH v1 0/3] hw/riscv: Add a serial property to the sifive_u machine Alistair Francis
2020-03-04 1:29 ` [PATCH v1 1/3] riscv/sifive_u: Fix up file ordering Alistair Francis
@ 2020-03-04 1:29 ` Alistair Francis
2020-03-04 14:47 ` Bin Meng
2020-03-04 1:29 ` [PATCH v1 3/3] riscv/sifive_u: Add a serial property to the sifive_u machine Alistair Francis
2 siblings, 1 reply; 12+ messages in thread
From: Alistair Francis @ 2020-03-04 1:29 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair.francis, palmer, alistair23
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 the sifive_u SoC to specify
the board serial number. When not given, the default serial number
1 is used.
Suggested-by: Bin Meng <bmeng.cn@gmail.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
hw/riscv/sifive_u.c | 8 +++++++-
include/hw/riscv/sifive_u.h | 2 ++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 9a0145b5b4..e52f9d0bd4 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -488,7 +488,7 @@ 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);
+ qdev_prop_set_uint32(DEVICE(&s->otp), "serial", s->serial);
sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem),
TYPE_CADENCE_GEM);
}
@@ -607,10 +607,16 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
}
+static Property riscv_sifive_u_soc_props[] = {
+ DEFINE_PROP_UINT32("serial", SiFiveUSoCState, serial, OTP_SERIAL),
+ DEFINE_PROP_END_OF_LIST()
+};
+
static void riscv_sifive_u_soc_class_init(ObjectClass *oc, void *data)
{
DeviceClass *dc = DEVICE_CLASS(oc);
+ device_class_set_props(dc, riscv_sifive_u_soc_props);
dc->realize = riscv_sifive_u_soc_realize;
/* Reason: Uses serial_hds in realize function, thus can't be used twice */
dc->user_creatable = false;
diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index 82667b5746..a2baa1de5f 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -42,6 +42,8 @@ typedef struct SiFiveUSoCState {
SiFiveUPRCIState prci;
SiFiveUOTPState otp;
CadenceGEMState gem;
+
+ uint32_t serial;
} SiFiveUSoCState;
#define TYPE_RISCV_U_MACHINE MACHINE_TYPE_NAME("sifive_u")
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v1 2/3] riscv/sifive_u: Add a serial property to the sifive_u SoC
2020-03-04 1:29 ` [PATCH v1 2/3] riscv/sifive_u: Add a serial property to the sifive_u SoC Alistair Francis
@ 2020-03-04 14:47 ` Bin Meng
2020-03-04 23:06 ` Alistair Francis
0 siblings, 1 reply; 12+ messages in thread
From: Bin Meng @ 2020-03-04 14:47 UTC (permalink / raw)
To: Alistair Francis
Cc: Palmer Dabbelt, open list:RISC-V,
qemu-devel@nongnu.org Developers, Alistair Francis
Hi Alistair,
On Wed, Mar 4, 2020 at 9:37 AM Alistair Francis
<alistair.francis@wdc.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 the sifive_u SoC to specify
> the board serial number. When not given, the default serial number
> 1 is used.
>
> Suggested-by: Bin Meng <bmeng.cn@gmail.com>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> hw/riscv/sifive_u.c | 8 +++++++-
> include/hw/riscv/sifive_u.h | 2 ++
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 9a0145b5b4..e52f9d0bd4 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -488,7 +488,7 @@ 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);
> + qdev_prop_set_uint32(DEVICE(&s->otp), "serial", s->serial);
> sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem),
> TYPE_CADENCE_GEM);
> }
> @@ -607,10 +607,16 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
> memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
> }
>
> +static Property riscv_sifive_u_soc_props[] = {
> + DEFINE_PROP_UINT32("serial", SiFiveUSoCState, serial, OTP_SERIAL),
> + DEFINE_PROP_END_OF_LIST()
I am not sure how adding another level of property in the SoC could
solve the 'make check' error.
> +};
> +
> static void riscv_sifive_u_soc_class_init(ObjectClass *oc, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(oc);
>
> + device_class_set_props(dc, riscv_sifive_u_soc_props);
> dc->realize = riscv_sifive_u_soc_realize;
> /* Reason: Uses serial_hds in realize function, thus can't be used twice */
> dc->user_creatable = false;
> diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> index 82667b5746..a2baa1de5f 100644
> --- a/include/hw/riscv/sifive_u.h
> +++ b/include/hw/riscv/sifive_u.h
> @@ -42,6 +42,8 @@ typedef struct SiFiveUSoCState {
> SiFiveUPRCIState prci;
> SiFiveUOTPState otp;
> CadenceGEMState gem;
> +
> + uint32_t serial;
> } SiFiveUSoCState;
>
> #define TYPE_RISCV_U_MACHINE MACHINE_TYPE_NAME("sifive_u")
> --
But anyway this patch does not actually work as expected. See below:
$ ./riscv64-softmmu/qemu-system-riscv64 -M sifive_u,serial=3
-nographic -m 2G -bios opensbi_u-boot_sifive_u_64.bin
OpenSBI v0.5 (Oct 31 2019 18:38:50)
____ _____ ____ _____
/ __ \ / ____| _ \_ _|
| | | |_ __ ___ _ __ | (___ | |_) || |
| | | | '_ \ / _ \ '_ \ \___ \| _ < | |
| |__| | |_) | __/ | | |____) | |_) || |_
\____/| .__/ \___|_| |_|_____/|____/_____|
| |
|_|
Platform Name : SiFive Freedom U540
Platform HART Features : RV64ACDFIMSU
Platform Max HARTs : 5
Current Hart : 1
Firmware Base : 0x80000000
Firmware Size : 96 KB
Runtime SBI Version : 0.2
PMP0: 0x0000000080000000-0x000000008001ffff (A)
PMP1: 0x0000000000000000-0xffffffffffffffff (A,R,W,X)
U-Boot 2019.10 (Oct 31 2019 - 18:38:33 +0800)
CPU: rv64imafdcsu
Model: SiFive HiFive Unleashed A00
DRAM: 2 GiB
MMC:
In: serial@10010000
Out: serial@10010000
Err: serial@10010000
Net:
Warning: ethernet@10090000 MAC addresses don't match:
Address in ROM is 52:54:00:12:34:56
Address in environment is 70:b3:d5:92:f0:01
eth0: ethernet@10090000
Hit any key to stop autoboot: 0
See this line:
Address in environment is 70:b3:d5:92:f0:01
It should be: 70:b3:d5:92:f0:03 since I specified serial number as 3.
Regards,
Bin
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v1 2/3] riscv/sifive_u: Add a serial property to the sifive_u SoC
2020-03-04 14:47 ` Bin Meng
@ 2020-03-04 23:06 ` Alistair Francis
2020-03-05 9:30 ` Bin Meng
0 siblings, 1 reply; 12+ messages in thread
From: Alistair Francis @ 2020-03-04 23:06 UTC (permalink / raw)
To: Bin Meng
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers
On Wed, Mar 4, 2020 at 6:47 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Alistair,
>
> On Wed, Mar 4, 2020 at 9:37 AM Alistair Francis
> <alistair.francis@wdc.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 the sifive_u SoC to specify
> > the board serial number. When not given, the default serial number
> > 1 is used.
> >
> > Suggested-by: Bin Meng <bmeng.cn@gmail.com>
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > hw/riscv/sifive_u.c | 8 +++++++-
> > include/hw/riscv/sifive_u.h | 2 ++
> > 2 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > index 9a0145b5b4..e52f9d0bd4 100644
> > --- a/hw/riscv/sifive_u.c
> > +++ b/hw/riscv/sifive_u.c
> > @@ -488,7 +488,7 @@ 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);
> > + qdev_prop_set_uint32(DEVICE(&s->otp), "serial", s->serial);
> > sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem),
> > TYPE_CADENCE_GEM);
> > }
> > @@ -607,10 +607,16 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
> > memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
> > }
> >
> > +static Property riscv_sifive_u_soc_props[] = {
> > + DEFINE_PROP_UINT32("serial", SiFiveUSoCState, serial, OTP_SERIAL),
> > + DEFINE_PROP_END_OF_LIST()
>
> I am not sure how adding another level of property in the SoC could
> solve the 'make check' error.
The problem is that you were adding a machine property and then you
had the SoC reach up to the machine object to get the serial value.
This isn't correct and is why the tests fail.
This patch series instead adds a property to the machine and the SoC,
where the machine sets the SoC property.
>
> > +};
> > +
> > static void riscv_sifive_u_soc_class_init(ObjectClass *oc, void *data)
> > {
> > DeviceClass *dc = DEVICE_CLASS(oc);
> >
> > + device_class_set_props(dc, riscv_sifive_u_soc_props);
> > dc->realize = riscv_sifive_u_soc_realize;
> > /* Reason: Uses serial_hds in realize function, thus can't be used twice */
> > dc->user_creatable = false;
> > diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> > index 82667b5746..a2baa1de5f 100644
> > --- a/include/hw/riscv/sifive_u.h
> > +++ b/include/hw/riscv/sifive_u.h
> > @@ -42,6 +42,8 @@ typedef struct SiFiveUSoCState {
> > SiFiveUPRCIState prci;
> > SiFiveUOTPState otp;
> > CadenceGEMState gem;
> > +
> > + uint32_t serial;
> > } SiFiveUSoCState;
> >
> > #define TYPE_RISCV_U_MACHINE MACHINE_TYPE_NAME("sifive_u")
> > --
>
> But anyway this patch does not actually work as expected. See below:
>
> $ ./riscv64-softmmu/qemu-system-riscv64 -M sifive_u,serial=3
> -nographic -m 2G -bios opensbi_u-boot_sifive_u_64.bin
>
> OpenSBI v0.5 (Oct 31 2019 18:38:50)
> ____ _____ ____ _____
> / __ \ / ____| _ \_ _|
> | | | |_ __ ___ _ __ | (___ | |_) || |
> | | | | '_ \ / _ \ '_ \ \___ \| _ < | |
> | |__| | |_) | __/ | | |____) | |_) || |_
> \____/| .__/ \___|_| |_|_____/|____/_____|
> | |
> |_|
>
> Platform Name : SiFive Freedom U540
> Platform HART Features : RV64ACDFIMSU
> Platform Max HARTs : 5
> Current Hart : 1
> Firmware Base : 0x80000000
> Firmware Size : 96 KB
> Runtime SBI Version : 0.2
>
> PMP0: 0x0000000080000000-0x000000008001ffff (A)
> PMP1: 0x0000000000000000-0xffffffffffffffff (A,R,W,X)
>
>
> U-Boot 2019.10 (Oct 31 2019 - 18:38:33 +0800)
>
> CPU: rv64imafdcsu
> Model: SiFive HiFive Unleashed A00
> DRAM: 2 GiB
> MMC:
> In: serial@10010000
> Out: serial@10010000
> Err: serial@10010000
> Net:
> Warning: ethernet@10090000 MAC addresses don't match:
> Address in ROM is 52:54:00:12:34:56
> Address in environment is 70:b3:d5:92:f0:01
> eth0: ethernet@10090000
> Hit any key to stop autoboot: 0
>
>
> See this line:
> Address in environment is 70:b3:d5:92:f0:01
>
> It should be: 70:b3:d5:92:f0:03 since I specified serial number as 3.
Fixed, The problem was setting the serial property too early.
Alistair
>
> Regards,
> Bin
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v1 2/3] riscv/sifive_u: Add a serial property to the sifive_u SoC
2020-03-04 23:06 ` Alistair Francis
@ 2020-03-05 9:30 ` Bin Meng
2020-03-05 16:45 ` Alistair Francis
0 siblings, 1 reply; 12+ messages in thread
From: Bin Meng @ 2020-03-05 9:30 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers
Hi Alistair,
On Thu, Mar 5, 2020 at 7:13 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Mar 4, 2020 at 6:47 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Alistair,
> >
> > On Wed, Mar 4, 2020 at 9:37 AM Alistair Francis
> > <alistair.francis@wdc.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 the sifive_u SoC to specify
> > > the board serial number. When not given, the default serial number
> > > 1 is used.
> > >
> > > Suggested-by: Bin Meng <bmeng.cn@gmail.com>
> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > ---
> > > hw/riscv/sifive_u.c | 8 +++++++-
> > > include/hw/riscv/sifive_u.h | 2 ++
> > > 2 files changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > > index 9a0145b5b4..e52f9d0bd4 100644
> > > --- a/hw/riscv/sifive_u.c
> > > +++ b/hw/riscv/sifive_u.c
> > > @@ -488,7 +488,7 @@ 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);
> > > + qdev_prop_set_uint32(DEVICE(&s->otp), "serial", s->serial);
> > > sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem),
> > > TYPE_CADENCE_GEM);
> > > }
> > > @@ -607,10 +607,16 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
> > > memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
> > > }
> > >
> > > +static Property riscv_sifive_u_soc_props[] = {
> > > + DEFINE_PROP_UINT32("serial", SiFiveUSoCState, serial, OTP_SERIAL),
> > > + DEFINE_PROP_END_OF_LIST()
> >
> > I am not sure how adding another level of property in the SoC could
> > solve the 'make check' error.
>
> The problem is that you were adding a machine property and then you
> had the SoC reach up to the machine object to get the serial value.
> This isn't correct and is why the tests fail.
>
So looks the failure was due to a check in the test codes only? As I
did not see QEMU crashed during my normal usage.
> This patch series instead adds a property to the machine and the SoC,
> where the machine sets the SoC property.
>
Regards,
Bin
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v1 2/3] riscv/sifive_u: Add a serial property to the sifive_u SoC
2020-03-05 9:30 ` Bin Meng
@ 2020-03-05 16:45 ` Alistair Francis
2020-03-06 0:09 ` Bin Meng
0 siblings, 1 reply; 12+ messages in thread
From: Alistair Francis @ 2020-03-05 16:45 UTC (permalink / raw)
To: Bin Meng
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers
On Thu, Mar 5, 2020 at 1:31 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Alistair,
>
> On Thu, Mar 5, 2020 at 7:13 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Mar 4, 2020 at 6:47 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Alistair,
> > >
> > > On Wed, Mar 4, 2020 at 9:37 AM Alistair Francis
> > > <alistair.francis@wdc.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 the sifive_u SoC to specify
> > > > the board serial number. When not given, the default serial number
> > > > 1 is used.
> > > >
> > > > Suggested-by: Bin Meng <bmeng.cn@gmail.com>
> > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > ---
> > > > hw/riscv/sifive_u.c | 8 +++++++-
> > > > include/hw/riscv/sifive_u.h | 2 ++
> > > > 2 files changed, 9 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > > > index 9a0145b5b4..e52f9d0bd4 100644
> > > > --- a/hw/riscv/sifive_u.c
> > > > +++ b/hw/riscv/sifive_u.c
> > > > @@ -488,7 +488,7 @@ 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);
> > > > + qdev_prop_set_uint32(DEVICE(&s->otp), "serial", s->serial);
> > > > sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem),
> > > > TYPE_CADENCE_GEM);
> > > > }
> > > > @@ -607,10 +607,16 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
> > > > memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
> > > > }
> > > >
> > > > +static Property riscv_sifive_u_soc_props[] = {
> > > > + DEFINE_PROP_UINT32("serial", SiFiveUSoCState, serial, OTP_SERIAL),
> > > > + DEFINE_PROP_END_OF_LIST()
> > >
> > > I am not sure how adding another level of property in the SoC could
> > > solve the 'make check' error.
> >
> > The problem is that you were adding a machine property and then you
> > had the SoC reach up to the machine object to get the serial value.
> > This isn't correct and is why the tests fail.
> >
>
> So looks the failure was due to a check in the test codes only? As I
> did not see QEMU crashed during my normal usage.
No, the bug was in the actual implementation. You were just lucky that
you didn't see any issues as in your case you could access the machine
state. The make check probably added the SoC individually and hence
caught the bug.
Alistair
>
> > This patch series instead adds a property to the machine and the SoC,
> > where the machine sets the SoC property.
> >
>
> Regards,
> Bin
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v1 2/3] riscv/sifive_u: Add a serial property to the sifive_u SoC
2020-03-05 16:45 ` Alistair Francis
@ 2020-03-06 0:09 ` Bin Meng
2020-03-06 19:51 ` Alistair Francis
0 siblings, 1 reply; 12+ messages in thread
From: Bin Meng @ 2020-03-06 0:09 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers
Hi Alistair,
On Fri, Mar 6, 2020 at 12:53 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, Mar 5, 2020 at 1:31 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Alistair,
> >
> > On Thu, Mar 5, 2020 at 7:13 AM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Wed, Mar 4, 2020 at 6:47 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > Hi Alistair,
> > > >
> > > > On Wed, Mar 4, 2020 at 9:37 AM Alistair Francis
> > > > <alistair.francis@wdc.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 the sifive_u SoC to specify
> > > > > the board serial number. When not given, the default serial number
> > > > > 1 is used.
> > > > >
> > > > > Suggested-by: Bin Meng <bmeng.cn@gmail.com>
> > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > ---
> > > > > hw/riscv/sifive_u.c | 8 +++++++-
> > > > > include/hw/riscv/sifive_u.h | 2 ++
> > > > > 2 files changed, 9 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > > > > index 9a0145b5b4..e52f9d0bd4 100644
> > > > > --- a/hw/riscv/sifive_u.c
> > > > > +++ b/hw/riscv/sifive_u.c
> > > > > @@ -488,7 +488,7 @@ 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);
> > > > > + qdev_prop_set_uint32(DEVICE(&s->otp), "serial", s->serial);
> > > > > sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem),
> > > > > TYPE_CADENCE_GEM);
> > > > > }
> > > > > @@ -607,10 +607,16 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
> > > > > memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
> > > > > }
> > > > >
> > > > > +static Property riscv_sifive_u_soc_props[] = {
> > > > > + DEFINE_PROP_UINT32("serial", SiFiveUSoCState, serial, OTP_SERIAL),
> > > > > + DEFINE_PROP_END_OF_LIST()
> > > >
> > > > I am not sure how adding another level of property in the SoC could
> > > > solve the 'make check' error.
> > >
> > > The problem is that you were adding a machine property and then you
> > > had the SoC reach up to the machine object to get the serial value.
> > > This isn't correct and is why the tests fail.
> > >
> >
> > So looks the failure was due to a check in the test codes only? As I
> > did not see QEMU crashed during my normal usage.
>
> No, the bug was in the actual implementation. You were just lucky that
> you didn't see any issues as in your case you could access the machine
> state. The make check probably added the SoC individually and hence
> caught the bug.
That sounds like the difference that caused the crash in the test.
Thanks for helping this!
Regards,
Bin
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v1 2/3] riscv/sifive_u: Add a serial property to the sifive_u SoC
2020-03-06 0:09 ` Bin Meng
@ 2020-03-06 19:51 ` Alistair Francis
0 siblings, 0 replies; 12+ messages in thread
From: Alistair Francis @ 2020-03-06 19:51 UTC (permalink / raw)
To: Bin Meng
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers
On Thu, Mar 5, 2020 at 4:09 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Alistair,
>
> On Fri, Mar 6, 2020 at 12:53 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Thu, Mar 5, 2020 at 1:31 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Alistair,
> > >
> > > On Thu, Mar 5, 2020 at 7:13 AM Alistair Francis <alistair23@gmail.com> wrote:
> > > >
> > > > On Wed, Mar 4, 2020 at 6:47 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > >
> > > > > Hi Alistair,
> > > > >
> > > > > On Wed, Mar 4, 2020 at 9:37 AM Alistair Francis
> > > > > <alistair.francis@wdc.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 the sifive_u SoC to specify
> > > > > > the board serial number. When not given, the default serial number
> > > > > > 1 is used.
> > > > > >
> > > > > > Suggested-by: Bin Meng <bmeng.cn@gmail.com>
> > > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > > ---
> > > > > > hw/riscv/sifive_u.c | 8 +++++++-
> > > > > > include/hw/riscv/sifive_u.h | 2 ++
> > > > > > 2 files changed, 9 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > > > > > index 9a0145b5b4..e52f9d0bd4 100644
> > > > > > --- a/hw/riscv/sifive_u.c
> > > > > > +++ b/hw/riscv/sifive_u.c
> > > > > > @@ -488,7 +488,7 @@ 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);
> > > > > > + qdev_prop_set_uint32(DEVICE(&s->otp), "serial", s->serial);
> > > > > > sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem),
> > > > > > TYPE_CADENCE_GEM);
> > > > > > }
> > > > > > @@ -607,10 +607,16 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
> > > > > > memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
> > > > > > }
> > > > > >
> > > > > > +static Property riscv_sifive_u_soc_props[] = {
> > > > > > + DEFINE_PROP_UINT32("serial", SiFiveUSoCState, serial, OTP_SERIAL),
> > > > > > + DEFINE_PROP_END_OF_LIST()
> > > > >
> > > > > I am not sure how adding another level of property in the SoC could
> > > > > solve the 'make check' error.
> > > >
> > > > The problem is that you were adding a machine property and then you
> > > > had the SoC reach up to the machine object to get the serial value.
> > > > This isn't correct and is why the tests fail.
> > > >
> > >
> > > So looks the failure was due to a check in the test codes only? As I
> > > did not see QEMU crashed during my normal usage.
> >
> > No, the bug was in the actual implementation. You were just lucky that
> > you didn't see any issues as in your case you could access the machine
> > state. The make check probably added the SoC individually and hence
> > caught the bug.
>
> That sounds like the difference that caused the crash in the test.
> Thanks for helping this!
No worries!
Alistair
>
> Regards,
> Bin
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v1 3/3] riscv/sifive_u: Add a serial property to the sifive_u machine
2020-03-04 1:29 [PATCH v1 0/3] hw/riscv: Add a serial property to the sifive_u machine Alistair Francis
2020-03-04 1:29 ` [PATCH v1 1/3] riscv/sifive_u: Fix up file ordering Alistair Francis
2020-03-04 1:29 ` [PATCH v1 2/3] riscv/sifive_u: Add a serial property to the sifive_u SoC Alistair Francis
@ 2020-03-04 1:29 ` Alistair Francis
2 siblings, 0 replies; 12+ messages in thread
From: Alistair Francis @ 2020-03-04 1:29 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair.francis, palmer, alistair23
From: Bin Meng <bmeng.cn@gmail.com>
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>
Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Message-Id: <1573916930-19068-1-git-send-email-bmeng.cn@gmail.com>
[ Changed by AF:
- Use the SoC's serial property to pass the info to the SoC
- Fixup commit title
- Rebase on file restructuring
]
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
hw/riscv/sifive_u.c | 20 ++++++++++++++++++++
include/hw/riscv/sifive_u.h | 1 +
2 files changed, 21 insertions(+)
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index e52f9d0bd4..f3f67cc0e3 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"
@@ -322,6 +323,8 @@ static void riscv_sifive_u_init(MachineState *machine)
object_initialize_child(OBJECT(machine), "soc", &s->soc,
sizeof(s->soc), TYPE_RISCV_U_SOC,
&error_abort, NULL);
+ object_property_set_uint(OBJECT(&s->soc), s->serial, "serial",
+ &error_abort);
object_property_set_bool(OBJECT(&s->soc), true, "realized",
&error_abort);
@@ -413,6 +416,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);
@@ -424,6 +439,11 @@ 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);
}
diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index a2baa1de5f..16c297ec5f 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -61,6 +61,7 @@ typedef struct SiFiveUState {
int fdt_size;
bool start_in_flash;
+ uint32_t serial;
} SiFiveUState;
enum {
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread