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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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://lore.kernel.org/xen-devel/20180717094830.54806-2-roger.pau@xxxxxxxxxx/
> 
> 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.

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

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

Thanks, Roger.