Re: [PULL 12/12] hw/misc/sifive_u_otp: Add backend drive support

[Green Wan]

OK, let me do it. I'll send it soon.

On Tue, Nov 3, 2020 at 11:53 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Mon, Nov 2, 2020 at 9:49 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Fri, 23 Oct 2020 at 16:27, Alistair Francis <alistair.francis@wdc.com> 
> > wrote:
> > >
> > > From: Green Wan <green.wan@sifive.com>
> > >
> > > Add '-drive' support to OTP device. Allow users to assign a raw file
> > > as OTP image.
> >
> > Hi; Coverity reports some issues with this code (CID 1435959,
> > CID 1435960, CID 1435961). They're all basically the same thing:
> > in read, write and reset functions this code calls blk_pread()
> > or blk_pwrite() but doesn't check the return value, so if the
> > underlying file operation fails then the guest will be
> > returned garbage data or have its write thrown away without
> > either the guest or the user being warned about that.
>
> Green Wan are you able to send a patch to check the error value?
>
> Alistair
>
> >
> > > @@ -54,6 +57,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 +158,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,
> > > +                           &s->fuse[s->pa], SIFIVE_U_OTP_FUSE_WORD, 0);
> > > +            }
> > > +
> > >              /* update written bit */
> > >              SET_FUSEARRAY_BIT(s->fuse_wo, s->pa, s->paio, 
> > > WRITTEN_BIT_ON);
> > >          }
> > > @@ -168,16 +187,48 @@ static const MemoryRegionOps sifive_u_otp_ops = {
> >
> > >  static void sifive_u_otp_reset(DeviceState *dev)
> > > @@ -191,6 +242,20 @@ static void sifive_u_otp_reset(DeviceState *dev)
> > >      s->fuse[SIFIVE_U_OTP_SERIAL_ADDR] = s->serial;
> > >      s->fuse[SIFIVE_U_OTP_SERIAL_ADDR + 1] = ~(s->serial);
> > >
> > > +    if (s->blk) {
> > > +        /* Put serial number to backend as well*/
> > > +        uint32_t serial_data;
> > > +        int index = SIFIVE_U_OTP_SERIAL_ADDR;
> > > +
> > > +        serial_data = s->serial;
> > > +        blk_pwrite(s->blk, index * SIFIVE_U_OTP_FUSE_WORD,
> > > +                   &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0);
> > > +
> > > +        serial_data = ~(s->serial);
> > > +        blk_pwrite(s->blk, (index + 1) * SIFIVE_U_OTP_FUSE_WORD,
> > > +                   &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0);
> > > +    }
> > > +
> > >      /* Initialize write-once map */
> > >      memset(s->fuse_wo, 0x00, sizeof(s->fuse_wo));
> > >  }
> > > --
> > > 2.28.0
> >
> > thanks
> > -- PMM