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

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

On Fri, Jun 29, 2018 at 11:19:57AM +0100, Wei Liu wrote:
> On Wed, Jun 20, 2018 at 04:42:25PM +0200, Roger Pau Monne wrote:
> > 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?

Previous code didn't support removing devices, so it's more about
making it capable of supporting vpci device removal.

> > +            /* 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));

Er, yes. But keep in mind that this is going to return `true` even if
vpci_lock is locked by another CPU. Asserting lock ownership only
works correctly with recursive locks.

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

There's no further usage of contents of the vpci struct, so I guess I
can move the unlock a little bit up. The same applies to the write
counterpart.

Thanks, Roger.

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