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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Roger,

On 29/06/18 14:27, Roger Pau Monné wrote:
> 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.
While I agree with your statement, the point of the ASSERT is to catch 
misuse, there are a fair amount of chance to have no contention on the 
lock (something would need to be done if it was the case anyway).
So in general, I still recommend developer to use 
ASSERT(spin_is_lock(...)) in any function relying on a lock taken. And 
who knows, maybe some day we would have a spin lock helper checking the 
CPU making the ASSERT more reliable.
Cheers,

--
Julien Grall

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