Re: [PATCH v4 5/9] x86/passthrough: Introduce pt_irq_bind_msi() as canonical MSI bind path

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.04.2026 15:54, Julian Vetter wrote:
> Change pt_irq_bind_msi() to accept raw MSI address and data values instead
> of pre-decoded gvec/gflags. Add msi_addr_to_gflags() to decode the
> destination ID and delivery attributes, including the Extended Destination
> ID bits from address[11:5] per Intel convention.
> 
> Update pt_irq_create_bind() to call pt_irq_bind_msi() via the existing
> gvec/gflags interface so domctl-based callers continue to work.
> 
> Signed-off-by: Julian Vetter <julian.vetter@xxxxxxxxxx>
> ---
> Changes in v4:
> - As suggested by Roger replace the v3 approach (v3 patches 2+4) of
>   extending the gflags ABI with XEN_DOMCTL_VMSI_X86_EXT_DEST_ID_MASK and
>   XEN_DOMCTL_VMSI_X86_FULL_DEST() so callers could pass extended bits
>   through XEN_DOMCTL_bind_pt_irq. pt_irq_bind_msi() now accepts raw MSI
>   address + data and decodes the destination internally via
>   msi_addr_to_gflags()
> - Replace the gmsi.gvec + gmsi.gflags fields in struct hvm_pirq_dpci
>   with gmsi.addr + gmsi.data
> - Replace msi_gflags() (v3 vmsi.c helper that packed the extended
>   destination bits into gflags) with msi_addr_to_gflags() which decodes
>   the raw MSI address directly
> - pt_irq_create_bind() now rejects PT_IRQ_TYPE_MSI with -EOPNOTSUPP and
>   all callers are redirected through the DM op path in patch 7

This does not look to match what the patch here does. Peeking ahead, patch
7 doesn't look to convert to -EOPNOTSUPP either.

> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -43,6 +43,7 @@
>  #include <asm/current.h>
>  #include <asm/event.h>
>  #include <asm/io_apic.h>
> +#include <asm/msi.h>
>  
>  static void vmsi_inj_irq(
>      struct vlapic *target,
> @@ -107,12 +108,12 @@ int vmsi_deliver(
>  
>  void vmsi_deliver_pirq(struct domain *d, const struct hvm_pirq_dpci 
> *pirq_dpci)
>  {
> -    uint32_t flags = pirq_dpci->gmsi.gflags;
> -    int vector = pirq_dpci->gmsi.gvec;
> -    uint8_t dest = (uint8_t)flags;
> -    bool dest_mode = flags & XEN_DOMCTL_VMSI_X86_DM_MASK;
> -    uint8_t delivery_mode = MASK_EXTR(flags, XEN_DOMCTL_VMSI_X86_DELIV_MASK);
> -    bool trig_mode = flags & XEN_DOMCTL_VMSI_X86_TRIG_MASK;
> +    uint32_t dest = MSI_ADDR_DEST(pirq_dpci->gmsi.addr);
> +    bool dest_mode = pirq_dpci->gmsi.addr & MSI_ADDR_DESTMODE_MASK;
> +    uint8_t delivery_mode = MASK_EXTR(pirq_dpci->gmsi.data,
> +                                      MSI_DATA_DELIVERY_MODE_MASK);
> +    bool trig_mode = pirq_dpci->gmsi.data & MSI_DATA_TRIGGER_MASK;
> +    int vector = pirq_dpci->gmsi.data & MSI_DATA_VECTOR_MASK;

Please consider types used, as indicated elsewhere before. I don't see how
"vector" could go negative, and I don't see how delivery_mode can sensibly
be uint8_t. Just to name the two most obvious issues; others may be on the
edge.

> @@ -850,17 +830,17 @@ static int vpci_msi_update(const struct pci_dev *pdev, 
> uint32_t data,
>      {
>          uint8_t vector = MASK_EXTR(data, MSI_DATA_VECTOR_MASK);
>          uint8_t vector_mask = 0xff >> (8 - fls(vectors) + 1);
> -        struct xen_domctl_bind_pt_irq bind = {
> -            .machine_irq = pirq + i,
> -            .irq_type = PT_IRQ_TYPE_MSI,
> -            .u.msi.gvec = (vector & ~vector_mask) |
> -                          ((vector + i) & vector_mask),
> -            .u.msi.gflags = msi_gflags(data, address, (mask >> i) & 1),
> -        };
> -        int rc = pt_irq_create_bind(pdev->domain, &bind);
> +        uint8_t gvec = (vector & ~vector_mask) | ((vector + i) & 
> vector_mask);
> +        uint32_t msi_data = (data & ~MSI_DATA_VECTOR_MASK) | gvec;

Please be consistent throughout with the use of MASK_INSR(): Here you're
open-coding MSI_DATA_VECTOR_SHIFT / MSI_DATA_VECTOR_MASK (of which only
the latter should really exist).

> +        int rc = pt_irq_bind_msi(pdev->domain, pirq + i,
> +                                 address, msi_data, 0, !((mask >> i) & 1));

The literal 0 here could do with a /* gtable */ comment.

>          if ( rc )
>          {
> +            struct xen_domctl_bind_pt_irq bind = {
> +                .irq_type = PT_IRQ_TYPE_MSI,
> +                .machine_irq = pirq + i,
> +            };
>              gdprintk(XENLOG_ERR, "%pp: failed to bind PIRQ %u: %d\n",

Blank line please between declaration(s) and statement(s).

> --- a/xen/arch/x86/include/asm/hvm/irq.h
> +++ b/xen/arch/x86/include/asm/hvm/irq.h
> @@ -120,8 +120,8 @@ struct dev_intx_gsi_link {
>  #define HVM_IRQ_DPCI_TRANSLATE       (1u << _HVM_IRQ_DPCI_TRANSLATE_SHIFT)
>  
>  struct hvm_gmsi_info {
> -    uint32_t gvec;
> -    uint32_t gflags;
> +    uint64_t addr;    /* raw MSI address (0xfeexxxxx, includes ext dest ID) 
> */

Is "includes" true? You need to cope with existing code passing rubbish there
(and I think we have said so before). E.g. in vpci_msi_update().

> --- a/xen/arch/x86/include/asm/msi.h
> +++ b/xen/arch/x86/include/asm/msi.h
> @@ -51,8 +51,22 @@
>  #define MSI_ADDR_REDIRECTION_MASK   (1 << MSI_ADDR_REDIRECTION_SHIFT)
>  
>  #define MSI_ADDR_DEST_ID_SHIFT               12
> -#define       MSI_ADDR_DEST_ID_MASK          0x00ff000
> -#define  MSI_ADDR_DEST_ID(dest)              (((dest) << 
> MSI_ADDR_DEST_ID_SHIFT) & MSI_ADDR_DEST_ID_MASK)
> +#define MSI_ADDR_DEST_ID_UPPER_BITS  8

The name doesn't make clear whether the constant describes a number of
bits, or a bit position, or yet something else. From the use below it
looks to instead describe the number of the _lower_ bits, or
(equivalently) the number of bits to shift left the raw value of the
(seven) upper bits. (In the end I think this value would want deriving
anyway, to make crystal clear where it is coming from.)

> +#define MSI_ADDR_DEST_ID_MASK                0x00ff000
> +#define MSI_ADDR_DEST_ID(dest)               (((dest) << 
> MSI_ADDR_DEST_ID_SHIFT) & MSI_ADDR_DEST_ID_MASK)

I understand there's cleanup potential here, but please leave this alone
when you don't need to touch the lines anyway, and when the patch is
already pretty involved. Plus you don't even finish tidying - the too
long like is left there.

> +/*
> + * Intel convention: in physical destination mode bits 11:5 of the MSI
> + * address carry APIC ID bits [14:8] (the "Extended Destination ID"),
> + * extending the addressable range from 8 to 15 bits.
> + */
> +#define MSI_ADDR_EXT_DEST_ID_MASK    0x0000fe0

What reference is "Intel convention" based upon?

> --- a/xen/drivers/passthrough/x86/hvm.c
> +++ b/xen/drivers/passthrough/x86/hvm.c
> @@ -21,6 +21,7 @@
>  #include <xen/event.h>
>  #include <xen/iommu.h>
>  #include <xen/cpu.h>
> +#include <xen/ioreq.h>
>  #include <xen/irq.h>
>  #include <asm/hvm/irq.h>
>  #include <asm/io_apic.h>

Why is this? (And didn't I see patch 7 remove it again, when I peeked there?)

> @@ -367,20 +369,22 @@ static int pt_irq_bind_msi(struct domain *d, uint32_t 
> machine_irq,
>          }
>  
>          /* If pirq is already mapped as vmsi, update guest data/addr. */
> -        if ( pirq_dpci->gmsi.gvec != gvec || pirq_dpci->gmsi.gflags != 
> gflags )
> +        if ( pirq_dpci->gmsi.addr != msi_addr ||
> +             pirq_dpci->gmsi.data != msi_data )

You suddenly compare much more here. To prove correctness of this imo requires
a sentence or two in the description.

>          {
>              /* Directly clear pending EOIs before enabling new MSI info. */
>              pirq_guest_eoi(info);
>  
> -            pirq_dpci->gmsi.gvec = gvec;
> -            pirq_dpci->gmsi.gflags = gflags;
> +            pirq_dpci->gmsi.addr = msi_addr;
> +            pirq_dpci->gmsi.data = msi_data;
>          }
>      }
> +
>      /* Calculate dest_vcpu_id for MSI-type pirq migration. */

Such a blank line would best be inserted when the function is being split out
(or as per the eralier suggesting, maybe when its body is re-indented).

> @@ -448,13 +451,29 @@ int pt_irq_create_bind(
>      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,
> +    {
> +        uint32_t gflags = pt_irq_bind->u.msi.gflags;
> +        uint64_t msi_addr;
> +        uint32_t msi_data;
> +
> +        msi_addr = MSI_ADDR_HEADER |
> +                   MASK_INSR(MASK_EXTR(gflags, 
> XEN_DOMCTL_VMSI_X86_DEST_ID_MASK),
> +                             MSI_ADDR_DEST_ID_MASK) |
> +                   (gflags & XEN_DOMCTL_VMSI_X86_RH_MASK ?
> +                    MSI_ADDR_REDIRECTION_LOWPRI : MSI_ADDR_REDIRECTION_CPU) |
> +                   (gflags & XEN_DOMCTL_VMSI_X86_DM_MASK ?
> +                    MSI_ADDR_DESTMODE_LOGIC : MSI_ADDR_DESTMODE_PHYS);

We prefer to treat the ?: operator a little special, to help readbility:

                   (gflags & XEN_DOMCTL_VMSI_X86_RH_MASK
                    ? MSI_ADDR_REDIRECTION_LOWPRI
                    : MSI_ADDR_REDIRECTION_CPU) |
                   (gflags & XEN_DOMCTL_VMSI_X86_DM_MASK ?
                    ? MSI_ADDR_DESTMODE_LOGIC
                    : MSI_ADDR_DESTMODE_PHYS);

> @@ -617,7 +636,6 @@ int pt_irq_create_bind(
>      }
>  
>      default:
> -        write_unlock(&d->event_lock);
>          return -EOPNOTSUPP;
>      }

Seeing no other locking change here - how is this hunk to be explained?

> @@ -858,11 +876,10 @@ static int cf_check _hvm_dpci_msi_eoi(
>      int vector = (long)arg;
>  
>      if ( (pirq_dpci->flags & HVM_IRQ_DPCI_MACH_MSI) &&
> -         (pirq_dpci->gmsi.gvec == vector) )
> +         ((pirq_dpci->gmsi.data & MSI_DATA_VECTOR_MASK) == vector) )

MASK_EXTR()

>      {
> -        unsigned int dest = MASK_EXTR(pirq_dpci->gmsi.gflags,
> -                                      XEN_DOMCTL_VMSI_X86_DEST_ID_MASK);
> -        bool dest_mode = pirq_dpci->gmsi.gflags & 
> XEN_DOMCTL_VMSI_X86_DM_MASK;
> +        unsigned int dest = MSI_ADDR_DEST(pirq_dpci->gmsi.addr);
> +        bool dest_mode = pirq_dpci->gmsi.addr & XEN_DOMCTL_VMSI_X86_DM_MASK;

If this is now the raw address, how come XEN_DOMCTL_VMSI_X86_DM_MASK can
be used on it?

Jan