Page MenuHomeFreeBSD

xen/intr: add check for intr_register_source() errors
ClosedPublic

Authored by ehem_freebsd_m5p.com on Sep 16 2021, 10:53 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 5, 5:37 PM
Unknown Object (File)
Nov 21 2024, 10:52 AM
Unknown Object (File)
Nov 15 2024, 8:03 AM
Unknown Object (File)
Nov 15 2024, 6:18 AM
Unknown Object (File)
Nov 14 2024, 3:00 AM
Unknown Object (File)
Nov 14 2024, 2:51 AM
Unknown Object (File)
Sep 30 2024, 2:28 AM
Unknown Object (File)
Sep 27 2024, 1:30 AM
Subscribers

Details

Summary

While unusual, intr_register_source() can return failure. A likely
cause might be another device grabbing from Xen's interrupt range.
This should NOT happen, but could happen due to a bug. As such check
for this and fail if it occurs.

This also covers the condition identified by the KASSERT(), so remove
the now redundant KASSERT().

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 45559
Build 42447: arc lint + arc unit

Event Timeline

This really doesn't fit with D30743. This one is checking for this one situation which should never occur, but reality is perverse.

Most likely trigger is intr_register_source() does a check for intr_lookup_source(vector) and will return EEXIST for the situation identical to the KASSERT(). This though would require something else to grab an interrupt from the Xen code's range; which in turn shouldn't happen.

This doesn't work as part of D30743, since this isn't focused on xen_intr_port_to_isrc[]. This is simply a check which is really needed. Isn't needed as part of the Xen/ARM64 project, just my entire patch series includes this. This shouldn't occur, but safety checks are for the conditions we don't expect to occur.

Looks fine to me. I would suggest keeping the KASSERT though, it is free and makes the point of failure more obvious if this condition did occur.

This revision is now accepted and ready to land.Sep 17 2021, 12:27 PM

On review, at HEAD the logic is inverted. When returning with NULL, the lock needs to be held. Updating to match actual situation.

This revision now requires review to proceed.Oct 23 2021, 8:56 PM

If you're wondering about D30726 referencing the call structures being perverse, this is one of the spots. Alternatively due to commit reordering this got reordered before D30726 which flips the logic. This is what should be done if before D30726.

I prefer pushing D31995 after D30726. That way the original (approved) form can be used and results in some later deltas being smaller. I don't like leaving mines lying around though.

Updating due to concern which arose. If this occurs another spot will need to deal with it. OTOH should this in fact be a panic() since it shouldn't occur?

This is mostly getting handled elsewhere, but update the Phabricator copy.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 29 2023, 7:55 AM
This revision was automatically updated to reflect the committed changes.