|
From: | sundeep subbaraya |
Subject: | Re: [Qemu-arm] [Qemu-devel] [Qemu devel v2 PATCH] msf2: Remove dead code reported by Coverity |
Date: | Sun, 22 Oct 2017 18:10:43 +0530 |
Hi Sundeep,That sounds fine then - definitely would suggest some sort of
On Wed, Oct 18, 2017 at 10:10:07AM +0000, sundeep subbaraya wrote:
Hi Darren,
On Wed, Oct 18, 2017 at 2:24 PM, Darren Kenny <address@hidden>
wrote:
On Wed, Oct 18, 2017 at 03:40:38AM +0000, Subbaraya Sundeep wrote:
Fixed incorrect frame size mask, validated maximum frame
size in spi_write and removed dead code.
Signed-off-by: Subbaraya Sundeep <address@hidden>
---
v2:
else if -> else in set_fifodepth
log guest error when frame size is more than 32
hw/ssi/mss-spi.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/hw/ssi/mss-spi.c b/hw/ssi/mss-spi.c
index 5a8e308..7fef2c3 100644
--- a/hw/ssi/mss-spi.c
+++ b/hw/ssi/mss-spi.c
@@ -76,9 +76,10 @@
#define C_BIGFIFO (1 << 29)
#define C_RESET (1 << 31)
-#define FRAMESZ_MASK 0x1F
+#define FRAMESZ_MASK 0x3F
#define FMCOUNT_MASK 0x00FFFF00
#define FMCOUNT_SHIFT 8
+#define FRAMESZ_MAX 32
static void txfifo_reset(MSSSpiState *s)
{
@@ -104,10 +105,8 @@ static void set_fifodepth(MSSSpiState *s)
s->fifo_depth = 32;
} else if (size <= 16) {
s->fifo_depth = 16;
- } else if (size <= 32) {
- s->fifo_depth = 8;
} else {
- s->fifo_depth = 4;
+ s->fifo_depth = 8;
}
}
@@ -301,6 +300,11 @@ static void spi_write(void *opaque, hwaddr addr,
if (s->enabled) {
break;
}
+ if ((value & FRAMESZ_MASK) > FRAMESZ_MAX) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: Maximum frame size is
%d\n",
+ __func__, FRAMESZ_MAX);
+ break;
+ }
s->regs[R_SPI_DFSIZE] = value;
break;
This test, and subsequent use of value appear to be out of sorts -
in that while it is testing for the value by ANDing it with
FRAMESZ_MASK, it is subsequently using the value without that mask
applied to it, which still has the potential to be larger than
FRAMESZ_MASK if it contains a value larger than 0x3F.
Is that the expected behaviour? If so, maybe include a comment on
it?
As per docs regarding [31:6]:
Software should not rely on the value of a reserved bit. To provide
compatibility with future products, the value of a reserved bit should be
preserved across a read-modify-write operation.
Hence we do not care about [31:6] and validate only [5:0] for size
during write. When reading size we AND with FRAMESZ_MASK. In other
words we let [31:6] bits like scratch bits where guest can read and
write. I am really not sure how hardware behaves if [5:0] is
greater than 32 hence guest error and write wont happen. If this is
not right we can discuss :)
comment w.r.t. the fact that we are intentionally preserving these
extra bits - in case anyone looks at this again in the future.
Also, it might be useful to include the incorrect value in the
logged output too, not just what the maximum is.
Ok I will change.
OK
Thanks,
Darren.
[Prev in Thread] | Current Thread | [Next in Thread] |