Re: [RFC PATCH v5 2/2] hw/riscv: sifive_u: Add backend drive support

[Green Wan]

On Wed, Sep 30, 2020 at 1:08 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Mon, Sep 28, 2020 at 2:18 AM Green Wan <green.wan@sifive.com> wrote:
> >
> > Hi Alistair,
> >
> > Thanks for the review. See the reply inline below.
> >
> >
> > On Sat, Sep 26, 2020 at 5:52 AM Alistair Francis <alistair23@gmail.com> 
> > wrote:
> > >
> > > On Tue, Sep 1, 2020 at 8:49 AM Green Wan <green.wan@sifive.com> wrote:
> > > >
> > > > Add '-drive' support to OTP device. Allow users to assign a raw file
> > > > as OTP image.
> > >
> > > Do you mind writing an example command line argument in the commit 
> > > message?
> > >
> > > Also, do you have a test case for this? I would like to add it to my CI.
> > >
> >
> > Do you mean qtest? I run uboot and use uboot driver to test it and
> > didn't create a qemu test case.
>
> No, I just mean how are you running and testing this.
>
> So you are booting U-Boot, then how do you test it in U-Boot?

Correct, I just enabled the configuration for
./drivers/misc/sifive-otp.c in uboot for normal booting access to OTP.
And manually modify some failures write case to test write-once
feature.

>
> > Here is the command I use:
> >
> > $ qemu-system-riscv64 -M sifive_u -m 256M -nographic -bios none
> > -kernel ../opensbi/build/platform/sifive/fu540/firmware/fw_payload.elf
> > -d guest_errors -drive if=none,format=raw,file=otp.img
> >
> > I'll check how to create a test case but maybe not that soon in the next 
> > patch.
> >
> > > >
> > > > Signed-off-by: Green Wan <green.wan@sifive.com>
> > > > ---
> > > >  hw/riscv/sifive_u_otp.c         | 50 +++++++++++++++++++++++++++++++++
> > > >  include/hw/riscv/sifive_u_otp.h |  2 ++
> > > >  2 files changed, 52 insertions(+)
> > > >
> > > > diff --git a/hw/riscv/sifive_u_otp.c b/hw/riscv/sifive_u_otp.c
> > > > index b8369e9035..477c54c7b8 100644
> > > > --- a/hw/riscv/sifive_u_otp.c
> > > > +++ b/hw/riscv/sifive_u_otp.c
> > > > @@ -24,6 +24,8 @@
> > > >  #include "qemu/log.h"
> > > >  #include "qemu/module.h"
> > > >  #include "hw/riscv/sifive_u_otp.h"
> > > > +#include "sysemu/blockdev.h"
> > > > +#include "sysemu/block-backend.h"
> > > >
> > > >  #define WRITTEN_BIT_ON 0x1
> > > >
> > > > @@ -54,6 +56,16 @@ static uint64_t sifive_u_otp_read(void *opaque, 
> > > > hwaddr addr, unsigned int size)
> > > >          if ((s->pce & SIFIVE_U_OTP_PCE_EN) &&
> > > >              (s->pdstb & SIFIVE_U_OTP_PDSTB_EN) &&
> > > >              (s->ptrim & SIFIVE_U_OTP_PTRIM_EN)) {
> > > > +
> > > > +            /* read from backend */
> > > > +            if (s->blk) {
> > > > +                int32_t buf;
> > > > +
> > > > +                blk_pread(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD, &buf,
> > > > +                          SIFIVE_U_OTP_FUSE_WORD);
> > > > +                return buf;
> > > > +            }
> > > > +
> > > >              return s->fuse[s->pa & SIFIVE_U_OTP_PA_MASK];
> > > >          } else {
> > > >              return 0xff;
> > > > @@ -145,6 +157,12 @@ static void sifive_u_otp_write(void *opaque, 
> > > > hwaddr addr,
> > > >              /* write bit data */
> > > >              SET_FUSEARRAY_BIT(s->fuse, s->pa, s->paio, s->pdin);
> > > >
> > > > +            /* write to backend */
> > > > +            if (s->blk) {
> > > > +                blk_pwrite(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD, 
> > > > &val32,
> > > > +                           SIFIVE_U_OTP_FUSE_WORD, 0);
> > > > +            }
> > > > +
> > > >              /* update written bit */
> > > >              SET_FUSEARRAY_BIT(s->fuse_wo, s->pa, s->paio, 
> > > > WRITTEN_BIT_ON);
> > > >          }
> > > > @@ -168,16 +186,48 @@ static const MemoryRegionOps sifive_u_otp_ops = {
> > > >
> > > >  static Property sifive_u_otp_properties[] = {
> > > >      DEFINE_PROP_UINT32("serial", SiFiveUOTPState, serial, 0),
> > > > +    DEFINE_PROP_DRIVE("drive", SiFiveUOTPState, blk),
> > > >      DEFINE_PROP_END_OF_LIST(),
> > > >  };
> > > >
> > > >  static void sifive_u_otp_realize(DeviceState *dev, Error **errp)
> > > >  {
> > > >      SiFiveUOTPState *s = SIFIVE_U_OTP(dev);
> > > > +    DriveInfo *dinfo;
> > > >
> > > >      memory_region_init_io(&s->mmio, OBJECT(dev), &sifive_u_otp_ops, s,
> > > >                            TYPE_SIFIVE_U_OTP, SIFIVE_U_OTP_REG_SIZE);
> > > >      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
> > > > +
> > > > +    dinfo = drive_get_next(IF_NONE);
> > > > +    if (dinfo) {
> > > > +        int ret;
> > > > +        uint64_t perm;
> > > > +        int filesize;
> > > > +        BlockBackend *blk;
> > > > +
> > > > +        blk = blk_by_legacy_dinfo(dinfo);
> > >
> > > I think this should be:
> > >
> > > blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
> > >
> >
> > The previous code, "if (dinfo)", should check NULL already. But we can
> > add more checks for blk such as qdev_prop_set_drive_err().
>
> You are right, but I don't see a fallback if !dinfo
>
Not sure whether we need it. Originally, I also added '!dinfo' check
logic and turned out I found it can be handled like without "-drive"
case. It just does a fallback to use the fuse[] array in memory. For
example,
"qemu-system-riscv64 -M sifive_u -m 256M -nographic -bios none -kernel
../opensbi/build/platform/sifive/fu540/firmware/fw_payload.elf"


> >
> > > > +        filesize = SIFIVE_U_OTP_NUM_FUSES * SIFIVE_U_OTP_FUSE_WORD;
> > > > +        if (blk_getlength(blk) < filesize) {
> > > > +            qemu_log_mask(LOG_GUEST_ERROR, "OTP drive size < 16K\n");
> > >
> > > You should probably be setting errp instead.
> > >
> > > If a user specified a -drive argument, they probably want to error if
> > > we aren't going to use it.
> > >
> >
> > Will set an errp.
>
> Great!
>
> >
> > > > +            return;
> > > > +        }
> > > > +
> > > > +        qdev_prop_set_drive(dev, "drive", blk);
> > > > +
> > > > +        perm = BLK_PERM_CONSISTENT_READ |
> > > > +                        (blk_is_read_only(s->blk) ? 0 : 
> > > > BLK_PERM_WRITE);
> > > > +        ret = blk_set_perm(s->blk, perm, BLK_PERM_ALL, errp);
> > > > +        if (ret < 0) {
> > > > +            qemu_log_mask(LOG_GUEST_ERROR, "set perm error.");
> > >
> > > Is it worth printing the error?
> > >
> > Probably add it when I test it. Will remove it.
>
> Thanks
>
> Alistair
>
> >
> > > > +        }
> > > > +
> > > > +        if (blk_pread(s->blk, 0, s->fuse, filesize) != filesize) {
> > > > +            qemu_log_mask(LOG_GUEST_ERROR,
> > > > +                          "failed to read the initial flash content");
> > > > +            return;
> > >
> > > You don't need a return here.
> > >
> > k, will remove it and set errp.
> >
> > > Is this error fatal?
> > >
> > This shouldn't be fatal but it might lead to unknown state if the
> > content is read partially.
> > But the checking, "filesize < 16K", is fatal. It leads qemu to abort.
> >
> >
> > > Alistair
> > >
> > > > +        }
> > > > +    }
> > > >  }
> > > >
> > > >  static void sifive_u_otp_reset(DeviceState *dev)
> > > > diff --git a/include/hw/riscv/sifive_u_otp.h 
> > > > b/include/hw/riscv/sifive_u_otp.h
> > > > index 4a5a0acf48..9bc781fd4f 100644
> > > > --- a/include/hw/riscv/sifive_u_otp.h
> > > > +++ b/include/hw/riscv/sifive_u_otp.h
> > > > @@ -45,6 +45,7 @@
> > > >
> > > >  #define SIFIVE_U_OTP_PA_MASK        0xfff
> > > >  #define SIFIVE_U_OTP_NUM_FUSES      0x1000
> > > > +#define SIFIVE_U_OTP_FUSE_WORD      4
> > > >  #define SIFIVE_U_OTP_SERIAL_ADDR    0xfc
> > > >
> > > >  #define SIFIVE_U_OTP_REG_SIZE       0x1000
> > > > @@ -78,6 +79,7 @@ typedef struct SiFiveUOTPState {
> > > >      uint32_t fuse_wo[SIFIVE_U_OTP_NUM_FUSES];
> > > >      /* config */
> > > >      uint32_t serial;
> > > > +    BlockBackend *blk;
> > > >  } SiFiveUOTPState;
> > > >
> > > >  #endif /* HW_SIFIVE_U_OTP_H */
> > > > --
> > > > 2.17.1
> > > >
> > > >