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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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/98b8c131-b0b9-f46c-5f46-c2136f2e3b4e@xxxxxxx/

> 
>> --- 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.