Re: [PATCH 3/3] x86/pci: Fix racy accesses to MSI-X Control register

[David Vrabel <dvrabel@xxxxxxxxxx>]

On 13/12/2022 11:50, Jan Beulich wrote:
> On 13.12.2022 12:34, David Vrabel wrote:
> > On 12/12/2022 17:04, Jan Beulich wrote:
> > > On 10.11.2022 17:59, David Vrabel wrote:
> > > > --- a/xen/arch/x86/include/asm/msi.h
> > > > +++ b/xen/arch/x86/include/asm/msi.h
> > > > @@ -237,7 +237,10 @@ struct arch_msix {
> > > >        int table_refcnt[MAX_MSIX_TABLE_PAGES];
> > > >        int table_idx[MAX_MSIX_TABLE_PAGES];
> > > >        spinlock_t table_lock;
> > > > +    spinlock_t control_lock;
> > > >        bool host_maskall, guest_maskall;
> > > > +    uint16_t host_enable;
> > > If you want to keep this more narrow than "unsigned int", then please
> > > add a BUILD_BUG_ON() against NR_CPUS, so the need to update the field
> > > can be easily noticed (in some perhaps distant future).
> > This is only incremented:
> >
> > - while holding the pci_devs lock, or
> > - while holding a lock for one of the associated IRQs.
> How do the locks held matter here, especially given that - as iirc you say
> in the description - neither lock is held uniformly?
Because the usage here is:

   lock()
   host_enable++
   ...
   host_enable--
   unlock()

> > Since there are at most 4096 MSI-X vectors (and thus at most 4096 IRQs),
> > the highest value this can be (even with >> 4096 PCPUs) is 4097, thus a
> > uint16_t is fine.
> Where's the 4096 coming from as a limit for MSI-X vectors? DYM 2048, which
> is the per-device limit (because the qsize field is 11 bits wide)? If so,
> yes, I think that's indeed restricting how large the number can get.
Yes, I did mean 2048 here.

> I could only think of less neat ones like host_enable_override or
> forced_enable or some such. If I had any good name in mind, I would > certainly 
> have said so.
host_enable_override works for me.

David