Re: [Xen-devel] [PATCH v5 7/7] VT-d: Fix vt-d Device-TLB flush timeout issue.

["Xu, Quan" <quan.xu@xxxxxxxxx>]

> On February 18, 2016 4:36pm, <JBeulich@xxxxxxxx> wrote:
> >>> On 18.02.16 at 08:36, <quan.xu@xxxxxxxxx> wrote:
> >>  On February 17, 2016 10:41pm, <JBeulich@xxxxxxxx> wrote:
> >> >>> On 05.02.16 at 11:18, <quan.xu@xxxxxxxxx> wrote:
> >> > --- a/xen/drivers/passthrough/vtd/qinval.c
> >> > +++ b/xen/drivers/passthrough/vtd/qinval.c
> >> > +            if ( pci_hide_device(bus, devfn) )
> >>
> >> But now I'm _really_ puzzled: You acquire the exact lock that
> >> pci_hide_device() acquires. Hence, unless I've overlooked an earlier
> >> change,
> > I
> >> can't see this as other than an unconditional dead lock. Did you test
> >> this
> > code
> >> path at all?
> >
> > Sorry, I didn't test this code path.
> > I did test the follows:
> >    1) Create domain with ATS device.
> >    2) Attach / Detach ATS device.
> >
> > I think I could add a variation of pci_hide_device(), without
> > "spin_lock(&pcidevs_lock) / spin_unlock(&pcidevs_lock)"
> > Or "__init".
> 
> Which we already have - _pci_hide_device(). The function would just need to be
> made non-static, but would otherwise fit your purposes since you also don't
> need the alloc_pdev() the other function does.
> 

But the _pci_hide_device() starts with '_', I think it is not a good idea to 
make it non-static.
I'd better introduce a new function to wrap the '_pci_hide_device()'. It is 
difficult to name the new function.
 

> > But it is sure that different lock state is possible for different
> > call trees when to flush an ATS device.
> 
> Sure, that's why you pass down the flag. Presumably easiest (albeit not 
> nicest)
> will be to call the respective of the two functions depending on the lock 
> holding
> flag.
> 

It is really a good idea. Thanks you for your advice.

Quan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel