Re: [XEN PATCH 09/11] x86/msi: address violation of MISRA C Rule 20.7 and coding style

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.03.2024 15:30, Nicola Vetrini wrote:
> On 2024-03-26 11:05, Jan Beulich wrote:
>> On 22.03.2024 17:01, Nicola Vetrini wrote:
>>> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
>>> of macro parameters shall be enclosed in parentheses". Therefore, some
>>> macro definitions should gain additional parentheses to ensure that 
>>> all
>>> current and future users will be safe with respect to expansions that
>>> can possibly alter the semantics of the passed-in macro parameter.
>>>
>>> While at it, the style of these macros has been somewhat uniformed.
>>
>> Hmm, yes, but they then still leave more to be desired. I guess I can 
>> see
>> though why you don't want to e.g. ...
>>
>>> --- a/xen/arch/x86/include/asm/msi.h
>>> +++ b/xen/arch/x86/include/asm/msi.h
>>> @@ -147,33 +147,34 @@ int msi_free_irq(struct msi_desc *entry);
>>>   */
>>>  #define NR_HP_RESERVED_VECTORS     20
>>>
>>> -#define msi_control_reg(base)              (base + PCI_MSI_FLAGS)
>>> -#define msi_lower_address_reg(base)        (base + PCI_MSI_ADDRESS_LO)
>>> -#define msi_upper_address_reg(base)        (base + PCI_MSI_ADDRESS_HI)
>>> -#define msi_data_reg(base, is64bit)        \
>>> -   ( (is64bit == 1) ? base+PCI_MSI_DATA_64 : base+PCI_MSI_DATA_32 )
>>> -#define msi_mask_bits_reg(base, is64bit) \
>>> -   ( (is64bit == 1) ? base+PCI_MSI_MASK_BIT : base+PCI_MSI_MASK_BIT-4)
>>> +#define msi_control_reg(base)        ((base) + PCI_MSI_FLAGS)
>>> +#define msi_lower_address_reg(base)  ((base) + PCI_MSI_ADDRESS_LO)
>>> +#define msi_upper_address_reg(base)  ((base) + PCI_MSI_ADDRESS_HI)
>>> +#define msi_data_reg(base, is64bit) \
>>> +    (((is64bit) == 1) ? (base) + PCI_MSI_DATA_64 : (base) + 
>>> PCI_MSI_DATA_32)
>>> +#define msi_mask_bits_reg(base, is64bit)                \
>>> +    (((is64bit) == 1) ? (base) + PCI_MSI_MASK_BIT       \
>>> +                      : (base) + PCI_MSI_MASK_BIT - 4)
>>
>> ... drop the bogus == 1 in these two, making them similar to ...
>>
>>>  #define msi_pending_bits_reg(base, is64bit) \
>>> -   ((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
>>> -#define msi_disable(control)               control &= ~PCI_MSI_FLAGS_ENABLE
>>> +    ((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
>>
>> ... this.
>>
>>> +#define msi_disable(control) (control) &= ~PCI_MSI_FLAGS_ENABLE
>>
>> Doesn't this need an outer pair of parentheses, too?
>>
> 
> Not necessarily.

And use of msi_disable() in another expression would then likely not do
what's expected?

> I'm in favour of a consistent style to be converted in. 
> This also applies below.

I'm all for consistency; I just don't know what you want to be consistent
with, here.

>>>  #define multi_msi_capable(control) \
>>> -   (1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
>>> +    (1 << (((control) & PCI_MSI_FLAGS_QMASK) >> 1))
>>>  #define multi_msi_enable(control, num) \
>>> -   control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
>>> -#define is_64bit_address(control)  (!!(control & PCI_MSI_FLAGS_64BIT))
>>> -#define is_mask_bit_support(control)       (!!(control & 
>>> PCI_MSI_FLAGS_MASKBIT))
>>> +    (control) |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
>>
>> And this, together with dropping the bogus semicolon?
>>
> 
> I'll drop the semicolon.
> 
>> There also look to be cases where MASK_EXTR() / MASK_INSR() would want 
>> using,
>> in favor of using open-coded numbers.
> 
> Yes, perhaps. However, the risk that I make some mistakes in doing so 
> are quite high, though.

Right, hence how I started my earlier reply. Question is - do we want to
go just half the way here, or would we better tidy things all in one go?
In the latter case I could see about getting to that (whether to take
your patch as basis or instead do it from scratch isn't quite clear to
me at this point).

Jan