* [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number
@ 2020-02-16 13:55 Bin Meng
2020-02-23 3:59 ` Bin Meng
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Bin Meng @ 2020-02-16 13:55 UTC (permalink / raw)
To: Alistair Francis, Bastian Koppelmann, Palmer Dabbelt,
Sagar Karandikar, qemu-devel, qemu-riscv
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number
2020-02-16 13:55 [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number Bin Meng
@ 2020-02-23 3:59 ` Bin Meng
2020-02-24 21:06 ` Alistair Francis
2020-03-04 23:00 ` Palmer Dabbelt
2 siblings, 0 replies; 14+ messages in thread
From: Bin Meng @ 2020-02-23 3:59 UTC (permalink / raw)
To: Alistair Francis, Bastian Koppelmann, Palmer Dabbelt,
Sagar Karandikar, qemu-devel@nongnu.org Developers,
open list:RISC-V
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number
2020-02-16 13:55 [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number Bin Meng
@ 2020-02-24 21:06 ` Alistair Francis
2020-02-24 21:06 ` Alistair Francis
2020-03-04 23:00 ` Palmer Dabbelt
2 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2020-02-24 21:06 UTC (permalink / raw)
To: Bin Meng
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, qemu-devel@nongnu.org Developers,
Alistair Francis
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
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number
@ 2020-02-24 21:06 ` Alistair Francis
0 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2020-02-24 21:06 UTC (permalink / raw)
To: Bin Meng
Cc: Alistair Francis, Bastian Koppelmann, Palmer Dabbelt,
Sagar Karandikar, qemu-devel@nongnu.org Developers,
open list:RISC-V
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
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number
2020-02-24 21:06 ` Alistair Francis
@ 2020-02-25 5:02 ` Bin Meng
-1 siblings, 0 replies; 14+ messages in thread
From: Bin Meng @ 2020-02-25 5:02 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, qemu-devel@nongnu.org Developers,
Alistair Francis
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number
@ 2020-02-25 5:02 ` Bin Meng
0 siblings, 0 replies; 14+ messages in thread
From: Bin Meng @ 2020-02-25 5:02 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, Bastian Koppelmann, Palmer Dabbelt,
Sagar Karandikar, qemu-devel@nongnu.org Developers,
open list:RISC-V
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number
2020-02-25 5:02 ` Bin Meng
@ 2020-03-02 23:59 ` Alistair Francis
-1 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2020-03-02 23:59 UTC (permalink / raw)
To: Bin Meng
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, qemu-devel@nongnu.org Developers,
Alistair Francis
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number
@ 2020-03-02 23:59 ` Alistair Francis
0 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2020-03-02 23:59 UTC (permalink / raw)
To: Bin Meng
Cc: Alistair Francis, Bastian Koppelmann, Palmer Dabbelt,
Sagar Karandikar, qemu-devel@nongnu.org Developers,
open list:RISC-V
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number
2020-03-02 23:59 ` Alistair Francis
@ 2020-03-03 9:30 ` Bin Meng
-1 siblings, 0 replies; 14+ messages in thread
From: Bin Meng @ 2020-03-03 9:30 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, qemu-devel@nongnu.org Developers,
Alistair Francis
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number
@ 2020-03-03 9:30 ` Bin Meng
0 siblings, 0 replies; 14+ messages in thread
From: Bin Meng @ 2020-03-03 9:30 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, Bastian Koppelmann, Palmer Dabbelt,
Sagar Karandikar, qemu-devel@nongnu.org Developers,
open list:RISC-V
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number
2020-02-16 13:55 [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number Bin Meng
@ 2020-03-04 23:00 ` Palmer Dabbelt
2020-02-24 21:06 ` Alistair Francis
2020-03-04 23:00 ` Palmer Dabbelt
2 siblings, 0 replies; 14+ messages in thread
From: Palmer Dabbelt @ 2020-03-04 23:00 UTC (permalink / raw)
To: bmeng.cn
Cc: Bastian Koppelmann, Alistair Francis, qemu-devel, sagark, qemu-riscv
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number
@ 2020-03-04 23:00 ` Palmer Dabbelt
0 siblings, 0 replies; 14+ messages in thread
From: Palmer Dabbelt @ 2020-03-04 23:00 UTC (permalink / raw)
To: bmeng.cn
Cc: Alistair Francis, Bastian Koppelmann, sagark, qemu-devel, qemu-riscv
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number
2020-03-04 23:00 ` Palmer Dabbelt
@ 2020-03-04 23:08 ` Alistair Francis
-1 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2020-03-04 23:08 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
qemu-devel@nongnu.org Developers, Alistair Francis, Bin Meng
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number
@ 2020-03-04 23:08 ` Alistair Francis
0 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2020-03-04 23:08 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: Bin Meng, Bastian Koppelmann, Alistair Francis,
qemu-devel@nongnu.org Developers, Sagar Karandikar,
open list:RISC-V
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-03-04 23:16 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-16 13:55 [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number Bin Meng
2020-02-23 3:59 ` Bin Meng
2020-02-24 21:06 ` Alistair Francis
2020-02-24 21:06 ` Alistair Francis
2020-02-25 5:02 ` Bin Meng
2020-02-25 5:02 ` Bin Meng
2020-03-02 23:59 ` Alistair Francis
2020-03-02 23:59 ` Alistair Francis
2020-03-03 9:30 ` Bin Meng
2020-03-03 9:30 ` Bin Meng
2020-03-04 23:00 ` Palmer Dabbelt
2020-03-04 23:00 ` Palmer Dabbelt
2020-03-04 23:08 ` Alistair Francis
2020-03-04 23:08 ` Alistair Francis
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.