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
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
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
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
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
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
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
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
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
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
>
© 2016 - 2024 Red Hat, Inc.