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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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.

>      }
>  
>      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);

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

> @@ -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. (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?

Jan