| Message ID | 1508298038-4156-1-git-send-email-sundeep.lkml@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.lkml@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? Also, it might be useful to include the incorrect value in the logged output too, not just what the maximum is. Thanks, Darren. > >-- >2.5.0 > >
Hi Darren, On Wed, Oct 18, 2017 at 2:24 PM, Darren Kenny <darren.kenny@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.lkml@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 >> >> >>
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.kenny@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.lkml@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. > > >> >> 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.
Hi Darren, On Wed, Oct 18, 2017 at 4:04 PM, Darren Kenny <darren.kenny@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.kenny@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.lkml@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. >
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;
Fixed incorrect frame size mask, validated maximum frame size in spi_write and removed dead code. Signed-off-by: Subbaraya Sundeep <sundeep.lkml@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(-)