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.
>
> 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.
vioapic_hwdom_map_gsi is just for the hardware domain, so likely also
OK.
> xen/arch/x86/irq.c | 31 +++++++-----------------------
> xen/arch/x86/physdev.c | 43 +++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 49 insertions(+), 25 deletions(-)
>
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index 285ac399fb..ddd3194fba 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2310,41 +2310,25 @@ int unmap_domain_pirq(struct domain *d, int pirq)
> struct pirq *info;
> struct msi_desc *msi_desc = NULL;
>
> - if ( (pirq < 0) || (pirq >= d->nr_pirqs) )
> - return -EINVAL;
> -
> + ASSERT(pirq >= 0);
> + ASSERT(pirq < d->nr_pirqs);
> ASSERT(pcidevs_locked());
> ASSERT(spin_is_locked(&d->event_lock));
>
> info = pirq_info(d, pirq);
> - if ( !info || (irq = info->arch.irq) <= 0 )
> - {
> - dprintk(XENLOG_G_ERR, "dom%d: pirq %d not mapped\n",
> - d->domain_id, pirq);
> - ret = -EINVAL;
> - goto done;
> - }
> + ASSERT(info);
> +
> + irq = info->arch.irq;
> + ASSERT(irq > 0);
>
> desc = irq_to_desc(irq);
> msi_desc = desc->msi_desc;
> if ( msi_desc && msi_desc->msi_attrib.type == PCI_CAP_ID_MSI )
> {
> - if ( msi_desc->msi_attrib.entry_nr )
> - {
> - printk(XENLOG_G_ERR
> - "dom%d: trying to unmap secondary MSI pirq %d\n",
> - d->domain_id, pirq);
> - ret = -EBUSY;
> - goto done;
> - }
> + ASSERT(msi_desc->msi_attrib.entry_nr == 0);
> nr = msi_desc->msi.nvec;
> }
>
> - ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
> - msi_desc ? msi_desc->dev : NULL);
Have you considered performing the check only if !d->is_dying?
Thanks, Roger.
