[RFC PATCH v5 0/2] Add file-backed and write-once features to OTP

[RFC PATCH v5 0/2] Add file-backed and write-once features to OTP

[Green Wan]

Based RFC v4 to fix:
 - Change the patch order
 - Add write operation to update pdin to fuse[] bit by bit 
 - Fix wrong protection for offset 0x0~0x38
 - Add SIFIVE_U_OTP_PWE_EN definition
 - Refine access macro for fuse[] and fuse_wo[]

Summary of Patches 
 - First patch is to add write opertion to update pdin data to fuse[] bit
   by bit. Add 'write-once' feature to block second write to same bit of
   the OTP memory.

 - Second patch is to add file-backed implementation to allow users to use
   '-drive' to assign an OTP raw image file. OTP image file must be bigger
   than 16K.

       For example, '-drive if=none,format=raw,file=otp.img'

Testing
 - Tested on sifive_u for both qemu and u-boot.

Green Wan (2):
  hw/riscv: sifive_u: Add write operation and write-once protection
  hw/riscv: sifive_u: Add backend drive support

 hw/riscv/sifive_u_otp.c         | 80 ++++++++++++++++++++++++++++++++-
 include/hw/riscv/sifive_u_otp.h |  5 +++
 2 files changed, 84 insertions(+), 1 deletion(-)

-- 
2.17.1

[RFC PATCH v5 1/2] hw/riscv: sifive_u: Add write operation and write-once protection

[Green Wan]

 - Add write operation to update fuse data bit when PWE bit is on.
 - Add array, fuse_wo, to store the 'written' status for all bits
   of OTP to block the write operation.

Signed-off-by: Green Wan <green.wan@sifive.com>
---
 hw/riscv/sifive_u_otp.c         | 30 +++++++++++++++++++++++++++++-
 include/hw/riscv/sifive_u_otp.h |  3 +++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/sifive_u_otp.c b/hw/riscv/sifive_u_otp.c
index f6ecbaa2ca..b8369e9035 100644
--- a/hw/riscv/sifive_u_otp.c
+++ b/hw/riscv/sifive_u_otp.c
@@ -25,6 +25,14 @@
 #include "qemu/module.h"
 #include "hw/riscv/sifive_u_otp.h"
 
+#define WRITTEN_BIT_ON 0x1
+
+#define SET_FUSEARRAY_BIT(map, i, off, bit)    \
+    map[i] = bit ? (map[i] | bit << off) : (map[i] & ~(bit << off))
+
+#define GET_FUSEARRAY_BIT(map, i, off)    \
+    ((map[i] >> off) & 0x1)
+
 static uint64_t sifive_u_otp_read(void *opaque, hwaddr addr, unsigned int size)
 {
     SiFiveUOTPState *s = opaque;
@@ -123,7 +131,24 @@ static void sifive_u_otp_write(void *opaque, hwaddr addr,
         s->ptrim = val32;
         break;
     case SIFIVE_U_OTP_PWE:
-        s->pwe = val32;
+        s->pwe = val32 & SIFIVE_U_OTP_PWE_EN;
+
+        /* PWE is enabled. Ignore PAS=1 (no redundancy cell) */
+        if (s->pwe && !s->pas) {
+            if (GET_FUSEARRAY_BIT(s->fuse_wo, s->pa, s->paio)) {
+                qemu_log_mask(LOG_GUEST_ERROR,
+                              "Error: write idx<%u>, bit<%u>\n",
+                              s->pa, s->paio);
+                break;
+            }
+
+            /* write bit data */
+            SET_FUSEARRAY_BIT(s->fuse, s->pa, s->paio, s->pdin);
+
+            /* update written bit */
+            SET_FUSEARRAY_BIT(s->fuse_wo, s->pa, s->paio, WRITTEN_BIT_ON);
+        }
+
         break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "%s: bad write: addr=0x%" HWADDR_PRIx
@@ -165,6 +190,9 @@ static void sifive_u_otp_reset(DeviceState *dev)
     /* Make a valid content of serial number */
     s->fuse[SIFIVE_U_OTP_SERIAL_ADDR] = s->serial;
     s->fuse[SIFIVE_U_OTP_SERIAL_ADDR + 1] = ~(s->serial);
+
+    /* Initialize write-once map */
+    memset(s->fuse_wo, 0x00, sizeof(s->fuse_wo));
 }
 
 static void sifive_u_otp_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/riscv/sifive_u_otp.h b/include/hw/riscv/sifive_u_otp.h
index 639297564a..4a5a0acf48 100644
--- a/include/hw/riscv/sifive_u_otp.h
+++ b/include/hw/riscv/sifive_u_otp.h
@@ -35,6 +35,8 @@
 #define SIFIVE_U_OTP_PTRIM      0x34
 #define SIFIVE_U_OTP_PWE        0x38
 
+#define SIFIVE_U_OTP_PWE_EN     (1 << 0)
+
 #define SIFIVE_U_OTP_PCE_EN     (1 << 0)
 
 #define SIFIVE_U_OTP_PDSTB_EN   (1 << 0)
@@ -73,6 +75,7 @@ typedef struct SiFiveUOTPState {
     uint32_t ptrim;
     uint32_t pwe;
     uint32_t fuse[SIFIVE_U_OTP_NUM_FUSES];
+    uint32_t fuse_wo[SIFIVE_U_OTP_NUM_FUSES];
     /* config */
     uint32_t serial;
 } SiFiveUOTPState;
-- 
2.17.1

Re: [RFC PATCH v5 1/2] hw/riscv: sifive_u: Add write operation and write-once protection

[Alistair Francis]

On Tue, Sep 1, 2020 at 9:09 AM Green Wan <green.wan@sifive.com> wrote:
>
>  - Add write operation to update fuse data bit when PWE bit is on.
>  - Add array, fuse_wo, to store the 'written' status for all bits
>    of OTP to block the write operation.
>
> Signed-off-by: Green Wan <green.wan@sifive.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/sifive_u_otp.c         | 30 +++++++++++++++++++++++++++++-
>  include/hw/riscv/sifive_u_otp.h |  3 +++
>  2 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/hw/riscv/sifive_u_otp.c b/hw/riscv/sifive_u_otp.c
> index f6ecbaa2ca..b8369e9035 100644
> --- a/hw/riscv/sifive_u_otp.c
> +++ b/hw/riscv/sifive_u_otp.c
> @@ -25,6 +25,14 @@
>  #include "qemu/module.h"
>  #include "hw/riscv/sifive_u_otp.h"
>
> +#define WRITTEN_BIT_ON 0x1
> +
> +#define SET_FUSEARRAY_BIT(map, i, off, bit)    \
> +    map[i] = bit ? (map[i] | bit << off) : (map[i] & ~(bit << off))
> +
> +#define GET_FUSEARRAY_BIT(map, i, off)    \
> +    ((map[i] >> off) & 0x1)
> +
>  static uint64_t sifive_u_otp_read(void *opaque, hwaddr addr, unsigned int size)
>  {
>      SiFiveUOTPState *s = opaque;
> @@ -123,7 +131,24 @@ static void sifive_u_otp_write(void *opaque, hwaddr addr,
>          s->ptrim = val32;
>          break;
>      case SIFIVE_U_OTP_PWE:
> -        s->pwe = val32;
> +        s->pwe = val32 & SIFIVE_U_OTP_PWE_EN;
> +
> +        /* PWE is enabled. Ignore PAS=1 (no redundancy cell) */
> +        if (s->pwe && !s->pas) {
> +            if (GET_FUSEARRAY_BIT(s->fuse_wo, s->pa, s->paio)) {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                              "Error: write idx<%u>, bit<%u>\n",
> +                              s->pa, s->paio);
> +                break;
> +            }
> +
> +            /* write bit data */
> +            SET_FUSEARRAY_BIT(s->fuse, s->pa, s->paio, s->pdin);
> +
> +            /* update written bit */
> +            SET_FUSEARRAY_BIT(s->fuse_wo, s->pa, s->paio, WRITTEN_BIT_ON);
> +        }
> +
>          break;
>      default:
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: bad write: addr=0x%" HWADDR_PRIx
> @@ -165,6 +190,9 @@ static void sifive_u_otp_reset(DeviceState *dev)
>      /* Make a valid content of serial number */
>      s->fuse[SIFIVE_U_OTP_SERIAL_ADDR] = s->serial;
>      s->fuse[SIFIVE_U_OTP_SERIAL_ADDR + 1] = ~(s->serial);
> +
> +    /* Initialize write-once map */
> +    memset(s->fuse_wo, 0x00, sizeof(s->fuse_wo));
>  }
>
>  static void sifive_u_otp_class_init(ObjectClass *klass, void *data)
> diff --git a/include/hw/riscv/sifive_u_otp.h b/include/hw/riscv/sifive_u_otp.h
> index 639297564a..4a5a0acf48 100644
> --- a/include/hw/riscv/sifive_u_otp.h
> +++ b/include/hw/riscv/sifive_u_otp.h
> @@ -35,6 +35,8 @@
>  #define SIFIVE_U_OTP_PTRIM      0x34
>  #define SIFIVE_U_OTP_PWE        0x38
>
> +#define SIFIVE_U_OTP_PWE_EN     (1 << 0)
> +
>  #define SIFIVE_U_OTP_PCE_EN     (1 << 0)
>
>  #define SIFIVE_U_OTP_PDSTB_EN   (1 << 0)
> @@ -73,6 +75,7 @@ typedef struct SiFiveUOTPState {
>      uint32_t ptrim;
>      uint32_t pwe;
>      uint32_t fuse[SIFIVE_U_OTP_NUM_FUSES];
> +    uint32_t fuse_wo[SIFIVE_U_OTP_NUM_FUSES];
>      /* config */
>      uint32_t serial;
>  } SiFiveUOTPState;
> --
> 2.17.1
>
>

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

[Green Wan]

Add '-drive' support to OTP device. Allow users to assign a raw file
as OTP image.

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);
+        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");
+            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.");
+        }
+
+        if (blk_pread(s->blk, 0, s->fuse, filesize) != filesize) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "failed to read the initial flash content");
+            return;
+        }
+    }
 }
 
 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

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

[Alistair Francis]

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.

>
> 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;

> +        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.

> +            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?

> +        }
> +
> +        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.

Is this error fatal?

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
>
>

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

[Green Wan]

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.
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().

> > +        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.

> > +            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.

> > +        }
> > +
> > +        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
> >
> >

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

[Alistair Francis]

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?

> 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

>
> > > +        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
> > >
> > >

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
> > > >
> > > >

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

[Alistair Francis]

On Wed, Sep 30, 2020 at 12:10 AM Green Wan <green.wan@sifive.com> wrote:
>
> 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?

Hey,

Sorry, this email didn't send and I only just noticed.

>
> 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.

Can you document this? I would like to include this in my tests.

Alistair

>
> >
> > > 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
> > > > >
> > > > >

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

[Bin Meng]

Hi Alistair,

On Wed, Oct 14, 2020 at 10:46 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Sep 30, 2020 at 12:10 AM Green Wan <green.wan@sifive.com> wrote:
> >
> > 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?
>
> Hey,
>
> Sorry, this email didn't send and I only just noticed.
>
> >
> > 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.
>
> Can you document this? I would like to include this in my tests.
>

See `QEMU Specific Instructions` in
https://github.com/riscv/opensbi/blob/master/docs/platform/sifive_fu540.md

Regards,
Bin

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

[Alistair Francis]

On Wed, Oct 14, 2020 at 8:02 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Alistair,
>
> On Wed, Oct 14, 2020 at 10:46 PM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Sep 30, 2020 at 12:10 AM Green Wan <green.wan@sifive.com> wrote:
> > >
> > > 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?
> >
> > Hey,
> >
> > Sorry, this email didn't send and I only just noticed.
> >
> > >
> > > 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.
> >
> > Can you document this? I would like to include this in my tests.
> >
>
> See `QEMU Specific Instructions` in
> https://github.com/riscv/opensbi/blob/master/docs/platform/sifive_fu540.md

Hmm... I am missing something. I don't see any details on how you
access the OTP and verify that reads/writes have occured. That link
just seems to document how to build OpenSBI with a U-boot payload.

Does U-Boot run the tests automatically after boot?

Alistair

>
> Regards,
> Bin

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

[Green Wan]

Hi Alistair,

That's correct. Once you configure 'CONFIG_SIFIVE_OTP=y', the uboot
driver performs some read/write during booting.

And I'll also include the test steps by using the 'misc' testing patch
shared by Bin.

Regards,
-
Green

On Thu, Oct 15, 2020 at 2:51 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Oct 14, 2020 at 8:02 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Alistair,
> >
> > On Wed, Oct 14, 2020 at 10:46 PM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Wed, Sep 30, 2020 at 12:10 AM Green Wan <green.wan@sifive.com> wrote:
> > > >
> > > > 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?
> > >
> > > Hey,
> > >
> > > Sorry, this email didn't send and I only just noticed.
> > >
> > > >
> > > > 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.
> > >
> > > Can you document this? I would like to include this in my tests.
> > >
> >
> > See `QEMU Specific Instructions` in
> > https://github.com/riscv/opensbi/blob/master/docs/platform/sifive_fu540.md
>
> Hmm... I am missing something. I don't see any details on how you
> access the OTP and verify that reads/writes have occured. That link
> just seems to document how to build OpenSBI with a U-boot payload.
>
> Does U-Boot run the tests automatically after boot?
>
> Alistair
>
> >
> > Regards,
> > Bin

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

[Bin Meng]

Hi Alistair,

On Thu, Oct 15, 2020 at 2:51 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Oct 14, 2020 at 8:02 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Alistair,
> >
> > On Wed, Oct 14, 2020 at 10:46 PM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Wed, Sep 30, 2020 at 12:10 AM Green Wan <green.wan@sifive.com> wrote:
> > > >
> > > > 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?
> > >
> > > Hey,
> > >
> > > Sorry, this email didn't send and I only just noticed.
> > >
> > > >
> > > > 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.
> > >
> > > Can you document this? I would like to include this in my tests.
> > >
> >
> > See `QEMU Specific Instructions` in
> > https://github.com/riscv/opensbi/blob/master/docs/platform/sifive_fu540.md
>
> Hmm... I am missing something. I don't see any details on how you
> access the OTP and verify that reads/writes have occured. That link
> just seems to document how to build OpenSBI with a U-boot payload.
>
> Does U-Boot run the tests automatically after boot?

So far U-Boot only performs the OTP content read during boot, because
U-Boot needs to determine its MAC address based on the serial number
programmed in the OTP. In normal situations, U-Boot does not perform
any write to OTP. That's why I posted a U-Boot patch to test this QEMU
functionality of OTP write. The U-Boot patch is a shell command to
test the reading / writing of the OTP content. So what you need to do
to test this, is to follow the instructions documented in the OpenSBI
doc, then use my patch to test the OTP write.

Regards,
Bin