Re: [PATCH v4 2/2] iommu/vt-d: don's issue devTLB flush request when device is disconnected

[Ethan Zhao]

On 12/22/2023 4:14 PM, Lukas Wunner wrote:
> On Fri, Dec 22, 2023 at 09:56:39AM +0800, Ethan Zhao wrote:
> > I don't know if the polling along sleeping for completion of meanningless
> > devTLB invalidation request blindly sent to (removed/powered down/link down)
> > device makes sense or not.
> If you have a way to get to the struct pci_dev * which you're waiting for
> in qi_submit_sync() then I guess you could check for its presence and bail
> out if it's gone, instead of issuing a cpu_relax().
One option to bail out the loop.
> > > Again, the proposed patch is not a proper solution.  It will paper over
> > > the issue most of the time but every once in a while someone will still
> > > get a hard lockup splat and it will then be more difficult to reproduce
> > > and fix if the proposed patch is accepted.
> > Could you point out why is not proper ? Is there any other window
> > the hard lockup still could happen with the ATS capable devcie
> > supprise_removal case if we checked the connection state first ?
> > Please help to elaberate it.
> Even though user space may have initiated orderly removal via sysfs,
> the device may be yanked from the slot (surprise removed) while the
> orderly removal is happening.

Yes, just after the wait descripor is submitted and before waiting in loop.

the rare but worst case.

> > Yes, this is the old kernel stack trace, but customer also tried lasted
> > 6.7rc4
> If you could provide a stacktrace for a contemporary kernel,
> I think that would be preferred.
Customer tried, but they didn't provide me the lastest trace.
> > (doesn't work) and the patched 6.7rc4 (fixed).
> Why is it fixed in v6.7-rc4?  Is the present patch thus unnecessary?
Not fixed in v6.7rc4, with this patch, they said the unplug works.
> > > Finally, it is common to adhere to terms
> > > used in the PCIe Base Spec in commit messages, so "ATC Invalidate Request"
> > > might be preferable to "devTLB flush request".
> > ATS Invalidate Request ? devTLB flush request has the same meaning,
> >
> > I thought all iommu/PCIe guys could understand.
> I'm just pointing out the preferred way to write commit messages
> in the PCI subsystem (as I've perceived it over the years) so that
> you can reduce the number of iterations you have to go through
> due to maintainer feedback.  I'm just trying to be helpful.
Understand.
> > How to define the point "some" msec to timeout while software
> > break out the waiting loop ? or polling if the target is gone ?
> I'd say adhere to the 1 min + 50% number provided in the spec.
>
> If you know the device is gone before that then you can break out
> of the loop in qi_submit_sync() of course.

I am trying to find a way to break it out in this qi_submit_sync().

 checking the device state in this loop, but seems not good in this

iommu low level code and need some interfaces to be modified.

That would cost me much more hours to make the rare case work,

to be perfect:

1.  check the pci device state in the loop

2.  modify the invalidation descriptor status in 
pciehp_ist()->intel_iommu_release_device() call.

> The question is, does the Intel IOMMU have a timeout at all for
> Invalidate Requests?  I guess we don't really know that because
> in the stack trace you've provided, the watchdog stops the machine
> before a timeout occurs.  So it's at least 12 sec.  Or there's
> no timeout at all.

The calltrace wouldn't tell us there is really timeout of 1min+50%

or not, event there is, meanlingless.

> If the Intel IOMMU doesn't enforce a timeout, you should probably amend
> qi_submit_sync() to break out of the loop once the 1 min + 50% limit
> is exceeded.  And you need to amend the function to sleep instead of
> polling in interrupt context.

Too many paths to call this function, and revise it to non-sync, to much

things impacted.

> Can you check with hardware engineers whether there's a timeout?

Combinated with third party PCIe switch chips ?


Thanks,

Ethan

> Thanks,
>
> Lukas