Hi Darren, On Wed, Oct 18, 2017 at 4:04 PM, Darren Kenny <darren.ke...@oracle.com> wrote:
> Hi Sundeep, > > > 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 <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 :) >> > > That sounds fine then - definitely would suggest some sort of > comment w.r.t. the fact that we are intentionally preserving these > extra bits - in case anyone looks at this again in the future. > Sure. Thank you, Sundeep > >> >> >>> 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. >