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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan