[PATCH] hw/misc/sifive_u_otp: handling the fails of blk_pread and blk_pwrite

[PATCH] hw/misc/sifive_u_otp: handling the fails of blk_pread and blk_pwrite

[Green Wan]

Fix code coverage issues by checking return value and handling fail case
of blk_pread() and blk_pwrite(). Return default value 0xff if read fails.

Signed-off-by: Green Wan <green.wan@sifive.com>
---
 hw/misc/sifive_u_otp.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
index 60066375ab..4314727d0d 100644
--- a/hw/misc/sifive_u_otp.c
+++ b/hw/misc/sifive_u_otp.c
@@ -62,8 +62,13 @@ static uint64_t sifive_u_otp_read(void *opaque, hwaddr addr, unsigned int size)
             if (s->blk) {
                 int32_t buf;
 
-                blk_pread(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD, &buf,
-                          SIFIVE_U_OTP_FUSE_WORD);
+                if (blk_pread(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD, &buf,
+                              SIFIVE_U_OTP_FUSE_WORD) < 0) {
+                    qemu_log_mask(LOG_GUEST_ERROR,
+                                  "read error index<%d>\n", s->pa);
+                    return 0xff;
+                }
+
                 return buf;
             }
 
@@ -160,8 +165,12 @@ static void sifive_u_otp_write(void *opaque, hwaddr addr,
 
             /* 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);
+                if (blk_pwrite(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD,
+                               &s->fuse[s->pa], SIFIVE_U_OTP_FUSE_WORD,
+                               0) < 0) {
+                    qemu_log_mask(LOG_GUEST_ERROR,
+                                  "write error index<%d>\n", s->pa);
+                }
             }
 
             /* update written bit */
@@ -248,12 +257,18 @@ static void sifive_u_otp_reset(DeviceState *dev)
         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);
+        if (blk_pwrite(s->blk, index * SIFIVE_U_OTP_FUSE_WORD,
+                       &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0) < 0) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "write error index<%d>\n", index);
+        }
 
         serial_data = ~(s->serial);
-        blk_pwrite(s->blk, (index + 1) * SIFIVE_U_OTP_FUSE_WORD,
-                   &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0);
+        if (blk_pwrite(s->blk, (index + 1) * SIFIVE_U_OTP_FUSE_WORD,
+                       &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0) < 0) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "write error index<%d>\n", index + 1);
+        }
     }
 
     /* Initialize write-once map */
-- 
2.17.1

Re: [PATCH] hw/misc/sifive_u_otp: handling the fails of blk_pread and blk_pwrite

[Peter Maydell]

Ping! This patch was trying to fix a Coverity issue (CID 1435959,
1435960, 1435961) -- is anybody planning to review it?

(I'm not entirely sure 'guest error' is the right warning category,
but I don't know the specifics of this device.)

thanks
-- PMM

On Wed, 4 Nov 2020 at 09:29, Green Wan <green.wan@sifive.com> wrote:
>
> Fix code coverage issues by checking return value and handling fail case
> of blk_pread() and blk_pwrite(). Return default value 0xff if read fails.
>
> Signed-off-by: Green Wan <green.wan@sifive.com>
> ---
>  hw/misc/sifive_u_otp.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
> index 60066375ab..4314727d0d 100644
> --- a/hw/misc/sifive_u_otp.c
> +++ b/hw/misc/sifive_u_otp.c
> @@ -62,8 +62,13 @@ static uint64_t sifive_u_otp_read(void *opaque, hwaddr addr, unsigned int size)
>              if (s->blk) {
>                  int32_t buf;
>
> -                blk_pread(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD, &buf,
> -                          SIFIVE_U_OTP_FUSE_WORD);
> +                if (blk_pread(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD, &buf,
> +                              SIFIVE_U_OTP_FUSE_WORD) < 0) {
> +                    qemu_log_mask(LOG_GUEST_ERROR,
> +                                  "read error index<%d>\n", s->pa);
> +                    return 0xff;
> +                }
> +
>                  return buf;
>              }
>
> @@ -160,8 +165,12 @@ static void sifive_u_otp_write(void *opaque, hwaddr addr,
>
>              /* 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);
> +                if (blk_pwrite(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD,
> +                               &s->fuse[s->pa], SIFIVE_U_OTP_FUSE_WORD,
> +                               0) < 0) {
> +                    qemu_log_mask(LOG_GUEST_ERROR,
> +                                  "write error index<%d>\n", s->pa);
> +                }
>              }
>
>              /* update written bit */
> @@ -248,12 +257,18 @@ static void sifive_u_otp_reset(DeviceState *dev)
>          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);
> +        if (blk_pwrite(s->blk, index * SIFIVE_U_OTP_FUSE_WORD,
> +                       &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0) < 0) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "write error index<%d>\n", index);
> +        }
>
>          serial_data = ~(s->serial);
> -        blk_pwrite(s->blk, (index + 1) * SIFIVE_U_OTP_FUSE_WORD,
> -                   &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0);
> +        if (blk_pwrite(s->blk, (index + 1) * SIFIVE_U_OTP_FUSE_WORD,
> +                       &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0) < 0) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "write error index<%d>\n", index + 1);
> +        }
>      }
>
>      /* Initialize write-once map */
> --
> 2.17.1

Re: [PATCH] hw/misc/sifive_u_otp: handling the fails of blk_pread and blk_pwrite

[Bin Meng]

On Fri, Jan 15, 2021 at 7:50 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Ping! This patch was trying to fix a Coverity issue (CID 1435959,
> 1435960, 1435961) -- is anybody planning to review it?
>
> (I'm not entirely sure 'guest error' is the right warning category,
> but I don't know the specifics of this device.)
>

I think we should just use 'printf' instead of log a "guest error"
because the guest does nothing wrong.

Regards,
Bin

Re: [PATCH] hw/misc/sifive_u_otp: handling the fails of blk_pread and blk_pwrite

[Peter Maydell]

On Fri, 15 Jan 2021 at 13:33, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, Jan 15, 2021 at 7:50 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > Ping! This patch was trying to fix a Coverity issue (CID 1435959,
> > 1435960, 1435961) -- is anybody planning to review it?
> >
> > (I'm not entirely sure 'guest error' is the right warning category,
> > but I don't know the specifics of this device.)
> >
>
> I think we should just use 'printf' instead of log a "guest error"
> because the guest does nothing wrong.

printf is definitely the wrong thing... you need to either report
the error back to the guest if the interface the guest is using
has a facility for reporting read/write failures, or log or report
it to the user using one of our APIs for that.

thanks
-- PMM

Re: [PATCH] hw/misc/sifive_u_otp: handling the fails of blk_pread and blk_pwrite

[Bin Meng]

On Fri, Jan 15, 2021 at 9:55 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 15 Jan 2021 at 13:33, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Fri, Jan 15, 2021 at 7:50 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > Ping! This patch was trying to fix a Coverity issue (CID 1435959,
> > > 1435960, 1435961) -- is anybody planning to review it?
> > >
> > > (I'm not entirely sure 'guest error' is the right warning category,
> > > but I don't know the specifics of this device.)
> > >
> >
> > I think we should just use 'printf' instead of log a "guest error"
> > because the guest does nothing wrong.
>
> printf is definitely the wrong thing... you need to either report
> the error back to the guest if the interface the guest is using
> has a facility for reporting read/write failures, or log or report
> it to the user using one of our APIs for that.

It seems the hardware does not have a mechanism to report to the
software when hardware cannot fulfill the task requested by software.

I checked all existence of block_pwrite() callers. It looks like this
is not handled consistently. Some indeed call printf(), some call
error_setg_errno(), some call fprintf(stderr), some call qemu_log()
...

Regards,
Bin

Re: [PATCH] hw/misc/sifive_u_otp: handling the fails of blk_pread and blk_pwrite

[Alistair Francis]

On Wed, Nov 4, 2020 at 1:29 AM Green Wan <green.wan@sifive.com> wrote:
>
> Fix code coverage issues by checking return value and handling fail case
> of blk_pread() and blk_pwrite(). Return default value 0xff if read fails.
>
> Signed-off-by: Green Wan <green.wan@sifive.com>

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

Alistair

> ---
>  hw/misc/sifive_u_otp.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
> index 60066375ab..4314727d0d 100644
> --- a/hw/misc/sifive_u_otp.c
> +++ b/hw/misc/sifive_u_otp.c
> @@ -62,8 +62,13 @@ static uint64_t sifive_u_otp_read(void *opaque, hwaddr addr, unsigned int size)
>              if (s->blk) {
>                  int32_t buf;
>
> -                blk_pread(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD, &buf,
> -                          SIFIVE_U_OTP_FUSE_WORD);
> +                if (blk_pread(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD, &buf,
> +                              SIFIVE_U_OTP_FUSE_WORD) < 0) {
> +                    qemu_log_mask(LOG_GUEST_ERROR,
> +                                  "read error index<%d>\n", s->pa);
> +                    return 0xff;
> +                }
> +
>                  return buf;
>              }
>
> @@ -160,8 +165,12 @@ static void sifive_u_otp_write(void *opaque, hwaddr addr,
>
>              /* 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);
> +                if (blk_pwrite(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD,
> +                               &s->fuse[s->pa], SIFIVE_U_OTP_FUSE_WORD,
> +                               0) < 0) {
> +                    qemu_log_mask(LOG_GUEST_ERROR,
> +                                  "write error index<%d>\n", s->pa);
> +                }
>              }
>
>              /* update written bit */
> @@ -248,12 +257,18 @@ static void sifive_u_otp_reset(DeviceState *dev)
>          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);
> +        if (blk_pwrite(s->blk, index * SIFIVE_U_OTP_FUSE_WORD,
> +                       &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0) < 0) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "write error index<%d>\n", index);
> +        }
>
>          serial_data = ~(s->serial);
> -        blk_pwrite(s->blk, (index + 1) * SIFIVE_U_OTP_FUSE_WORD,
> -                   &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0);
> +        if (blk_pwrite(s->blk, (index + 1) * SIFIVE_U_OTP_FUSE_WORD,
> +                       &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0) < 0) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "write error index<%d>\n", index + 1);
> +        }
>      }
>
>      /* Initialize write-once map */
> --
> 2.17.1
>

Re: [PATCH] hw/misc/sifive_u_otp: handling the fails of blk_pread and blk_pwrite

[Alistair Francis]

On Fri, Jan 15, 2021 at 6:09 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, Jan 15, 2021 at 9:55 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Fri, 15 Jan 2021 at 13:33, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > On Fri, Jan 15, 2021 at 7:50 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > > >
> > > > Ping! This patch was trying to fix a Coverity issue (CID 1435959,
> > > > 1435960, 1435961) -- is anybody planning to review it?
> > > >
> > > > (I'm not entirely sure 'guest error' is the right warning category,
> > > > but I don't know the specifics of this device.)
> > > >
> > >
> > > I think we should just use 'printf' instead of log a "guest error"
> > > because the guest does nothing wrong.
> >
> > printf is definitely the wrong thing... you need to either report
> > the error back to the guest if the interface the guest is using
> > has a facility for reporting read/write failures, or log or report
> > it to the user using one of our APIs for that.
>
> It seems the hardware does not have a mechanism to report to the
> software when hardware cannot fulfill the task requested by software.
>
> I checked all existence of block_pwrite() callers. It looks like this
> is not handled consistently. Some indeed call printf(), some call
> error_setg_errno(), some call fprintf(stderr), some call qemu_log()
> ...

Logging a guest error seems like the best bet, I'm not really sure
what else we would do.

Alistair

>
> Regards,
> Bin

Re: [PATCH] hw/misc/sifive_u_otp: handling the fails of blk_pread and blk_pwrite

[Alistair Francis]

On Fri, Jan 15, 2021 at 3:50 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Ping! This patch was trying to fix a Coverity issue (CID 1435959,
> 1435960, 1435961) -- is anybody planning to review it?
>
> (I'm not entirely sure 'guest error' is the right warning category,
> but I don't know the specifics of this device.)

Thanks for the ping, this feel through the cracks somehow.

Applied to riscv-to-apply.next

Alistair

>
> thanks
> -- PMM
>
> On Wed, 4 Nov 2020 at 09:29, Green Wan <green.wan@sifive.com> wrote:
> >
> > Fix code coverage issues by checking return value and handling fail case
> > of blk_pread() and blk_pwrite(). Return default value 0xff if read fails.
> >
> > Signed-off-by: Green Wan <green.wan@sifive.com>
> > ---
> >  hw/misc/sifive_u_otp.c | 31 +++++++++++++++++++++++--------
> >  1 file changed, 23 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
> > index 60066375ab..4314727d0d 100644
> > --- a/hw/misc/sifive_u_otp.c
> > +++ b/hw/misc/sifive_u_otp.c
> > @@ -62,8 +62,13 @@ static uint64_t sifive_u_otp_read(void *opaque, hwaddr addr, unsigned int size)
> >              if (s->blk) {
> >                  int32_t buf;
> >
> > -                blk_pread(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD, &buf,
> > -                          SIFIVE_U_OTP_FUSE_WORD);
> > +                if (blk_pread(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD, &buf,
> > +                              SIFIVE_U_OTP_FUSE_WORD) < 0) {
> > +                    qemu_log_mask(LOG_GUEST_ERROR,
> > +                                  "read error index<%d>\n", s->pa);
> > +                    return 0xff;
> > +                }
> > +
> >                  return buf;
> >              }
> >
> > @@ -160,8 +165,12 @@ static void sifive_u_otp_write(void *opaque, hwaddr addr,
> >
> >              /* 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);
> > +                if (blk_pwrite(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD,
> > +                               &s->fuse[s->pa], SIFIVE_U_OTP_FUSE_WORD,
> > +                               0) < 0) {
> > +                    qemu_log_mask(LOG_GUEST_ERROR,
> > +                                  "write error index<%d>\n", s->pa);
> > +                }
> >              }
> >
> >              /* update written bit */
> > @@ -248,12 +257,18 @@ static void sifive_u_otp_reset(DeviceState *dev)
> >          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);
> > +        if (blk_pwrite(s->blk, index * SIFIVE_U_OTP_FUSE_WORD,
> > +                       &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0) < 0) {
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "write error index<%d>\n", index);
> > +        }
> >
> >          serial_data = ~(s->serial);
> > -        blk_pwrite(s->blk, (index + 1) * SIFIVE_U_OTP_FUSE_WORD,
> > -                   &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0);
> > +        if (blk_pwrite(s->blk, (index + 1) * SIFIVE_U_OTP_FUSE_WORD,
> > +                       &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0) < 0) {
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "write error index<%d>\n", index + 1);
> > +        }
> >      }
> >
> >      /* Initialize write-once map */
> > --
> > 2.17.1

Re: [PATCH] hw/misc/sifive_u_otp: handling the fails of blk_pread and blk_pwrite

[Peter Maydell]

On Fri, 15 Jan 2021 at 21:43, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Fri, Jan 15, 2021 at 6:09 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Fri, Jan 15, 2021 at 9:55 PM Peter Maydell <peter.maydell@linaro.org> wrote
> > > printf is definitely the wrong thing... you need to either report
> > > the error back to the guest if the interface the guest is using
> > > has a facility for reporting read/write failures, or log or report
> > > it to the user using one of our APIs for that.
> >
> > It seems the hardware does not have a mechanism to report to the
> > software when hardware cannot fulfill the task requested by software.
> >
> > I checked all existence of block_pwrite() callers. It looks like this
> > is not handled consistently. Some indeed call printf(), some call
> > error_setg_errno(), some call fprintf(stderr), some call qemu_log()
> > ...
>
> Logging a guest error seems like the best bet, I'm not really sure
> what else we would do.

Looking at the other options, I think error_report() of some kind is
probably the best bet here.

thanks
-- PMM

Re: [PATCH] hw/misc/sifive_u_otp: handling the fails of blk_pread and blk_pwrite

[Alistair Francis]

On Fri, Jan 15, 2021 at 2:17 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 15 Jan 2021 at 21:43, Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Fri, Jan 15, 2021 at 6:09 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > On Fri, Jan 15, 2021 at 9:55 PM Peter Maydell <peter.maydell@linaro.org> wrote
> > > > printf is definitely the wrong thing... you need to either report
> > > > the error back to the guest if the interface the guest is using
> > > > has a facility for reporting read/write failures, or log or report
> > > > it to the user using one of our APIs for that.
> > >
> > > It seems the hardware does not have a mechanism to report to the
> > > software when hardware cannot fulfill the task requested by software.
> > >
> > > I checked all existence of block_pwrite() callers. It looks like this
> > > is not handled consistently. Some indeed call printf(), some call
> > > error_setg_errno(), some call fprintf(stderr), some call qemu_log()
> > > ...
> >
> > Logging a guest error seems like the best bet, I'm not really sure
> > what else we would do.
>
> Looking at the other options, I think error_report() of some kind is
> probably the best bet here.

Ok

@Green Wan do you mind sending a new version?

Alistair

>
> thanks
> -- PMM