Re: [PATCH] x86/physdev: Call xsm_unmap_domain_irq earlier

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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 <jandryuk@xxxxxxxxx>
> ---
> 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.