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

[Alistair Francis]

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