Re: [PATCH v4 3/9] x86/passthrough: Extract pt_irq_dpci_setup() from pt_irq_create_bind()

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Le 27/04/2026 à 15:57, Julian Vetter a écrit :
> The setup preamble in pt_irq_create_bind(), lazily allocating
> hvm_irq_dpci, looking up the struct pirq, and spinning until any pending
> hvm_dirq_assist softirq has drained, is needed by pt_irq_bind_msi() as
> well. Extract it into a static helper pt_irq_dpci_setup() that returns
> with d->event_lock write-locked on success. Replace the open-coded goto
> restart loop with a do { } while (true) loop and a continue, making the
> retry structure explicit without a label. No functional change.
> 
> Signed-off-by: Julian Vetter <julian.vetter@xxxxxxxxxx>
> ---
> Changes in v4:
> - New patch
> - Split out as a preparatory no-functional-change step to make the diff
>    in patch 5 (pt_irq_bind_msi() interface change) easier to review
> ---
>   xen/drivers/passthrough/x86/hvm.c | 54 +++++++++++++++++++++++--------
>   1 file changed, 40 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/x86/hvm.c 
> b/xen/drivers/passthrough/x86/hvm.c
> index 691fa1b2c7..19463c3406 100644
> --- a/xen/drivers/passthrough/x86/hvm.c
> +++ b/xen/drivers/passthrough/x86/hvm.c
> @@ -217,18 +217,22 @@ static struct vcpu *vector_hashing_dest(const struct 
> domain *d,
>       return dest;
>   }
>   
> -int pt_irq_create_bind(
> -    struct domain *d, const struct xen_domctl_bind_pt_irq *pt_irq_bind)
> +/*
> + * Acquire d->event_lock (write), lazily allocate hvm_irq_dpci if needed, 
> look
> + * up the struct pirq for @pirq, and drain any pending hvm_dirq_assist 
> softirq
> + * on it before returning. Returns 0 with d->event_lock held on success,
> + * negative errno otherwise (lock not held).
> + */
> +static int pt_irq_dpci_setup(struct domain *d, unsigned int pirq,
> +                              struct hvm_irq_dpci **hvm_irq_dpci_out,
> +                              struct hvm_pirq_dpci **pirq_dpci_out,
> +                              struct pirq **info_out)
>   {
>       struct hvm_irq_dpci *hvm_irq_dpci;
>       struct hvm_pirq_dpci *pirq_dpci;
>       struct pirq *info;
> -    int rc, pirq = pt_irq_bind->machine_irq;
>   
> -    if ( pirq < 0 || pirq >= d->nr_pirqs )
> -        return -EINVAL;
> -
> - restart:
> +    do
>       {
>           write_lock(&d->event_lock);
>   
> @@ -238,10 +242,11 @@ int pt_irq_create_bind(
>               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.
> +             * 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 )
> @@ -269,15 +274,36 @@ int pt_irq_create_bind(
>            * 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.
> +         * outstanding hvm_dirq_assist calls).
>            */
>           if ( pt_pirq_softirq_active(pirq_dpci) )
>           {
>               write_unlock(&d->event_lock);
>               cpu_relax();
> -            goto restart;
> +            continue;
>           }
> -    }
> +
> +        *hvm_irq_dpci_out = hvm_irq_dpci;
> +        *pirq_dpci_out = pirq_dpci;
> +        *info_out = info;
> +        return 0;
> +    } while ( true );

I would prefer something like

do {
   ...
} while (false);

return 0;

> +}
> +
> +int pt_irq_create_bind(
> +    struct domain *d, const struct xen_domctl_bind_pt_irq *pt_irq_bind)
> +{
> +    struct hvm_irq_dpci *hvm_irq_dpci;
> +    struct hvm_pirq_dpci *pirq_dpci;
> +    struct pirq *info;
> +    int rc, pirq = pt_irq_bind->machine_irq;
> +
> +    if ( pirq < 0 || pirq >= d->nr_pirqs )
> +        return -EINVAL;
> +
> +    rc = pt_irq_dpci_setup(d, pirq, &hvm_irq_dpci, &pirq_dpci, &info);
> +    if ( rc )
> +        return rc;
>   
>       switch ( pt_irq_bind->irq_type )
>       {

The rest looks good to me.
With the do { ... } while (false); change:

Reviewed-by: Teddy Astie <teddy.astie@xxxxxxxxxx>


--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech