Re: [PATCH] vPCI: resolve MISRA R10.1 boolean arithmetic type violation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.06.2026 22:43, Stefano Stabellini wrote:
> On Wed, 3 Jun 2026, Jan Beulich wrote:
>> On 03.06.2026 14:54, Roger Pau Monné wrote:
>>> On Wed, Jun 03, 2026 at 08:04:25AM +0200, Jan Beulich wrote:
>>>> On 03.06.2026 03:41, Stefano Stabellini wrote:
>>>>> On Tue, 2 Jun 2026, Jan Beulich wrote:
>>>>>> On 27.05.2026 00:12, Stefano Stabellini wrote:
>>>>>>> On Fri, 22 May 2026, Jan Beulich wrote:
>>>>>>>> (extending Cc list)
>>>>>>>>
>>>>>>>> On 22.05.2026 08:13, Dmytro Prokopchuk1 wrote:
>>>>>>>>> --- a/xen/drivers/vpci/header.c
>>>>>>>>> +++ b/xen/drivers/vpci/header.c
>>>>>>>>> @@ -586,7 +586,7 @@ static void cf_check bar_write(
>>>>>>>>>          if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
>>>>>>>>>              gprintk(XENLOG_WARNING,
>>>>>>>>>                      "%pp: ignored BAR %zu write while mapped\n",
>>>>>>>>> -                    &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
>>>>>>>>> +                    &pdev->sbdf, bar - pdev->vpci->header.bars + (hi 
>>>>>>>>> ? 1 : 0));
>>>>>>>>>          return;
>>>>>>>>>      }
>>>>>>>>>  
>>>>>>>>> @@ -647,7 +647,7 @@ static void cf_check guest_mem_bar_write(const 
>>>>>>>>> struct pci_dev *pdev,
>>>>>>>>>          if ( guest_addr != bar->guest_addr )
>>>>>>>>>              gprintk(XENLOG_WARNING,
>>>>>>>>>                      "%pp: ignored guest BAR %zu write while 
>>>>>>>>> mapped\n",
>>>>>>>>> -                    &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
>>>>>>>>> +                    &pdev->sbdf, bar - pdev->vpci->header.bars + (hi 
>>>>>>>>> ? 1 : 0));
>>>>>>>>>          return;
>>>>>>>>>      }
>>>>>>>>>      bar->guest_addr = guest_addr;
>>>>>>>>
>>>>>>>> Well. If I'm not mistaken we had discussed situations like this (long 
>>>>>>>> ago).
>>>>>>>> Imo the added verbosity gets in the way of readability. If we 
>>>>>>>> absolutely
>>>>>>>> cannot or don't want to deviate such constructs (of which I expect we 
>>>>>>>> have
>>>>>>>> more), then we ought to consider alternatives (like changing the 
>>>>>>>> variables'
>>>>>>>> types in the case here).
>>>>>>>>
>>>>>>>> As to deviating: rules.rst, according to my reading, says that &, |, 
>>>>>>>> ^, or
>>>>>>>> shifts would be okay to use with a bool operand. What's wrong with also
>>>>>>>> permitting this for other operators?
>>>>>>>
>>>>>>> In my opinion, if we are going to treat bool as its own type, it makes
>>>>>>> sense not to silently mix bools into arithmetic with int types. I also
>>>>>>> do not find this patch less readable -- I actually find it more
>>>>>>> readable, since it makes it more obvious that hi is a bool.
>>>>>>
>>>>>> Well, okay, we have different opinions there. This reply of yours applies
>>>>>> to the first paragraph of my earlier reply though, despite its placement.
>>>>>> What about the aspect mentioned in the second paragraph?
>>>>>
>>>>> You mean "then we ought to consider alternatives (like changing the
>>>>> variables' types in the case here)" ?
>>>>
>>>> That's another option, but not what I meant. I simply don't understand why
>>>> some operators are okay to use with booleans while others aren't. Adding
>>>> (for example) booleans can be quite helpful. Take this example from gas
>>>> sources as example:
>>>>
>>>>       if (overlap.bitfield.imm8
>>>>      + overlap.bitfield.imm8s
>>>>      + overlap.bitfield.imm16
>>>>      + overlap.bitfield.imm32
>>>>      + overlap.bitfield.imm32s
>>>>      + overlap.bitfield.imm64 != 1)
>>>>
>>>> And then see how the added verbosity would hamper readability:
>>>>
>>>>       if ((overlap.bitfield.imm8 ? 1 : 0)
>>>>      + (overlap.bitfield.imm8s ? 1 : 0)
>>>>      + (overlap.bitfield.imm16 ? 1 : 0)
>>>>      + (overlap.bitfield.imm32 ? 1 : 0)
>>>>      + (overlap.bitfield.imm32s ? 1 : 0)
>>>>      + (overlap.bitfield.imm64 ? 1 : 0) != 1)
>>>>
>>>>> Other alternatives could be OK, but also this patch as-is is OK to me.
>>>>
>>>> I'm not going to veto it (not being a maintainer of the code I really
>>>> can't), but as per above the transformation imo is setting a bad example.
>>>
>>> What about getting the BAR index based on the register value, and
>>> hence avoiding the pointer arithmetic plus the boolean type addition?
>>> I think that's clear and doesn't violate any MISRA rules, it would
>>> obviously not settle the discussion about boolean type abuse as
>>> integers, but would be fine to solve the specific issue in vPCI IMO.
>>
>> For the case here - sure, that should be fine. But I specifically
>> wanted to understand (generally) why we are limiting ourselves, as
>> surely other cases are going to show up.
> 
> My view on this is that booleans should be treated as booleans, and we
> should not rely on implicit conversions to int types. I prefer the
> second form because it makes it clear these are booleans. The added
> verbosity helps me see at a glance that these are booleans and should be
> treated as such. The first form is more dangerous because I might forget
> they are booleans, assume they are int types, and use them in an
> operation that would result in undefined or implementation-specific
> behavior.

Can you give a realistic example of such? Default conversion (to int,
with well-known false => 0, true => 1 values) should take care of most
if not all issues. Oddities I can think of are ++ or -- on boolean
variables (perhaps similarly += etc with the lhs being boolean), but
those we could indeed exclude if so desired.

Jan