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?
> 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.
>>> +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.
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.
Jan
