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 <jbeul...@suse.com> >> LGTM: >> >> Reviewed-by: Roger Pau Monné <roger....@citrix.com> > 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. -1 to any use in Xen, even in the ARM code. (I realise this is not my call, but this clearly expresses my opinion about it.) ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
