Re: [PATCH v4 4/9] x86/passthrough: Extract PT_IRQ_TYPE_MSI body into pt_irq_bind_msi()

On 27.04.2026 15:54, Julian Vetter wrote:
> --- a/xen/drivers/passthrough/x86/hvm.c
> +++ b/xen/drivers/passthrough/x86/hvm.c
> @@ -290,161 +290,186 @@ static int pt_irq_dpci_setup(struct domain *d, unsigned int pirq,
>      } while ( true );
>  }
>  
> -int pt_irq_create_bind(
> -    struct domain *d, const struct xen_domctl_bind_pt_irq *pt_irq_bind)
> +static int pt_irq_bind_msi(struct domain *d, uint32_t machine_irq,
> +                            uint8_t gvec, uint32_t gflags, uint64_t gtable,

Please see ./CODING_STYLE for the use of fixed-width types. With relaxed
interpretation of them, at least machine_irq and gflags should be simply
unsigned int. (gvec and gtable I think are tolerable as you have them.)

> +                            bool unmasked)

Nit (for both wrapped lines): Indentation.

>  {
>      struct hvm_irq_dpci *hvm_irq_dpci;
>      struct hvm_pirq_dpci *pirq_dpci;
>      struct pirq *info;
> -    int rc, pirq = pt_irq_bind->machine_irq;
> +    uint8_t dest, delivery_mode;
> +    bool dest_mode;
> +    int dest_vcpu_id, rc;
> +    const struct vcpu *vcpu;
>  
> -    if ( pirq < 0 || pirq >= d->nr_pirqs )
> +    if ( machine_irq >= (unsigned int)d->nr_pirqs )
>          return -EINVAL;

Rather than merely asking on the cast: What use is this check, when the
caller has done it already?

> -    rc = pt_irq_dpci_setup(d, pirq, &hvm_irq_dpci, &pirq_dpci, &info);
> +    rc = pt_irq_dpci_setup(d, machine_irq, &hvm_irq_dpci, &pirq_dpci, &info);
>      if ( rc )
>          return rc;
>  
> -    switch ( pt_irq_bind->irq_type )
> +    if ( !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
>      {
> -    case PT_IRQ_TYPE_MSI:
> -    {
> -        uint8_t dest, delivery_mode;
> -        bool dest_mode;
> -        int dest_vcpu_id;
> -        const struct vcpu *vcpu;
> -        uint32_t gflags = pt_irq_bind->u.msi.gflags &
> -                          ~XEN_DOMCTL_VMSI_X86_UNMASKED;
> -
> -        if ( !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
> +        pirq_dpci->flags = HVM_IRQ_DPCI_MAPPED | HVM_IRQ_DPCI_MACH_MSI |
> +                           HVM_IRQ_DPCI_GUEST_MSI;
> +        pirq_dpci->gmsi.gvec = gvec;
> +        pirq_dpci->gmsi.gflags = gflags;
> +        /*
> +         * 'pt_irq_bind_msi' can be called after 'pt_irq_destroy_bind'.
> +         * The 'pirq_cleanup_check' which would free the structure is only
> +         * called if the event channel for the PIRQ is active. However
> +         * OS-es that use event channels usually bind PIRQs to eventds
> +         * and unbind them before calling 'pt_irq_destroy_bind' - with the
> +         * result that we re-use the 'dpci' structure. This can be
> +         * reproduced with unloading and loading the driver for a device.
> +         *
> +         * As such on every 'pt_irq_bind_msi' call we MUST set it.
> +         */
> +        pirq_dpci->dom = d;
> +        /* bind after hvm_irq_dpci is setup to avoid race with irq handler */

Much like you add the missing blank at the end, please also correct the start
of this comment (to use a capital 'B').

> +        rc = pirq_guest_bind(d->vcpu[0], info, 0);
> +        if ( rc == 0 && gtable )
>          {
> -            pirq_dpci->flags = HVM_IRQ_DPCI_MAPPED | HVM_IRQ_DPCI_MACH_MSI |
> -                               HVM_IRQ_DPCI_GUEST_MSI;
> -            pirq_dpci->gmsi.gvec = pt_irq_bind->u.msi.gvec;
> -            pirq_dpci->gmsi.gflags = gflags;
> -            /*
> -             * 'pt_irq_create_bind' can be called after 'pt_irq_destroy_bind'.
> -             * The 'pirq_cleanup_check' which would free the structure is only
> -             * called if the event channel for the PIRQ is active. However
> -             * OS-es that use event channels usually bind PIRQs to eventds
> -             * and unbind them before calling 'pt_irq_destroy_bind' - with the
> -             * result that we re-use the 'dpci' structure. This can be
> -             * reproduced with unloading and loading the driver for a device.
> -             *
> -             * As such on every 'pt_irq_create_bind' call we MUST set it.
> -             */
> -            pirq_dpci->dom = d;
> -            /* bind after hvm_irq_dpci is setup to avoid race with irq handler*/
> -            rc = pirq_guest_bind(d->vcpu[0], info, 0);
> -            if ( rc == 0 && pt_irq_bind->u.msi.gtable )
> -            {
> -                rc = msixtbl_pt_register(d, info, pt_irq_bind->u.msi.gtable);
> -                if ( unlikely(rc) )
> -                {
> -                    pirq_guest_unbind(d, info);
> -                    /*
> -                     * Between 'pirq_guest_bind' and before 'pirq_guest_unbind'
> -                     * an interrupt can be scheduled. No more of them are going
> -                     * to be scheduled but we must deal with the one that may be
> -                     * in the queue.
> -                     */
> -                    pt_pirq_softirq_reset(pirq_dpci);
> -                }
> -            }
> +            rc = msixtbl_pt_register(d, info, gtable);
>              if ( unlikely(rc) )
>              {
> -                pirq_dpci->gmsi.gflags = 0;
> -                pirq_dpci->gmsi.gvec = 0;
> -                pirq_dpci->dom = NULL;
> -                pirq_dpci->flags = 0;
> -                if ( !info->evtchn )
> -                    pirq_cleanup_check(info, d);
> -                write_unlock(&d->event_lock);
> -                return rc;
> +                pirq_guest_unbind(d, info);
> +                /*
> +                 * Between 'pirq_guest_bind' and before 'pirq_guest_unbind'
> +                 * an interrupt can be scheduled. No more of them are going
> +                 * to be scheduled but we must deal with the one that may be
> +                 * in the queue.
> +                 */
> +                pt_pirq_softirq_reset(pirq_dpci);
>              }
>          }
> -        else
> +        if ( unlikely(rc) )
>          {
> -            uint32_t mask = HVM_IRQ_DPCI_MACH_MSI | HVM_IRQ_DPCI_GUEST_MSI;
> -
> -            if ( (pirq_dpci->flags & mask) != mask )
> -            {
> -                write_unlock(&d->event_lock);
> -                return -EBUSY;
> -            }
> -
> -            /* If pirq is already mapped as vmsi, update guest data/addr. */
> -            if ( pirq_dpci->gmsi.gvec != pt_irq_bind->u.msi.gvec ||
> -                 pirq_dpci->gmsi.gflags != gflags )
> -            {
> -                /* Directly clear pending EOIs before enabling new MSI info. */
> -                pirq_guest_eoi(info);
> -
> -                pirq_dpci->gmsi.gvec = pt_irq_bind->u.msi.gvec;
> -                pirq_dpci->gmsi.gflags = gflags;
> -            }
> +            pirq_dpci->gmsi.gflags = 0;
> +            pirq_dpci->gmsi.gvec = 0;
> +            pirq_dpci->dom = NULL;
> +            pirq_dpci->flags = 0;
> +            if ( !info->evtchn )
> +                pirq_cleanup_check(info, d);
> +            write_unlock(&d->event_lock);
> +            return rc;
>          }
> -        /* Calculate dest_vcpu_id for MSI-type pirq migration. */
> -        dest = MASK_EXTR(pirq_dpci->gmsi.gflags,
> -                         XEN_DOMCTL_VMSI_X86_DEST_ID_MASK);
> -        dest_mode = pirq_dpci->gmsi.gflags & XEN_DOMCTL_VMSI_X86_DM_MASK;
> -        delivery_mode = MASK_EXTR(pirq_dpci->gmsi.gflags,
> -                                  XEN_DOMCTL_VMSI_X86_DELIV_MASK);
> -
> -        dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
> -        pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id;
> -        write_unlock(&d->event_lock);
> +    }
> +    else
> +    {
> +        uint32_t mask = HVM_IRQ_DPCI_MACH_MSI | HVM_IRQ_DPCI_GUEST_MSI;
>  
> -        pirq_dpci->gmsi.posted = false;
> -        vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL;
> -        if ( iommu_intpost )
> +        if ( (pirq_dpci->flags & mask) != mask )
>          {
> -            if ( delivery_mode == dest_LowestPrio )
> -                vcpu = vector_hashing_dest(d, dest, dest_mode,
> -                                           pirq_dpci->gmsi.gvec);
> -            if ( vcpu )
> -                pirq_dpci->gmsi.posted = true;
> +            write_unlock(&d->event_lock);
> +            return -EBUSY;
>          }
> -        if ( vcpu && is_iommu_enabled(d) )
> -            hvm_migrate_pirq(pirq_dpci, vcpu);
>  
> -        /* Use interrupt posting if it is supported. */
> -        if ( iommu_intpost )
> +        /* If pirq is already mapped as vmsi, update guest data/addr. */
> +        if ( pirq_dpci->gmsi.gvec != gvec || pirq_dpci->gmsi.gflags != gflags )
>          {
> -            rc = hvm_pi_update_irte(vcpu, info, pirq_dpci->gmsi.gvec);
> +            /* Directly clear pending EOIs before enabling new MSI info. */
> +            pirq_guest_eoi(info);
>  
> -            if ( rc )
> -            {
> -                pt_irq_destroy_bind(d, pt_irq_bind);
> -                return rc;
> -            }
> +            pirq_dpci->gmsi.gvec = gvec;
> +            pirq_dpci->gmsi.gflags = gflags;
>          }
> +    }
> +    /* Calculate dest_vcpu_id for MSI-type pirq migration. */
> +    dest = MASK_EXTR(pirq_dpci->gmsi.gflags, XEN_DOMCTL_VMSI_X86_DEST_ID_MASK);
> +    dest_mode = pirq_dpci->gmsi.gflags & XEN_DOMCTL_VMSI_X86_DM_MASK;
> +    delivery_mode = MASK_EXTR(pirq_dpci->gmsi.gflags,
> +                               XEN_DOMCTL_VMSI_X86_DELIV_MASK);
> +
> +    dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
> +    pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id;
> +    write_unlock(&d->event_lock);
>  
> -        if ( pt_irq_bind->u.msi.gflags & XEN_DOMCTL_VMSI_X86_UNMASKED )
> +    pirq_dpci->gmsi.posted = false;
> +    vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL;
> +    if ( iommu_intpost )
> +    {
> +        if ( delivery_mode == dest_LowestPrio )
> +            vcpu = vector_hashing_dest(d, dest, dest_mode,
> +                                       pirq_dpci->gmsi.gvec);
> +        if ( vcpu )
> +            pirq_dpci->gmsi.posted = true;
> +    }
> +    if ( vcpu && is_iommu_enabled(d) )
> +        hvm_migrate_pirq(pirq_dpci, vcpu);
> +
> +    /* Use interrupt posting if it is supported. */
> +    if ( iommu_intpost )
> +    {
> +        struct xen_domctl_bind_pt_irq bind = {
> +            .machine_irq = machine_irq,
> +            .irq_type = PT_IRQ_TYPE_MSI,
> +        };
> +
> +        rc = hvm_pi_update_irte(vcpu, info, pirq_dpci->gmsi.gvec);
> +        if ( rc )
>          {
> -            unsigned long flags;
> -            struct irq_desc *desc = pirq_spin_lock_irq_desc(info, &flags);
> +            pt_irq_destroy_bind(d, &bind);
> +            return rc;
> +        }
> +    }
>  
> -            if ( !desc )
> -            {
> -                pt_irq_destroy_bind(d, pt_irq_bind);
> -                return -EINVAL;
> -            }
> +    if ( unmasked )
> +    {
> +        struct xen_domctl_bind_pt_irq bind = {
> +            .machine_irq = machine_irq,
> +            .irq_type = PT_IRQ_TYPE_MSI,
> +        };
> +        unsigned long flags;
> +        struct irq_desc *desc = pirq_spin_lock_irq_desc(info, &flags);
>  
> -            guest_mask_msi_irq(desc, false);
> -            spin_unlock_irqrestore(&desc->lock, flags);
> +        if ( !desc )
> +        {
> +            pt_irq_destroy_bind(d, &bind);
> +            return -EINVAL;
>          }
>  
> -        break;
> +        guest_mask_msi_irq(desc, false);
> +        spin_unlock_irqrestore(&desc->lock, flags);
>      }
>  
> +    return 0;
> +}

For all of the above, going in two steps would again help review quite a
bit: First introduce the new function, but leave excess indentation alone.
Then have a purely mechanical patch removing one indentation level (and
the associated leftover figure braces).

> +int pt_irq_create_bind(
> +    struct domain *d, const struct xen_domctl_bind_pt_irq *pt_irq_bind)
> +{
> +    int rc, pirq = pt_irq_bind->machine_irq;

rc, afaict, is now only used in the more narrow scope below.

> +    if ( pirq < 0 || pirq >= d->nr_pirqs )
> +        return -EINVAL;
> +
> +    switch ( pt_irq_bind->irq_type )
> +    {
> +    case PT_IRQ_TYPE_MSI:
> +        return pt_irq_bind_msi(d, pirq,
> +                               pt_irq_bind->u.msi.gvec,
> +                               pt_irq_bind->u.msi.gflags &
> +                                   ~XEN_DOMCTL_VMSI_X86_UNMASKED,
> +                               pt_irq_bind->u.msi.gtable,
> +                               !!(pt_irq_bind->u.msi.gflags &
> +                                  XEN_DOMCTL_VMSI_X86_UNMASKED));

No need for !!.

Jan