Re: [RFC PATCH v4 2/2] hw/riscv: sifive_u: Add write-once protection

[Bin Meng]

Hi Green,

On Thu, Aug 27, 2020 at 3:47 PM Green Wan <green.wan@sifive.com> wrote:
>
> Add array to store the 'written' status for all bit of OTP to block
> the write operation to the same bit. Ignore the control register
> offset from 0x0 to 0x38 of OTP memory mapping.
>
> Signed-off-by: Green Wan <green.wan@sifive.com>
> ---
>  hw/riscv/sifive_u_otp.c         | 21 +++++++++++++++++++++
>  include/hw/riscv/sifive_u_otp.h |  1 +
>  2 files changed, 22 insertions(+)
>
> diff --git a/hw/riscv/sifive_u_otp.c b/hw/riscv/sifive_u_otp.c
> index aab2220494..e9605b9ae9 100644
> --- a/hw/riscv/sifive_u_otp.c
> +++ b/hw/riscv/sifive_u_otp.c
> @@ -27,6 +27,12 @@
>  #include "sysemu/blockdev.h"
>  #include "sysemu/block-backend.h"
>
> +#define SET_WRITTEN_BIT(map, idx, bit)    \
> +    (map[idx] |= (0x1 << bit))
> +
> +#define GET_WRITTEN_BIT(map, idx, bit)    \
> +    ((map[idx] >> bit) & 0x1)
> +
>  static uint64_t sifive_u_otp_read(void *opaque, hwaddr addr, unsigned int 
> size)
>  {
>      SiFiveUOTPState *s = opaque;
> @@ -135,6 +141,18 @@ static void sifive_u_otp_write(void *opaque, hwaddr addr,
>          s->ptrim = val32;
>          break;
>      case SIFIVE_U_OTP_PWE:
> +        /* Keep written state for data only and PWE is enabled. Ignore PAS=1 
> */
> +        if ((s->pa > SIFIVE_U_OTP_PWE) && (val32 & 0x1) && !s->pas) {

s->pa represents the OTP memory address, and it has nothing to do with
the OTP controller register SIFIVE_U_OTP_PWE.

Please replace 0x1 with a macro like SIFIVE_U_OTP_PWE_EN.

> +            if (GET_WRITTEN_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;
> +            }
> +
> +            SET_WRITTEN_BIT(s->fuse_wo, s->pa, s->paio);

Note this only protects 1 bit, so you should probably change the write
operation to operate on a bit basis instead of 4 bytes
(SIFIVE_U_OTP_FUSE_WORD).

> +        }
> +
>          /* write to backend */
>          if (s->blk) {
>              blk_pwrite(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD, &val32,
> @@ -215,6 +233,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 13d2552e43..49d40a6430 100644
> --- a/include/hw/riscv/sifive_u_otp.h
> +++ b/include/hw/riscv/sifive_u_otp.h
> @@ -74,6 +74,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;
>      BlockBackend *blk;
> --

Regards,
Bin