hw/ssi/mss-spi.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
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;
--
2.5.0
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.
>
© 2016 - 2024 Red Hat, Inc.