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