On 2/19/24 07:10, Jan Beulich wrote:
> On 19.02.2024 12:47, Stewart Hildebrand wrote:
>> @@ -895,6 +891,15 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
>> {
>> unsigned int i;
>>
>> + /*
>> + * Assert that d->pdev_list doesn't change.
>> ASSERT_PDEV_LIST_IS_READ_LOCKED
>> + * is not suitable here because it may allow either pcidevs_lock() or
>> + * d->pci_lock to be held, but here we rely on d->pci_lock being held,
>> not
>> + * pcidevs_lock().
>> + */
>> + ASSERT(rw_is_locked(&msix->pdev->domain->pci_lock));
>> + ASSERT(spin_is_locked(&msix->pdev->vpci->lock));
>
> There's no "d" in sight here, so it's a little odd that "d" is being talked
> about. But I guess people can infer what's meant without too much trouble.
I can s/d->pci_lock/msix->pdev->domain->pci_lock/ for the next rev.
>
>> @@ -313,17 +316,36 @@ void vpci_dump_msi(void)
>> {
>> /*
>> * On error vpci_msix_arch_print will always return
>> without
>> - * holding the lock.
>> + * holding the locks.
>> */
>> printk("unable to print all MSI-X entries: %d\n", rc);
>> - process_pending_softirqs();
>> - continue;
>> + goto pdev_done;
>> }
>> }
>>
>> + /*
>> + * Unlock locks to process pending softirqs. This is
>> + * potentially unsafe, as d->pdev_list can be changed in
>> + * meantime.
>> + */
>> spin_unlock(&pdev->vpci->lock);
>> + read_unlock(&d->pci_lock);
>> + pdev_done:
>> process_pending_softirqs();
>> + if ( !read_trylock(&d->pci_lock) )
>> + {
>> + printk("unable to access other devices for the domain\n");
>> + goto domain_done;
>> + }
>> }
>> + read_unlock(&d->pci_lock);
>> + domain_done:
>> + /*
>> + * We need this label at the end of the loop, but some
>> + * compilers might not be happy about label at the end of the
>> + * compound statement so we adding an empty statement here.
>> + */
>> + ;
>
> As to "some compilers": Are there any which accept a label not followed
> by a statement? Depending on the answer, this comment may be viewed as
> superfluous. Or else I'd ask about wording: Besides a grammar issue I
> also don't view it as appropriate that a comment talks about "adding"
> something when its adjacent code that is meant. That something is there
> when the comment is there, hence respective wording should imo be used.
It seems like hit or miss whether gcc would accept it or not (prior
discussion at [1]). I agree the comment is rather lengthy for what it's
trying to convey. I'd be happy to either remove the comment or reduce
it to:
domain_done:
; /* Empty statement to make some compilers happy */
[1]
https://lore.kernel.org/xen-devel/[email protected]/
>
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -171,6 +171,19 @@ void pcidevs_lock(void);
>> void pcidevs_unlock(void);
>> bool __must_check pcidevs_locked(void);
>>
>> +#ifndef NDEBUG
>> +/*
>> + * Check to ensure there will be no changes to the entries in d->pdev_list
>> (but
>> + * not the contents of each entry).
>> + * This check is not suitable for protecting other state or critical
>> regions.
>> + */
>> +#define ASSERT_PDEV_LIST_IS_READ_LOCKED(d) \
>> + /* NB: d may be evaluated multiple times, or not at all */ \
>> + ASSERT(pcidevs_locked() || ((d) && rw_is_locked(&(d)->pci_lock)))
>
> Is there actually any case where d can be NULL here?
Yes, when called from ns16550 driver, if the driver failed to make the
device RO.
>
>> +#else
>> +#define ASSERT_PDEV_LIST_IS_READ_LOCKED(d) ({ (void)(d); })
>
> Evaluating d here isn't very useful when the assertion expression doesn't
> guarantee single evaluation. Plus even if it needed evaluating, there would
> be no need to use a compiler extension here:
>
> #define ASSERT_PDEV_LIST_IS_READ_LOCKED(d) ((void)(d))
OK, I can make this change.
