Re: [PATCH v2] riscv: sifive_u: Add a "serial" property for board serial number

[Bin Meng]

Hi Alistair,

On Tue, Mar 3, 2020 at 8:07 AM Alistair Francis <address@hidden> wrote:
>
> On Mon, Feb 24, 2020 at 9:02 PM Bin Meng <address@hidden> wrote:
> >
> > Hi Alistair,
> >
> > On Tue, Feb 25, 2020 at 5:14 AM Alistair Francis <address@hidden> wrote:
> > >
> > > On Sun, Feb 16, 2020 at 5:56 AM Bin Meng <address@hidden> 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 <address@hidden>
> > > >
> > > > ---
> > > >
> > > > 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