Hi Darren, On Wed, Oct 18, 2017 at 2:24 PM, Darren Kenny <darren.ke...@oracle.com> 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 <sundeep.l...@gmail.com> >> --- >> 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 :) > > 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. Thanks, Sundeep > Thanks, > > Darren. > > >> -- >> 2.5.0 >> >> >>