[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 3/3] x86/pci: Fix racy accesses to MSI-X Control register
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.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. +static void msix_update_unlock(struct pci_dev *dev, unsigned int pos, uint16_t control) +{ + uint16_t new_control; + unsigned long flags; + + spin_lock_irqsave(&dev->msix->control_lock, flags); + + dev->msix->host_enable--; + + new_control = control & ~(PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL); + + if ( dev->msix->host_enable || dev->msix->guest_enable ) + new_control |= PCI_MSIX_FLAGS_ENABLE; + if ( dev->msix->host_maskall || dev->msix->guest_maskall || dev->msix->host_enable ) + new_control |= PCI_MSIX_FLAGS_MASKALL;In particular this use of "host_enable" suggests the field wants to be named differently: It makes no sense to derive MASKALL from ENABLE without it being clear (from the name) that the field is an init-time override only. I think the name as-is makes sense. It is analogous to the host_maskall and complements guest_enable. I can't think of a better name, so what name do you suggest. David
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |