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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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?

Thanks, Roger.

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