Re: [PATCH v6 03/13] vpci: move lock outside of struct vpci

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Jan!

On 04.02.22 09:52, Jan Beulich wrote:
> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
>> @@ -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);
>>       }
> At the first glance this simply looks like another unjustified (in the
> description) change, as you're not converting anything here but you
> actually add locking (and I realize this was there before, so I'm sorry
> for not pointing this out earlier).
Well, I thought that the description already has "...the lock can be
used (and in a few cases is used right away) to check whether vpci
is present" and this is enough for such uses as here.
>   But then I wonder whether you
> actually tested this, since I can't help getting the impression that
> you're introducing a live-lock: The function is called from cmd_write()
> and rom_write(), which in turn are called out of vpci_write(). Yet that
> function already holds the lock, and the lock is not (currently)
> recursive. (For the 3rd caller of the function - init_bars() - otoh
> the locking looks to be entirely unnecessary.)
Well, you are correct: if tmp != pdev then it is correct to acquire
the lock. But if tmp == pdev and rom_only == true
then we'll deadlock.

It seems we need to have the locking conditional, e.g. only lock
if tmp != pdev
>
> Then again this was present already even in Roger's original patch, so
> I guess I must be missing something ...
>
>> --- a/xen/drivers/vpci/msix.c
>> +++ b/xen/drivers/vpci/msix.c
>> @@ -138,7 +138,7 @@ static void control_write(const struct pci_dev *pdev, 
>> unsigned int reg,
>>           pci_conf_write16(pdev->sbdf, reg, val);
>>   }
>>   
>> -static struct vpci_msix *msix_find(const struct domain *d, unsigned long 
>> addr)
>> +static struct vpci_msix *msix_get(const struct domain *d, unsigned long 
>> addr)
>>   {
>>       struct vpci_msix *msix;
>>   
>> @@ -150,15 +150,29 @@ static struct vpci_msix *msix_find(const struct domain 
>> *d, unsigned long addr)
>>           for ( i = 0; i < ARRAY_SIZE(msix->tables); i++ )
>>               if ( bars[msix->tables[i] & PCI_MSIX_BIRMASK].enabled &&
>>                    VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, i) )
>> +            {
>> +                spin_lock(&msix->pdev->vpci_lock);
>>                   return msix;
>> +            }
> I think deliberately returning with a lock held requires a respective
> comment ahead of the function.
Ok, will add a comment
>
>>       }
>>   
>>       return NULL;
>>   }
>>   
>> +static void msix_put(struct vpci_msix *msix)
>> +{
>> +    if ( !msix )
>> +        return;
>> +
>> +    spin_unlock(&msix->pdev->vpci_lock);
>> +}
> Maybe shorter
>
>      if ( msix )
>          spin_unlock(&msix->pdev->vpci_lock);
Looks good
>
> ? Yet there's only one case where you may pass NULL in here, so
> maybe it's better anyway to move the conditional ...
>
>>   static int msix_accept(struct vcpu *v, unsigned long addr)
>>   {
>> -    return !!msix_find(v->domain, addr);
>> +    struct vpci_msix *msix = msix_get(v->domain, addr);
>> +
>> +    msix_put(msix);
>> +    return !!msix;
>>   }
> ... here?
Yes, I can have that check here, but what if there is yet
another caller of the same? I am not sure whether it is better
to have the check in msix_get or at the caller site.
At the moment (with a single place with NULL possible) I can
move the check. @Roger?
>
>> @@ -186,7 +200,7 @@ static int msix_read(struct vcpu *v, unsigned long addr, 
>> unsigned int len,
>>                        unsigned long *data)
>>   {
>>       const struct domain *d = v->domain;
>> -    struct vpci_msix *msix = msix_find(d, addr);
>> +    struct vpci_msix *msix = msix_get(d, addr);
>>       const struct vpci_msix_entry *entry;
>>       unsigned int offset;
>>   
>> @@ -196,7 +210,10 @@ static int msix_read(struct vcpu *v, unsigned long 
>> addr, unsigned int len,
>>           return X86EMUL_RETRY;
>>   
>>       if ( !access_allowed(msix->pdev, addr, len) )
>> +    {
>> +        msix_put(msix);
>>           return X86EMUL_OKAY;
>> +    }
>>   
>>       if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
>>       {
>> @@ -222,10 +239,10 @@ static int msix_read(struct vcpu *v, unsigned long 
>> addr, unsigned int len,
>>               break;
>>           }
>>   
>> +        msix_put(msix);
>>           return X86EMUL_OKAY;
>>       }
>>   
>> -    spin_lock(&msix->pdev->vpci->lock);
>>       entry = get_entry(msix, addr);
>>       offset = addr & (PCI_MSIX_ENTRY_SIZE - 1);
> You're increasing the locked region quite a bit here. If this is really
> needed, it wants explaining. And if this is deemed acceptable as a
> "side effect", it wants justifying or at least stating imo. Same for
> msix_write() then, obviously.
Yes, I do increase the locking region here, but the msix variable needs
to be protected all the time, so it seems to be obvious that it remains
under the lock
>   (I'm not sure Roger actually implied this
> when suggesting to switch to the get/put pair.)
>
>> @@ -327,7 +334,12 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, 
>> unsigned int size)
>>       if ( !pdev )
>>           return vpci_read_hw(sbdf, reg, size);
>>   
>> -    spin_lock(&pdev->vpci->lock);
>> +    spin_lock(&pdev->vpci_lock);
>> +    if ( !pdev->vpci )
>> +    {
>> +        spin_unlock(&pdev->vpci_lock);
>> +        return vpci_read_hw(sbdf, reg, size);
>> +    }
> Didn't you say you would add justification of this part of the change
> (and its vpci_write() counterpart) to the description?
Again, I am referring to the commit message as described above
>
> Jan
>
Thank you,
Oleksandr