Re: [PATCH v13.2 01/14] vpci: use per-domain PCI lock to protect vpci structure

[Jan Beulich <jbeulich@xxxxxxxx>]

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