[v1] ati-vga fixes

[PATCH 4/4] ati-vga: Avoid warning about sign extension

Posted by BALATON Zoltan 3 weeks, 1 day ago

Coverity reports possible sign extension error (CID 1645615) when
reading DEFAULT_SC_BOTTOM_RIGHT register. It cannot happen because the
values are limited when writing the register but change the code to
match other similar registers and to avoid this warning.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/ati.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/display/ati.c b/hw/display/ati.c
index 1a6a5ad4fd..d194f15002 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -513,8 +513,8 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
         val |= s->regs.default_tile << 16;
         break;
     case DEFAULT_SC_BOTTOM_RIGHT:
-        val = (s->regs.default_sc_bottom << 16) |
-              s->regs.default_sc_right;
+        val = s->regs.default_sc_right;
+        val |= s->regs.default_sc_bottom << 16;
         break;
     case SC_TOP:
         val = s->regs.sc_top;
-- 
2.41.3

Re: [PATCH 4/4] ati-vga: Avoid warning about sign extension

Posted by Peter Maydell 2 weeks, 6 days ago

On Mon, 16 Mar 2026 at 00:06, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> Coverity reports possible sign extension error (CID 1645615) when
> reading DEFAULT_SC_BOTTOM_RIGHT register. It cannot happen because the
> values are limited when writing the register but change the code to
> match other similar registers and to avoid this warning.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/display/ati.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index 1a6a5ad4fd..d194f15002 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -513,8 +513,8 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
>          val |= s->regs.default_tile << 16;
>          break;
>      case DEFAULT_SC_BOTTOM_RIGHT:
> -        val = (s->regs.default_sc_bottom << 16) |
> -              s->regs.default_sc_right;
> +        val = s->regs.default_sc_right;
> +        val |= s->regs.default_sc_bottom << 16;

I don't think this is going to make Coverity any happier, because
we're still doing:
 * take a value in a 16-bit unsigned type
 * shift it without a cast, giving a 'signed int' (32-bit)
 * promote that to 64-bits to do the |=, because 'val' is
   64 bits, causing a potential sign extension
The rephrasing of the code doesn't avoid the sign-extension.

Indeed there are other (previously dismissed as false positives
years ago) issues in the coverity DB for the other cases in this
switch where we put together values like this without a cast.

But as you say this is a "can't happen" case for this register,
so I've already marked CID 1645615 as a false positive.
We should do whatever we think is reasonable and readable for
this code.

-- PMM

Re: [PATCH 4/4] ati-vga: Avoid warning about sign extension

Posted by BALATON Zoltan 2 weeks, 6 days ago

On Tue, 17 Mar 2026, Peter Maydell wrote:
> On Mon, 16 Mar 2026 at 00:06, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>
>> Coverity reports possible sign extension error (CID 1645615) when
>> reading DEFAULT_SC_BOTTOM_RIGHT register. It cannot happen because the
>> values are limited when writing the register but change the code to
>> match other similar registers and to avoid this warning.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/display/ati.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/display/ati.c b/hw/display/ati.c
>> index 1a6a5ad4fd..d194f15002 100644
>> --- a/hw/display/ati.c
>> +++ b/hw/display/ati.c
>> @@ -513,8 +513,8 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
>>          val |= s->regs.default_tile << 16;
>>          break;
>>      case DEFAULT_SC_BOTTOM_RIGHT:
>> -        val = (s->regs.default_sc_bottom << 16) |
>> -              s->regs.default_sc_right;
>> +        val = s->regs.default_sc_right;
>> +        val |= s->regs.default_sc_bottom << 16;
>
> I don't think this is going to make Coverity any happier, because
> we're still doing:
> * take a value in a 16-bit unsigned type
> * shift it without a cast, giving a 'signed int' (32-bit)
> * promote that to 64-bits to do the |=, because 'val' is
>   64 bits, causing a potential sign extension
> The rephrasing of the code doesn't avoid the sign-extension.

OK then I'd additionally change val to uint32_t which should avoid it 
then. Would that work?

> Indeed there are other (previously dismissed as false positives
> years ago) issues in the coverity DB for the other cases in this
> switch where we put together values like this without a cast.

I'll try to have a look but I'm new to Coverity and have not checked it 
yet.

> But as you say this is a "can't happen" case for this register,
> so I've already marked CID 1645615 as a false positive.
> We should do whatever we think is reasonable and readable for
> this code.

At least this matches what other places in this device do so good for 
consistency and we generally avoid casts in QEMU, I think changing val to 
uint32_t would help all of these as it does not have to be 64-bit, it's 
just what the ops callback interface has.

Regards,
BALATON Zoltan

Re: [PATCH 4/4] ati-vga: Avoid warning about sign extension

Posted by Peter Maydell 2 weeks, 6 days ago

On Tue, 17 Mar 2026 at 10:34, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Tue, 17 Mar 2026, Peter Maydell wrote:
> > On Mon, 16 Mar 2026 at 00:06, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> >>
> >> Coverity reports possible sign extension error (CID 1645615) when
> >> reading DEFAULT_SC_BOTTOM_RIGHT register. It cannot happen because the
> >> values are limited when writing the register but change the code to
> >> match other similar registers and to avoid this warning.
> >>
> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >> ---
> >>  hw/display/ati.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/display/ati.c b/hw/display/ati.c
> >> index 1a6a5ad4fd..d194f15002 100644
> >> --- a/hw/display/ati.c
> >> +++ b/hw/display/ati.c
> >> @@ -513,8 +513,8 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
> >>          val |= s->regs.default_tile << 16;
> >>          break;
> >>      case DEFAULT_SC_BOTTOM_RIGHT:
> >> -        val = (s->regs.default_sc_bottom << 16) |
> >> -              s->regs.default_sc_right;
> >> +        val = s->regs.default_sc_right;
> >> +        val |= s->regs.default_sc_bottom << 16;
> >
> > I don't think this is going to make Coverity any happier, because
> > we're still doing:
> > * take a value in a 16-bit unsigned type
> > * shift it without a cast, giving a 'signed int' (32-bit)
> > * promote that to 64-bits to do the |=, because 'val' is
> >   64 bits, causing a potential sign extension
> > The rephrasing of the code doesn't avoid the sign-extension.
>
> OK then I'd additionally change val to uint32_t which should avoid it
> then. Would that work?

Yes, I think that using uint32_t for val would silence the
coverity warnings.

thanks
-- PMM

Re: [PATCH 4/4] ati-vga: Avoid warning about sign extension

Posted by Philippe Mathieu-Daudé 2 weeks, 6 days ago

On 16/3/26 01:05, BALATON Zoltan wrote:
> Coverity reports possible sign extension error (CID 1645615) when
> reading DEFAULT_SC_BOTTOM_RIGHT register. It cannot happen because the
> values are limited when writing the register but change the code to
> match other similar registers and to avoid this warning.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/display/ati.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>