Re: [PATCH v1 2/3] riscv/sifive_u: Add a serial property to the sifive_u SoC

[Alistair Francis]

On Thu, Mar 5, 2020 at 1:31 AM Bin Meng <address@hidden> wrote:
>
> Hi Alistair,
>
> On Thu, Mar 5, 2020 at 7:13 AM Alistair Francis <address@hidden> wrote:
> >
> > On Wed, Mar 4, 2020 at 6:47 AM Bin Meng <address@hidden> wrote:
> > >
> > > Hi Alistair,
> > >
> > > On Wed, Mar 4, 2020 at 9:37 AM Alistair Francis
> > > <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 the sifive_u SoC to specify
> > > > the board serial number. When not given, the default serial number
> > > > 1 is used.
> > > >
> > > > Suggested-by: Bin Meng <address@hidden>
> > > > Signed-off-by: Alistair Francis <address@hidden>
> > > > ---
> > > >  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