Re: [PATCH] x86/irq: Skip unmap_domain_pirq XSM during destruction

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.03.2022 20:17, Jason Andryuk wrote:
> 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,
> we don't want the XSM hook denying the operation.
> 
> Check d->is_dying and skip the check when the domain is dead.  The RCU
> callback runs when a domain is in that state.

One question which has always been puzzling me (perhaps to Daniel): While
I can see why mapping of an IRQ needs to be subject to an XSM check, it's
not really clear to me why unmapping would need to be, at least as long
as it's the domain itself which requests the unmap (and which I would
view to extend to the domain being cleaned up). But maybe that's why it's
XSM_HOOK ...

> ---
> Dan wants to change current to point at DOMID_IDLE when the RCU callback
> runs.  I think Juergen's commit 53594c7bd197 "rcu: don't use
> stop_machine_run() for rcu_barrier()" may have changed this since it
> mentions stop_machine_run scheduled the idle vcpus to run the callbacks
> for the old code.
> 
> Would that be as easy as changing rcu_do_batch() to do:
> 
> +        /* Run as "Xen" not a random domain's vcpu. */
> +        vcpu = get_current();
> +        set_current(idle_vcpu[smp_processor_id()]);
>          list->func(list);
> +        set_current(vcpu);
> 
> or is using set_current() only acceptable as part of context_switch?

Indeed I would question any uses outside of context_switch() (and
system bringup).

> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2340,10 +2340,14 @@ int unmap_domain_pirq(struct domain *d, int pirq)
>          nr = msi_desc->msi.nvec;
>      }
>  
> -    ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
> -                               msi_desc ? msi_desc->dev : NULL);
> -    if ( ret )
> -        goto done;
> +    /* When called by complete_domain_destroy via RCU, current is a random
> +     * domain.  Skip the XSM check since this is a Xen-initiated action. */

Comment style.

> +    if ( d->is_dying != DOMDYING_dead ) {

Please use !d->is_dying. Also please correct the placement of the brace.
Or you could avoid the need for a brace by leveraging that ret is zero
ahead of this if(), i.e. ...

> +        ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
> +                                   msi_desc ? msi_desc->dev : NULL);
> +        if ( ret )
> +            goto done;
> +    }


    if ( !d->is_dying )
        ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
                                   msi_desc ? msi_desc->dev : NULL);
    if ( ret )
        goto done;

Jan