Re: [Xen-devel] [PATCH v6 3/8] x86/PCI: read maximum MSI vector count early

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.09.2019 15:31, Andrew Cooper wrote:
> On 23/09/2019 15:41, Jan Beulich wrote:
>> On 23.09.2019 16:22, Roger Pau Monné  wrote:
>>> On Thu, Sep 19, 2019 at 03:22:54PM +0200, Jan Beulich wrote:
>>>> Rather than doing this every time we set up interrupts for a device
>>>> anew (and then in several places) fill this invariant field right after
>>>> allocating struct pci_dev.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> LGTM:
>>>
>>> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> Thanks.
>>
>>> Just one nit below.
>>>
>>>> @@ -711,7 +710,7 @@ static int msi_capability_init(struct pc
>>>>  
>>>>          /* All MSIs are unmasked by default, Mask them all */
>>>>          maskbits = pci_conf_read32(dev->sbdf, mpos);
>>>> -        maskbits |= ~(u32)0 >> (32 - maxvec);
>>>> +        maskbits |= ~(u32)0 >> (32 - dev->msi_maxvec);
>>> GENMASK would be slightly easier to parse IMO (here and below).
>> Besides this being an unrelated change, I'm afraid I'm going to
>> object to use of a macro where it's unclear what its parameters
>> mean: Even the example in xen/bitops.h is so confusing that I
>> can't tell whether "h" is meant to be exclusive or inclusive
>> (looks like the latter is intended). To me the two parameters
>> also look reversed - I'd expect "low" first and "high" second.
>> (ISTR having voiced reservations against its introduction, and
>> I'm happy to see that it's used in Arm code only.)
> 
> Furthermore, Linux is having enough problems with it that they were
> considering abolishing it entirely.
> 
> Getting the two numbers the wrong way around gets you a mask of 0.  It
> is a very unsafe macro.

Where, having looked at it some when replying to Roger, it seemed
to me that it would have been quite simple to make the macro
tolerate either order.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel