Re: [Qemu-devel] [Qemu devel v2 PATCH] msf2: Remove dead code reported by Coverity

[sundeep subbaraya]

Hi Darren,

On Wed, Oct 18, 2017 at 4:04 PM, Darren Kenny <address@hidden>
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 <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 :)
>>
>
> 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.
>