Re: [PATCH v2 1/4] pci: add rwlock to pcidevs_lock machinery

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Volodymyr,

On 2022/7/19 5:15, Volodymyr Babchuk wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>   
>       if ( !use_msi )
>           return -EOPNOTSUPP;
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 938821e593..f93922acc8 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -50,21 +50,74 @@ struct pci_seg {
>       } bus2bridge[MAX_BUSES];
>   };
>   
> -static spinlock_t _pcidevs_lock = SPIN_LOCK_UNLOCKED;
> +static DEFINE_RWLOCK(_pcidevs_rwlock);
> +static DEFINE_PER_CPU(unsigned int, pcidevs_read_cnt);
> +static DEFINE_PER_CPU(unsigned int, pcidevs_write_cnt);
>   
>   void pcidevs_lock(void)
>   {
> -    spin_lock_recursive(&_pcidevs_lock);
> +    pcidevs_write_lock();
>   }
>   
>   void pcidevs_unlock(void)
>   {
> -    spin_unlock_recursive(&_pcidevs_lock);
> +    pcidevs_write_unlock();
>   }
>   
> -bool_t pcidevs_locked(void)
> +bool pcidevs_locked(void)
>   {
> -    return !!spin_is_locked(&_pcidevs_lock);
> +    return pcidevs_write_locked();
> +}
> +
> +void pcidevs_read_lock(void)
> +{
> +    if ( this_cpu(pcidevs_read_cnt)++ == 0 )
> +        read_lock(&_pcidevs_rwlock);
> +}
> +
For my understanding, if pcidevs_read_cnt > 0, pcidevs_read_lock
will be unblocked.I am not sure if this behavior is consistent with
the original lock? According to my understanding, the original spinlock 
should be blocked all the time, if the lock is not acquired. Maybe
I have misunderstanding something, I am not very familiar with PCI
subsystem.

Cheers,
Wei Chen

> +int pcidevs_read_trylock(void)
> +{
> +    int ret = 1;
> +
> +    if ( this_cpu(pcidevs_read_cnt) == 0 )
> +        ret = read_trylock(&_pcidevs_rwlock);
> +    if ( ret )
> +        this_cpu(pcidevs_read_cnt)++;
> +
> +    return ret;
> +}
> +
> +void pcidevs_read_unlock(void)
> +{
> +    ASSERT(this_cpu(pcidevs_read_cnt));
> +
> +    if ( --this_cpu(pcidevs_read_cnt) == 0 )
> +        read_unlock(&_pcidevs_rwlock);
> +}
> +
> +bool pcidevs_read_locked(void)
> +{
> +    /* Write lock implies read lock */
> +    return this_cpu(pcidevs_read_cnt) || this_cpu(pcidevs_write_cnt);
> +}
> +
> +void pcidevs_write_lock(void)
> +{
> +    if ( this_cpu(pcidevs_write_cnt)++ == 0 )
> +        write_lock(&_pcidevs_rwlock);
> +}
> +
> +void pcidevs_write_unlock(void)
> +{
> +    ASSERT(this_cpu(pcidevs_write_cnt));
> +
> +    if ( --this_cpu(pcidevs_write_cnt) == 0 )
> +        write_unlock(&_pcidevs_rwlock);
> +}
> +
> +bool pcidevs_write_locked(void)
> +{
> +    return rw_is_write_locked(&_pcidevs_rwlock);
>   }
>   
>   static struct radix_tree_root pci_segments;
> @@ -581,7 +634,7 @@ struct pci_dev *pci_get_pdev(int seg, int bus, int devfn)
>       struct pci_seg *pseg = get_pseg(seg);
>       struct pci_dev *pdev = NULL;
>   
> -    ASSERT(pcidevs_locked());
> +    ASSERT(pcidevs_read_locked());
>       ASSERT(seg != -1 || bus == -1);
>       ASSERT(bus != -1 || devfn == -1);
>   
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index f34368643c..052b2ddf9f 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -158,7 +158,15 @@ struct pci_dev {
>   
>   void pcidevs_lock(void);
>   void pcidevs_unlock(void);
> -bool_t __must_check pcidevs_locked(void);
> +bool __must_check pcidevs_locked(void);
> +
> +void pcidevs_read_lock(void);
> +void pcidevs_read_unlock(void);
> +bool __must_check pcidevs_read_locked(void);
> +
> +void pcidevs_write_lock(void);
> +void pcidevs_write_unlock(void);
> +bool __must_check pcidevs_write_locked(void);
>   
>   bool_t pci_known_segment(u16 seg);
>   bool_t pci_device_detect(u16 seg, u8 bus, u8 dev, u8 func);