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