On 28.03.2022 13:40, Roger Pau Monné wrote: > On Fri, Mar 25, 2022 at 10:18:26AM -0400, Jason Andryuk wrote: >> Pull the XSM check up out of unmap_domain_pirq into physdev_map_pirq. >> >> xsm_unmap_domain_irq was seen denying unmap_domain_pirq when called from >> complete_domain_destroy as an RCU callback. The source context was an >> unexpected, random domain. Since this is a xen-internal operation, >> going through the XSM hook is inapproriate. >> >> Move the XSM hook up into physdev_unmap_pirq, which is the >> guest-accessible path. This requires moving some of the sanity check >> upwards as well since the hook needs the additional data to make its >> decision. Since complete_domain_destroy still calls unmap_domain_pirq, >> replace the moved runtime checking with assert. Only valid pirqs should >> make their way into unmap_domain_pirq from complete_domain_destroy. >> >> This is mostly code movement, but one style change is to pull `irq = >> info->arch.irq` out of the if condition.
And why is this better? You now have an extra conditional expression there. >> Label done is now unused and removed. >> >> Signed-off-by: Jason Andryuk <jandr...@gmail.com> >> --- >> unmap_domain_pirq is also called in vioapic_hwdom_map_gsi and >> vpci_msi_disable. vioapic_hwdom_map_gsi is a cleanup path after going >> through map_domain_pirq, and I don't think the vpci code is directly >> guest-accessible. So I think those are okay, but I not familiar with >> that code. Hence, I am highlighting it. > > Hm, for vpci_msi_disable it's not technically correct to drop the XSM > check. This is a guests accessible path, however the value of PIRQ is > not settable by the guest, so it's kind of OK just for that reason. Kind of - perhaps. But better to avoid if possible. Hence I would prefer the ->is_dying alternative you suggest at the end. Jan
