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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.09.2019 17:18, Roger Pau Monné  wrote:
> On Mon, Sep 23, 2019 at 04:41:01PM +0200, 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.)
> 
> I'm not specially trilled to switch to GENMASK, but would you be
> willing to change u32 to uint32_t?

Noticing your remark's context, I've done that change already (and
I don't know why I missed doing so right away).

Jan

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