Re: [PATCH v5 02/11] x86/passthrough: Wrap pt_irq_create_bind() restart block in braces

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.09.2026 16:14, Julian Vetter wrote:
> Enclose the restart/retry block in pt_irq_create_bind() in an explicit
> compound statement to prepare for its extraction into a helper function
> ("x86/passthrough: Extract pt_irq_dpci_setup() from
> pt_irq_create_bind()"). Reflow the two comments inside the block that no
> longer fit in 80 columns at the increased indentation, and fix an
> unbalanced parenthesis in the second one. No functional change.
> 
> Signed-off-by: Julian Vetter <julian.vetter@xxxxxxxxxx>
> ---
> Changes in v5:
> - Fixed two comments to fit 80-columns (and fixed unbalanced-parenthesis
>   in comment)
> - Commit message now names the follow-up patch correctly.
Having this properly split (taken together with the subsequent two patches)
is pretty helpful. Now that one can see what's going on, I wonder though:
Do we actually need all the re-indentation here? The setup ...

> --- a/xen/drivers/passthrough/x86/hvm.c
> +++ b/xen/drivers/passthrough/x86/hvm.c
> @@ -229,52 +229,55 @@ int pt_irq_create_bind(
>          return -EINVAL;
>  
>   restart:
> -    write_lock(&d->event_lock);
> -
> -    hvm_irq_dpci = domain_get_irq_dpci(d);
> -    if ( !hvm_irq_dpci && !is_hardware_domain(d) )
>      {
> -        unsigned int i;
> +        write_lock(&d->event_lock);
>  
> -        /*
> -         * NB: the hardware domain doesn't use a hvm_irq_dpci struct because
> -         * it's only allowed to identity map GSIs, and so the data contained 
> in
> -         * that struct (used to map guest GSIs into machine GSIs and perform
> -         * interrupt routing) is completely useless to it.
> -         */
> -        hvm_irq_dpci = xzalloc(struct hvm_irq_dpci);
> -        if ( hvm_irq_dpci == NULL )
> +        hvm_irq_dpci = domain_get_irq_dpci(d);
> +        if ( !hvm_irq_dpci && !is_hardware_domain(d) )
> +        {
> +            unsigned int i;
> +
> +            /*
> +             * NB: the hardware domain doesn't use a hvm_irq_dpci struct
> +             * because it's only allowed to identity map GSIs, and so the
> +             * data contained in that struct (used to map guest GSIs into
> +             * machine GSIs and perform interrupt routing) is completely
> +             * useless to it.
> +             */
> +            hvm_irq_dpci = xzalloc(struct hvm_irq_dpci);
> +            if ( hvm_irq_dpci == NULL )
> +            {
> +                write_unlock(&d->event_lock);
> +                return -ENOMEM;
> +            }
> +            for ( i = 0; i < NR_HVM_DOMU_IRQS; i++ )
> +                INIT_LIST_HEAD(&hvm_irq_dpci->girq[i]);
> +
> +            hvm_domain_irq(d)->dpci = hvm_irq_dpci;
> +        }

... here and the logic ...

> +        info = pirq_get_info(d, pirq);
> +        if ( !info )
>          {
>              write_unlock(&d->event_lock);
>              return -ENOMEM;
>          }

... here don't need going through again after a restart. Therefore what
wants converting from goto to for() is potentially far less: It's the
->is_dying check at the top and ...

> -        for ( i = 0; i < NR_HVM_DOMU_IRQS; i++ )
> -            INIT_LIST_HEAD(&hvm_irq_dpci->girq[i]);
> -
> -        hvm_domain_irq(d)->dpci = hvm_irq_dpci;
> -    }
> -
> -    info = pirq_get_info(d, pirq);
> -    if ( !info )
> -    {
> -        write_unlock(&d->event_lock);
> -        return -ENOMEM;
> -    }
> -    pirq_dpci = pirq_dpci(info);
> +        pirq_dpci = pirq_dpci(info);
>  
> -    /*
> -     * A crude 'while' loop with us dropping the spinlock and giving
> -     * the softirq_dpci a chance to run.
> -     * We MUST check for this condition as the softirq could be scheduled
> -     * and hasn't run yet. Note that this code replaced tasklet_kill which
> -     * would have spun forever and would do the same thing (wait to flush out
> -     * outstanding hvm_dirq_assist calls.
> -     */
> -    if ( pt_pirq_softirq_active(pirq_dpci) )
> -    {
> -        write_unlock(&d->event_lock);
> -        cpu_relax();
> -        goto restart;
> +        /*
> +         * A crude 'while' loop with us dropping the spinlock and giving
> +         * the softirq_dpci a chance to run.
> +         * We MUST check for this condition as the softirq could be scheduled
> +         * and hasn't run yet. Note that this code replaced tasklet_kill
> +         * which would have spun forever and would do the same thing (wait
> +         * to flush out outstanding hvm_dirq_assist calls).
> +         */
> +        if ( pt_pirq_softirq_active(pirq_dpci) )
> +        {
> +            write_unlock(&d->event_lock);
> +            cpu_relax();
> +            goto restart;
> +        }

... the logic here. Of course we could also tidy this afterwards, so I'm
not going to insist that it be done up front. Yet I wanted to at least ask
whether you might be up to doing that transformation in order to reduce
overall churn.

Btw, the first sentence of this comment likely wants re-wording in the next
patch, as it goes from "crude" to "normal".

Jan