[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu devel v2 PATCH] msf2: Remove dead code reported b
From: |
sundeep subbaraya |
Subject: |
Re: [Qemu-devel] [Qemu devel v2 PATCH] msf2: Remove dead code reported by Coverity |
Date: |
Wed, 18 Oct 2017 15:40:07 +0530 |
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 :)
>
> 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
>>
>>
>>