1. Patchew
  2. QEMU
  3. View series

[PATCH v2] hw/misc: use extract64 instead of 1 << i

Tigran Sogomonian posted 1 patch 3 months, 1 week ago 
hw/misc/mps2-fpgaio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH v2] hw/misc: use extract64 instead of 1 << i
Posted by Tigran Sogomonian 3 months, 1 week ago 
1 << i is casted to uint64_t while bitwise and with val.
So this value may become 0xffffffff80000000 but only
31th "start" bit is required.
Use the bitfield extract() API instead.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Tigran Sogomonian <tsogomonian@astralinux.ru>
---
 hw/misc/mps2-fpgaio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/mps2-fpgaio.c b/hw/misc/mps2-fpgaio.c
index d07568248d..04a3da5db0 100644
--- a/hw/misc/mps2-fpgaio.c
+++ b/hw/misc/mps2-fpgaio.c
@@ -198,7 +198,7 @@ static void mps2_fpgaio_write(void *opaque, hwaddr offset, uint64_t value,
 
             s->led0 = value & MAKE_64BIT_MASK(0, s->num_leds);
             for (i = 0; i < s->num_leds; i++) {
-                led_set_state(s->led[i], value & (1 << i));
+                led_set_state(s->led[i], extract64(value, i, 1));
             }
         }
         break;
-- 
2.30.2
Re: [PATCH v2] hw/misc: use extract64 instead of 1 << i
Posted by Richard Henderson 3 months, 1 week ago 
On 12/27/24 02:46, Tigran Sogomonian wrote:
> 1 << i is casted to uint64_t while bitwise and with val.
> So this value may become 0xffffffff80000000 but only
> 31th "start" bit is required.
> Use the bitfield extract() API instead.

Again, I < 32.  There is no overflow.  The type of value is irrelevant.


> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Tigran Sogomonian <tsogomonian@astralinux.ru>
> ---
>   hw/misc/mps2-fpgaio.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/misc/mps2-fpgaio.c b/hw/misc/mps2-fpgaio.c
> index d07568248d..04a3da5db0 100644
> --- a/hw/misc/mps2-fpgaio.c
> +++ b/hw/misc/mps2-fpgaio.c
> @@ -198,7 +198,7 @@ static void mps2_fpgaio_write(void *opaque, hwaddr offset, uint64_t value,
>   
>               s->led0 = value & MAKE_64BIT_MASK(0, s->num_leds);
>               for (i = 0; i < s->num_leds; i++) {
> -                led_set_state(s->led[i], value & (1 << i));
> +                led_set_state(s->led[i], extract64(value, i, 1));
>               }
>           }
>           break;
Re: [PATCH v2] hw/misc: use extract64 instead of 1 << i
Posted by Тигран Согомонян 2 months ago 
27/12/24 18:16, Richard Henderson пишет:
> On 12/27/24 02:46, Tigran Sogomonian wrote:
>> 1 << i is casted to uint64_t while bitwise and with val.
>> So this value may become 0xffffffff80000000 but only
>> 31th "start" bit is required.
>> Use the bitfield extract() API instead.
>
> Again, I < 32.  There is no overflow.  The type of value is irrelevant.
>
s->num_leds maximum MPS2FPGAIO_MAX_LEDS (32). For bitwise AND,
the result of shift 1 << i will be converted to uint64_t because
value is of type uint64_t (integer promotion) and for i = 31 instead of
the expected 0b10000000000000000000000000000000  we get 
0b1111111111111111111111111111111100000000000000000000000000000000.
Need to be corrected to value & (1U << i).
>
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Signed-off-by: Tigran Sogomonian <tsogomonian@astralinux.ru>
>> ---
>>   hw/misc/mps2-fpgaio.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/misc/mps2-fpgaio.c b/hw/misc/mps2-fpgaio.c
>> index d07568248d..04a3da5db0 100644
>> --- a/hw/misc/mps2-fpgaio.c
>> +++ b/hw/misc/mps2-fpgaio.c
>> @@ -198,7 +198,7 @@ static void mps2_fpgaio_write(void *opaque, 
>> hwaddr offset, uint64_t value,
>>                 s->led0 = value & MAKE_64BIT_MASK(0, s->num_leds);
>>               for (i = 0; i < s->num_leds; i++) {
>> -                led_set_state(s->led[i], value & (1 << i));
>> +                led_set_state(s->led[i], extract64(value, i, 1));
>>               }
>>           }
>>           break;
>
>
Re: [PATCH v2] hw/misc: use extract64 instead of 1 << i
Posted by Тигран Согомонян 3 weeks, 4 days ago 
04/02/25 15:24, Тигран Согомонян пишет:
> 27/12/24 18:16, Richard Henderson пишет:
>> On 12/27/24 02:46, Tigran Sogomonian wrote:
>>> 1 << i is casted to uint64_t while bitwise and with val.
>>> So this value may become 0xffffffff80000000 but only
>>> 31th "start" bit is required.
>>> Use the bitfield extract() API instead.
>>
>> Again, I < 32.  There is no overflow.  The type of value is irrelevant.
>>
> s->num_leds maximum MPS2FPGAIO_MAX_LEDS (32). For bitwise AND,
> the result of shift 1 << i will be converted to uint64_t because
> value is of type uint64_t (integer promotion) and for i = 31 instead of
> the expected 0b10000000000000000000000000000000  we get 
> 0b1111111111111111111111111111111100000000000000000000000000000000.
> Need to be corrected to value & (1U << i).
>>
>>>
>>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>>
>>> Signed-off-by: Tigran Sogomonian <tsogomonian@astralinux.ru>
>>> ---
>>>   hw/misc/mps2-fpgaio.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/misc/mps2-fpgaio.c b/hw/misc/mps2-fpgaio.c
>>> index d07568248d..04a3da5db0 100644
>>> --- a/hw/misc/mps2-fpgaio.c
>>> +++ b/hw/misc/mps2-fpgaio.c
>>> @@ -198,7 +198,7 @@ static void mps2_fpgaio_write(void *opaque, 
>>> hwaddr offset, uint64_t value,
>>>                 s->led0 = value & MAKE_64BIT_MASK(0, s->num_leds);
>>>               for (i = 0; i < s->num_leds; i++) {
>>> -                led_set_state(s->led[i], value & (1 << i));
>>> +                led_set_state(s->led[i], extract64(value, i, 1));
>>>               }
>>>           }
>>>           break;
>>
>>
>
>
Just a friendly reminder)
Re: [PATCH v2] hw/misc: use extract64 instead of 1 << i
Posted by Тигран Согомонян 2 months ago 
27/12/24 18:16, Richard Henderson пишет:
> On 12/27/24 02:46, Tigran Sogomonian wrote:
>> 1 << i is casted to uint64_t while bitwise and with val.
>> So this value may become 0xffffffff80000000 but only
>> 31th "start" bit is required.
>> Use the bitfield extract() API instead.
>
> Again, I < 32.  There is no overflow.  The type of value is irrelevant.
>
s->num_leds maximum MPS2FPGAIO_MAX_LEDS (32). For bitwise AND, the 
result of shift 1 << i will be converted to uint64_t because value is of 
type uint64_t (integer promotion) and for i = 31 instead of the expected 
0b10000000000000000000000000000000  we get 
0b1111111111111111111111111111111100000000000000000000000000000000. Need 
to be corrected to value & (1U << i).
>
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Signed-off-by: Tigran Sogomonian <tsogomonian@astralinux.ru>
>> ---
>>   hw/misc/mps2-fpgaio.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/misc/mps2-fpgaio.c b/hw/misc/mps2-fpgaio.c
>> index d07568248d..04a3da5db0 100644
>> --- a/hw/misc/mps2-fpgaio.c
>> +++ b/hw/misc/mps2-fpgaio.c
>> @@ -198,7 +198,7 @@ static void mps2_fpgaio_write(void *opaque, 
>> hwaddr offset, uint64_t value,
>>                 s->led0 = value & MAKE_64BIT_MASK(0, s->num_leds);
>>               for (i = 0; i < s->num_leds; i++) {
>> -                led_set_state(s->led[i], value & (1 << i));
>> +                led_set_state(s->led[i], extract64(value, i, 1));
>>               }
>>           }
>>           break;
>
>
Re: [PATCH v2] hw/misc: use extract64 instead of 1 << i
Posted by Alex Bennée 3 months, 1 week ago 
Tigran Sogomonian <tsogomonian@astralinux.ru> writes:

> 1 << i is casted to uint64_t while bitwise and with val.
> So this value may become 0xffffffff80000000 but only
> 31th "start" bit is required.
> Use the bitfield extract() API instead.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Signed-off-by: Tigran Sogomonian <tsogomonian@astralinux.ru>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.