Re: [Xen-devel] [PATCH 01/10] vpci: move lock

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Jun 20, 2018 at 04:42:25PM +0200, Roger Pau Monne wrote:
> To the outside of the vpci struct. 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 log was dropped prior to freeing

log -> lock.

> pdev->vpci.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
[...]
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 0ec4c082a6..9d5607d5f8 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -131,11 +131,12 @@ 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.map,
> -                        !rc && v->vpci.rom_only);
> -        spin_unlock(&v->vpci.pdev->vpci->lock);
> +        spin_lock(&v->vpci.pdev->vpci_lock);
> +        if ( v->vpci.pdev->vpci )

The purpose of this check is to fix a latent bug in the original code?

> +            /* Disable memory decoding unconditionally on failure. */
> +            modify_decoding(v->vpci.pdev, !rc && v->vpci.map,
> +                            !rc && v->vpci.rom_only);
> +        spin_unlock(&v->vpci.pdev->vpci_lock);
>  
[...]
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index 82607bdb9a..7d52bcf8d0 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -35,9 +35,8 @@ 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)
>  {
> -    spin_lock(&pdev->vpci->lock);

ASSERT(spin_is_locked(&pdev->vpci_lock));

>      while ( !list_empty(&pdev->vpci->handlers) )
>      {
>          struct vpci_register *r = list_first_entry(&pdev->vpci->handlers,
> @@ -47,13 +46,20 @@ void vpci_remove_device(struct pci_dev *pdev)
>          list_del(&r->node);
>          xfree(r);
>      }
> -    spin_unlock(&pdev->vpci->lock);
>      xfree(pdev->vpci->msix);
>      xfree(pdev->vpci->msi);
>      xfree(pdev->vpci);
>      pdev->vpci = NULL;
>  }
>  
> +void vpci_remove_device(struct pci_dev *pdev)
> +{
> +    spin_lock(&pdev->vpci_lock);
> +    vpci_remove_device_locked(pdev);
> +    spin_unlock(&pdev->vpci_lock);
> +}
> +
> +

Too many blank lines.

>  int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
>  {
>      unsigned int i;
> @@ -62,12 +68,15 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
>      if ( !has_vpci(pdev->domain) )
>          return 0;
>  
> +    spin_lock(&pdev->vpci_lock);
>      pdev->vpci = xzalloc(struct vpci);
>      if ( !pdev->vpci )
> +    {
> +        spin_unlock(&pdev->vpci_lock);
>          return -ENOMEM;
> +    }
>  
>      INIT_LIST_HEAD(&pdev->vpci->handlers);
> -    spin_lock_init(&pdev->vpci->lock);
>  
>      for ( i = 0; i < NUM_VPCI_INIT; i++ )
>      {
[...]
> @@ -77,7 +86,8 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
> @@ -315,7 +318,7 @@ static uint32_t merge_result(uint32_t data, uint32_t new, 
> unsigned int size,
>  uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
>  {
>      const struct domain *d = current->domain;
> -    const struct pci_dev *pdev;
> +    struct pci_dev *pdev;
>      const struct vpci_register *r;
>      unsigned int data_offset = 0;
>      uint32_t data = ~(uint32_t)0;
> @@ -331,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);
> +    }
>  
>      /* Read from the hardware or the emulated register handlers. */
>      list_for_each_entry ( r, &pdev->vpci->handlers, node )
> @@ -383,7 +391,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, 
> unsigned int size)
>  
>          data = merge_result(data, tmp_data, size - data_offset, data_offset);
>      }
> -    spin_unlock(&pdev->vpci->lock);
> +    spin_unlock(&pdev->vpci_lock);

I think the critical section in this function and the write function can
shrink a bit. Reading from / writing to hardware shouldn't need to be
protected by vpci_lock.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel