Re: [PATCH 4/4] vpci: move lock outside of struct vpci

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Roger!

On 02.02.22 11:45, Roger Pau Monné wrote:
> On Tue, Feb 01, 2022 at 06:25:08PM +0200, Oleksandr Andrushchenko wrote:
>> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
>>
>> This way the lock can be used to check whether vpci is present, and
>> removal can be performed while holding the lock, in order to make
>> sure there are no accesses to the contents of the vpci struct.
>> Previously removal could race with vpci_read for example, since the
>> lock was dropped prior to freeing pdev->vpci.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>> ---
>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>> Cc: Julien Grall <julien@xxxxxxx>
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> ---
>> New in v5 of this series: this is an updated version of the patch published 
>> at
>> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/20180717094830.54806-2-roger.pau@xxxxxxxxxx/__;!!GF_29dbcQIUBPA!ilhEn2M-44dKY8rsVO7qGmUSY22vSHwNaGJNUqhwvr-V2kdb-FDYL6f39FS8pKJm3q8HnOzyuA$
>>  [lore[.]kernel[.]org]
>>
>> Changes since v5:
>>   - do not split code into vpci_remove_device_handlers_locked yet
>>   - move INIT_LIST_HEAD outside the locked region (Jan)
>>   - stripped out locking optimizations for vpci_{read|write} into a
>>     dedicated patch
>> Changes since v2:
>>   - fixed pdev->vpci = xzalloc(struct vpci); under spin_lock (Jan)
>> Changes since v1:
>>   - Assert that vpci_lock is locked in vpci_remove_device_locked.
>>   - Remove double newline.
>>   - Shrink critical section in vpci_{read/write}.
>> ---
>>   tools/tests/vpci/emul.h       |  5 ++-
>>   tools/tests/vpci/main.c       |  4 +--
>>   xen/arch/x86/hvm/vmsi.c       |  8 ++---
>>   xen/drivers/passthrough/pci.c |  1 +
>>   xen/drivers/vpci/header.c     | 21 ++++++++----
>>   xen/drivers/vpci/msi.c        | 11 ++++--
>>   xen/drivers/vpci/msix.c       |  8 ++---
>>   xen/drivers/vpci/vpci.c       | 63 ++++++++++++++++++++++-------------
>>   xen/include/xen/pci.h         |  1 +
>>   xen/include/xen/vpci.h        |  3 +-
>>   10 files changed, 78 insertions(+), 47 deletions(-)
>>
>> diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h
>> index 2e1d3057c9d8..d018fb5eef21 100644
>> --- a/tools/tests/vpci/emul.h
>> +++ b/tools/tests/vpci/emul.h
>> @@ -44,6 +44,7 @@ struct domain {
>>   };
>>   
>>   struct pci_dev {
>> +    bool vpci_lock;
>>       struct vpci *vpci;
>>   };
>>   
>> @@ -53,10 +54,8 @@ struct vcpu
>>   };
>>   
>>   extern const struct vcpu *current;
>> -extern const struct pci_dev test_pdev;
>> +extern struct pci_dev test_pdev;
>>   
>> -typedef bool spinlock_t;
>> -#define spin_lock_init(l) (*(l) = false)
>>   #define spin_lock(l) (*(l) = true)
>>   #define spin_unlock(l) (*(l) = false)
>>   
>> diff --git a/tools/tests/vpci/main.c b/tools/tests/vpci/main.c
>> index b9a0a6006bb9..26c95b08b6b1 100644
>> --- a/tools/tests/vpci/main.c
>> +++ b/tools/tests/vpci/main.c
>> @@ -23,7 +23,8 @@ static struct vpci vpci;
>>   
>>   const static struct domain d;
>>   
>> -const struct pci_dev test_pdev = {
>> +struct pci_dev test_pdev = {
>> +    .vpci_lock = false,
> Nit: vpci_lock will already be initialized to false by default, so
> this is redundant.
Will remove
>
>>       .vpci = &vpci,
>>   };
>>   
>> @@ -158,7 +159,6 @@ main(int argc, char **argv)
>>       int rc;
>>   
>>       INIT_LIST_HEAD(&vpci.handlers);
>> -    spin_lock_init(&vpci.lock);
>>   
>>       VPCI_ADD_REG(vpci_read32, vpci_write32, 0, 4, r0);
>>       VPCI_READ_CHECK(0, 4, r0);
>> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
>> index 13e2a190b439..1f7a37f78264 100644
>> --- a/xen/arch/x86/hvm/vmsi.c
>> +++ b/xen/arch/x86/hvm/vmsi.c
>> @@ -910,14 +910,14 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
>>           {
>>               struct pci_dev *pdev = msix->pdev;
>>   
>> -            spin_unlock(&msix->pdev->vpci->lock);
>> +            spin_unlock(&msix->pdev->vpci_lock);
>>               process_pending_softirqs();
>>               /* NB: we assume that pdev cannot go away for an alive domain. 
>> */
>> -            if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) )
>> +            if ( !spin_trylock(&pdev->vpci_lock) )
>>                   return -EBUSY;
>> -            if ( pdev->vpci->msix != msix )
>> +            if ( !pdev->vpci || pdev->vpci->msix != msix )
>>               {
>> -                spin_unlock(&pdev->vpci->lock);
>> +                spin_unlock(&pdev->vpci_lock);
>>                   return -EAGAIN;
>>               }
>>           }
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 1fad80362f0e..af648c6a19b5 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -328,6 +328,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, 
>> u8 bus, u8 devfn)
>>       *((u8*) &pdev->bus) = bus;
>>       *((u8*) &pdev->devfn) = devfn;
>>       pdev->domain = NULL;
>> +    spin_lock_init(&pdev->vpci_lock);
>>   
>>       arch_pci_init_pdev(pdev);
>>   
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index 40ff79c33f8f..bd23c0274d48 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -142,12 +142,13 @@ bool vpci_process_pending(struct vcpu *v)
>>           if ( rc == -ERESTART )
>>               return true;
>>   
>> -        spin_lock(&v->vpci.pdev->vpci->lock);
>> -        /* Disable memory decoding unconditionally on failure. */
>> -        modify_decoding(v->vpci.pdev,
>> -                        rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : 
>> v->vpci.cmd,
>> -                        !rc && v->vpci.rom_only);
>> -        spin_unlock(&v->vpci.pdev->vpci->lock);
>> +        spin_lock(&v->vpci.pdev->vpci_lock);
>> +        if ( v->vpci.pdev->vpci )
>> +            /* Disable memory decoding unconditionally on failure. */
>> +            modify_decoding(v->vpci.pdev,
>> +                            rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : 
>> v->vpci.cmd,
>> +                            !rc && v->vpci.rom_only);
>> +        spin_unlock(&v->vpci.pdev->vpci_lock);
>>   
>>           rangeset_destroy(v->vpci.mem);
>>           v->vpci.mem = NULL;
>> @@ -285,6 +286,12 @@ static int modify_bars(const struct pci_dev *pdev, 
>> uint16_t cmd, bool rom_only)
>>                   continue;
>>           }
>>   
>> +        spin_lock(&tmp->vpci_lock);
>> +        if ( !tmp->vpci )
>> +        {
>> +            spin_unlock(&tmp->vpci_lock);
>> +            continue;
>> +        }
>>           for ( i = 0; i < ARRAY_SIZE(tmp->vpci->header.bars); i++ )
>>           {
>>               const struct vpci_bar *bar = &tmp->vpci->header.bars[i];
>> @@ -303,12 +310,14 @@ static int modify_bars(const struct pci_dev *pdev, 
>> uint16_t cmd, bool rom_only)
>>               rc = rangeset_remove_range(mem, start, end);
>>               if ( rc )
>>               {
>> +                spin_unlock(&tmp->vpci_lock);
>>                   printk(XENLOG_G_WARNING "Failed to remove [%lx, %lx]: 
>> %d\n",
>>                          start, end, rc);
>>                   rangeset_destroy(mem);
>>                   return rc;
>>               }
>>           }
>> +        spin_unlock(&tmp->vpci_lock);
>>       }
>>   
>>       ASSERT(dev);
>> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
>> index 5757a7aed20f..e3ce46869dad 100644
>> --- a/xen/drivers/vpci/msi.c
>> +++ b/xen/drivers/vpci/msi.c
>> @@ -270,7 +270,7 @@ void vpci_dump_msi(void)
>>       rcu_read_lock(&domlist_read_lock);
>>       for_each_domain ( d )
>>       {
>> -        const struct pci_dev *pdev;
>> +        struct pci_dev *pdev;
>>   
>>           if ( !has_vpci(d) )
>>               continue;
>> @@ -282,8 +282,13 @@ void vpci_dump_msi(void)
>>               const struct vpci_msi *msi;
>>               const struct vpci_msix *msix;
>>   
>> -            if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) )
>> +            if ( !spin_trylock(&pdev->vpci_lock) )
>>                   continue;
>> +            if ( !pdev->vpci )
>> +            {
>> +                spin_unlock(&pdev->vpci_lock);
>> +                continue;
>> +            }
>>   
>>               msi = pdev->vpci->msi;
>>               if ( msi && msi->enabled )
>> @@ -323,7 +328,7 @@ void vpci_dump_msi(void)
>>                   }
>>               }
>>   
>> -            spin_unlock(&pdev->vpci->lock);
>> +            spin_unlock(&pdev->vpci_lock);
>>               process_pending_softirqs();
>>           }
>>       }
>> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
>> index 846f1b8d7038..5310cc3ff520 100644
>> --- a/xen/drivers/vpci/msix.c
>> +++ b/xen/drivers/vpci/msix.c
>> @@ -225,7 +225,7 @@ static int msix_read(struct vcpu *v, unsigned long addr, 
>> unsigned int len,
> I think you also need to add locking to msix_find, otherwise it will
> dereference pdev->vpci without holding the vpci_lock.
>
> It might be a better approach to rename msix_find to msix_get and
> return the vpci_msix struct with the vpci_lock taken, so we can assert
> it's not going to disappear under our feet. Then you will also need to
> add a msix_put function that releases the lock.
Ok, sounds good: so, I'll implement msix_{get|put} then
>
>>           return X86EMUL_OKAY;
>>       }
>>   
>> -    spin_lock(&msix->pdev->vpci->lock);
>> +    spin_lock(&msix->pdev->vpci_lock);
>>       entry = get_entry(msix, addr);
>>       offset = addr & (PCI_MSIX_ENTRY_SIZE - 1);
>>   
>> @@ -254,7 +254,7 @@ static int msix_read(struct vcpu *v, unsigned long addr, 
>> unsigned int len,
>>           ASSERT_UNREACHABLE();
>>           break;
>>       }
>> -    spin_unlock(&msix->pdev->vpci->lock);
>> +    spin_unlock(&msix->pdev->vpci_lock);
>>   
>>       return X86EMUL_OKAY;
>>   }
>> @@ -297,7 +297,7 @@ static int msix_write(struct vcpu *v, unsigned long 
>> addr, unsigned int len,
>>           return X86EMUL_OKAY;
>>       }
>>   
>> -    spin_lock(&msix->pdev->vpci->lock);
>> +    spin_lock(&msix->pdev->vpci_lock);
>>       entry = get_entry(msix, addr);
>>       offset = addr & (PCI_MSIX_ENTRY_SIZE - 1);
>>   
>> @@ -370,7 +370,7 @@ static int msix_write(struct vcpu *v, unsigned long 
>> addr, unsigned int len,
>>           ASSERT_UNREACHABLE();
>>           break;
>>       }
>> -    spin_unlock(&msix->pdev->vpci->lock);
>> +    spin_unlock(&msix->pdev->vpci_lock);
>>   
>>       return X86EMUL_OKAY;
>>   }
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index fb0947179b79..c015a4d77540 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -35,12 +35,10 @@ extern vpci_register_init_t *const __start_vpci_array[];
>>   extern vpci_register_init_t *const __end_vpci_array[];
>>   #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
>>   
>> -void vpci_remove_device(struct pci_dev *pdev)
>> +static void vpci_remove_device_locked(struct pci_dev *pdev)
> Nit: since it's a static function you can drop the vpci_ prefix here.
This function is going to be used outside later on, but not yet.
So, I can change the name and then change it back once it is
used by others.
What's your preference here?
>
> Thanks, Roger.
Thank you,
Oleksandr