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

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

[Dmytro Prokopchuk1]

MISRA C Rule 10.1 states: "Operands shall not be of an
inappropriate essential type".

Boolean values cannot be directly used in arithmetic operations.
Convert boolean to integer in vPCI header bar index calculation
using the ternary operator to satisfy strict type checking rule.

No functional changes.

Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
---
Test CI pipeline:
https://gitlab.com/xen-project/people/dimaprkp4k/xen/-/pipelines/2545399814
---
 xen/drivers/vpci/header.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index a760d8c32f..2b7f78728d 100644
--- 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;
-- 
2.43.0

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

[Jan Beulich]

(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?

Jan

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

[Stefano Stabellini]

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.

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

[Roger Pau Monné]

On Tue, May 26, 2026 at 03:12:23PM -0700, 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.

Another possibly less controversial option is doing the arithmetic
based on the register value directly: (reg - PCI_BASE_ADDRESS_0) / 4.

TBH that's likely faster than the dereferences done to get the base
address of the bars array.

Thanks, Roger.

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

[Jan Beulich]

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?

Jan

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

[Stefano Stabellini]

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)" ?

Other alternatives could be OK, but also this patch as-is is OK to me.

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

[Jan Beulich]

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.

Jan

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

[Roger Pau Monné]

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.

Thanks, Roger.

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

[Jan Beulich]

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

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4965 bytes --]

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.

I am also fine with Roger's proposal.

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

[Nicola Vetrini]

On 2026-06-03 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.
> 
> I am also fine with Roger's proposal.

I will try to give some context on why the configuration is written in 
its current formulation. There are now about 100 violations on x86, but 
just 2 left on Arm64 with Dmytro's patch applied, from what I can see. 
So, I think the reason why a code change was proposed is because the 
actual changes to make this clean on Arm would be quite limited.

>> >>>>>> 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?

well, you'd need to say that it is fine to use booleans as rhs or lhs of 
just about any operator (+, -, and their compound counterpart, as well 
as assignment come to mind). The deviation justification talks 
intentionally about the value, not the type, to avoid ambiguity, but it 
is not extended to other operators, such as addition, because we deem it 
more likely to have an unintended type mismatch there (for bitwise op 
you are already working with bits, so a boolean-valued operand is less 
surprising). Later, there is another clause for the opposite conversion 
(int-to-bool) for conditionals, which is quite natural. You are right in 
saying that (bool-to-int) conversions would be low-risk for most 
operators, and we could write a deviation with a corresponding 
justification for + or - for instance, but ultimately we decided that it 
would go too far against the spirit of the rule. Perhaps there had been 
a discussion among the maintainers when the rule was discussed for 
adoption, but I do not have notes regarding this specific aspect of the 
rule. If it is agreed upon to use booleans in arithmetic expressions, 
then I can help draft an appropriate deviation.

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

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

[Jan Beulich]

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