[PATCH v4 1/9] x86/vioapic: Add ioapic_check() to validate IO-APIC state before restore

[PATCH v4 1/9] x86/vioapic: Add ioapic_check() to validate IO-APIC state before restore

[Julian Vetter]

[-- Attachment #1: Type: text/plain, Size: 2815 bytes --]

Register a check callback for the IOAPIC HVM save/restore entry,
following the pattern established by vpic_check() for the virtual PIC.
The function first verifies the target domain actually has a virtual
IO-APIC, returning -ENODEV otherwise. It then validates individual
fields of the saved state: the base_address must be non-zero (as 0 is
never valid for the IO-APIC MMIO window), the APIC ID must fit within
its 4-bit hardware field, and ioregsel must address a defined register.

Signed-off-by: Julian Vetter <julian.vetter@vates.tech>
---
Changes in v4:
- Replaced the reserved-bit loop from v3 (iterating all redirection
  table entries and rejecting any with non-zero reserved fields) with
  targeted named-field validation (base_address != 0, APIC ID < 0xF, and
  ioregsel addresses a defined register)
- The extended-destination migration safety check (refusing to restore
  IO-APIC state with ext_dest_id bits set on a domain that does not
  advertise XEN_HVM_CPUID_EXT_DEST_ID) is added in patch 8, once the
  flag exists
---
 xen/arch/x86/hvm/vioapic.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 7c725f9e47..43fb165f84 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -594,6 +594,32 @@ int vioapic_get_trigger_mode(const struct domain *d, unsigned int gsi)
     return vioapic->redirtbl[pin].fields.trig_mode;
 }
 
+static int cf_check ioapic_check(const struct domain *d, hvm_domain_context_t *h)
+{
+    const HVM_SAVE_TYPE(IOAPIC) *s;
+
+    if ( !has_vioapic(d) )
+        return -ENODEV;
+
+    s = hvm_get_entry(IOAPIC, h);
+    if ( !s )
+        return -ENODATA;
+
+    /* base_address of 0 is never valid for the IO-APIC MMIO window. */
+    if ( !s->base_address )
+        return -EINVAL;
+
+    /* IO-APIC APIC ID is a 4-bit field. */
+    if ( s->id > 0xf )
+        return -EINVAL;
+
+    /* ioregsel must address a defined register. */
+    if ( s->ioregsel > VIOAPIC_REG_RTE0 + (ARRAY_SIZE(s->redirtbl) - 1) * 2 + 1 )
+        return -EINVAL;
+
+    return 0;
+}
+
 static int cf_check ioapic_save(struct vcpu *v, hvm_domain_context_t *h)
 {
     const struct domain *d = v->domain;
@@ -630,7 +656,7 @@ static int cf_check ioapic_load(struct domain *d, hvm_domain_context_t *h)
     return 0;
 }
 
-HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, NULL, ioapic_load, 1,
+HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, ioapic_check, ioapic_load, 1,
                           HVMSR_PER_DOM);
 
 void vioapic_reset(struct domain *d)
-- 
2.53.0



--
Julian Vetter | Vates Hypervisor & Kernel Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v4 1/9] x86/vioapic: Add ioapic_check() to validate IO-APIC state before restore

[Teddy Astie]

[-- Attachment #1: Type: text/plain, Size: 3175 bytes --]

Le 27/04/2026 à 15:57, Julian Vetter a écrit :
> Register a check callback for the IOAPIC HVM save/restore entry,
> following the pattern established by vpic_check() for the virtual PIC.
> The function first verifies the target domain actually has a virtual
> IO-APIC, returning -ENODEV otherwise. It then validates individual
> fields of the saved state: the base_address must be non-zero (as 0 is
> never valid for the IO-APIC MMIO window), the APIC ID must fit within
> its 4-bit hardware field, and ioregsel must address a defined register.
> 
> Signed-off-by: Julian Vetter <julian.vetter@vates.tech>
> ---
> Changes in v4:
> - Replaced the reserved-bit loop from v3 (iterating all redirection
>    table entries and rejecting any with non-zero reserved fields) with
>    targeted named-field validation (base_address != 0, APIC ID < 0xF, and
>    ioregsel addresses a defined register)
> - The extended-destination migration safety check (refusing to restore
>    IO-APIC state with ext_dest_id bits set on a domain that does not
>    advertise XEN_HVM_CPUID_EXT_DEST_ID) is added in patch 8, once the
>    flag exists
> ---
>   xen/arch/x86/hvm/vioapic.c | 28 +++++++++++++++++++++++++++-
>   1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
> index 7c725f9e47..43fb165f84 100644
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -594,6 +594,32 @@ int vioapic_get_trigger_mode(const struct domain *d, unsigned int gsi)
>       return vioapic->redirtbl[pin].fields.trig_mode;
>   }
>   
> +static int cf_check ioapic_check(const struct domain *d, hvm_domain_context_t *h)
> +{
> +    const HVM_SAVE_TYPE(IOAPIC) *s;
> +
> +    if ( !has_vioapic(d) )
> +        return -ENODEV;
> +
> +    s = hvm_get_entry(IOAPIC, h);
> +    if ( !s )
> +        return -ENODATA;
> +
> +    /* base_address of 0 is never valid for the IO-APIC MMIO window. */
> +    if ( !s->base_address )
> +        return -EINVAL;
> +
> +    /* IO-APIC APIC ID is a 4-bit field. */
> +    if ( s->id > 0xf )
> +        return -EINVAL;
> +
> +    /* ioregsel must address a defined register. */
> +    if ( s->ioregsel > VIOAPIC_REG_RTE0 + (ARRAY_SIZE(s->redirtbl) - 1) * 2 + 1 )

you can rewrite it as

s->ioregsel >= VIOAPIC_REG_RTE0 + ARRAY_SIZE(s->redirtbl) * 2

> +        return -EINVAL;
> +
> +    return 0;
> +}
> +
>   static int cf_check ioapic_save(struct vcpu *v, hvm_domain_context_t *h)
>   {
>       const struct domain *d = v->domain;
> @@ -630,7 +656,7 @@ static int cf_check ioapic_load(struct domain *d, hvm_domain_context_t *h)
>       return 0;
>   }
>   
> -HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, NULL, ioapic_load, 1,
> +HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, ioapic_check, ioapic_load, 1,
>                             HVMSR_PER_DOM);

You think you can relax some of the checks in ioapic_load as you moved 
some of them in ioapic_check.

>   
>   void vioapic_reset(struct domain *d)

Teddy


--
 | Vates

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v4 1/9] x86/vioapic: Add ioapic_check() to validate IO-APIC state before restore

[Jan Beulich]

On 27.04.2026 15:53, Julian Vetter wrote:
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -594,6 +594,32 @@ int vioapic_get_trigger_mode(const struct domain *d, unsigned int gsi)
>      return vioapic->redirtbl[pin].fields.trig_mode;
>  }
>  
> +static int cf_check ioapic_check(const struct domain *d, hvm_domain_context_t *h)
> +{
> +    const HVM_SAVE_TYPE(IOAPIC) *s;
> +
> +    if ( !has_vioapic(d) )
> +        return -ENODEV;
> +
> +    s = hvm_get_entry(IOAPIC, h);
> +    if ( !s )
> +        return -ENODATA;
> +
> +    /* base_address of 0 is never valid for the IO-APIC MMIO window. */
> +    if ( !s->base_address )
> +        return -EINVAL;

It also wants to be suitably aligned, and it wants to be below the domain's
phys-addr limit.

> +    /* IO-APIC APIC ID is a 4-bit field. */
> +    if ( s->id > 0xf )
> +        return -EINVAL;

This wants to remain in sync with what vioapic_write_indirect() does, i.e.
there likely wants to be a cross-referencing comment at each site, and the
logic likely also wants to be similar here.

> +    /* ioregsel must address a defined register. */
> +    if ( s->ioregsel > VIOAPIC_REG_RTE0 + (ARRAY_SIZE(s->redirtbl) - 1) * 2 + 1 )
> +        return -EINVAL;

Why would this be? vioapic_write() doesn't apply any restrictions.

> +    return 0;
> +}

What about the redirtbl[] entries? Values vioapic_write_redirent() would
never store shouldn't be accepted here.

Jan

[PATCH v4 2/9] x86/passthrough: Wrap pt_irq_create_bind() restart block in braces

[Julian Vetter]

[-- Attachment #1: Type: text/plain, Size: 4269 bytes --]

Enclose the restart/retry block in pt_irq_create_bind() in an explicit
compound statement to prepare for its extraction into a helper function.
No functional change.

Signed-off-by: Julian Vetter <julian.vetter@vates.tech>
---
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 | 80 ++++++++++++++++---------------
 1 file changed, 41 insertions(+), 39 deletions(-)

diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
index b73bb55055..691fa1b2c7 100644
--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -229,52 +229,54 @@ 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;
+        }
+
+        info = pirq_get_info(d, pirq);
+        if ( !info )
         {
             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;
-    }
-
-    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;
+        }
     }
 
     switch ( pt_irq_bind->irq_type )
-- 
2.53.0



--
Julian Vetter | Vates Hypervisor & Kernel Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v4 2/9] x86/passthrough: Wrap pt_irq_create_bind() restart block in braces

[Teddy Astie]

[-- Attachment #1: Type: text/plain, Size: 4639 bytes --]

Le 27/04/2026 à 15:57, Julian Vetter a écrit :
> Enclose the restart/retry block in pt_irq_create_bind() in an explicit
> compound statement to prepare for its extraction into a helper function.
> No functional change.
> 
> Signed-off-by: Julian Vetter <julian.vetter@vates.tech>
> ---
> 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 | 80 ++++++++++++++++---------------
>   1 file changed, 41 insertions(+), 39 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
> index b73bb55055..691fa1b2c7 100644
> --- a/xen/drivers/passthrough/x86/hvm.c
> +++ b/xen/drivers/passthrough/x86/hvm.c
> @@ -229,52 +229,54 @@ 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;
> +        }
> +
> +        info = pirq_get_info(d, pirq);
> +        if ( !info )
>           {
>               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;
> -    }
> -
> -    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;
> +        }
>       }
>   
>       switch ( pt_irq_bind->irq_type )

shows up as whitespace only changes (aside brackets)

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


--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v4 2/9] x86/passthrough: Wrap pt_irq_create_bind() restart block in braces

[Jan Beulich]

On 27.04.2026 15:53, 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.
> No functional change.
> 
> Signed-off-by: Julian Vetter <julian.vetter@vates.tech>
> ---
> 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

I've been staring at patch 5 for quite some time, but I can't make the
connection. Perhaps you mean patches 3 or 4, but without you quoting the
title of the patch in question that would be pure guesswork.

Jan

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

[Julian Vetter]

[-- Attachment #1: Type: text/plain, Size: 4416 bytes --]

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@vates.tech>
---
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 );
+}
+
+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 )
     {
-- 
2.53.0



--
Julian Vetter | Vates Hypervisor & Kernel Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

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

[Teddy Astie]

[-- Attachment #1: Type: text/plain, Size: 4894 bytes --]

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@vates.tech>
> ---
> 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@vates.tech>


--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

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

[Jan Beulich]

On 27.04.2026 15:54, Julian Vetter wrote:
> @@ -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 )

This wants to be part of the patch increasing indentation.

> @@ -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).
>           */

This would also better be part of the earlier patch. And this block needs re-
flowing there as well.

>          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 );

do { } while ( false ) (as Teddy suggests) wouldn't be much better. Why not
simply for ( ; ; ), as we have it in quite a few places elsewhere? Yet then
I'm not overly happy to see this secondary change (to complicated code) be
folded into a change of entirely different purpose. Please consider
(further) splitting.

Jan

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

[Julian Vetter]

[-- Attachment #1: Type: text/plain, Size: 13230 bytes --]

No functional change: move the PT_IRQ_TYPE_MSI case of pt_irq_create_bind()
into a new static helper pt_irq_bind_msi() taking the same gvec/gflags/gtable
parameters. Restructure pt_irq_create_bind() so the MSI case delegates to the
helper and pt_irq_dpci_setup() is called inside each case rather than shared.

Signed-off-by: Julian Vetter <julian.vetter@vates.tech>
---
Changes in v4:
- New patch
- Split out as a preparatory no-functional-change step so that the
  interface change in patch 5 (switching pt_irq_bind_msi() from gvec +
  gflags to raw MSI addr + data) shows as a clean diff against an
  already-extracted helper, rather than being tangled with the 'case
  PT_IRQ_TYPE_MSI' code
---
 xen/drivers/passthrough/x86/hvm.c | 255 ++++++++++++++++--------------
 1 file changed, 140 insertions(+), 115 deletions(-)

diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
index 19463c3406..eff1e8a79e 100644
--- 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,
+                            bool unmasked)
 {
     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;
 
-    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 */
+        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;
+}
+
+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;
+
+    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));
+
     case PT_IRQ_TYPE_PCI:
     case PT_IRQ_TYPE_MSI_TRANSLATE:
     {
+        struct hvm_irq_dpci *hvm_irq_dpci;
+        struct hvm_pirq_dpci *pirq_dpci;
+        struct pirq *info;
         struct dev_intx_gsi_link *digl = NULL;
         struct hvm_girq_dpci_mapping *girq = NULL;
         unsigned int guest_gsi;
 
+        rc = pt_irq_dpci_setup(d, pirq, &hvm_irq_dpci, &pirq_dpci, &info);
+        if ( rc )
+            return rc;
+
         /*
          * Mapping GSIs for the hardware domain is different than doing it for
          * an unpriviledged guest, the hardware domain is only allowed to
-- 
2.53.0



--
Julian Vetter | Vates Hypervisor & Kernel Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

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

[Teddy Astie]

[-- Attachment #1: Type: text/plain, Size: 14131 bytes --]

Le 27/04/2026 à 15:57, Julian Vetter a écrit :
> No functional change: move the PT_IRQ_TYPE_MSI case of pt_irq_create_bind()
> into a new static helper pt_irq_bind_msi() taking the same gvec/gflags/gtable
> parameters. Restructure pt_irq_create_bind() so the MSI case delegates to the
> helper and pt_irq_dpci_setup() is called inside each case rather than shared.
> 
> Signed-off-by: Julian Vetter <julian.vetter@vates.tech>
> ---
> Changes in v4:
> - New patch
> - Split out as a preparatory no-functional-change step so that the
>    interface change in patch 5 (switching pt_irq_bind_msi() from gvec +
>    gflags to raw MSI addr + data) shows as a clean diff against an
>    already-extracted helper, rather than being tangled with the 'case
>    PT_IRQ_TYPE_MSI' code
> ---
>   xen/drivers/passthrough/x86/hvm.c | 255 ++++++++++++++++--------------
>   1 file changed, 140 insertions(+), 115 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
> index 19463c3406..eff1e8a79e 100644
> --- 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,
> +                            bool unmasked)
>   {
>       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 )

is that (unsigned int) cast required ?

>           return -EINVAL;
>   
> -    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 */
> +        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;
> +}
> +
> +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;
> +
> +    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));
> +
>       case PT_IRQ_TYPE_PCI:
>       case PT_IRQ_TYPE_MSI_TRANSLATE:
>       {
> +        struct hvm_irq_dpci *hvm_irq_dpci;
> +        struct hvm_pirq_dpci *pirq_dpci;
> +        struct pirq *info;
>           struct dev_intx_gsi_link *digl = NULL;
>           struct hvm_girq_dpci_mapping *girq = NULL;
>           unsigned int guest_gsi;
>   
> +        rc = pt_irq_dpci_setup(d, pirq, &hvm_irq_dpci, &pirq_dpci, &info);
> +        if ( rc )
> +            return rc;
> +
>           /*
>            * Mapping GSIs for the hardware domain is different than doing it for
>            * an unpriviledged guest, the hardware domain is only allowed t
Aside that, the rest looks good to me.

with or without the cast change :
Reviewed-by: Teddy Astie <teddy.astie@vates.tech>

Teddy


--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

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

[Jan Beulich]

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

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

[Julian Vetter]

[-- Attachment #1: Type: text/plain, Size: 15950 bytes --]

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@vates.tech>
---
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
- As suggested by Roger adapt the comment in msi.h in regards to the
  extended destination encoding since it's not part of any specification
---
 xen/arch/x86/hvm/vmsi.c            | 50 ++++++------------
 xen/arch/x86/include/asm/hvm/irq.h |  4 +-
 xen/arch/x86/include/asm/msi.h     | 18 ++++++-
 xen/drivers/passthrough/x86/hvm.c  | 83 ++++++++++++++++++------------
 xen/include/xen/iommu.h            |  3 ++
 5 files changed, 86 insertions(+), 72 deletions(-)

diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 27b1f089e2..2a4b97e2e1 100644
--- 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;
 
     HVM_DBG_LOG(DBG_LEVEL_IOAPIC,
                 "msi: dest=%x dest_mode=%x delivery_mode=%x "
@@ -793,27 +794,6 @@ void msix_write_completion(struct vcpu *v)
 }
 
 #ifdef CONFIG_HAS_VPCI
-static unsigned int msi_gflags(uint16_t data, uint64_t addr, bool masked)
-{
-    /*
-     * We need to use the DOMCTL constants here because the output of this
-     * function is used as input to pt_irq_create_bind, which also takes the
-     * input from the DOMCTL itself.
-     */
-    return MASK_INSR(MASK_EXTR(addr, MSI_ADDR_DEST_ID_MASK),
-                     XEN_DOMCTL_VMSI_X86_DEST_ID_MASK) |
-           MASK_INSR(MASK_EXTR(addr, MSI_ADDR_REDIRECTION_MASK),
-                     XEN_DOMCTL_VMSI_X86_RH_MASK) |
-           MASK_INSR(MASK_EXTR(addr, MSI_ADDR_DESTMODE_MASK),
-                     XEN_DOMCTL_VMSI_X86_DM_MASK) |
-           MASK_INSR(MASK_EXTR(data, MSI_DATA_DELIVERY_MODE_MASK),
-                     XEN_DOMCTL_VMSI_X86_DELIV_MASK) |
-           MASK_INSR(MASK_EXTR(data, MSI_DATA_TRIGGER_MASK),
-                     XEN_DOMCTL_VMSI_X86_TRIG_MASK) |
-           /* NB: by default MSI vectors are bound masked. */
-           (masked ? 0 : XEN_DOMCTL_VMSI_X86_UNMASKED);
-}
-
 static void vpci_mask_pirq(struct domain *d, int pirq, bool mask)
 {
     unsigned long flags;
@@ -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;
+        int rc = pt_irq_bind_msi(pdev->domain, pirq + i,
+                                 address, msi_data, 0, !((mask >> i) & 1));
 
         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",
                      &pdev->sbdf, pirq + i, rc);
             while ( bind.machine_irq-- > pirq )
diff --git a/xen/arch/x86/include/asm/hvm/irq.h b/xen/arch/x86/include/asm/hvm/irq.h
index 77595fb3f4..c50eee9996 100644
--- 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) */
+    uint32_t data;    /* raw MSI data (vector, delivery mode, trigger mode) */
     int dest_vcpu_id; /* -1 :multi-dest, non-negative: dest_vcpu_id */
     bool posted; /* directly deliver to guest via VT-d PI? */
 };
diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
index 00059d4a3a..93aaf20e27 100644
--- 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
+#define MSI_ADDR_DEST_ID_MASK		0x00ff000
+#define MSI_ADDR_DEST_ID(dest)		(((dest) << MSI_ADDR_DEST_ID_SHIFT) & MSI_ADDR_DEST_ID_MASK)
+
+/*
+ * 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
+
+/* Extract the combined 15-bit destination ID from an MSI address. */
+#define MSI_ADDR_DEST(addr) \
+    (MASK_EXTR((addr), MSI_ADDR_DEST_ID_MASK) | \
+     (MASK_EXTR((addr), MSI_ADDR_EXT_DEST_ID_MASK) << \
+      MSI_ADDR_DEST_ID_UPPER_BITS))
 
 /* MAX fixed pages reserved for mapping MSIX tables. */
 #define FIX_MSIX_MAX_PAGES              512
diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
index eff1e8a79e..026534530f 100644
--- 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>
@@ -290,14 +291,15 @@ static int pt_irq_dpci_setup(struct domain *d, unsigned int pirq,
     } while ( true );
 }
 
-static int pt_irq_bind_msi(struct domain *d, uint32_t machine_irq,
-                            uint8_t gvec, uint32_t gflags, uint64_t gtable,
-                            bool unmasked)
+int pt_irq_bind_msi(struct domain *d, uint32_t machine_irq,
+                    uint64_t msi_addr, uint32_t msi_data,
+                    uint64_t gtable, bool unmasked)
 {
     struct hvm_irq_dpci *hvm_irq_dpci;
     struct hvm_pirq_dpci *pirq_dpci;
     struct pirq *info;
-    uint8_t dest, delivery_mode;
+    uint8_t gvec, delivery_mode;
+    uint32_t dest;
     bool dest_mode;
     int dest_vcpu_id, rc;
     const struct vcpu *vcpu;
@@ -313,8 +315,8 @@ static int pt_irq_bind_msi(struct domain *d, uint32_t machine_irq,
     {
         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;
+        pirq_dpci->gmsi.addr = msi_addr;
+        pirq_dpci->gmsi.data = msi_data;
         /*
          * 'pt_irq_bind_msi' can be called after 'pt_irq_destroy_bind'.
          * The 'pirq_cleanup_check' which would free the structure is only
@@ -346,8 +348,8 @@ static int pt_irq_bind_msi(struct domain *d, uint32_t machine_irq,
         }
         if ( unlikely(rc) )
         {
-            pirq_dpci->gmsi.gflags = 0;
-            pirq_dpci->gmsi.gvec = 0;
+            pirq_dpci->gmsi.addr = 0;
+            pirq_dpci->gmsi.data = 0;
             pirq_dpci->dom = NULL;
             pirq_dpci->flags = 0;
             if ( !info->evtchn )
@@ -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 )
         {
             /* 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. */
-    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);
+    gvec = msi_data & MSI_DATA_VECTOR_MASK;
+    dest = MSI_ADDR_DEST(msi_addr);
+    dest_mode = msi_addr & MSI_ADDR_DESTMODE_MASK;
+    delivery_mode = MASK_EXTR(msi_data, MSI_DATA_DELIVERY_MODE_MASK);
 
     dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
     pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id;
@@ -391,8 +395,7 @@ static int pt_irq_bind_msi(struct domain *d, uint32_t machine_irq,
     if ( iommu_intpost )
     {
         if ( delivery_mode == dest_LowestPrio )
-            vcpu = vector_hashing_dest(d, dest, dest_mode,
-                                       pirq_dpci->gmsi.gvec);
+            vcpu = vector_hashing_dest(d, dest, dest_mode, gvec);
         if ( vcpu )
             pirq_dpci->gmsi.posted = true;
     }
@@ -407,7 +410,7 @@ static int pt_irq_bind_msi(struct domain *d, uint32_t machine_irq,
             .irq_type = PT_IRQ_TYPE_MSI,
         };
 
-        rc = hvm_pi_update_irte(vcpu, info, pirq_dpci->gmsi.gvec);
+        rc = hvm_pi_update_irte(vcpu, info, gvec);
         if ( rc )
         {
             pt_irq_destroy_bind(d, &bind);
@@ -417,15 +420,15 @@ static int pt_irq_bind_msi(struct domain *d, uint32_t machine_irq,
 
     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);
 
         if ( !desc )
         {
+            struct xen_domctl_bind_pt_irq bind = {
+                .machine_irq = machine_irq,
+                .irq_type = PT_IRQ_TYPE_MSI,
+            };
             pt_irq_destroy_bind(d, &bind);
             return -EINVAL;
         }
@@ -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);
+        msi_data = pt_irq_bind->u.msi.gvec |
+                   MASK_INSR(MASK_EXTR(gflags, XEN_DOMCTL_VMSI_X86_DELIV_MASK),
+                             MSI_DATA_DELIVERY_MODE_MASK) |
+                   (gflags & XEN_DOMCTL_VMSI_X86_TRIG_MASK ?
+                    MSI_DATA_TRIGGER_LEVEL : 0);
+
+        return pt_irq_bind_msi(d, pt_irq_bind->machine_irq,
+                               msi_addr, msi_data,
                                pt_irq_bind->u.msi.gtable,
-                               !!(pt_irq_bind->u.msi.gflags &
-                                  XEN_DOMCTL_VMSI_X86_UNMASKED));
+                               !!(gflags & XEN_DOMCTL_VMSI_X86_UNMASKED));
+    }
 
     case PT_IRQ_TYPE_PCI:
     case PT_IRQ_TYPE_MSI_TRANSLATE:
@@ -617,7 +636,6 @@ int pt_irq_create_bind(
     }
 
     default:
-        write_unlock(&d->event_lock);
         return -EOPNOTSUPP;
     }
 
@@ -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) )
     {
-        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 ( vlapic_match_dest(vcpu_vlapic(current), NULL, 0, dest,
                                dest_mode) )
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 37c4a1dc82..4672d114e3 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -222,6 +222,9 @@ int pt_irq_create_bind(struct domain *d,
                        const struct xen_domctl_bind_pt_irq *pt_irq_bind);
 int pt_irq_destroy_bind(struct domain *d,
                         const struct xen_domctl_bind_pt_irq *pt_irq_bind);
+int pt_irq_bind_msi(struct domain *d, uint32_t machine_irq,
+                    uint64_t msi_addr, uint32_t msi_data,
+                    uint64_t gtable, bool unmasked);
 
 struct hvm_irq_dpci *domain_get_irq_dpci(const struct domain *d);
 void free_hvm_irq_dpci(struct hvm_irq_dpci *dpci);
-- 
2.53.0



--
Julian Vetter | Vates Hypervisor & Kernel Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

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

[Teddy Astie]

[-- Attachment #1: Type: text/plain, Size: 16945 bytes --]

Le 27/04/2026 à 15:57, Julian Vetter a écrit :
> 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@vates.tech>
> ---
> 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
> - As suggested by Roger adapt the comment in msi.h in regards to the
>    extended destination encoding since it's not part of any specification
> ---
>   xen/arch/x86/hvm/vmsi.c            | 50 ++++++------------
>   xen/arch/x86/include/asm/hvm/irq.h |  4 +-
>   xen/arch/x86/include/asm/msi.h     | 18 ++++++-
>   xen/drivers/passthrough/x86/hvm.c  | 83 ++++++++++++++++++------------
>   xen/include/xen/iommu.h            |  3 ++
>   5 files changed, 86 insertions(+), 72 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
> index 27b1f089e2..2a4b97e2e1 100644
> --- 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;
>   
>       HVM_DBG_LOG(DBG_LEVEL_IOAPIC,
>                   "msi: dest=%x dest_mode=%x delivery_mode=%x "
> @@ -793,27 +794,6 @@ void msix_write_completion(struct vcpu *v)
>   }
>   
>   #ifdef CONFIG_HAS_VPCI
> -static unsigned int msi_gflags(uint16_t data, uint64_t addr, bool masked)
> -{
> -    /*
> -     * We need to use the DOMCTL constants here because the output of this
> -     * function is used as input to pt_irq_create_bind, which also takes the
> -     * input from the DOMCTL itself.
> -     */
> -    return MASK_INSR(MASK_EXTR(addr, MSI_ADDR_DEST_ID_MASK),
> -                     XEN_DOMCTL_VMSI_X86_DEST_ID_MASK) |
> -           MASK_INSR(MASK_EXTR(addr, MSI_ADDR_REDIRECTION_MASK),
> -                     XEN_DOMCTL_VMSI_X86_RH_MASK) |
> -           MASK_INSR(MASK_EXTR(addr, MSI_ADDR_DESTMODE_MASK),
> -                     XEN_DOMCTL_VMSI_X86_DM_MASK) |
> -           MASK_INSR(MASK_EXTR(data, MSI_DATA_DELIVERY_MODE_MASK),
> -                     XEN_DOMCTL_VMSI_X86_DELIV_MASK) |
> -           MASK_INSR(MASK_EXTR(data, MSI_DATA_TRIGGER_MASK),
> -                     XEN_DOMCTL_VMSI_X86_TRIG_MASK) |
> -           /* NB: by default MSI vectors are bound masked. */
> -           (masked ? 0 : XEN_DOMCTL_VMSI_X86_UNMASKED);
> -}
> -
>   static void vpci_mask_pirq(struct domain *d, int pirq, bool mask)
>   {
>       unsigned long flags;
> @@ -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;
> +        int rc = pt_irq_bind_msi(pdev->domain, pirq + i,
> +                                 address, msi_data, 0, !((mask >> i) & 1));
>   
>           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",
>                        &pdev->sbdf, pirq + i, rc);
>               while ( bind.machine_irq-- > pirq )
> diff --git a/xen/arch/x86/include/asm/hvm/irq.h b/xen/arch/x86/include/asm/hvm/irq.h
> index 77595fb3f4..c50eee9996 100644
> --- 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) */
> +    uint32_t data;    /* raw MSI data (vector, delivery mode, trigger mode) */
>       int dest_vcpu_id; /* -1 :multi-dest, non-negative: dest_vcpu_id */
>       bool posted; /* directly deliver to guest via VT-d PI? */
>   };
> diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
> index 00059d4a3a..93aaf20e27 100644
> --- 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
> +#define MSI_ADDR_DEST_ID_MASK		0x00ff000
> +#define MSI_ADDR_DEST_ID(dest)		(((dest) << MSI_ADDR_DEST_ID_SHIFT) & MSI_ADDR_DEST_ID_MASK)
> +
> +/*
> + * 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
> +
> +/* Extract the combined 15-bit destination ID from an MSI address. */
> +#define MSI_ADDR_DEST(addr) \
> +    (MASK_EXTR((addr), MSI_ADDR_DEST_ID_MASK) | \
> +     (MASK_EXTR((addr), MSI_ADDR_EXT_DEST_ID_MASK) << \
> +      MSI_ADDR_DEST_ID_UPPER_BITS))
>   
>   /* MAX fixed pages reserved for mapping MSIX tables. */
>   #define FIX_MSIX_MAX_PAGES              512
> diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
> index eff1e8a79e..026534530f 100644
> --- 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>
> @@ -290,14 +291,15 @@ static int pt_irq_dpci_setup(struct domain *d, unsigned int pirq,
>       } while ( true );
>   }
>   
> -static int pt_irq_bind_msi(struct domain *d, uint32_t machine_irq,
> -                            uint8_t gvec, uint32_t gflags, uint64_t gtable,
> -                            bool unmasked)
> +int pt_irq_bind_msi(struct domain *d, uint32_t machine_irq,
> +                    uint64_t msi_addr, uint32_t msi_data,
> +                    uint64_t gtable, bool unmasked)
>   {
>       struct hvm_irq_dpci *hvm_irq_dpci;
>       struct hvm_pirq_dpci *pirq_dpci;
>       struct pirq *info;
> -    uint8_t dest, delivery_mode;
> +    uint8_t gvec, delivery_mode;
> +    uint32_t dest;
>       bool dest_mode;
>       int dest_vcpu_id, rc;
>       const struct vcpu *vcpu;
> @@ -313,8 +315,8 @@ static int pt_irq_bind_msi(struct domain *d, uint32_t machine_irq,
>       {
>           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;
> +        pirq_dpci->gmsi.addr = msi_addr;
> +        pirq_dpci->gmsi.data = msi_data;
>           /*
>            * 'pt_irq_bind_msi' can be called after 'pt_irq_destroy_bind'.
>            * The 'pirq_cleanup_check' which would free the structure is only
> @@ -346,8 +348,8 @@ static int pt_irq_bind_msi(struct domain *d, uint32_t machine_irq,
>           }
>           if ( unlikely(rc) )
>           {
> -            pirq_dpci->gmsi.gflags = 0;
> -            pirq_dpci->gmsi.gvec = 0;
> +            pirq_dpci->gmsi.addr = 0;
> +            pirq_dpci->gmsi.data = 0;
>               pirq_dpci->dom = NULL;
>               pirq_dpci->flags = 0;
>               if ( !info->evtchn )
> @@ -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 )
>           {
>               /* 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. */
> -    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);
> +    gvec = msi_data & MSI_DATA_VECTOR_MASK;
> +    dest = MSI_ADDR_DEST(msi_addr);
> +    dest_mode = msi_addr & MSI_ADDR_DESTMODE_MASK;
> +    delivery_mode = MASK_EXTR(msi_data, MSI_DATA_DELIVERY_MODE_MASK);
>   
>       dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
>       pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id;
> @@ -391,8 +395,7 @@ static int pt_irq_bind_msi(struct domain *d, uint32_t machine_irq,
>       if ( iommu_intpost )
>       {
>           if ( delivery_mode == dest_LowestPrio )
> -            vcpu = vector_hashing_dest(d, dest, dest_mode,
> -                                       pirq_dpci->gmsi.gvec);
> +            vcpu = vector_hashing_dest(d, dest, dest_mode, gvec);
>           if ( vcpu )
>               pirq_dpci->gmsi.posted = true;
>       }
> @@ -407,7 +410,7 @@ static int pt_irq_bind_msi(struct domain *d, uint32_t machine_irq,
>               .irq_type = PT_IRQ_TYPE_MSI,
>           };
>   
> -        rc = hvm_pi_update_irte(vcpu, info, pirq_dpci->gmsi.gvec);
> +        rc = hvm_pi_update_irte(vcpu, info, gvec);
>           if ( rc )
>           {
>               pt_irq_destroy_bind(d, &bind);
> @@ -417,15 +420,15 @@ static int pt_irq_bind_msi(struct domain *d, uint32_t machine_irq,
>   
>       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);
>   
>           if ( !desc )
>           {
> +            struct xen_domctl_bind_pt_irq bind = {
> +                .machine_irq = machine_irq,
> +                .irq_type = PT_IRQ_TYPE_MSI,
> +            };
>               pt_irq_destroy_bind(d, &bind);
>               return -EINVAL;
>           }
> @@ -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);
> +        msi_data = pt_irq_bind->u.msi.gvec |
> +                   MASK_INSR(MASK_EXTR(gflags, XEN_DOMCTL_VMSI_X86_DELIV_MASK),
> +                             MSI_DATA_DELIVERY_MODE_MASK) |
> +                   (gflags & XEN_DOMCTL_VMSI_X86_TRIG_MASK ?
> +                    MSI_DATA_TRIGGER_LEVEL : 0);
> +
> +        return pt_irq_bind_msi(d, pt_irq_bind->machine_irq,
> +                               msi_addr, msi_data,
>                                  pt_irq_bind->u.msi.gtable,
> -                               !!(pt_irq_bind->u.msi.gflags &
> -                                  XEN_DOMCTL_VMSI_X86_UNMASKED));
> +                               !!(gflags & XEN_DOMCTL_VMSI_X86_UNMASKED));
> +    }
>   
>       case PT_IRQ_TYPE_PCI:
>       case PT_IRQ_TYPE_MSI_TRANSLATE:
> @@ -617,7 +636,6 @@ int pt_irq_create_bind(
>       }
>   
>       default:
> -        write_unlock(&d->event_lock);
>           return -EOPNOTSUPP;
>       }
>   
> @@ -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) )
>       {
> -        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 ( vlapic_match_dest(vcpu_vlapic(current), NULL, 0, dest,
>                                  dest_mode) )
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 37c4a1dc82..4672d114e3 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -222,6 +222,9 @@ int pt_irq_create_bind(struct domain *d,
>                          const struct xen_domctl_bind_pt_irq *pt_irq_bind);
>   int pt_irq_destroy_bind(struct domain *d,
>                           const struct xen_domctl_bind_pt_irq *pt_irq_bind);
> +int pt_irq_bind_msi(struct domain *d, uint32_t machine_irq,
> +                    uint64_t msi_addr, uint32_t msi_data,
> +                    uint64_t gtable, bool unmasked);
>   
>   struct hvm_irq_dpci *domain_get_irq_dpci(const struct domain *d);
>   void free_hvm_irq_dpci(struct hvm_irq_dpci *dpci);

There is a lot of bitwise manipulations there, and I wonder if using 
bitfields could help here ?

Teddy


--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

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

[Jan Beulich]

On 27.04.2026 15:54, Julian Vetter wrote:
> @@ -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);
> +        msi_data = pt_irq_bind->u.msi.gvec |
> +                   MASK_INSR(MASK_EXTR(gflags, XEN_DOMCTL_VMSI_X86_DELIV_MASK),
> +                             MSI_DATA_DELIVERY_MODE_MASK) |
> +                   (gflags & XEN_DOMCTL_VMSI_X86_TRIG_MASK ?
> +                    MSI_DATA_TRIGGER_LEVEL : 0);
> +
> +        return pt_irq_bind_msi(d, pt_irq_bind->machine_irq,
> +                               msi_addr, msi_data,
>                                 pt_irq_bind->u.msi.gtable,
> -                               !!(pt_irq_bind->u.msi.gflags &
> -                                  XEN_DOMCTL_VMSI_X86_UNMASKED));
> +                               !!(gflags & XEN_DOMCTL_VMSI_X86_UNMASKED));
> +    }
>  
>      case PT_IRQ_TYPE_PCI:
>      case PT_IRQ_TYPE_MSI_TRANSLATE:
> @@ -617,7 +636,6 @@ int pt_irq_create_bind(
>      }
>  
>      default:
> -        write_unlock(&d->event_lock);
>          return -EOPNOTSUPP;
>      }

Just as a preliminary comment, as I had to come look here for patch 2:
With no other locking changes in this function, how can the above be
correct? Does this hunk belong into another patch?

Jan

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

[Jan Beulich]

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@vates.tech>
> ---
> 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

[PATCH v4 6/9] x86/hvm: Support extended destination IDs in virtual MSI and IO-APIC

[Julian Vetter]

[-- Attachment #1: Type: text/plain, Size: 7119 bytes --]

Add IO_APIC_REDIR_DEST_MASK and IO_APIC_REDIR_EXT_DEST_MASK for the
standard and extended destination fields of the IO-APIC RTE, and a
VIOAPIC_RTE_DEST() helper that extracts the combined 15-bit destination
ID. Extend the IO-APIC RTE save/restore struct with an ext_dest_id field
so migration preserves the extended bits.

Use the newly defined masks to extract the full 15-bit destination ID
from guest MSI addresses and IO-APIC RTEs. In hvm_inject_msi() combine
the standard bits [19:12] with the extended bits [11:5] of the MSI
address into a 15-bit destination ID for LAPIC delivery. Widen the dest
parameter of vmsi_deliver() and hvm_girq_dest_2_vcpu_id() from uint8_t
to uint32_t to accommodate the larger range. In vioapic_deliver() read
the combined 15-bit destination using the VIOAPIC_RTE_DEST() macro.
Extend ioapic_check() to check for extended destination bits set in a
domain that does not advertise XEN_HVM_CPUID_EXT_DEST_ID and refuse to
restore the IO-APIC state, preventing silent interrupt misrouting after
live migration.

Signed-off-by: Julian Vetter <julian.vetter@vates.tech>
---
Changes in v4:
- Corresponds to v3 patch 3, but adapted for the new raw-addr/data
  storage introduced in patch 5:
    - In v3, vmsi_deliver_pirq() extracted the full destination from
      gflags via XEN_DOMCTL_VMSI_X86_FULL_DEST(), and msi_gflags()
      packed the extended address bits into gflags
    - In v4 both helpers are gone. vmsi_deliver_pirq() reads
      pirq_dpci->gmsi.addr and pirq_dpci->gmsi.data directly using the
      standard MSI masks
- Moved the IO-APIC masks and VIOAPIC_RTE_DEST() helper (previously in
  v3 patch 2) into this patch
- Added ioapic_check() ext destination safety check (refusing migration
  with ext_dest_id bits set when XEN_HVM_CPUID_EXT_DEST_ID is not
  advertised)
---
 xen/arch/x86/hvm/irq.c                 |  9 ++++++++-
 xen/arch/x86/hvm/vioapic.c             |  2 +-
 xen/arch/x86/hvm/vmsi.c                |  4 ++--
 xen/arch/x86/include/asm/hvm/hvm.h     |  4 ++--
 xen/arch/x86/include/asm/hvm/vioapic.h | 12 ++++++++++++
 xen/include/public/arch-x86/hvm/save.h |  4 +++-
 6 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index 5f64361113..b43adf8b96 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -374,7 +374,14 @@ int hvm_set_pci_link_route(struct domain *d, u8 link, u8 isa_irq)
 int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
 {
     uint32_t tmp = (uint32_t) addr;
-    uint8_t  dest = (tmp & MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT;
+    /*
+     * Standard MSI destination address bits 19:12 carry the 8-bit APIC ID.
+     * When XEN_HVM_CPUID_EXT_DEST_ID is enabled, bits 11:5 carry APIC ID bits
+     * [14:8], extending the addressable range to 15 bits. Guests that do not
+     * use extended IDs leave these bits at zero, so the combined extraction is
+     * safe regardless.
+     */
+    uint32_t dest = MSI_ADDR_DEST(tmp);
     uint8_t  dest_mode = !!(tmp & MSI_ADDR_DESTMODE_MASK);
     uint8_t  delivery_mode = (data & MSI_DATA_DELIVERY_MODE_MASK)
         >> MSI_DATA_DELIVERY_MODE_SHIFT;
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 43fb165f84..527cc770b7 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -411,7 +411,7 @@ static void ioapic_inj_irq(
 
 static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
 {
-    uint16_t dest = vioapic->redirtbl[pin].fields.dest_id;
+    uint32_t dest = VIOAPIC_RTE_DEST(vioapic->redirtbl[pin].bits);
     uint8_t dest_mode = vioapic->redirtbl[pin].fields.dest_mode;
     uint8_t delivery_mode = vioapic->redirtbl[pin].fields.delivery_mode;
     uint8_t vector = vioapic->redirtbl[pin].fields.vector;
diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 2a4b97e2e1..7b338c4ddc 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -67,7 +67,7 @@ static void vmsi_inj_irq(
 
 int vmsi_deliver(
     struct domain *d, int vector,
-    uint8_t dest, uint8_t dest_mode,
+    uint32_t dest, uint8_t dest_mode,
     uint8_t delivery_mode, uint8_t trig_mode)
 {
     struct vlapic *target;
@@ -126,7 +126,7 @@ void vmsi_deliver_pirq(struct domain *d, const struct hvm_pirq_dpci *pirq_dpci)
 }
 
 /* Return value, -1 : multi-dests, non-negative value: dest_vcpu_id */
-int hvm_girq_dest_2_vcpu_id(struct domain *d, uint8_t dest, uint8_t dest_mode)
+int hvm_girq_dest_2_vcpu_id(struct domain *d, uint32_t dest, uint8_t dest_mode)
 {
     int dest_vcpu_id = -1, w = 0;
     struct vcpu *v;
diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
index e7c1364802..884dd44c81 100644
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -294,11 +294,11 @@ uint64_t hvm_get_guest_time_fixed(const struct vcpu *v, uint64_t at_tsc);
 
 int vmsi_deliver(
     struct domain *d, int vector,
-    uint8_t dest, uint8_t dest_mode,
+    uint32_t dest, uint8_t dest_mode,
     uint8_t delivery_mode, uint8_t trig_mode);
 struct hvm_pirq_dpci;
 void vmsi_deliver_pirq(struct domain *d, const struct hvm_pirq_dpci *pirq_dpci);
-int hvm_girq_dest_2_vcpu_id(struct domain *d, uint8_t dest, uint8_t dest_mode);
+int hvm_girq_dest_2_vcpu_id(struct domain *d, uint32_t dest, uint8_t dest_mode);
 
 enum hvm_intblk
 hvm_interrupt_blocked(struct vcpu *v, struct hvm_intack intack);
diff --git a/xen/arch/x86/include/asm/hvm/vioapic.h b/xen/arch/x86/include/asm/hvm/vioapic.h
index 68af6dce79..4499208bad 100644
--- a/xen/arch/x86/include/asm/hvm/vioapic.h
+++ b/xen/arch/x86/include/asm/hvm/vioapic.h
@@ -32,6 +32,18 @@
 #define VIOAPIC_EDGE_TRIG  0
 #define VIOAPIC_LEVEL_TRIG 1
 
+/*
+ * Extract the destination ID from a 64-bit IO-APIC RTE, including the
+ * extended bits (55:49) used when XEN_HVM_CPUID_EXT_DEST_ID is advertised.
+ */
+#define IO_APIC_REDIR_DEST_MASK         (0xffULL << 56)
+#define IO_APIC_REDIR_EXT_DEST_MASK     (0x7fULL << 49)
+
+#define VIOAPIC_RTE_DEST(rte) \
+    (MASK_EXTR((rte), IO_APIC_REDIR_DEST_MASK) | \
+     (MASK_EXTR((rte), IO_APIC_REDIR_EXT_DEST_MASK) << \
+      MSI_ADDR_DEST_ID_UPPER_BITS))
+
 #define VIOAPIC_DEFAULT_BASE_ADDRESS  0xfec00000U
 #define VIOAPIC_MEM_LENGTH            0x100
 
diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
index 9c4bfc7ebd..483097d940 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -359,7 +359,9 @@ union vioapic_redir_entry
         uint8_t trig_mode:1;
         uint8_t mask:1;
         uint8_t reserve:7;
-        uint8_t reserved[4];
+        uint8_t reserved[3];
+        uint8_t reserved2:1;
+        uint8_t ext_dest_id:7;
         uint8_t dest_id;
     } fields;
 };
-- 
2.53.0



--
Julian Vetter | Vates Hypervisor & Kernel Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v4 6/9] x86/hvm: Support extended destination IDs in virtual MSI and IO-APIC

[Teddy Astie]

[-- Attachment #1: Type: text/plain, Size: 7923 bytes --]

Le 27/04/2026 à 15:57, Julian Vetter a écrit :
> Add IO_APIC_REDIR_DEST_MASK and IO_APIC_REDIR_EXT_DEST_MASK for the
> standard and extended destination fields of the IO-APIC RTE, and a
> VIOAPIC_RTE_DEST() helper that extracts the combined 15-bit destination
> ID. Extend the IO-APIC RTE save/restore struct with an ext_dest_id field
> so migration preserves the extended bits.
> 
> Use the newly defined masks to extract the full 15-bit destination ID
> from guest MSI addresses and IO-APIC RTEs. In hvm_inject_msi() combine
> the standard bits [19:12] with the extended bits [11:5] of the MSI
> address into a 15-bit destination ID for LAPIC delivery. Widen the dest
> parameter of vmsi_deliver() and hvm_girq_dest_2_vcpu_id() from uint8_t
> to uint32_t to accommodate the larger range. In vioapic_deliver() read
> the combined 15-bit destination using the VIOAPIC_RTE_DEST() macro.
> Extend ioapic_check() to check for extended destination bits set in a
> domain that does not advertise XEN_HVM_CPUID_EXT_DEST_ID and refuse to
> restore the IO-APIC state, preventing silent interrupt misrouting after
> live migration.
> 
> Signed-off-by: Julian Vetter <julian.vetter@vates.tech>
> ---
> Changes in v4:
> - Corresponds to v3 patch 3, but adapted for the new raw-addr/data
>    storage introduced in patch 5:
>      - In v3, vmsi_deliver_pirq() extracted the full destination from
>        gflags via XEN_DOMCTL_VMSI_X86_FULL_DEST(), and msi_gflags()
>        packed the extended address bits into gflags
>      - In v4 both helpers are gone. vmsi_deliver_pirq() reads
>        pirq_dpci->gmsi.addr and pirq_dpci->gmsi.data directly using the
>        standard MSI masks
> - Moved the IO-APIC masks and VIOAPIC_RTE_DEST() helper (previously in
>    v3 patch 2) into this patch
> - Added ioapic_check() ext destination safety check (refusing migration
>    with ext_dest_id bits set when XEN_HVM_CPUID_EXT_DEST_ID is not
>    advertised)
> ---
>   xen/arch/x86/hvm/irq.c                 |  9 ++++++++-
>   xen/arch/x86/hvm/vioapic.c             |  2 +-
>   xen/arch/x86/hvm/vmsi.c                |  4 ++--
>   xen/arch/x86/include/asm/hvm/hvm.h     |  4 ++--
>   xen/arch/x86/include/asm/hvm/vioapic.h | 12 ++++++++++++
>   xen/include/public/arch-x86/hvm/save.h |  4 +++-
>   6 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
> index 5f64361113..b43adf8b96 100644
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -374,7 +374,14 @@ int hvm_set_pci_link_route(struct domain *d, u8 link, u8 isa_irq)
>   int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
>   {
>       uint32_t tmp = (uint32_t) addr;
> -    uint8_t  dest = (tmp & MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT;
> +    /*
> +     * Standard MSI destination address bits 19:12 carry the 8-bit APIC ID.
> +     * When XEN_HVM_CPUID_EXT_DEST_ID is enabled, bits 11:5 carry APIC ID bits
> +     * [14:8], extending the addressable range to 15 bits. Guests that do not
> +     * use extended IDs leave these bits at zero, so the combined extraction is
> +     * safe regardless.
> +     */
> +    uint32_t dest = MSI_ADDR_DEST(tmp);
>       uint8_t  dest_mode = !!(tmp & MSI_ADDR_DESTMODE_MASK);
>       uint8_t  delivery_mode = (data & MSI_DATA_DELIVERY_MODE_MASK)
>           >> MSI_DATA_DELIVERY_MODE_SHIFT;
> diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
> index 43fb165f84..527cc770b7 100644
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -411,7 +411,7 @@ static void ioapic_inj_irq(
>   
>   static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
>   {
> -    uint16_t dest = vioapic->redirtbl[pin].fields.dest_id;
> +    uint32_t dest = VIOAPIC_RTE_DEST(vioapic->redirtbl[pin].bits);
>       uint8_t dest_mode = vioapic->redirtbl[pin].fields.dest_mode;
>       uint8_t delivery_mode = vioapic->redirtbl[pin].fields.delivery_mode;
>       uint8_t vector = vioapic->redirtbl[pin].fields.vector;
> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
> index 2a4b97e2e1..7b338c4ddc 100644
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -67,7 +67,7 @@ static void vmsi_inj_irq(
>   
>   int vmsi_deliver(
>       struct domain *d, int vector,
> -    uint8_t dest, uint8_t dest_mode,
> +    uint32_t dest, uint8_t dest_mode,
>       uint8_t delivery_mode, uint8_t trig_mode)
>   {
>       struct vlapic *target;
> @@ -126,7 +126,7 @@ void vmsi_deliver_pirq(struct domain *d, const struct hvm_pirq_dpci *pirq_dpci)
>   }
>   
>   /* Return value, -1 : multi-dests, non-negative value: dest_vcpu_id */
> -int hvm_girq_dest_2_vcpu_id(struct domain *d, uint8_t dest, uint8_t dest_mode)
> +int hvm_girq_dest_2_vcpu_id(struct domain *d, uint32_t dest, uint8_t dest_mode)
>   {
>       int dest_vcpu_id = -1, w = 0;
>       struct vcpu *v;
> diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
> index e7c1364802..884dd44c81 100644
> --- a/xen/arch/x86/include/asm/hvm/hvm.h
> +++ b/xen/arch/x86/include/asm/hvm/hvm.h
> @@ -294,11 +294,11 @@ uint64_t hvm_get_guest_time_fixed(const struct vcpu *v, uint64_t at_tsc);
>   
>   int vmsi_deliver(
>       struct domain *d, int vector,
> -    uint8_t dest, uint8_t dest_mode,
> +    uint32_t dest, uint8_t dest_mode,
>       uint8_t delivery_mode, uint8_t trig_mode);
>   struct hvm_pirq_dpci;
>   void vmsi_deliver_pirq(struct domain *d, const struct hvm_pirq_dpci *pirq_dpci);
> -int hvm_girq_dest_2_vcpu_id(struct domain *d, uint8_t dest, uint8_t dest_mode);
> +int hvm_girq_dest_2_vcpu_id(struct domain *d, uint32_t dest, uint8_t dest_mode);
>   
>   enum hvm_intblk
>   hvm_interrupt_blocked(struct vcpu *v, struct hvm_intack intack);
> diff --git a/xen/arch/x86/include/asm/hvm/vioapic.h b/xen/arch/x86/include/asm/hvm/vioapic.h
> index 68af6dce79..4499208bad 100644
> --- a/xen/arch/x86/include/asm/hvm/vioapic.h
> +++ b/xen/arch/x86/include/asm/hvm/vioapic.h
> @@ -32,6 +32,18 @@
>   #define VIOAPIC_EDGE_TRIG  0
>   #define VIOAPIC_LEVEL_TRIG 1
>   
> +/*
> + * Extract the destination ID from a 64-bit IO-APIC RTE, including the
> + * extended bits (55:49) used when XEN_HVM_CPUID_EXT_DEST_ID is advertised.
> + */
> +#define IO_APIC_REDIR_DEST_MASK         (0xffULL << 56)
> +#define IO_APIC_REDIR_EXT_DEST_MASK     (0x7fULL << 49)
> +
> +#define VIOAPIC_RTE_DEST(rte) \
> +    (MASK_EXTR((rte), IO_APIC_REDIR_DEST_MASK) | \
> +     (MASK_EXTR((rte), IO_APIC_REDIR_EXT_DEST_MASK) << \
> +      MSI_ADDR_DEST_ID_UPPER_BITS))
> +

We can probably simplify that by using vioapic_redir_entry.dest_id and 
vioapic_redir_entry.ext_dest_id directly instead of reparsing it from 
vioapic_redir_entry.bits.

IOW, replace MASK_EXTR((rte), IO_APIC_REDIR_DEST_MASK) with 
(rte).dest_id and MASK_EXTR((rte), IO_APIC_REDIR_EXT_DEST_MASK) with 
(rte).ext_dest_id. So VIOAPIC_RTE_DEST() would now take 
vioapic_redir_entry as parameter.

>   #define VIOAPIC_DEFAULT_BASE_ADDRESS  0xfec00000U
>   #define VIOAPIC_MEM_LENGTH            0x100
>   > diff --git a/xen/include/public/arch-x86/hvm/save.h 
b/xen/include/public/arch-x86/hvm/save.h
> index 9c4bfc7ebd..483097d940 100644
> --- a/xen/include/public/arch-x86/hvm/save.h
> +++ b/xen/include/public/arch-x86/hvm/save.h
> @@ -359,7 +359,9 @@ union vioapic_redir_entry
>           uint8_t trig_mode:1;
>           uint8_t mask:1;
>           uint8_t reserve:7;
> -        uint8_t reserved[4];
> +        uint8_t reserved[3];
> +        uint8_t reserved2:1;
> +        uint8_t ext_dest_id:7;
>           uint8_t dest_id;
>       } fields;
>   };

Teddy


--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v4 6/9] x86/hvm: Support extended destination IDs in virtual MSI and IO-APIC

[Jan Beulich]

On 27.04.2026 15:54, Julian Vetter wrote:
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -374,7 +374,14 @@ int hvm_set_pci_link_route(struct domain *d, u8 link, u8 isa_irq)
>  int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
>  {
>      uint32_t tmp = (uint32_t) addr;
> -    uint8_t  dest = (tmp & MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT;
> +    /*
> +     * Standard MSI destination address bits 19:12 carry the 8-bit APIC ID.
> +     * When XEN_HVM_CPUID_EXT_DEST_ID is enabled, bits 11:5 carry APIC ID bits
> +     * [14:8], extending the addressable range to 15 bits. Guests that do not
> +     * use extended IDs leave these bits at zero, so the combined extraction is
> +     * safe regardless.
> +     */

How do you know what guests do?

I also don't think such a comment needs to be put at every ...

> +    uint32_t dest = MSI_ADDR_DEST(tmp);

... use site of MSI_ADDR_DEST().

> --- a/xen/arch/x86/include/asm/hvm/vioapic.h
> +++ b/xen/arch/x86/include/asm/hvm/vioapic.h
> @@ -32,6 +32,18 @@
>  #define VIOAPIC_EDGE_TRIG  0
>  #define VIOAPIC_LEVEL_TRIG 1
>  
> +/*
> + * Extract the destination ID from a 64-bit IO-APIC RTE, including the
> + * extended bits (55:49) used when XEN_HVM_CPUID_EXT_DEST_ID is advertised.
> + */
> +#define IO_APIC_REDIR_DEST_MASK         (0xffULL << 56)
> +#define IO_APIC_REDIR_EXT_DEST_MASK     (0x7fULL << 49)
> +
> +#define VIOAPIC_RTE_DEST(rte) \
> +    (MASK_EXTR((rte), IO_APIC_REDIR_DEST_MASK) | \
> +     (MASK_EXTR((rte), IO_APIC_REDIR_EXT_DEST_MASK) << \
> +      MSI_ADDR_DEST_ID_UPPER_BITS))

Following Teddy's comment this may go away altogether, but if not: Please
avoid unnecessary parentheses (around "rte" here). They only hamper
readability.

Further, with ...

> --- a/xen/include/public/arch-x86/hvm/save.h
> +++ b/xen/include/public/arch-x86/hvm/save.h
> @@ -359,7 +359,9 @@ union vioapic_redir_entry
>          uint8_t trig_mode:1;
>          uint8_t mask:1;
>          uint8_t reserve:7;
> -        uint8_t reserved[4];
> +        uint8_t reserved[3];
> +        uint8_t reserved2:1;
> +        uint8_t ext_dest_id:7;
>          uint8_t dest_id;
>      } fields;
>  };

... this change, and with ioapic_check() as added by patch 1 not needing
a change here, it is clear that non-zero bits in ext_dest_id could possibly
be seen irrespective of the guest being aware of the new feature. You may
not interpret them as extended ID. (And I'm pretty sure I or someone else
did say so before.)

Jan

[PATCH v4 7/9] x86/dmop: Add XEN_DMOP_{bind,unbind}_pt_msi_irq DM ops

[Julian Vetter]

[-- Attachment #1: Type: text/plain, Size: 15507 bytes --]

Add two DM ops for MSI passthrough IRQs. These new DM ops take the raw
MSI address and data fields rather than pre-decoded gflags values. Xen
decodes the destination ID via msi_addr_to_gflags(), including any
extended destination bits in address[11:5]. This means the device model
does not need to understand the extended destination ID encoding, and
simply forwards the MSI address it observes from the guest.

With these DM ops in place, redirect xc_domain_update_msi_irq() and
xc_domain_unbind_msi_irq() in libxenctrl to use
xendevicemodel_bind_pt_msi_irq() / xendevicemodel_unbind_pt_msi_irq()
via xch->dmod. The gflags/gvec arguments are translated to the raw MSI
address and data words at the libxc level using the standard x86 MSI
address format.

Reject the PT_IRQ_TYPE_MSI sub-case in XEN_DOMCTL_bind_pt_irq and
XEN_DOMCTL_unbind_pt_irq: all callers now go through the DM op path, so
the domctl sub-case is fully obsolete.

Signed-off-by: Julian Vetter <julian.vetter@vates.tech>
---
Changes in v4:
- Corresponds to v3 patch 5, but with feedback from Jan
- Redirect xc_domain_{update,unbind}_msi_irq() in libxenctrl to call
  xendevicemodel_{un}bind_pt_msi_irq() via xch->dmod, translating the
  existing gflags/gvec arguments to raw MSI address and data in libxc
- As suggested by Jan, reject the PT_IRQ_TYPE_MSI sub-case in
  XEN_DOMCTL_{bind,unbind}_pt_irq with -EOPNOTSUPP -> All callers now go
  through the DM op path, making the domctl sub-case fully obsolete
- Added the xlat.lst entry for dm_op_unbind_pt_msi_irq (v3 added only the
  bind entry)
---
 tools/include/xendevicemodel.h    | 31 ++++++++++++++
 tools/libs/ctrl/xc_domain.c       | 52 +++++++++++------------
 tools/libs/devicemodel/core.c     | 38 +++++++++++++++++
 xen/arch/x86/domctl.c             | 10 ++++-
 xen/arch/x86/hvm/dm.c             | 68 +++++++++++++++++++++++++++++++
 xen/drivers/passthrough/x86/hvm.c |  1 -
 xen/include/public/hvm/dm_op.h    | 37 +++++++++++++++++
 xen/include/xlat.lst              |  2 +
 8 files changed, 208 insertions(+), 31 deletions(-)

diff --git a/tools/include/xendevicemodel.h b/tools/include/xendevicemodel.h
index 227e7fd810..f15b35fa33 100644
--- a/tools/include/xendevicemodel.h
+++ b/tools/include/xendevicemodel.h
@@ -375,6 +375,37 @@ int xendevicemodel_nr_vcpus(
  */
 int xendevicemodel_restrict(xendevicemodel_handle *dmod, domid_t domid);
 
+/**
+ * This function binds a passthrough physical IRQ to a guest MSI vector
+ * using raw MSI address/data fields. Unlike XEN_DOMCTL_bind_pt_irq,
+ * this interface supports extended (15-bit) destination IDs by having
+ * Xen decode the MSI address internally.
+ *
+ * @parm dmod a handle to an open devicemodel interface.
+ * @parm domid the domain id to be serviced.
+ * @parm machine_irq the physical IRQ number (pirq).
+ * @parm msi_addr the MSI address (includes ext. dest. ID bits [11:5]).
+ * @parm msi_data the MSI data word (bits [7:0] are the guest vector).
+ * @parm gtable the MSI-X table base GFN, or 0 for plain MSI.
+ * @parm unmasked if non-zero, leave the IRQ unmasked after binding.
+ * @return 0 on success, -1 on failure.
+ */
+int xendevicemodel_bind_pt_msi_irq(
+    xendevicemodel_handle *dmod, domid_t domid, uint32_t machine_irq,
+    uint64_t msi_addr, uint32_t msi_data, uint64_t gtable, int unmasked);
+
+/**
+ * This function unbinds a passthrough physical IRQ previously bound
+ * with xendevicemodel_bind_pt_msi_irq.
+ *
+ * @parm dmod a handle to an open devicemodel interface.
+ * @parm domid the domain id to be serviced.
+ * @parm machine_irq the physical IRQ number (pirq).
+ * @return 0 on success, -1 on failure.
+ */
+int xendevicemodel_unbind_pt_msi_irq(
+    xendevicemodel_handle *dmod, domid_t domid, uint32_t machine_irq);
+
 #endif /* XENDEVICEMODEL_H */
 
 /*
diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
index 01c0669c88..7e3b7a0dc6 100644
--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -1677,6 +1677,21 @@ int xc_deassign_dt_device(
 
 
 
+static void xc_msi_gflags_to_addr_data(uint32_t gvec, uint32_t gflags,
+                                        uint64_t *msi_addr, uint32_t *msi_data)
+{
+    *msi_addr = 0xfee00000U |
+        ((uint64_t)((gflags & XEN_DOMCTL_VMSI_X86_DEST_ID_MASK) << 12)) |
+        (gflags & XEN_DOMCTL_VMSI_X86_RH_MASK ? (1U << 3) : 0) |
+        (gflags & XEN_DOMCTL_VMSI_X86_DM_MASK ? (1U << 2) : 0);
+
+    *msi_data = (gvec & 0xff) |
+        (uint32_t)(((gflags & XEN_DOMCTL_VMSI_X86_DELIV_MASK) >>
+                    (/* shift of XEN_DOMCTL_VMSI_X86_DELIV_MASK */ 12 -
+                     /* MSI data delivery shift */ 8))) |
+        (gflags & XEN_DOMCTL_VMSI_X86_TRIG_MASK ? (1U << 15) : 0);
+}
+
 int xc_domain_update_msi_irq(
     xc_interface *xch,
     uint32_t domid,
@@ -1685,22 +1700,15 @@ int xc_domain_update_msi_irq(
     uint32_t gflags,
     uint64_t gtable)
 {
-    int rc;
-    struct xen_domctl_bind_pt_irq *bind;
-    struct xen_domctl domctl = {};
-
-    domctl.cmd = XEN_DOMCTL_bind_pt_irq;
-    domctl.domain = domid;
+    uint64_t msi_addr;
+    uint32_t msi_data;
+    int unmasked = !!(gflags & XEN_DOMCTL_VMSI_X86_UNMASKED);
 
-    bind = &(domctl.u.bind_pt_irq);
-    bind->irq_type = PT_IRQ_TYPE_MSI;
-    bind->machine_irq = pirq;
-    bind->u.msi.gvec = gvec;
-    bind->u.msi.gflags = gflags;
-    bind->u.msi.gtable = gtable;
+    xc_msi_gflags_to_addr_data(gvec, gflags, &msi_addr, &msi_data);
 
-    rc = do_domctl(xch, &domctl);
-    return rc;
+    return xendevicemodel_bind_pt_msi_irq(xch->dmod, domid, pirq,
+                                          msi_addr, msi_data, gtable,
+                                          unmasked);
 }
 
 int xc_domain_unbind_msi_irq(
@@ -1710,21 +1718,7 @@ int xc_domain_unbind_msi_irq(
     uint32_t pirq,
     uint32_t gflags)
 {
-    int rc;
-    struct xen_domctl_bind_pt_irq *bind;
-    struct xen_domctl domctl = {};
-
-    domctl.cmd = XEN_DOMCTL_unbind_pt_irq;
-    domctl.domain = domid;
-
-    bind = &(domctl.u.bind_pt_irq);
-    bind->irq_type = PT_IRQ_TYPE_MSI;
-    bind->machine_irq = pirq;
-    bind->u.msi.gvec = gvec;
-    bind->u.msi.gflags = gflags;
-
-    rc = do_domctl(xch, &domctl);
-    return rc;
+    return xendevicemodel_unbind_pt_msi_irq(xch->dmod, domid, pirq);
 }
 
 /* Pass-through: binds machine irq to guests irq */
diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
index 8e619eeb0a..adf2c41a96 100644
--- a/tools/libs/devicemodel/core.c
+++ b/tools/libs/devicemodel/core.c
@@ -645,6 +645,44 @@ int xendevicemodel_nr_vcpus(
     return 0;
 }
 
+int xendevicemodel_bind_pt_msi_irq(
+    xendevicemodel_handle *dmod, domid_t domid, uint32_t machine_irq,
+    uint64_t msi_addr, uint32_t msi_data, uint64_t gtable, int unmasked)
+{
+    struct xen_dm_op op;
+    struct xen_dm_op_bind_pt_msi_irq *data;
+
+    memset(&op, 0, sizeof(op));
+
+    op.op = XEN_DMOP_bind_pt_msi_irq;
+    data = &op.u.bind_pt_msi_irq;
+
+    data->machine_irq = machine_irq;
+    data->data = msi_data;
+    data->addr = msi_addr;
+    data->gtable = gtable;
+    if ( unmasked )
+        data->flags |= XEN_DMOP_MSI_FLAG_UNMASKED;
+
+    return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
+}
+
+int xendevicemodel_unbind_pt_msi_irq(
+    xendevicemodel_handle *dmod, domid_t domid, uint32_t machine_irq)
+{
+    struct xen_dm_op op;
+    struct xen_dm_op_unbind_pt_msi_irq *data;
+
+    memset(&op, 0, sizeof(op));
+
+    op.op = XEN_DMOP_unbind_pt_msi_irq;
+    data = &op.u.unbind_pt_msi_irq;
+
+    data->machine_irq = machine_irq;
+
+    return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
+}
+
 int xendevicemodel_restrict(xendevicemodel_handle *dmod, domid_t domid)
 {
     return osdep_xendevicemodel_restrict(dmod, domid);
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index bfbc35c08b..d80a33fe40 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -574,6 +574,14 @@ long arch_do_domctl(
         if ( !is_hvm_domain(d) )
             break;
 
+        /*
+         * PT_IRQ_TYPE_MSI is obsoleted by XEN_DMOP_bind_pt_msi_irq, which
+         * passes raw MSI address/data so Xen can decode extended destination
+         * ID bits. Device models must use the DM op path instead.
+         */
+        if ( bind->irq_type == PT_IRQ_TYPE_MSI )
+            break;
+
         ret = xsm_bind_pt_irq(XSM_HOOK, d, bind);
         if ( ret )
             break;
@@ -602,7 +610,7 @@ long arch_do_domctl(
         int irq = domain_pirq_to_irq(d, bind->machine_irq);
 
         ret = -EINVAL;
-        if ( !is_hvm_domain(d) )
+        if ( !is_hvm_domain(d) || bind->irq_type == PT_IRQ_TYPE_MSI )
             break;
 
         ret = -EPERM;
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index 3b53471af0..ac70cb6405 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -7,6 +7,8 @@
 #include <xen/guest_access.h>
 #include <xen/dm.h>
 #include <xen/hypercall.h>
+#include <xen/iocap.h>
+#include <xen/iommu.h>
 #include <xen/ioreq.h>
 #include <xen/nospec.h>
 #include <xen/sched.h>
@@ -350,6 +352,8 @@ int dm_op(const struct dmop_args *op_args)
         [XEN_DMOP_relocate_memory]                  = sizeof(struct xen_dm_op_relocate_memory),
         [XEN_DMOP_pin_memory_cacheattr]             = sizeof(struct xen_dm_op_pin_memory_cacheattr),
         [XEN_DMOP_nr_vcpus]                         = sizeof(struct xen_dm_op_nr_vcpus),
+        [XEN_DMOP_bind_pt_msi_irq]                  = sizeof(struct xen_dm_op_bind_pt_msi_irq),
+        [XEN_DMOP_unbind_pt_msi_irq]                = sizeof(struct xen_dm_op_unbind_pt_msi_irq),
     };
 
     rc = rcu_lock_remote_domain_by_id(op_args->domid, &d);
@@ -607,6 +611,68 @@ int dm_op(const struct dmop_args *op_args)
         break;
     }
 
+    case XEN_DMOP_bind_pt_msi_irq:
+    {
+        const struct xen_dm_op_bind_pt_msi_irq *data =
+            &op.u.bind_pt_msi_irq;
+        int irq;
+
+        rc = -EINVAL;
+        if ( data->pad || (data->flags & ~XEN_DMOP_MSI_FLAG_UNMASKED) )
+            break;
+
+        irq = domain_pirq_to_irq(d, data->machine_irq);
+
+        rc = -EPERM;
+        if ( irq <= 0 || !irq_access_permitted(current->domain, irq) )
+            break;
+
+        rc = -ESRCH;
+        if ( is_iommu_enabled(d) )
+        {
+            read_lock(&d->pci_lock);
+            rc = pt_irq_bind_msi(d, data->machine_irq, data->addr, data->data,
+                                 data->gtable,
+                                 !!(data->flags & XEN_DMOP_MSI_FLAG_UNMASKED));
+            read_unlock(&d->pci_lock);
+        }
+        if ( rc < 0 )
+            printk(XENLOG_G_ERR
+                   "XEN_DMOP_bind_pt_msi_irq: pt_irq_bind_msi failed (%ld) for %pd\n",
+                   rc, d);
+        break;
+    }
+
+    case XEN_DMOP_unbind_pt_msi_irq:
+    {
+        const struct xen_dm_op_unbind_pt_msi_irq *data =
+            &op.u.unbind_pt_msi_irq;
+        struct xen_domctl_bind_pt_irq bind = {
+            .machine_irq = data->machine_irq,
+            .irq_type = PT_IRQ_TYPE_MSI,
+        };
+        int irq;
+
+        irq = domain_pirq_to_irq(d, bind.machine_irq);
+
+        rc = -EPERM;
+        if ( irq <= 0 || !irq_access_permitted(current->domain, irq) )
+            break;
+
+        rc = -ESRCH;
+        if ( is_iommu_enabled(d) )
+        {
+            read_lock(&d->pci_lock);
+            rc = pt_irq_destroy_bind(d, &bind);
+            read_unlock(&d->pci_lock);
+        }
+        if ( rc < 0 )
+            printk(XENLOG_G_ERR
+                   "XEN_DMOP_unbind_pt_msi_irq: pt_irq_destroy_bind failed (%ld) for %pd\n",
+                   rc, d);
+        break;
+    }
+
     default:
         rc = ioreq_server_dm_op(&op, d, &const_op);
         break;
@@ -643,6 +709,8 @@ CHECK_dm_op_remote_shutdown;
 CHECK_dm_op_relocate_memory;
 CHECK_dm_op_pin_memory_cacheattr;
 CHECK_dm_op_nr_vcpus;
+CHECK_dm_op_bind_pt_msi_irq;
+CHECK_dm_op_unbind_pt_msi_irq;
 
 int compat_dm_op(
     domid_t domid, unsigned int nr_bufs, XEN_GUEST_HANDLE_PARAM(void) bufs)
diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
index 026534530f..6fb4f8b7dc 100644
--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -21,7 +21,6 @@
 #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>
diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
index 2bf0fdc1ae..43571b7713 100644
--- a/xen/include/public/hvm/dm_op.h
+++ b/xen/include/public/hvm/dm_op.h
@@ -444,6 +444,41 @@ struct xen_dm_op_nr_vcpus {
 };
 typedef struct xen_dm_op_nr_vcpus xen_dm_op_nr_vcpus_t;
 
+#define XEN_DMOP_bind_pt_msi_irq   21
+#define XEN_DMOP_unbind_pt_msi_irq 22
+
+struct xen_dm_op_bind_pt_msi_irq {
+    /* IN - physical IRQ (pirq) */
+    uint32_t machine_irq;
+    /* IN - MSI data word (bits [7:0] are the guest vector) */
+    uint32_t data;
+    /* IN - flags */
+    uint32_t flags;
+#define XEN_DMOP_MSI_FLAG_UNMASKED (1u << 0)
+    uint32_t pad;
+    /* IN - MSI address (includes extended destination ID in bits [11:5]) */
+    uint64_aligned_t addr;
+    /* IN - MSI-X table base GFN, 0 for plain MSI */
+    uint64_aligned_t gtable;
+};
+
+typedef struct xen_dm_op_bind_pt_msi_irq xen_dm_op_bind_pt_msi_irq_t;
+
+struct xen_dm_op_unbind_pt_msi_irq {
+    /* IN - physical IRQ (pirq) */
+    uint32_t machine_irq;
+};
+typedef struct xen_dm_op_unbind_pt_msi_irq xen_dm_op_unbind_pt_msi_irq_t;
+
+/*
+ * XEN_DMOP_enable_ext_dest_id: Signal to Xen that this device model will use
+ * XEN_DMOP_bind_pt_msi_irq for all passthrough MSI bindings, passing raw MSI
+ * address/data fields. Once called, Xen will advertise
+ * XEN_HVM_CPUID_EXT_DEST_ID to the guest. Must be called before the guest
+ * starts.
+ */
+#define XEN_DMOP_enable_ext_dest_id 23
+
 struct xen_dm_op {
     uint32_t op;
     uint32_t pad;
@@ -468,6 +503,8 @@ struct xen_dm_op {
         xen_dm_op_relocate_memory_t relocate_memory;
         xen_dm_op_pin_memory_cacheattr_t pin_memory_cacheattr;
         xen_dm_op_nr_vcpus_t nr_vcpus;
+        xen_dm_op_bind_pt_msi_irq_t bind_pt_msi_irq;
+        xen_dm_op_unbind_pt_msi_irq_t unbind_pt_msi_irq;
     } u;
 };
 
diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
index 9d08dcc4bb..6dc5f5796a 100644
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -98,6 +98,7 @@
 ?	grant_entry_v2			grant_table.h
 
 !	dm_op_buf			hvm/dm_op.h
+?	dm_op_bind_pt_msi_irq		hvm/dm_op.h
 ?	dm_op_create_ioreq_server	hvm/dm_op.h
 ?	dm_op_destroy_ioreq_server	hvm/dm_op.h
 ?	dm_op_get_ioreq_server_info	hvm/dm_op.h
@@ -116,6 +117,7 @@
 ?	dm_op_set_pci_intx_level	hvm/dm_op.h
 ?	dm_op_set_pci_link_route	hvm/dm_op.h
 ?	dm_op_track_dirty_vram		hvm/dm_op.h
+?	dm_op_unbind_pt_msi_irq		hvm/dm_op.h
 
 !	hvm_altp2m_set_mem_access_multi	hvm/hvm_op.h
 
-- 
2.53.0



--
Julian Vetter | Vates Hypervisor & Kernel Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v4 7/9] x86/dmop: Add XEN_DMOP_{bind,unbind}_pt_msi_irq DM ops

[Teddy Astie]

[-- Attachment #1: Type: text/plain, Size: 16992 bytes --]

Le 27/04/2026 à 15:57, Julian Vetter a écrit :
> Add two DM ops for MSI passthrough IRQs. These new DM ops take the raw
> MSI address and data fields rather than pre-decoded gflags values. Xen
> decodes the destination ID via msi_addr_to_gflags(), including any
> extended destination bits in address[11:5]. This means the device model
> does not need to understand the extended destination ID encoding, and
> simply forwards the MSI address it observes from the guest.
> 
> With these DM ops in place, redirect xc_domain_update_msi_irq() and
> xc_domain_unbind_msi_irq() in libxenctrl to use
> xendevicemodel_bind_pt_msi_irq() / xendevicemodel_unbind_pt_msi_irq()
> via xch->dmod. The gflags/gvec arguments are translated to the raw MSI
> address and data words at the libxc level using the standard x86 MSI
> address format.
> 
> Reject the PT_IRQ_TYPE_MSI sub-case in XEN_DOMCTL_bind_pt_irq and
> XEN_DOMCTL_unbind_pt_irq: all callers now go through the DM op path, so
> the domctl sub-case is fully obsolete.
> 

We probably want to reflect that on XEN_DOMCTL_{un}bind_pt_irq interface 
in domctl.h (e.g through a note saying that PT_IRQ_TYPE_MSI type is now 
deprecated and unsupported).

> Signed-off-by: Julian Vetter <julian.vetter@vates.tech>
> ---
> Changes in v4:
> - Corresponds to v3 patch 5, but with feedback from Jan
> - Redirect xc_domain_{update,unbind}_msi_irq() in libxenctrl to call
>    xendevicemodel_{un}bind_pt_msi_irq() via xch->dmod, translating the
>    existing gflags/gvec arguments to raw MSI address and data in libxc
> - As suggested by Jan, reject the PT_IRQ_TYPE_MSI sub-case in
>    XEN_DOMCTL_{bind,unbind}_pt_irq with -EOPNOTSUPP -> All callers now go
>    through the DM op path, making the domctl sub-case fully obsolete
> - Added the xlat.lst entry for dm_op_unbind_pt_msi_irq (v3 added only the
>    bind entry)
> ---
>   tools/include/xendevicemodel.h    | 31 ++++++++++++++
>   tools/libs/ctrl/xc_domain.c       | 52 +++++++++++------------
>   tools/libs/devicemodel/core.c     | 38 +++++++++++++++++
>   xen/arch/x86/domctl.c             | 10 ++++-
>   xen/arch/x86/hvm/dm.c             | 68 +++++++++++++++++++++++++++++++
>   xen/drivers/passthrough/x86/hvm.c |  1 -
>   xen/include/public/hvm/dm_op.h    | 37 +++++++++++++++++
>   xen/include/xlat.lst              |  2 +
>   8 files changed, 208 insertions(+), 31 deletions(-)
> 
> diff --git a/tools/include/xendevicemodel.h b/tools/include/xendevicemodel.h
> index 227e7fd810..f15b35fa33 100644
> --- a/tools/include/xendevicemodel.h
> +++ b/tools/include/xendevicemodel.h
> @@ -375,6 +375,37 @@ int xendevicemodel_nr_vcpus(
>    */
>   int xendevicemodel_restrict(xendevicemodel_handle *dmod, domid_t domid);
>   
> +/**
> + * This function binds a passthrough physical IRQ to a guest MSI vector
> + * using raw MSI address/data fields. Unlike XEN_DOMCTL_bind_pt_irq,
> + * this interface supports extended (15-bit) destination IDs by having
> + * Xen decode the MSI address internally.
> + *

"unlike XEN_DOMCTL_bind_pt_irq" feels a bit odd since that's not a 
supported interface anymore for MSI.

> + * @parm dmod a handle to an open devicemodel interface.
> + * @parm domid the domain id to be serviced.
> + * @parm machine_irq the physical IRQ number (pirq).
> + * @parm msi_addr the MSI address (includes ext. dest. ID bits [11:5]).
> + * @parm msi_data the MSI data word (bits [7:0] are the guest vector).
> + * @parm gtable the MSI-X table base GFN, or 0 for plain MSI.
> + * @parm unmasked if non-zero, leave the IRQ unmasked after binding.
> + * @return 0 on success, -1 on failure.
> + */
> +int xendevicemodel_bind_pt_msi_irq(
> +    xendevicemodel_handle *dmod, domid_t domid, uint32_t machine_irq,
> +    uint64_t msi_addr, uint32_t msi_data, uint64_t gtable, int unmasked);
> +
> +/**
> + * This function unbinds a passthrough physical IRQ previously bound
> + * with xendevicemodel_bind_pt_msi_irq.
> + *
> + * @parm dmod a handle to an open devicemodel interface.
> + * @parm domid the domain id to be serviced.
> + * @parm machine_irq the physical IRQ number (pirq).
> + * @return 0 on success, -1 on failure.
> + */
> +int xendevicemodel_unbind_pt_msi_irq(
> +    xendevicemodel_handle *dmod, domid_t domid, uint32_t machine_irq);
> +
>   #endif /* XENDEVICEMODEL_H */
>   
>   /*
> diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
> index 01c0669c88..7e3b7a0dc6 100644
> --- a/tools/libs/ctrl/xc_domain.c
> +++ b/tools/libs/ctrl/xc_domain.c
> @@ -1677,6 +1677,21 @@ int xc_deassign_dt_device(
>   
>   
>   
> +static void xc_msi_gflags_to_addr_data(uint32_t gvec, uint32_t gflags,
> +                                        uint64_t *msi_addr, uint32_t *msi_data)
> +{
> +    *msi_addr = 0xfee00000U |
> +        ((uint64_t)((gflags & XEN_DOMCTL_VMSI_X86_DEST_ID_MASK) << 12)) |
> +        (gflags & XEN_DOMCTL_VMSI_X86_RH_MASK ? (1U << 3) : 0) |
> +        (gflags & XEN_DOMCTL_VMSI_X86_DM_MASK ? (1U << 2) : 0);
> +
> +    *msi_data = (gvec & 0xff) |
> +        (uint32_t)(((gflags & XEN_DOMCTL_VMSI_X86_DELIV_MASK) >>
> +                    (/* shift of XEN_DOMCTL_VMSI_X86_DELIV_MASK */ 12 -
> +                     /* MSI data delivery shift */ 8))) |
> +        (gflags & XEN_DOMCTL_VMSI_X86_TRIG_MASK ? (1U << 15) : 0);
> +}
> +
>   int xc_domain_update_msi_irq(
>       xc_interface *xch,
>       uint32_t domid,
> @@ -1685,22 +1700,15 @@ int xc_domain_update_msi_irq(
>       uint32_t gflags,
>       uint64_t gtable)
>   {
> -    int rc;
> -    struct xen_domctl_bind_pt_irq *bind;
> -    struct xen_domctl domctl = {};
> -
> -    domctl.cmd = XEN_DOMCTL_bind_pt_irq;
> -    domctl.domain = domid;
> +    uint64_t msi_addr;
> +    uint32_t msi_data;
> +    int unmasked = !!(gflags & XEN_DOMCTL_VMSI_X86_UNMASKED);
>   
> -    bind = &(domctl.u.bind_pt_irq);
> -    bind->irq_type = PT_IRQ_TYPE_MSI;
> -    bind->machine_irq = pirq;
> -    bind->u.msi.gvec = gvec;
> -    bind->u.msi.gflags = gflags;
> -    bind->u.msi.gtable = gtable;
> +    xc_msi_gflags_to_addr_data(gvec, gflags, &msi_addr, &msi_data);
>   
> -    rc = do_domctl(xch, &domctl);
> -    return rc;
> +    return xendevicemodel_bind_pt_msi_irq(xch->dmod, domid, pirq,
> +                                          msi_addr, msi_data, gtable,
> +                                          unmasked);
>   }
>   
>   int xc_domain_unbind_msi_irq(
> @@ -1710,21 +1718,7 @@ int xc_domain_unbind_msi_irq(
>       uint32_t pirq,
>       uint32_t gflags)
>   {
> -    int rc;
> -    struct xen_domctl_bind_pt_irq *bind;
> -    struct xen_domctl domctl = {};
> -
> -    domctl.cmd = XEN_DOMCTL_unbind_pt_irq;
> -    domctl.domain = domid;
> -
> -    bind = &(domctl.u.bind_pt_irq);
> -    bind->irq_type = PT_IRQ_TYPE_MSI;
> -    bind->machine_irq = pirq;
> -    bind->u.msi.gvec = gvec;
> -    bind->u.msi.gflags = gflags;
> -
> -    rc = do_domctl(xch, &domctl);
> -    return rc;
> +    return xendevicemodel_unbind_pt_msi_irq(xch->dmod, domid, pirq);
>   }
>   
>   /* Pass-through: binds machine irq to guests irq */
> diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
> index 8e619eeb0a..adf2c41a96 100644
> --- a/tools/libs/devicemodel/core.c
> +++ b/tools/libs/devicemodel/core.c
> @@ -645,6 +645,44 @@ int xendevicemodel_nr_vcpus(
>       return 0;
>   }
>   
> +int xendevicemodel_bind_pt_msi_irq(
> +    xendevicemodel_handle *dmod, domid_t domid, uint32_t machine_irq,
> +    uint64_t msi_addr, uint32_t msi_data, uint64_t gtable, int unmasked)
> +{
> +    struct xen_dm_op op;
> +    struct xen_dm_op_bind_pt_msi_irq *data;
> +
> +    memset(&op, 0, sizeof(op));
> +
> +    op.op = XEN_DMOP_bind_pt_msi_irq;
> +    data = &op.u.bind_pt_msi_irq;
> +
> +    data->machine_irq = machine_irq;
> +    data->data = msi_data;
> +    data->addr = msi_addr;
> +    data->gtable = gtable;
> +    if ( unmasked )
> +        data->flags |= XEN_DMOP_MSI_FLAG_UNMASKED;
> +
> +    return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
> +}
> +
> +int xendevicemodel_unbind_pt_msi_irq(
> +    xendevicemodel_handle *dmod, domid_t domid, uint32_t machine_irq)
> +{
> +    struct xen_dm_op op;
> +    struct xen_dm_op_unbind_pt_msi_irq *data;
> +
> +    memset(&op, 0, sizeof(op));
> +
> +    op.op = XEN_DMOP_unbind_pt_msi_irq;
> +    data = &op.u.unbind_pt_msi_irq;
> +
> +    data->machine_irq = machine_irq;
> +
> +    return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
> +}
> +

I think we want to mark 
xc_domain_update_msi_irq/xc_domain_unbind_msi_irq as deprecated since we 
implemented a newer (better) version of it in xendevicemodel; and the 
old one is now a wrapper.

>   int xendevicemodel_restrict(xendevicemodel_handle *dmod, domid_t domid)
>   {
>       return osdep_xendevicemodel_restrict(dmod, domid);
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index bfbc35c08b..d80a33fe40 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -574,6 +574,14 @@ long arch_do_domctl(
>           if ( !is_hvm_domain(d) )
>               break;
>   
> +        /*
> +         * PT_IRQ_TYPE_MSI is obsoleted by XEN_DMOP_bind_pt_msi_irq, which
> +         * passes raw MSI address/data so Xen can decode extended destination
> +         * ID bits. Device models must use the DM op path instead.
> +         */
> +        if ( bind->irq_type == PT_IRQ_TYPE_MSI )
> +            break;
> +
>           ret = xsm_bind_pt_irq(XSM_HOOK, d, bind);
>           if ( ret )
>               break;
> @@ -602,7 +610,7 @@ long arch_do_domctl(
>           int irq = domain_pirq_to_irq(d, bind->machine_irq);
>   
>           ret = -EINVAL;
> -        if ( !is_hvm_domain(d) )
> +        if ( !is_hvm_domain(d) || bind->irq_type == PT_IRQ_TYPE_MSI )
>               break;
>   
>           ret = -EPERM;
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index 3b53471af0..ac70cb6405 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -7,6 +7,8 @@
>   #include <xen/guest_access.h>
>   #include <xen/dm.h>
>   #include <xen/hypercall.h>
> +#include <xen/iocap.h>
> +#include <xen/iommu.h>
>   #include <xen/ioreq.h>
>   #include <xen/nospec.h>
>   #include <xen/sched.h>
> @@ -350,6 +352,8 @@ int dm_op(const struct dmop_args *op_args)
>           [XEN_DMOP_relocate_memory]                  = sizeof(struct xen_dm_op_relocate_memory),
>           [XEN_DMOP_pin_memory_cacheattr]             = sizeof(struct xen_dm_op_pin_memory_cacheattr),
>           [XEN_DMOP_nr_vcpus]                         = sizeof(struct xen_dm_op_nr_vcpus),
> +        [XEN_DMOP_bind_pt_msi_irq]                  = sizeof(struct xen_dm_op_bind_pt_msi_irq),
> +        [XEN_DMOP_unbind_pt_msi_irq]                = sizeof(struct xen_dm_op_unbind_pt_msi_irq),
>       };
>   
>       rc = rcu_lock_remote_domain_by_id(op_args->domid, &d);
> @@ -607,6 +611,68 @@ int dm_op(const struct dmop_args *op_args)
>           break;
>       }
>   
> +    case XEN_DMOP_bind_pt_msi_irq:
> +    {
> +        const struct xen_dm_op_bind_pt_msi_irq *data =
> +            &op.u.bind_pt_msi_irq;
> +        int irq;
> +
> +        rc = -EINVAL;
> +        if ( data->pad || (data->flags & ~XEN_DMOP_MSI_FLAG_UNMASKED) )
> +            break;
> +
> +        irq = domain_pirq_to_irq(d, data->machine_irq);
> +
> +        rc = -EPERM;
> +        if ( irq <= 0 || !irq_access_permitted(current->domain, irq) )
> +            break;
> +
> +        rc = -ESRCH;
> +        if ( is_iommu_enabled(d) )
> +        {
> +            read_lock(&d->pci_lock);
> +            rc = pt_irq_bind_msi(d, data->machine_irq, data->addr, data->data,
> +                                 data->gtable,
> +                                 !!(data->flags & XEN_DMOP_MSI_FLAG_UNMASKED));
> +            read_unlock(&d->pci_lock);
> +        }
> +        if ( rc < 0 )
> +            printk(XENLOG_G_ERR
> +                   "XEN_DMOP_bind_pt_msi_irq: pt_irq_bind_msi failed (%ld) for %pd\n",
> +                   rc, d);
> +        break;
> +    }
> +
> +    case XEN_DMOP_unbind_pt_msi_irq:
> +    {
> +        const struct xen_dm_op_unbind_pt_msi_irq *data =
> +            &op.u.unbind_pt_msi_irq;
> +        struct xen_domctl_bind_pt_irq bind = {
> +            .machine_irq = data->machine_irq,
> +            .irq_type = PT_IRQ_TYPE_MSI,
> +        };
> +        int irq;
> +
> +        irq = domain_pirq_to_irq(d, bind.machine_irq);
> +
> +        rc = -EPERM;
> +        if ( irq <= 0 || !irq_access_permitted(current->domain, irq) )
> +            break;
> +
> +        rc = -ESRCH;
> +        if ( is_iommu_enabled(d) )
> +        {
> +            read_lock(&d->pci_lock);
> +            rc = pt_irq_destroy_bind(d, &bind);
> +            read_unlock(&d->pci_lock);
> +        }
> +        if ( rc < 0 )
> +            printk(XENLOG_G_ERR
> +                   "XEN_DMOP_unbind_pt_msi_irq: pt_irq_destroy_bind failed (%ld) for %pd\n",
> +                   rc, d);
> +        break;
> +    }
> +
>       default:
>           rc = ioreq_server_dm_op(&op, d, &const_op);
>           break;
> @@ -643,6 +709,8 @@ CHECK_dm_op_remote_shutdown;
>   CHECK_dm_op_relocate_memory;
>   CHECK_dm_op_pin_memory_cacheattr;
>   CHECK_dm_op_nr_vcpus;
> +CHECK_dm_op_bind_pt_msi_irq;
> +CHECK_dm_op_unbind_pt_msi_irq;
>   
>   int compat_dm_op(
>       domid_t domid, unsigned int nr_bufs, XEN_GUEST_HANDLE_PARAM(void) bufs)
> diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
> index 026534530f..6fb4f8b7dc 100644
> --- a/xen/drivers/passthrough/x86/hvm.c
> +++ b/xen/drivers/passthrough/x86/hvm.c
> @@ -21,7 +21,6 @@
>   #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>
> diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
> index 2bf0fdc1ae..43571b7713 100644
> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -444,6 +444,41 @@ struct xen_dm_op_nr_vcpus {
>   };
>   typedef struct xen_dm_op_nr_vcpus xen_dm_op_nr_vcpus_t;
>   
> +#define XEN_DMOP_bind_pt_msi_irq   21
> +#define XEN_DMOP_unbind_pt_msi_irq 22
> +
> +struct xen_dm_op_bind_pt_msi_irq {
> +    /* IN - physical IRQ (pirq) */
> +    uint32_t machine_irq;
> +    /* IN - MSI data word (bits [7:0] are the guest vector) */
> +    uint32_t data;
> +    /* IN - flags */
> +    uint32_t flags;
> +#define XEN_DMOP_MSI_FLAG_UNMASKED (1u << 0)
> +    uint32_t pad;
> +    /* IN - MSI address (includes extended destination ID in bits [11:5]) */
> +    uint64_aligned_t addr;
> +    /* IN - MSI-X table base GFN, 0 for plain MSI */
> +    uint64_aligned_t gtable;
> +};
> +
> +typedef struct xen_dm_op_bind_pt_msi_irq xen_dm_op_bind_pt_msi_irq_t;
> +
> +struct xen_dm_op_unbind_pt_msi_irq {
> +    /* IN - physical IRQ (pirq) */
> +    uint32_t machine_irq;
> +};
> +typedef struct xen_dm_op_unbind_pt_msi_irq xen_dm_op_unbind_pt_msi_irq_t;
> +
> +/*
> + * XEN_DMOP_enable_ext_dest_id: Signal to Xen that this device model will use
> + * XEN_DMOP_bind_pt_msi_irq for all passthrough MSI bindings, passing raw MSI
> + * address/data fields. Once called, Xen will advertise
> + * XEN_HVM_CPUID_EXT_DEST_ID to the guest. Must be called before the guest
> + * starts.
> + */
> +#define XEN_DMOP_enable_ext_dest_id 23
> +
>   struct xen_dm_op {
>       uint32_t op;
>       uint32_t pad;
> @@ -468,6 +503,8 @@ struct xen_dm_op {
>           xen_dm_op_relocate_memory_t relocate_memory;
>           xen_dm_op_pin_memory_cacheattr_t pin_memory_cacheattr;
>           xen_dm_op_nr_vcpus_t nr_vcpus;
> +        xen_dm_op_bind_pt_msi_irq_t bind_pt_msi_irq;
> +        xen_dm_op_unbind_pt_msi_irq_t unbind_pt_msi_irq;
>       } u;
>   };
>   
> diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
> index 9d08dcc4bb..6dc5f5796a 100644
> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -98,6 +98,7 @@
>   ?	grant_entry_v2			grant_table.h
>   
>   !	dm_op_buf			hvm/dm_op.h
> +?	dm_op_bind_pt_msi_irq		hvm/dm_op.h
>   ?	dm_op_create_ioreq_server	hvm/dm_op.h
>   ?	dm_op_destroy_ioreq_server	hvm/dm_op.h
>   ?	dm_op_get_ioreq_server_info	hvm/dm_op.h
> @@ -116,6 +117,7 @@
>   ?	dm_op_set_pci_intx_level	hvm/dm_op.h
>   ?	dm_op_set_pci_link_route	hvm/dm_op.h
>   ?	dm_op_track_dirty_vram		hvm/dm_op.h
> +?	dm_op_unbind_pt_msi_irq		hvm/dm_op.h
>   
>   !	hvm_altp2m_set_mem_access_multi	hvm/hvm_op.h
>   



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v4 7/9] x86/dmop: Add XEN_DMOP_{bind,unbind}_pt_msi_irq DM ops

[Jan Beulich]

On 28.04.2026 18:02, Teddy Astie wrote:
> Le 27/04/2026 à 15:57, Julian Vetter a écrit :
>> Add two DM ops for MSI passthrough IRQs. These new DM ops take the raw
>> MSI address and data fields rather than pre-decoded gflags values. Xen
>> decodes the destination ID via msi_addr_to_gflags(), including any
>> extended destination bits in address[11:5]. This means the device model
>> does not need to understand the extended destination ID encoding, and
>> simply forwards the MSI address it observes from the guest.
>>
>> With these DM ops in place, redirect xc_domain_update_msi_irq() and
>> xc_domain_unbind_msi_irq() in libxenctrl to use
>> xendevicemodel_bind_pt_msi_irq() / xendevicemodel_unbind_pt_msi_irq()
>> via xch->dmod. The gflags/gvec arguments are translated to the raw MSI
>> address and data words at the libxc level using the standard x86 MSI
>> address format.
>>
>> Reject the PT_IRQ_TYPE_MSI sub-case in XEN_DOMCTL_bind_pt_irq and
>> XEN_DOMCTL_unbind_pt_irq: all callers now go through the DM op path, so
>> the domctl sub-case is fully obsolete.
> 
> We probably want to reflect that on XEN_DOMCTL_{un}bind_pt_irq interface 
> in domctl.h (e.g through a note saying that PT_IRQ_TYPE_MSI type is now 
> deprecated and unsupported).

Which may further want mentioning in ./CHANGELOG.md.

>> --- a/tools/libs/devicemodel/core.c
>> +++ b/tools/libs/devicemodel/core.c
>> @@ -645,6 +645,44 @@ int xendevicemodel_nr_vcpus(
>>       return 0;
>>   }
>>   
>> +int xendevicemodel_bind_pt_msi_irq(
>> +    xendevicemodel_handle *dmod, domid_t domid, uint32_t machine_irq,
>> +    uint64_t msi_addr, uint32_t msi_data, uint64_t gtable, int unmasked)
>> +{
>> +    struct xen_dm_op op;
>> +    struct xen_dm_op_bind_pt_msi_irq *data;
>> +
>> +    memset(&op, 0, sizeof(op));
>> +
>> +    op.op = XEN_DMOP_bind_pt_msi_irq;
>> +    data = &op.u.bind_pt_msi_irq;
>> +
>> +    data->machine_irq = machine_irq;
>> +    data->data = msi_data;
>> +    data->addr = msi_addr;
>> +    data->gtable = gtable;
>> +    if ( unmasked )
>> +        data->flags |= XEN_DMOP_MSI_FLAG_UNMASKED;
>> +
>> +    return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
>> +}
>> +
>> +int xendevicemodel_unbind_pt_msi_irq(
>> +    xendevicemodel_handle *dmod, domid_t domid, uint32_t machine_irq)
>> +{
>> +    struct xen_dm_op op;
>> +    struct xen_dm_op_unbind_pt_msi_irq *data;
>> +
>> +    memset(&op, 0, sizeof(op));
>> +
>> +    op.op = XEN_DMOP_unbind_pt_msi_irq;
>> +    data = &op.u.unbind_pt_msi_irq;
>> +
>> +    data->machine_irq = machine_irq;
>> +
>> +    return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
>> +}
>> +
> 
> I think we want to mark 
> xc_domain_update_msi_irq/xc_domain_unbind_msi_irq as deprecated since we 
> implemented a newer (better) version of it in xendevicemodel; and the 
> old one is now a wrapper.

Why mark it deprecated? It can be removed right away when there are no callers
left. libxc doesn't offer a stable API.

Jan

Re: [PATCH v4 7/9] x86/dmop: Add XEN_DMOP_{bind,unbind}_pt_msi_irq DM ops

[Jan Beulich]

On 27.04.2026 15:54, Julian Vetter wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -574,6 +574,14 @@ long arch_do_domctl(
>          if ( !is_hvm_domain(d) )
>              break;
>  
> +        /*
> +         * PT_IRQ_TYPE_MSI is obsoleted by XEN_DMOP_bind_pt_msi_irq, which
> +         * passes raw MSI address/data so Xen can decode extended destination
> +         * ID bits. Device models must use the DM op path instead.
> +         */
> +        if ( bind->irq_type == PT_IRQ_TYPE_MSI )
> +            break;

Oh, here is where you have put the reject logic. With the other call to
pt_irq_create_bind() having been removed by patch 5, respective logic in
that function (as last modified also by patch 5) is now unreachable,
violating Misra rule 2.1.

Then again you cannot do this anyway, as it breaks older DMs. You want to
reject this only when XEN_DMOP_enable_ext_dest_id (subject to rename) was
called earlier. And you want to reject XEN_DMOP_enable_ext_dest_id when
XEN_DOMCTL_bind_pt_irq with PT_IRQ_TYPE_MSI was called earlier on. We
want to make sure that we get to see uses of only one kind of interface
(unless both interfaces can be made interoperate cleanly).

> @@ -607,6 +611,68 @@ int dm_op(const struct dmop_args *op_args)
>          break;
>      }
>  
> +    case XEN_DMOP_bind_pt_msi_irq:
> +    {
> +        const struct xen_dm_op_bind_pt_msi_irq *data =
> +            &op.u.bind_pt_msi_irq;
> +        int irq;
> +
> +        rc = -EINVAL;
> +        if ( data->pad || (data->flags & ~XEN_DMOP_MSI_FLAG_UNMASKED) )
> +            break;
> +
> +        irq = domain_pirq_to_irq(d, data->machine_irq);
> +
> +        rc = -EPERM;
> +        if ( irq <= 0 || !irq_access_permitted(current->domain, irq) )
> +            break;
> +
> +        rc = -ESRCH;
> +        if ( is_iommu_enabled(d) )
> +        {
> +            read_lock(&d->pci_lock);
> +            rc = pt_irq_bind_msi(d, data->machine_irq, data->addr, data->data,
> +                                 data->gtable,
> +                                 !!(data->flags & XEN_DMOP_MSI_FLAG_UNMASKED));

As before, no need for !!.

> +            read_unlock(&d->pci_lock);
> +        }
> +        if ( rc < 0 )
> +            printk(XENLOG_G_ERR
> +                   "XEN_DMOP_bind_pt_msi_irq: pt_irq_bind_msi failed (%ld) for %pd\n",

Imo this is too verbose. If anything needs logging here at all (which I
question), "%pd: pt_irq_bind_msi() failed: %ld\n" would likely do, without
becoming ambiguous. (Same below then, obviously.)

> +                   rc, d);
> +        break;
> +    }

Where did, btw, the XSM check go that the original code has? Daniel - I
don't think such can simply be dropped, despite there being xsm_dm_op()
on the path here?

> +    case XEN_DMOP_unbind_pt_msi_irq:
> +    {
> +        const struct xen_dm_op_unbind_pt_msi_irq *data =
> +            &op.u.unbind_pt_msi_irq;
> +        struct xen_domctl_bind_pt_irq bind = {
> +            .machine_irq = data->machine_irq,
> +            .irq_type = PT_IRQ_TYPE_MSI,
> +        };
> +        int irq;
> +
> +        irq = domain_pirq_to_irq(d, bind.machine_irq);
> +
> +        rc = -EPERM;
> +        if ( irq <= 0 || !irq_access_permitted(current->domain, irq) )
> +            break;

As we're making a new interface, we need to consider getting rid of bogus
aspects of the old one. Along the lines of what 6df6f24251db ("domctl:
restrict permission check for XEN_DOMCTL_memory_mapping's remove form")
says, and as then also mirrored by 6e42fa383c70 ("x86/domctl: don't imply
I/O port permissions from I/O port mapping"), a permission check on unmap
(here: unbind) for current->domain may be excessive: Even if permission
was already removed, the DM should still be able to unbind the guest's
IRQ.

> +        rc = -ESRCH;
> +        if ( is_iommu_enabled(d) )
> +        {
> +            read_lock(&d->pci_lock);
> +            rc = pt_irq_destroy_bind(d, &bind);
> +            read_unlock(&d->pci_lock);

Here and above - please pay attention to impending locking changes at the
original site, as per (much) earlier discussion. (As said there, I don't
think a lock needs taking here - or above - at all.)

> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -444,6 +444,41 @@ struct xen_dm_op_nr_vcpus {
>  };
>  typedef struct xen_dm_op_nr_vcpus xen_dm_op_nr_vcpus_t;
>  
> +#define XEN_DMOP_bind_pt_msi_irq   21
> +#define XEN_DMOP_unbind_pt_msi_irq 22
> +
> +struct xen_dm_op_bind_pt_msi_irq {
> +    /* IN - physical IRQ (pirq) */
> +    uint32_t machine_irq;

Please can comment and field identifier match up with one another? We don't
want to carry over such an inconsistency from the old interface.

> +    /* IN - MSI data word (bits [7:0] are the guest vector) */

The part in parentheses is x86-centric, which we'd better avoid in the public
headers.

> +    uint32_t data;
> +    /* IN - flags */
> +    uint32_t flags;
> +#define XEN_DMOP_MSI_FLAG_UNMASKED (1u << 0)

s/FLAG/BIND/ perhaps?

> +    uint32_t pad;
> +    /* IN - MSI address (includes extended destination ID in bits [11:5]) */

Please again omit the x86-centric part.

> +    uint64_aligned_t addr;
> +    /* IN - MSI-X table base GFN, 0 for plain MSI */
> +    uint64_aligned_t gtable;

This is a GADDR, not a GFN, isn't it?

With this, the earlier field being named just "addr" also ends up potentially
ambiguous. Perhaps msg_addr (and then also msg_data)?

More generally: Why does the DM need to be bothered about IRQ numbers in the
first place? To identify a particular MSI, what you need are device coordinates
and an index. Once passed in like this, the need for passing in "gtable" for
MSI-X should then also disappear. That said, re-working accordingly may incur
significant effort. That needs weighing against the downsides of introducing
another partly screwed interface.

> +};
> +
> +typedef struct xen_dm_op_bind_pt_msi_irq xen_dm_op_bind_pt_msi_irq_t;

Please omit the intermediate blank line, just like ...

> +struct xen_dm_op_unbind_pt_msi_irq {
> +    /* IN - physical IRQ (pirq) */
> +    uint32_t machine_irq;
> +};
> +typedef struct xen_dm_op_unbind_pt_msi_irq xen_dm_op_unbind_pt_msi_irq_t;

... you do here. That said - are these typedefs needed anywhere in the
first place?

> +/*
> + * XEN_DMOP_enable_ext_dest_id: Signal to Xen that this device model will use
> + * XEN_DMOP_bind_pt_msi_irq for all passthrough MSI bindings, passing raw MSI
> + * address/data fields. Once called, Xen will advertise
> + * XEN_HVM_CPUID_EXT_DEST_ID to the guest. Must be called before the guest
> + * starts.
> + */
> +#define XEN_DMOP_enable_ext_dest_id 23

I don't understand this. With XEN_DOMCTL_bind_pt_irq's PT_IRQ_TYPE_MSI case
cut off, DMs have no alternative besides using XEN_DMOP_bind_pt_msi_irq. If
that cut-off was viable, I think this comment would want re-wording almost
from scratch. As the cut-off needs dropping / constraining, some less severe
edit may do. The requirement to call this before the guest starts isn't
enough imo: It also needs to be called ahead of any binding, as the behavior
of the binding logic will need to be dependent upon whether this call was
issued.

The identifier XEN_DMOP_enable_ext_dest_id isn't suitable, though, as this
is about the choice of interface the DM is going to use. The newer interface
offering extended-ID support is merely a wanted side effect.

And then it's pretty odd that you add this #define here, but there's no
handling of the new sub-op. Was this perhaps meant to go in the next patch?

Jan

[PATCH v4 8/9] hvm/ioreq: Negotiate extended destination ID support per ioreq server

[Julian Vetter]

[-- Attachment #1: Type: text/plain, Size: 18200 bytes --]

Add a per-server capability flag in XEN_DMOP_create_ioreq_server to
signal extended destination ID support. Repurpose the first byte of the
existing pad[3] as a flags field, and define
XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID (bit 0) for a server to signal it will
use XEN_DMOP_bind_pt_msi_irq for all passthrough MSI bindings.

Track the flag in struct ioreq_server ext_dest_id.
hvm_ext_dest_id_enabled() returns true only if all registered ioreq
servers have opted in and at least one server is present. A single
server without the flag is sufficient to suppress the feature.

Lock the feature at domain creation time:
arch_domain_creation_finished() computes the levelled result into struct
hvm_domain.ext_dest_id using OR to preserve any value previously
restored from an HVM save record. After creation_finished,
arch_ioreq_server_create_check() rejects new servers that lack
XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID if the feature was already advertised
to the guest.

Persist the locked state in a new HVM_SAVE_TYPE(EXT_DEST_ID) record so
that migration preserves the guest-visible CPUID bit independently of
when the device model re-registers its ioreq servers on the destination
host.

On restore, ioapic_check() uses d->arch.hvm.ext_dest_id (restored from
the EXT_DEST_ID record) rather than the per-server dynamic check, since
the DM has not yet re-registered its servers at that point.

Update xendevicemodel_create_ioreq_server() in libxendevicemodel to
accept the new flags parameter, remove
xendevicemodel_enable_ext_dest_id(), and fix the
xc_hvm_create_ioreq_server() compat wrapper to pass zero flags.

Signed-off-by: Julian Vetter <julian.vetter@vates.tech>
---
Changes in v4:
- As suggested by Roger, replaced XEN_DMOP_enable_ext_dest_id (v3 patch
  6), a separate DM op the device model had to call before starting
  vCPUs, with a flags byte repurposed from the existing pad[3] field of
  xen_dm_op_create_ioreq_server
- New XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID flag (bit 0) lets each ioreq
  server signal support at registration time
- As suggested by Roger level the feature across all ioreq servers.
  XEN_HVM_CPUID_EXT_DEST_ID is only advertised when every server
  registered before arch_domain_creation_finished() sets the flag. A
  single server without the flag suppresses the feature for the whole
  domain!
- Lock the levelled result at domain creation time and enforce it for
  servers registered afterwards, preventing a late opt-out from breaking
  guests that already see the feature in CPUID
- Persist the locked flag via HVM_SAVE_TYPE(EXT_DEST_ID) so that live
  migration preserves the guest-visible CPUID bit independently of when
  the device model registers its ioreq servers on the destination host
---
 tools/include/xendevicemodel.h          |  3 +-
 tools/libs/ctrl/xc_devicemodel_compat.c |  2 +-
 tools/libs/devicemodel/core.c           |  3 +-
 xen/arch/arm/ioreq.c                    |  5 ++++
 xen/arch/x86/domain.c                   | 10 +++++++
 xen/arch/x86/hvm/ioreq.c                | 37 +++++++++++++++++++++++++
 xen/arch/x86/hvm/vioapic.c              | 20 +++++++++++++
 xen/arch/x86/include/asm/hvm/domain.h   |  9 ++++++
 xen/common/ioreq.c                      | 13 +++++++--
 xen/drivers/passthrough/x86/hvm.c       | 13 +++++++++
 xen/include/public/arch-x86/hvm/save.h  | 17 +++++++++++-
 xen/include/public/hvm/dm_op.h          | 16 +++++++++--
 xen/include/xen/ioreq.h                 | 27 ++++++++++++++++++
 13 files changed, 165 insertions(+), 10 deletions(-)

diff --git a/tools/include/xendevicemodel.h b/tools/include/xendevicemodel.h
index f15b35fa33..bc6764bd58 100644
--- a/tools/include/xendevicemodel.h
+++ b/tools/include/xendevicemodel.h
@@ -44,12 +44,13 @@ int xendevicemodel_close(xendevicemodel_handle *dmod);
  * @parm domid the domain id to be serviced
  * @parm handle_bufioreq how should the IOREQ Server handle buffered
  *                       requests (HVM_IOREQSRV_BUFIOREQ_*)?
+ * @parm flags bitmask of XEN_DMOP_IOREQ_SERVER_* capability flags.
  * @parm id pointer to an ioservid_t to receive the IOREQ Server id.
  * @return 0 on success, -1 on failure.
  */
 int xendevicemodel_create_ioreq_server(
     xendevicemodel_handle *dmod, domid_t domid, int handle_bufioreq,
-    ioservid_t *id);
+    uint8_t flags, ioservid_t *id);
 
 /**
  * This function retrieves the necessary information to allow an
diff --git a/tools/libs/ctrl/xc_devicemodel_compat.c b/tools/libs/ctrl/xc_devicemodel_compat.c
index a46011cd17..91366e250c 100644
--- a/tools/libs/ctrl/xc_devicemodel_compat.c
+++ b/tools/libs/ctrl/xc_devicemodel_compat.c
@@ -11,7 +11,7 @@ int xc_hvm_create_ioreq_server(
     ioservid_t *id)
 {
     return xendevicemodel_create_ioreq_server(xch->dmod, domid,
-                                              handle_bufioreq, id);
+                                              handle_bufioreq, 0, id);
 }
 
 int xc_hvm_get_ioreq_server_info(
diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
index adf2c41a96..49b9bf8a13 100644
--- a/tools/libs/devicemodel/core.c
+++ b/tools/libs/devicemodel/core.c
@@ -167,7 +167,7 @@ static int xendevicemodel_op(
 
 int xendevicemodel_create_ioreq_server(
     xendevicemodel_handle *dmod, domid_t domid, int handle_bufioreq,
-    ioservid_t *id)
+    uint8_t flags, ioservid_t *id)
 {
     struct xen_dm_op op;
     struct xen_dm_op_create_ioreq_server *data;
@@ -179,6 +179,7 @@ int xendevicemodel_create_ioreq_server(
     data = &op.u.create_ioreq_server;
 
     data->handle_bufioreq = handle_bufioreq;
+    data->flags = flags;
 
     rc = xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
     if (rc)
diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
index b4211f0159..d45228717a 100644
--- a/xen/arch/arm/ioreq.c
+++ b/xen/arch/arm/ioreq.c
@@ -201,6 +201,11 @@ void arch_ioreq_domain_init(struct domain *d)
 {
 }
 
+int arch_ioreq_server_create_check(const struct domain *d, uint8_t flags)
+{
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 1d458f1372..68ff315460 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -25,6 +25,7 @@
 #include <xen/init.h>
 #include <xen/iocap.h>
 #include <xen/iommu.h>
+#include <xen/ioreq.h>
 #include <xen/irq.h>
 #include <xen/kernel.h>
 #include <xen/lib.h>
@@ -1106,7 +1107,16 @@ int arch_domain_soft_reset(struct domain *d)
 void arch_domain_creation_finished(struct domain *d)
 {
     if ( is_hvm_domain(d) )
+    {
+        /*
+         * Lock the extended destination ID state. OR preserves any value
+         * already restored from an HVM save record (migration path). For a
+         * fresh domain, ext_dest_id starts false and the dynamic check
+         * supplies the levelled result across all registered ioreq servers.
+         */
+        d->arch.hvm.ext_dest_id |= hvm_ext_dest_id_enabled(d);
         hvm_domain_creation_finished(d);
+    }
 }
 
 #ifdef CONFIG_COMPAT
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index a5fa97e149..894a63c522 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -19,6 +19,7 @@
 
 #include <asm/hvm/emulate.h>
 #include <asm/hvm/hvm.h>
+#include <asm/hvm/support.h>
 #include <asm/hvm/vmx/vmx.h>
 #include <asm/msr.h>
 
@@ -325,6 +326,42 @@ void arch_ioreq_domain_init(struct domain *d)
     register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
 }
 
+int arch_ioreq_server_create_check(const struct domain *d, uint8_t flags)
+{
+    if ( !is_hvm_domain(d) || !d->creation_finished )
+        return 0;
+
+    if ( d->arch.hvm.ext_dest_id &&
+         !(flags & XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID) )
+        return -EPERM;
+
+    return 0;
+}
+
+static int cf_check ext_dest_id_save(struct vcpu *v, hvm_domain_context_t *h)
+{
+    struct hvm_hw_ext_dest_id s = {
+        .enabled = v->domain->arch.hvm.ext_dest_id,
+    };
+
+    return hvm_save_entry(EXT_DEST_ID, 0, h, &s);
+}
+
+static int cf_check ext_dest_id_load(struct domain *d, hvm_domain_context_t *h)
+{
+    struct hvm_hw_ext_dest_id s;
+
+    if ( hvm_load_entry(EXT_DEST_ID, h, &s) )
+        return -EINVAL;
+
+    d->arch.hvm.ext_dest_id = s.enabled;
+
+    return 0;
+}
+
+HVM_REGISTER_SAVE_RESTORE(EXT_DEST_ID, ext_dest_id_save, NULL,
+                          ext_dest_id_load, 1, HVMSR_PER_DOM);
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 527cc770b7..7d037a53e1 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -24,6 +24,7 @@
  *  Ported to xen by using virtual IRQ line.
  */
 
+#include <xen/ioreq.h>
 #include <xen/types.h>
 #include <xen/mm.h>
 #include <xen/xmalloc.h>
@@ -597,6 +598,7 @@ int vioapic_get_trigger_mode(const struct domain *d, unsigned int gsi)
 static int cf_check ioapic_check(const struct domain *d, hvm_domain_context_t *h)
 {
     const HVM_SAVE_TYPE(IOAPIC) *s;
+    unsigned int i;
 
     if ( !has_vioapic(d) )
         return -ENODEV;
@@ -617,6 +619,24 @@ static int cf_check ioapic_check(const struct domain *d, hvm_domain_context_t *h
     if ( s->ioregsel > VIOAPIC_REG_RTE0 + (ARRAY_SIZE(s->redirtbl) - 1) * 2 + 1 )
         return -EINVAL;
 
+    /*
+     * If any RTE uses extended destination ID bits, the EXT_DEST_ID save
+     * record must have been loaded first (restoring d->arch.hvm.ext_dest_id).
+     * The ioreq server re-registration by the DM happens later, so use the
+     * domain-level locked flag rather than the per-server dynamic check.
+     */
+    for ( i = 0; i < ARRAY_SIZE(s->redirtbl); i++ )
+    {
+        if ( s->redirtbl[i].fields.ext_dest_id && !d->arch.hvm.ext_dest_id )
+        {
+            printk(XENLOG_G_ERR "HVM restore: %pd IO-APIC RTE %u has "
+                                "extended destination ID bits set but "
+                                "EXT_DEST_ID is not enabled\n",
+                                d, i);
+            return -EINVAL;
+        }
+    }
+
     return 0;
 }
 
diff --git a/xen/arch/x86/include/asm/hvm/domain.h b/xen/arch/x86/include/asm/hvm/domain.h
index abf9bc448d..895b2e12ba 100644
--- a/xen/arch/x86/include/asm/hvm/domain.h
+++ b/xen/arch/x86/include/asm/hvm/domain.h
@@ -102,6 +102,15 @@ struct hvm_domain {
 
     bool                   is_s3_suspended;
 
+    /*
+     * True when XEN_HVM_CPUID_EXT_DEST_ID was advertised to the guest. Locked
+     * at domain creation time once every registered ioreq server has opted in
+     * via XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID. Persisted in HVM save/restore so
+     * migration preserves the guest-visible state independently of when the
+     * device model re-registers its ioreq servers on the destination host.
+     */
+    bool                   ext_dest_id;
+
     /* Compatibility setting for a bug in x2APIC LDR */
     bool bug_x2apic_ldr_vcpu_id;
 
diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
index f5fd30ce12..56a7eb8282 100644
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -641,7 +641,7 @@ static void ioreq_server_deinit(struct ioreq_server *s)
 }
 
 static int ioreq_server_create(struct domain *d, int bufioreq_handling,
-                               ioservid_t *id)
+                               uint8_t flags, ioservid_t *id)
 {
     struct ioreq_server *s;
     unsigned int i;
@@ -683,6 +683,8 @@ static int ioreq_server_create(struct domain *d, int bufioreq_handling,
         goto fail;
     }
 
+    s->ext_dest_id = flags & XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID;
+
     if ( id )
         *id = i;
 
@@ -1350,11 +1352,16 @@ int ioreq_server_dm_op(struct xen_dm_op *op, struct domain *d, bool *const_op)
         *const_op = false;
 
         rc = -EINVAL;
-        if ( data->pad[0] || data->pad[1] || data->pad[2] )
+        if ( data->flags & ~XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID ||
+             data->pad[0] || data->pad[1] )
+            break;
+
+        rc = arch_ioreq_server_create_check(d, data->flags);
+        if ( rc )
             break;
 
         rc = ioreq_server_create(d, data->handle_bufioreq,
-                                 &data->id);
+                                 data->flags, &data->id);
         break;
     }
 
diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
index 6fb4f8b7dc..f7f7c02076 100644
--- 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>
@@ -455,6 +456,18 @@ int pt_irq_create_bind(
         uint64_t msi_addr;
         uint32_t msi_data;
 
+        /*
+         * Refuse the old MSI bind path when extended destination IDs are
+         * in use. The caller must use XEN_DMOP_bind_pt_msi_irq instead,
+         * which passes the raw MSI address so Xen can decode the extended
+         * bits. This old path only carries an 8-bit destination ID and
+         * would silently misroute interrupts to vCPUs with APIC IDs > 255.
+         */
+        if ( hvm_ext_dest_id_enabled(d) )
+        {
+            return -EPERM;
+        }
+
         msi_addr = MSI_ADDR_HEADER |
                    MASK_INSR(MASK_EXTR(gflags, XEN_DOMCTL_VMSI_X86_DEST_ID_MASK),
                              MSI_ADDR_DEST_ID_MASK) |
diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
index 483097d940..dd70ce18c6 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -627,12 +627,27 @@ struct hvm_msr {
 
 #define CPU_MSR_CODE  20
 
+/*
+ * HVM_SAVE_TYPE(EXT_DEST_ID): domain-level extended MSI destination ID state.
+ *
+ * Records whether the extended destination ID feature was enabled for this
+ * domain at the time guest vCPUs were started. This allows migration to
+ * preserve the setting across hosts without relying on the device model to
+ * re-register its ioreq servers before the guest's first CPUID query.
+ */
+struct hvm_hw_ext_dest_id {
+    uint8_t enabled;
+    uint8_t pad[7];
+};
+
+DECLARE_HVM_SAVE_TYPE(EXT_DEST_ID, 21, struct hvm_hw_ext_dest_id);
+
 /* Range 22 - 34 (inclusive) reserved for Amazon */
 
 /*
  * Largest type-code in use
  */
-#define HVM_SAVE_CODE_MAX 20
+#define HVM_SAVE_CODE_MAX 21
 
 #endif /* __XEN_PUBLIC_HVM_SAVE_X86_H__ */
 
diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
index 43571b7713..73f33b3c46 100644
--- a/xen/include/public/hvm/dm_op.h
+++ b/xen/include/public/hvm/dm_op.h
@@ -39,18 +39,28 @@ typedef uint16_t ioservid_t;
  * XEN_DMOP_create_ioreq_server: Instantiate a new IOREQ Server for a
  *                               secondary emulator.
  *
- * The <id> handed back is unique for target domain. The valur of
+ * The <id> handed back is unique for target domain. The value of
  * <handle_bufioreq> should be one of HVM_IOREQSRV_BUFIOREQ_* defined in
- * hvm_op.h. If the value is HVM_IOREQSRV_BUFIOREQ_OFF then  the buffered
+ * hvm_op.h. If the value is HVM_IOREQSRV_BUFIOREQ_OFF then the buffered
  * ioreq ring will not be allocated and hence all emulation requests to
  * this server will be synchronous.
+ *
+ * If <flags> contains XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID, the server will
+ * use XEN_DMOP_bind_pt_msi_irq for all passthrough MSI bindings, passing
+ * raw MSI address/data fields so Xen can decode extended destination ID
+ * bits. Once any server sets this flag, Xen will advertise
+ * XEN_HVM_CPUID_EXT_DEST_ID to the guest. Must be set before the guest
+ * vCPUs are started.
  */
 #define XEN_DMOP_create_ioreq_server 1
 
 struct xen_dm_op_create_ioreq_server {
     /* IN - should server handle buffered ioreqs */
     uint8_t handle_bufioreq;
-    uint8_t pad[3];
+    /* IN - server capability flags */
+    uint8_t flags;
+#define XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID (1u << 0)
+    uint8_t pad[2];
     /* OUT - server id */
     ioservid_t id;
 };
diff --git a/xen/include/xen/ioreq.h b/xen/include/xen/ioreq.h
index e86f0869fa..ec78b63942 100644
--- a/xen/include/xen/ioreq.h
+++ b/xen/include/xen/ioreq.h
@@ -54,9 +54,35 @@ struct ioreq_server {
     evtchn_port_t          bufioreq_evtchn;
     struct rangeset        *range[NR_IO_RANGE_TYPES];
     bool                   enabled;
+    bool                   ext_dest_id;
     uint8_t                bufioreq_handling;
 };
 
+/*
+ * Return true if every registered ioreq server has opted in to extended
+ * destination IDs (XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID) and at least one
+ * server exists. A single server without the flag is enough to suppress
+ * XEN_HVM_CPUID_EXT_DEST_ID, preventing misrouted interrupts.
+ */
+static inline bool hvm_ext_dest_id_enabled(const struct domain *d)
+{
+    unsigned int i;
+    bool found = false;
+
+    for ( i = 0; i < MAX_NR_IOREQ_SERVERS; i++ )
+    {
+        const struct ioreq_server *s = d->ioreq_server.server[i];
+
+        if ( !s )
+            continue;
+        if ( !s->ext_dest_id )
+            return false;
+        found = true;
+    }
+
+    return found;
+}
+
 static inline paddr_t ioreq_mmio_first_byte(const ioreq_t *p)
 {
     return unlikely(p->df) ?
@@ -137,6 +163,7 @@ bool arch_ioreq_server_destroy_all(struct domain *d);
 bool arch_ioreq_server_get_type_addr(const struct domain *d, const ioreq_t *p,
                                      uint8_t *type, uint64_t *addr);
 void arch_ioreq_domain_init(struct domain *d);
+int arch_ioreq_server_create_check(const struct domain *d, uint8_t flags);
 
 #endif /* __XEN_IOREQ_H__ */
 
-- 
2.53.0



--
Julian Vetter | Vates Hypervisor & Kernel Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v4 8/9] hvm/ioreq: Negotiate extended destination ID support per ioreq server

[Teddy Astie]

[-- Attachment #1: Type: text/plain, Size: 20322 bytes --]

Le 27/04/2026 à 15:57, Julian Vetter a écrit :
> Add a per-server capability flag in XEN_DMOP_create_ioreq_server to
> signal extended destination ID support. Repurpose the first byte of the
> existing pad[3] as a flags field, and define
> XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID (bit 0) for a server to signal it will
> use XEN_DMOP_bind_pt_msi_irq for all passthrough MSI bindings.
> 
> Track the flag in struct ioreq_server ext_dest_id.
> hvm_ext_dest_id_enabled() returns true only if all registered ioreq
> servers have opted in and at least one server is present. A single
> server without the flag is sufficient to suppress the feature.
> 
> Lock the feature at domain creation time:
> arch_domain_creation_finished() computes the levelled result into struct
> hvm_domain.ext_dest_id using OR to preserve any value previously
> restored from an HVM save record. After creation_finished,
> arch_ioreq_server_create_check() rejects new servers that lack
> XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID if the feature was already advertised
> to the guest.
> 
> Persist the locked state in a new HVM_SAVE_TYPE(EXT_DEST_ID) record so
> that migration preserves the guest-visible CPUID bit independently of
> when the device model re-registers its ioreq servers on the destination
> host.
> 
> On restore, ioapic_check() uses d->arch.hvm.ext_dest_id (restored from
> the EXT_DEST_ID record) rather than the per-server dynamic check, since
> the DM has not yet re-registered its servers at that point.
> 
> Update xendevicemodel_create_ioreq_server() in libxendevicemodel to
> accept the new flags parameter, remove
> xendevicemodel_enable_ext_dest_id(), and fix the
> xc_hvm_create_ioreq_server() compat wrapper to pass zero flags.
> 
> Signed-off-by: Julian Vetter <julian.vetter@vates.tech>

That has somewhat already being discussed previously, but AFAIU, 
extended destination ID is only meaningful when guest APIC IDs cannot be 
represented with the "non-extended" model which can only happen in 
practice when having more than 128 vCPUs in the guest.

I don't think we need to check for device model support unless the guest 
can have more than 128 vCPUs, where in such case it becomes mandatory 
(unless some form of interrupt remapping is implemented).

So I would rather check if domain->max_vcpus is more than 128 and 
require device models to implement support for extended destination ID 
in these cases.

In some way, that would imply that extended destination ID is only 
exposed to guests with domain->max_vcpus > 128.

Overall, what I propose would be to keep the new 
XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID flag, and if d->max_vcpus > 128, we 
require the device model to support XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID.

> ---
> Changes in v4:
> - As suggested by Roger, replaced XEN_DMOP_enable_ext_dest_id (v3 patch
>    6), a separate DM op the device model had to call before starting
>    vCPUs, with a flags byte repurposed from the existing pad[3] field of
>    xen_dm_op_create_ioreq_server
> - New XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID flag (bit 0) lets each ioreq
>    server signal support at registration time
> - As suggested by Roger level the feature across all ioreq servers.
>    XEN_HVM_CPUID_EXT_DEST_ID is only advertised when every server
>    registered before arch_domain_creation_finished() sets the flag. A
>    single server without the flag suppresses the feature for the whole
>    domain!
> - Lock the levelled result at domain creation time and enforce it for
>    servers registered afterwards, preventing a late opt-out from breaking
>    guests that already see the feature in CPUID
> - Persist the locked flag via HVM_SAVE_TYPE(EXT_DEST_ID) so that live
>    migration preserves the guest-visible CPUID bit independently of when
>    the device model registers its ioreq servers on the destination host
> ---
>   tools/include/xendevicemodel.h          |  3 +-
>   tools/libs/ctrl/xc_devicemodel_compat.c |  2 +-
>   tools/libs/devicemodel/core.c           |  3 +-
>   xen/arch/arm/ioreq.c                    |  5 ++++
>   xen/arch/x86/domain.c                   | 10 +++++++
>   xen/arch/x86/hvm/ioreq.c                | 37 +++++++++++++++++++++++++
>   xen/arch/x86/hvm/vioapic.c              | 20 +++++++++++++
>   xen/arch/x86/include/asm/hvm/domain.h   |  9 ++++++
>   xen/common/ioreq.c                      | 13 +++++++--
>   xen/drivers/passthrough/x86/hvm.c       | 13 +++++++++
>   xen/include/public/arch-x86/hvm/save.h  | 17 +++++++++++-
>   xen/include/public/hvm/dm_op.h          | 16 +++++++++--
>   xen/include/xen/ioreq.h                 | 27 ++++++++++++++++++
>   13 files changed, 165 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/include/xendevicemodel.h b/tools/include/xendevicemodel.h
> index f15b35fa33..bc6764bd58 100644
> --- a/tools/include/xendevicemodel.h
> +++ b/tools/include/xendevicemodel.h
> @@ -44,12 +44,13 @@ int xendevicemodel_close(xendevicemodel_handle *dmod);
>    * @parm domid the domain id to be serviced
>    * @parm handle_bufioreq how should the IOREQ Server handle buffered
>    *                       requests (HVM_IOREQSRV_BUFIOREQ_*)?
> + * @parm flags bitmask of XEN_DMOP_IOREQ_SERVER_* capability flags.
>    * @parm id pointer to an ioservid_t to receive the IOREQ Server id.
>    * @return 0 on success, -1 on failure.
>    */
>   int xendevicemodel_create_ioreq_server(
>       xendevicemodel_handle *dmod, domid_t domid, int handle_bufioreq,
> -    ioservid_t *id);
> +    uint8_t flags, ioservid_t *id);
>   
>   /**
>    * This function retrieves the necessary information to allow an
> diff --git a/tools/libs/ctrl/xc_devicemodel_compat.c b/tools/libs/ctrl/xc_devicemodel_compat.c
> index a46011cd17..91366e250c 100644
> --- a/tools/libs/ctrl/xc_devicemodel_compat.c
> +++ b/tools/libs/ctrl/xc_devicemodel_compat.c
> @@ -11,7 +11,7 @@ int xc_hvm_create_ioreq_server(
>       ioservid_t *id)
>   {
>       return xendevicemodel_create_ioreq_server(xch->dmod, domid,
> -                                              handle_bufioreq, id);
> +                                              handle_bufioreq, 0, id);
>   }
>   
>   int xc_hvm_get_ioreq_server_info(
> diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
> index adf2c41a96..49b9bf8a13 100644
> --- a/tools/libs/devicemodel/core.c
> +++ b/tools/libs/devicemodel/core.c
> @@ -167,7 +167,7 @@ static int xendevicemodel_op(
>   
>   int xendevicemodel_create_ioreq_server(
>       xendevicemodel_handle *dmod, domid_t domid, int handle_bufioreq,
> -    ioservid_t *id)
> +    uint8_t flags, ioservid_t *id)
>   {
>       struct xen_dm_op op;
>       struct xen_dm_op_create_ioreq_server *data;
> @@ -179,6 +179,7 @@ int xendevicemodel_create_ioreq_server(
>       data = &op.u.create_ioreq_server;
>   
>       data->handle_bufioreq = handle_bufioreq;
> +    data->flags = flags;
>   
>       rc = xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
>       if (rc)
> diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
> index b4211f0159..d45228717a 100644
> --- a/xen/arch/arm/ioreq.c
> +++ b/xen/arch/arm/ioreq.c
> @@ -201,6 +201,11 @@ void arch_ioreq_domain_init(struct domain *d)
>   {
>   }
>   
> +int arch_ioreq_server_create_check(const struct domain *d, uint8_t flags)
> +{
> +    return 0;
> +}
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 1d458f1372..68ff315460 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -25,6 +25,7 @@
>   #include <xen/init.h>
>   #include <xen/iocap.h>
>   #include <xen/iommu.h>
> +#include <xen/ioreq.h>
>   #include <xen/irq.h>
>   #include <xen/kernel.h>
>   #include <xen/lib.h>
> @@ -1106,7 +1107,16 @@ int arch_domain_soft_reset(struct domain *d)
>   void arch_domain_creation_finished(struct domain *d)
>   {
>       if ( is_hvm_domain(d) )
> +    {
> +        /*
> +         * Lock the extended destination ID state. OR preserves any value
> +         * already restored from an HVM save record (migration path). For a
> +         * fresh domain, ext_dest_id starts false and the dynamic check
> +         * supplies the levelled result across all registered ioreq servers.
> +         */
> +        d->arch.hvm.ext_dest_id |= hvm_ext_dest_id_enabled(d);
>           hvm_domain_creation_finished(d);
> +    }
>   }
>   
>   #ifdef CONFIG_COMPAT
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index a5fa97e149..894a63c522 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -19,6 +19,7 @@
>   
>   #include <asm/hvm/emulate.h>
>   #include <asm/hvm/hvm.h>
> +#include <asm/hvm/support.h>
>   #include <asm/hvm/vmx/vmx.h>
>   #include <asm/msr.h>
>   
> @@ -325,6 +326,42 @@ void arch_ioreq_domain_init(struct domain *d)
>       register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
>   }
>   
> +int arch_ioreq_server_create_check(const struct domain *d, uint8_t flags)
> +{
> +    if ( !is_hvm_domain(d) || !d->creation_finished )
> +        return 0;
> +
> +    if ( d->arch.hvm.ext_dest_id &&
> +         !(flags & XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID) )
> +        return -EPERM;
> +
> +    return 0;
> +}
> +
> +static int cf_check ext_dest_id_save(struct vcpu *v, hvm_domain_context_t *h)
> +{
> +    struct hvm_hw_ext_dest_id s = {
> +        .enabled = v->domain->arch.hvm.ext_dest_id,
> +    };
> +
> +    return hvm_save_entry(EXT_DEST_ID, 0, h, &s);
> +}
> +
> +static int cf_check ext_dest_id_load(struct domain *d, hvm_domain_context_t *h)
> +{
> +    struct hvm_hw_ext_dest_id s;
> +
> +    if ( hvm_load_entry(EXT_DEST_ID, h, &s) )
> +        return -EINVAL;
> +
> +    d->arch.hvm.ext_dest_id = s.enabled;
> +
> +    return 0;
> +}
> +
> +HVM_REGISTER_SAVE_RESTORE(EXT_DEST_ID, ext_dest_id_save, NULL,
> +                          ext_dest_id_load, 1, HVMSR_PER_DOM);
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
> index 527cc770b7..7d037a53e1 100644
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -24,6 +24,7 @@
>    *  Ported to xen by using virtual IRQ line.
>    */
>   
> +#include <xen/ioreq.h>
>   #include <xen/types.h>
>   #include <xen/mm.h>
>   #include <xen/xmalloc.h>
> @@ -597,6 +598,7 @@ int vioapic_get_trigger_mode(const struct domain *d, unsigned int gsi)
>   static int cf_check ioapic_check(const struct domain *d, hvm_domain_context_t *h)
>   {
>       const HVM_SAVE_TYPE(IOAPIC) *s;
> +    unsigned int i;
>   
>       if ( !has_vioapic(d) )
>           return -ENODEV;
> @@ -617,6 +619,24 @@ static int cf_check ioapic_check(const struct domain *d, hvm_domain_context_t *h
>       if ( s->ioregsel > VIOAPIC_REG_RTE0 + (ARRAY_SIZE(s->redirtbl) - 1) * 2 + 1 )
>           return -EINVAL;
>   
> +    /*
> +     * If any RTE uses extended destination ID bits, the EXT_DEST_ID save
> +     * record must have been loaded first (restoring d->arch.hvm.ext_dest_id).
> +     * The ioreq server re-registration by the DM happens later, so use the
> +     * domain-level locked flag rather than the per-server dynamic check.
> +     */
> +    for ( i = 0; i < ARRAY_SIZE(s->redirtbl); i++ )
> +    {
> +        if ( s->redirtbl[i].fields.ext_dest_id && !d->arch.hvm.ext_dest_id )
> +        {
> +            printk(XENLOG_G_ERR "HVM restore: %pd IO-APIC RTE %u has "
> +                                "extended destination ID bits set but "
> +                                "EXT_DEST_ID is not enabled\n",
> +                                d, i);
> +            return -EINVAL;
> +        }
> +    }
> +
>       return 0;
>   }
>   
> diff --git a/xen/arch/x86/include/asm/hvm/domain.h b/xen/arch/x86/include/asm/hvm/domain.h
> index abf9bc448d..895b2e12ba 100644
> --- a/xen/arch/x86/include/asm/hvm/domain.h
> +++ b/xen/arch/x86/include/asm/hvm/domain.h
> @@ -102,6 +102,15 @@ struct hvm_domain {
>   
>       bool                   is_s3_suspended;
>   
> +    /*
> +     * True when XEN_HVM_CPUID_EXT_DEST_ID was advertised to the guest. Locked
> +     * at domain creation time once every registered ioreq server has opted in
> +     * via XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID. Persisted in HVM save/restore so
> +     * migration preserves the guest-visible state independently of when the
> +     * device model re-registers its ioreq servers on the destination host.
> +     */
> +    bool                   ext_dest_id;
> +
>       /* Compatibility setting for a bug in x2APIC LDR */
>       bool bug_x2apic_ldr_vcpu_id;
>   
> diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
> index f5fd30ce12..56a7eb8282 100644
> --- a/xen/common/ioreq.c
> +++ b/xen/common/ioreq.c
> @@ -641,7 +641,7 @@ static void ioreq_server_deinit(struct ioreq_server *s)
>   }
>   
>   static int ioreq_server_create(struct domain *d, int bufioreq_handling,
> -                               ioservid_t *id)
> +                               uint8_t flags, ioservid_t *id)
>   {
>       struct ioreq_server *s;
>       unsigned int i;
> @@ -683,6 +683,8 @@ static int ioreq_server_create(struct domain *d, int bufioreq_handling,
>           goto fail;
>       }
>   
> +    s->ext_dest_id = flags & XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID;
> +
>       if ( id )
>           *id = i;
>   
> @@ -1350,11 +1352,16 @@ int ioreq_server_dm_op(struct xen_dm_op *op, struct domain *d, bool *const_op)
>           *const_op = false;
>   
>           rc = -EINVAL;
> -        if ( data->pad[0] || data->pad[1] || data->pad[2] )
> +        if ( data->flags & ~XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID ||
> +             data->pad[0] || data->pad[1] )
> +            break;
> +
> +        rc = arch_ioreq_server_create_check(d, data->flags);
> +        if ( rc )
>               break;
>   
>           rc = ioreq_server_create(d, data->handle_bufioreq,
> -                                 &data->id);
> +                                 data->flags, &data->id);
>           break;
>       }
>   
> diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
> index 6fb4f8b7dc..f7f7c02076 100644
> --- 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>
> @@ -455,6 +456,18 @@ int pt_irq_create_bind(
>           uint64_t msi_addr;
>           uint32_t msi_data;
>   
> +        /*
> +         * Refuse the old MSI bind path when extended destination IDs are
> +         * in use. The caller must use XEN_DMOP_bind_pt_msi_irq instead,
> +         * which passes the raw MSI address so Xen can decode the extended
> +         * bits. This old path only carries an 8-bit destination ID and
> +         * would silently misroute interrupts to vCPUs with APIC IDs > 255.
> +         */
> +        if ( hvm_ext_dest_id_enabled(d) )
> +        {
> +            return -EPERM;
> +        }
> +
>           msi_addr = MSI_ADDR_HEADER |
>                      MASK_INSR(MASK_EXTR(gflags, XEN_DOMCTL_VMSI_X86_DEST_ID_MASK),
>                                MSI_ADDR_DEST_ID_MASK) |
> diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
> index 483097d940..dd70ce18c6 100644
> --- a/xen/include/public/arch-x86/hvm/save.h
> +++ b/xen/include/public/arch-x86/hvm/save.h
> @@ -627,12 +627,27 @@ struct hvm_msr {
>   
>   #define CPU_MSR_CODE  20
>   
> +/*
> + * HVM_SAVE_TYPE(EXT_DEST_ID): domain-level extended MSI destination ID state.
> + *
> + * Records whether the extended destination ID feature was enabled for this
> + * domain at the time guest vCPUs were started. This allows migration to
> + * preserve the setting across hosts without relying on the device model to
> + * re-register its ioreq servers before the guest's first CPUID query.
> + */
> +struct hvm_hw_ext_dest_id {
> +    uint8_t enabled;
> +    uint8_t pad[7];
> +};
> +
> +DECLARE_HVM_SAVE_TYPE(EXT_DEST_ID, 21, struct hvm_hw_ext_dest_id);
> +
>   /* Range 22 - 34 (inclusive) reserved for Amazon */
>   
>   /*
>    * Largest type-code in use
>    */
> -#define HVM_SAVE_CODE_MAX 20
> +#define HVM_SAVE_CODE_MAX 21
>   
>   #endif /* __XEN_PUBLIC_HVM_SAVE_X86_H__ */
>   
> diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
> index 43571b7713..73f33b3c46 100644
> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -39,18 +39,28 @@ typedef uint16_t ioservid_t;
>    * XEN_DMOP_create_ioreq_server: Instantiate a new IOREQ Server for a
>    *                               secondary emulator.
>    *
> - * The <id> handed back is unique for target domain. The valur of
> + * The <id> handed back is unique for target domain. The value of
>    * <handle_bufioreq> should be one of HVM_IOREQSRV_BUFIOREQ_* defined in
> - * hvm_op.h. If the value is HVM_IOREQSRV_BUFIOREQ_OFF then  the buffered
> + * hvm_op.h. If the value is HVM_IOREQSRV_BUFIOREQ_OFF then the buffered
>    * ioreq ring will not be allocated and hence all emulation requests to
>    * this server will be synchronous.
> + *
> + * If <flags> contains XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID, the server will
> + * use XEN_DMOP_bind_pt_msi_irq for all passthrough MSI bindings, passing
> + * raw MSI address/data fields so Xen can decode extended destination ID
> + * bits. Once any server sets this flag, Xen will advertise
> + * XEN_HVM_CPUID_EXT_DEST_ID to the guest. Must be set before the guest
> + * vCPUs are started.
>    */
>   #define XEN_DMOP_create_ioreq_server 1
>   
>   struct xen_dm_op_create_ioreq_server {
>       /* IN - should server handle buffered ioreqs */
>       uint8_t handle_bufioreq;
> -    uint8_t pad[3];
> +    /* IN - server capability flags */
> +    uint8_t flags;
> +#define XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID (1u << 0)
> +    uint8_t pad[2];
>       /* OUT - server id */
>       ioservid_t id;
>   };
> diff --git a/xen/include/xen/ioreq.h b/xen/include/xen/ioreq.h
> index e86f0869fa..ec78b63942 100644
> --- a/xen/include/xen/ioreq.h
> +++ b/xen/include/xen/ioreq.h
> @@ -54,9 +54,35 @@ struct ioreq_server {
>       evtchn_port_t          bufioreq_evtchn;
>       struct rangeset        *range[NR_IO_RANGE_TYPES];
>       bool                   enabled;
> +    bool                   ext_dest_id;
>       uint8_t                bufioreq_handling;
>   };
>   
> +/*
> + * Return true if every registered ioreq server has opted in to extended
> + * destination IDs (XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID) and at least one
> + * server exists. A single server without the flag is enough to suppress
> + * XEN_HVM_CPUID_EXT_DEST_ID, preventing misrouted interrupts.
> + */
> +static inline bool hvm_ext_dest_id_enabled(const struct domain *d)
> +{
> +    unsigned int i;
> +    bool found = false;
> +
> +    for ( i = 0; i < MAX_NR_IOREQ_SERVERS; i++ )
> +    {
> +        const struct ioreq_server *s = d->ioreq_server.server[i];
> +
> +        if ( !s )
> +            continue;
> +        if ( !s->ext_dest_id )
> +            return false;
> +        found = true;
> +    }
> +
> +    return found;
> +}
> +
>   static inline paddr_t ioreq_mmio_first_byte(const ioreq_t *p)
>   {
>       return unlikely(p->df) ?
> @@ -137,6 +163,7 @@ bool arch_ioreq_server_destroy_all(struct domain *d);
>   bool arch_ioreq_server_get_type_addr(const struct domain *d, const ioreq_t *p,
>                                        uint8_t *type, uint64_t *addr);
>   void arch_ioreq_domain_init(struct domain *d);
> +int arch_ioreq_server_create_check(const struct domain *d, uint8_t flags);
>   
>   #endif /* __XEN_IOREQ_H__ */
>   

Teddy


--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v4 8/9] hvm/ioreq: Negotiate extended destination ID support per ioreq server

[Jan Beulich]

On 28.04.2026 18:35, Teddy Astie wrote:
> Le 27/04/2026 à 15:57, Julian Vetter a écrit :
>> Add a per-server capability flag in XEN_DMOP_create_ioreq_server to
>> signal extended destination ID support. Repurpose the first byte of the
>> existing pad[3] as a flags field, and define
>> XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID (bit 0) for a server to signal it will
>> use XEN_DMOP_bind_pt_msi_irq for all passthrough MSI bindings.
>>
>> Track the flag in struct ioreq_server ext_dest_id.
>> hvm_ext_dest_id_enabled() returns true only if all registered ioreq
>> servers have opted in and at least one server is present. A single
>> server without the flag is sufficient to suppress the feature.
>>
>> Lock the feature at domain creation time:
>> arch_domain_creation_finished() computes the levelled result into struct
>> hvm_domain.ext_dest_id using OR to preserve any value previously
>> restored from an HVM save record. After creation_finished,
>> arch_ioreq_server_create_check() rejects new servers that lack
>> XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID if the feature was already advertised
>> to the guest.
>>
>> Persist the locked state in a new HVM_SAVE_TYPE(EXT_DEST_ID) record so
>> that migration preserves the guest-visible CPUID bit independently of
>> when the device model re-registers its ioreq servers on the destination
>> host.
>>
>> On restore, ioapic_check() uses d->arch.hvm.ext_dest_id (restored from
>> the EXT_DEST_ID record) rather than the per-server dynamic check, since
>> the DM has not yet re-registered its servers at that point.
>>
>> Update xendevicemodel_create_ioreq_server() in libxendevicemodel to
>> accept the new flags parameter, remove
>> xendevicemodel_enable_ext_dest_id(), and fix the
>> xc_hvm_create_ioreq_server() compat wrapper to pass zero flags.
>>
>> Signed-off-by: Julian Vetter <julian.vetter@vates.tech>
> 
> That has somewhat already being discussed previously, but AFAIU, 
> extended destination ID is only meaningful when guest APIC IDs cannot be 
> represented with the "non-extended" model which can only happen in 
> practice when having more than 128 vCPUs in the guest.

As Andrew has been pointing out many times, we need to stop thinking in
terms of 128 vCPU-s being the limit because of the vCPU ID times 2
calculation for the APIC IDs. With a non-HT topology, more than 128
vCPU-s would already be possible from an APIC ID perspective. Hence
tying "extended dest ID" to the vCPU count is unlikely to be viable.

Jan

> I don't think we need to check for device model support unless the guest 
> can have more than 128 vCPUs, where in such case it becomes mandatory 
> (unless some form of interrupt remapping is implemented).
> 
> So I would rather check if domain->max_vcpus is more than 128 and 
> require device models to implement support for extended destination ID 
> in these cases.
> 
> In some way, that would imply that extended destination ID is only 
> exposed to guests with domain->max_vcpus > 128.
> 
> Overall, what I propose would be to keep the new 
> XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID flag, and if d->max_vcpus > 128, we 
> require the device model to support XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID.

Re: [PATCH v4 8/9] hvm/ioreq: Negotiate extended destination ID support per ioreq server

[Jan Beulich]

On 27.04.2026 15:54, Julian Vetter wrote:
> ---
> Changes in v4:
> - As suggested by Roger, replaced XEN_DMOP_enable_ext_dest_id (v3 patch
>   6), a separate DM op the device model had to call before starting
>   vCPUs, with a flags byte repurposed from the existing pad[3] field of
>   xen_dm_op_create_ioreq_server

IOW the presence of XEN_DMOP_enable_ext_dest_id in the earlier patch is
entirely stale?

> - New XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID flag (bit 0) lets each ioreq
>   server signal support at registration time
> - As suggested by Roger level the feature across all ioreq servers.
>   XEN_HVM_CPUID_EXT_DEST_ID is only advertised when every server
>   registered before arch_domain_creation_finished() sets the flag. A
>   single server without the flag suppresses the feature for the whole
>   domain!
> - Lock the levelled result at domain creation time and enforce it for
>   servers registered afterwards, preventing a late opt-out from breaking
>   guests that already see the feature in CPUID
> - Persist the locked flag via HVM_SAVE_TYPE(EXT_DEST_ID) so that live
>   migration preserves the guest-visible CPUID bit independently of when
>   the device model registers its ioreq servers on the destination host

So a new save record for a single bit. That doesn't look very efficient
to me.

> @@ -1106,7 +1107,16 @@ int arch_domain_soft_reset(struct domain *d)
>  void arch_domain_creation_finished(struct domain *d)
>  {
>      if ( is_hvm_domain(d) )
> +    {
> +        /*
> +         * Lock the extended destination ID state. OR preserves any value
> +         * already restored from an HVM save record (migration path). For a
> +         * fresh domain, ext_dest_id starts false and the dynamic check
> +         * supplies the levelled result across all registered ioreq servers.
> +         */
> +        d->arch.hvm.ext_dest_id |= hvm_ext_dest_id_enabled(d);

For an unaware guest, after migration it'll suddenly get the flag set
if all servers are capable. That can't be right. It looks pretty much
unavoidable for the field to become tristate (unset / false / true).

>          hvm_domain_creation_finished(d);

Blank line please between what you add and what was already there.

> @@ -325,6 +326,42 @@ void arch_ioreq_domain_init(struct domain *d)
>      register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
>  }
>  
> +int arch_ioreq_server_create_check(const struct domain *d, uint8_t flags)

Bogus use of a fixed-width type again.

> +{
> +    if ( !is_hvm_domain(d) || !d->creation_finished )
> +        return 0;

Why the HVM check? ioreq_server_dm_op(), the sole caller, will only ever
be called for HVM domains (as per the check near the top of dm_op()).

> +    if ( d->arch.hvm.ext_dest_id &&
> +         !(flags & XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID) )
> +        return -EPERM;
> +
> +    return 0;
> +}
> +
> +static int cf_check ext_dest_id_save(struct vcpu *v, hvm_domain_context_t *h)
> +{
> +    struct hvm_hw_ext_dest_id s = {
> +        .enabled = v->domain->arch.hvm.ext_dest_id,
> +    };
> +
> +    return hvm_save_entry(EXT_DEST_ID, 0, h, &s);
> +}
> +
> +static int cf_check ext_dest_id_load(struct domain *d, hvm_domain_context_t *h)
> +{
> +    struct hvm_hw_ext_dest_id s;
> +
> +    if ( hvm_load_entry(EXT_DEST_ID, h, &s) )
> +        return -EINVAL;
> +
> +    d->arch.hvm.ext_dest_id = s.enabled;

Afaict this can load arbitrary values other than 0 or 1. In fact ...

> +    return 0;
> +}
> +
> +HVM_REGISTER_SAVE_RESTORE(EXT_DEST_ID, ext_dest_id_save, NULL,
> +                          ext_dest_id_load, 1, HVMSR_PER_DOM);

... I think there's ext_dest_id_check() missing.

> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -24,6 +24,7 @@
>   *  Ported to xen by using virtual IRQ line.
>   */
>  
> +#include <xen/ioreq.h>
>  #include <xen/types.h>
>  #include <xen/mm.h>
>  #include <xen/xmalloc.h>

Is this hunk stale?

> @@ -597,6 +598,7 @@ int vioapic_get_trigger_mode(const struct domain *d, unsigned int gsi)
>  static int cf_check ioapic_check(const struct domain *d, hvm_domain_context_t *h)
>  {
>      const HVM_SAVE_TYPE(IOAPIC) *s;
> +    unsigned int i;

Better ...

> @@ -617,6 +619,24 @@ static int cf_check ioapic_check(const struct domain *d, hvm_domain_context_t *h
>      if ( s->ioregsel > VIOAPIC_REG_RTE0 + (ARRAY_SIZE(s->redirtbl) - 1) * 2 + 1 )
>          return -EINVAL;
>  
> +    /*
> +     * If any RTE uses extended destination ID bits, the EXT_DEST_ID save
> +     * record must have been loaded first (restoring d->arch.hvm.ext_dest_id).
> +     * The ioreq server re-registration by the DM happens later, so use the
> +     * domain-level locked flag rather than the per-server dynamic check.
> +     */
> +    for ( i = 0; i < ARRAY_SIZE(s->redirtbl); i++ )

...

    for ( unsigned int i = 0; i < ARRAY_SIZE(s->redirtbl); i++ )

> +    {
> +        if ( s->redirtbl[i].fields.ext_dest_id && !d->arch.hvm.ext_dest_id )
> +        {
> +            printk(XENLOG_G_ERR "HVM restore: %pd IO-APIC RTE %u has "
> +                                "extended destination ID bits set but "
> +                                "EXT_DEST_ID is not enabled\n",
> +                                d, i);

No, this is the wrong way round. As long as we permit guests to put
non-zero in these bits, we can't demand the bits to be zero here. You
need to avoid interpreting them as extended-ID when the feature isn't
enabled for a guest.

> --- a/xen/common/ioreq.c
> +++ b/xen/common/ioreq.c
> @@ -641,7 +641,7 @@ static void ioreq_server_deinit(struct ioreq_server *s)
>  }
>  
>  static int ioreq_server_create(struct domain *d, int bufioreq_handling,
> -                               ioservid_t *id)
> +                               uint8_t flags, ioservid_t *id)

Inappropriate use of a fixed-width type again.

> @@ -1350,11 +1352,16 @@ int ioreq_server_dm_op(struct xen_dm_op *op, struct domain *d, bool *const_op)
>          *const_op = false;
>  
>          rc = -EINVAL;
> -        if ( data->pad[0] || data->pad[1] || data->pad[2] )
> +        if ( data->flags & ~XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID ||

Parentheses please around bitwise logic being operands to boolean logic.

> +             data->pad[0] || data->pad[1] )
> +            break;
> +
> +        rc = arch_ioreq_server_create_check(d, data->flags);

It's a little odd to have an arch hook here, yet at the same time an
x86-specific check a few lines up (XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID
really is meaningless on non-x86, and should hence either be constrained
to x86 [with the bit position reusable for something else on other
architectures], or be properly rejected on non-x86).

> @@ -455,6 +456,18 @@ int pt_irq_create_bind(
>          uint64_t msi_addr;
>          uint32_t msi_data;
>  
> +        /*
> +         * Refuse the old MSI bind path when extended destination IDs are
> +         * in use. The caller must use XEN_DMOP_bind_pt_msi_irq instead,
> +         * which passes the raw MSI address so Xen can decode the extended
> +         * bits. This old path only carries an 8-bit destination ID and
> +         * would silently misroute interrupts to vCPUs with APIC IDs > 255.

"to" looks ambiguous to me here. Maybe better "targeted at"? It's also >= 255,
I think.

> +         */
> +        if ( hvm_ext_dest_id_enabled(d) )
> +        {
> +            return -EPERM;
> +        }

No need for curly braces here. Further I think -EPERM isn't a good choice, as
that's what xsm_default_action() returns. -EOPNOTSUPP may be an option, or
some other, more "exotic" indicator.

> --- a/xen/include/public/arch-x86/hvm/save.h
> +++ b/xen/include/public/arch-x86/hvm/save.h
> @@ -627,12 +627,27 @@ struct hvm_msr {
>  
>  #define CPU_MSR_CODE  20
>  
> +/*
> + * HVM_SAVE_TYPE(EXT_DEST_ID): domain-level extended MSI destination ID state.

Why MSI when the vIO-APIC uses it as well?

> + * Records whether the extended destination ID feature was enabled for this
> + * domain at the time guest vCPUs were started. This allows migration to
> + * preserve the setting across hosts without relying on the device model to
> + * re-register its ioreq servers before the guest's first CPUID query.
> + */

The guest's first CPUID query surely is going to happen after at least one DM
has registered a server? (For HVM, that is. No server may ever be registered
for PVH, aiui.) It's not quite clear to me why this connection to CPUID
queries is being made here. The flag is necessary at server registration time,
as ones not supporting the feature need to be rejected.

> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -39,18 +39,28 @@ typedef uint16_t ioservid_t;
>   * XEN_DMOP_create_ioreq_server: Instantiate a new IOREQ Server for a
>   *                               secondary emulator.
>   *
> - * The <id> handed back is unique for target domain. The valur of
> + * The <id> handed back is unique for target domain. The value of
>   * <handle_bufioreq> should be one of HVM_IOREQSRV_BUFIOREQ_* defined in
> - * hvm_op.h. If the value is HVM_IOREQSRV_BUFIOREQ_OFF then  the buffered
> + * hvm_op.h. If the value is HVM_IOREQSRV_BUFIOREQ_OFF then the buffered
>   * ioreq ring will not be allocated and hence all emulation requests to
>   * this server will be synchronous.
> + *
> + * If <flags> contains XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID, the server will
> + * use XEN_DMOP_bind_pt_msi_irq for all passthrough MSI bindings, passing
> + * raw MSI address/data fields so Xen can decode extended destination ID
> + * bits. Once any server sets this flag, Xen will advertise
> + * XEN_HVM_CPUID_EXT_DEST_ID to the guest. Must be set before the guest
> + * vCPUs are started.

I don't understand the last sentence. Is it perhaps stale from how things
were earlier? There's nothing to "set" here.

> --- a/xen/include/xen/ioreq.h
> +++ b/xen/include/xen/ioreq.h
> @@ -54,9 +54,35 @@ struct ioreq_server {
>      evtchn_port_t          bufioreq_evtchn;
>      struct rangeset        *range[NR_IO_RANGE_TYPES];
>      bool                   enabled;
> +    bool                   ext_dest_id;
>      uint8_t                bufioreq_handling;
>  };
>  
> +/*
> + * Return true if every registered ioreq server has opted in to extended
> + * destination IDs (XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID) and at least one
> + * server exists.

Why is one server existing relevant?

> A single server without the flag is enough to suppress
> + * XEN_HVM_CPUID_EXT_DEST_ID, preventing misrouted interrupts.
> + */
> +static inline bool hvm_ext_dest_id_enabled(const struct domain *d)
> +{
> +    unsigned int i;
> +    bool found = false;
> +
> +    for ( i = 0; i < MAX_NR_IOREQ_SERVERS; i++ )

Please use ARRAY_SIZE() in such cases.

> +    {
> +        const struct ioreq_server *s = d->ioreq_server.server[i];
> +
> +        if ( !s )
> +            continue;
> +        if ( !s->ext_dest_id )

As there's no locking here, and as the comment ahead of the function also
doesn't mention any locking requirements: What guarantees s to still be
valid to deref here? Furthermore, what guarantees the result of this
function to not be stale by the time the caller looks at it? (Some of
this may be easier if this wasn't an inline function in a globally
visible header.)

Jan

[PATCH v4 9/9] x86/cpuid: Advertise XEN_HVM_CPUID_EXT_DEST_ID when device model opts in

[Julian Vetter]

[-- Attachment #1: Type: text/plain, Size: 3229 bytes --]

Set the XEN_HVM_CPUID_EXT_DEST_ID bit in the HVM hypervisor CPUID leaf
based on the domain-level ext_dest_id flag, which is locked at domain
creation time by taking the AND across all registered ioreq servers that
set XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID. This guarantees that the bit is
only advertised when every active device model will use
XEN_DMOP_bind_pt_msi_irq for passthrough MSIs, so Xen can decode the
extended destination bits from the raw MSI address internally.

After creation_finished the locked d->arch.hvm.ext_dest_id is used
directly, providing a stable and migration-safe value independent of
whether ioreq servers have been re-registered yet. Before
creation_finished the dynamic per-server check is used so toolstack
queries during domain setup reflect the current state.

An old device model that never sets XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID
will keep the bit clear, preserving backwards compatibility with guests
that only use 8-bit APIC destination IDs. A device model that opts in
enables support for x2APIC destination IDs above 255.

Signed-off-by: Julian Vetter <julian.vetter@vates.tech>
---
Changes in v4:
- Corresponds to v3 patch 7, updated for the new negotiation mechanism
  introduced in patch 8. The bit now reflects the levelled result
  computed at arch_domain_creation_finished()
    - Before creation_finished(): the dynamic hvm_ext_dest_id_enabled()
      check is used (so toolstack CPUID queries during domain setup see
      the current state)
    - After creation_finished(): the locked d->arch.hvm.ext_dest_id is used
      directly for migration-safe value that does not depend
      on whether ioreq servers have been re-registered on the
      destination host yet
---
 xen/arch/x86/cpuid.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 6e9b15c9c3..828aaa9f5b 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -1,3 +1,4 @@
+#include <xen/ioreq.h>
 #include <xen/sched.h>
 #include <xen/types.h>
 #include <xen/version.h>
@@ -148,6 +149,18 @@ static void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
         res->a |= XEN_HVM_CPUID_DOMID_PRESENT;
         res->c = d->domain_id;
 
+        /*
+         * Advertise extended destination ID support. Once domain creation has
+         * finished the locked domain flag is authoritative (ensuring a
+         * consistent view across migration, before the device model
+         * re-registers its ioreq servers on the destination). Before
+         * creation_finished the dynamic per-server check is used so that
+         * toolstack queries during domain setup reflect the current state.
+         */
+        if ( d->creation_finished ? d->arch.hvm.ext_dest_id
+                                  : hvm_ext_dest_id_enabled(d) )
+            res->a |= XEN_HVM_CPUID_EXT_DEST_ID;
+
         /*
          * Per-vCPU event channel upcalls are implemented and work
          * correctly with PIRQs routed over event channels.
-- 
2.53.0



--
Julian Vetter | Vates Hypervisor & Kernel Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v4 9/9] x86/cpuid: Advertise XEN_HVM_CPUID_EXT_DEST_ID when device model opts in

[Jan Beulich]

On 27.04.2026 15:54, Julian Vetter wrote:
> Set the XEN_HVM_CPUID_EXT_DEST_ID bit in the HVM hypervisor CPUID leaf
> based on the domain-level ext_dest_id flag, which is locked at domain
> creation time by taking the AND across all registered ioreq servers that
> set XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID. This guarantees that the bit is
> only advertised when every active device model will use
> XEN_DMOP_bind_pt_msi_irq for passthrough MSIs, so Xen can decode the
> extended destination bits from the raw MSI address internally.
> 
> After creation_finished the locked d->arch.hvm.ext_dest_id is used
> directly, providing a stable and migration-safe value independent of
> whether ioreq servers have been re-registered yet. Before
> creation_finished the dynamic per-server check is used so toolstack
> queries during domain setup reflect the current state.

What toolstack queries to you have in mind here? Did you check ...

> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -1,3 +1,4 @@
> +#include <xen/ioreq.h>
>  #include <xen/sched.h>
>  #include <xen/types.h>
>  #include <xen/version.h>
> @@ -148,6 +149,18 @@ static void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,

... call sites of this function? It's solely guest_cpuid(), and all
callers of the latter act on current. I.e. if there was a need for a
toolstack to find out (transient) state, it would need to be through
a different interface anyway.

Jan

Re: [PATCH v4 0/9] x86/hvm: Add Extended MSI destination ID support

[Julian Vetter]

[-- Attachment #1: Type: text/plain, Size: 4674 bytes --]

Thank you Teddy for your feedback on the patchset. Jan and Roger do you 
have any additional remarks on the patchset? Is the new structure what 
you had in mind Roger?

Thank you.

Julian

On 4/27/26 15:54, Julian Vetter wrote:
> Thank you very much Roger and Jan for your feedback! I have addressed
> your feedback, see below. For me there is only one question, I'm not
> sure if this is what you had in mind Roger when you wrote:
> "Retrofitting the new interface into the old one seems weird.  I would
> do it the other way around - implement the old bind domctl on top of
> an interface that's more suited for the new DM op."
> It requires some preparatory work. But I tried to make the
> restructunring diffs as easy to review as possible by splitting the
> refactoring into mutliple steps.
> 
> Signed-off-by: Julian Vetter <julian.vetter@vates.tech>
> ---
> Changes in v4:
> - Replaced the v3 approach of extending the XEN_DOMCTL_bind_pt_irq
>    gflags ABI (XEN_DOMCTL_VMSI_X86_EXT_DEST_ID_MASK / VMSI_X86_FULL_DEST)
>    with raw MSI addr + data storage throughout. pt_irq_bind_msi() now
>    accepts the raw address + data values and decodes the destination
>    internally. No public domctl ABI changes
> - As suggested by Roger, replaced XEN_DMOP_enable_ext_dest_id (v3 patch
>    6, a separate call before domain start) with a flags byte from the
>    existing pad[3] field of XEN_DMOP_create_ioreq_server. The new
>    XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID flag lets each server signal support
>    at registration time and allows the feature to be levelled across all
>    servers before arch_domain_creation_finished()
> - Redirect xc_domain_{update,unbind}_msi_irq() in libxenctrl to use the
>    new DM ops, and reject PT_IRQ_TYPE_MSI in
>    XEN_DOMCTL_{bind,unbind}_pt_irq (as suggested by Jan)
> - Add three preparatory no-functional-change commits (patches 2-4) that
>    wrap the restart block in braces, extract pt_irq_dpci_setup(), and
>    extract the PT_IRQ_TYPE_MSI case body into pt_irq_bind_msi(), making
>    the interface change in patch 5 reviewable as a clean diff
> - Rework ioapic_check() to validate named fields (base_address, APIC ID
>    width, ioregsel range) instead of rejecting any non-zero reserved
>    bits, which would have falsely rejected RTEs carrying extended
>    destination IDs
> ---
> Julian Vetter (9):
>    x86/vioapic: Add ioapic_check() to validate IO-APIC state before
>      restore
>    x86/passthrough: Wrap pt_irq_create_bind() restart block in braces
>    x86/passthrough: Extract pt_irq_dpci_setup() from pt_irq_create_bind()
>    x86/passthrough: Extract PT_IRQ_TYPE_MSI body into pt_irq_bind_msi()
>    x86/passthrough: Introduce pt_irq_bind_msi() as canonical MSI bind
>      path
>    x86/hvm: Support extended destination IDs in virtual MSI and IO-APIC
>    x86/dmop: Add XEN_DMOP_{bind,unbind}_pt_msi_irq DM ops
>    hvm/ioreq: Negotiate extended destination ID support per ioreq server
>    x86/cpuid: Advertise XEN_HVM_CPUID_EXT_DEST_ID when device model opts
>      in
> 
>   tools/include/xendevicemodel.h          |  34 +-
>   tools/libs/ctrl/xc_devicemodel_compat.c |   2 +-
>   tools/libs/ctrl/xc_domain.c             |  52 ++--
>   tools/libs/devicemodel/core.c           |  41 ++-
>   xen/arch/arm/ioreq.c                    |   5 +
>   xen/arch/x86/cpuid.c                    |  13 +
>   xen/arch/x86/domain.c                   |  10 +
>   xen/arch/x86/domctl.c                   |  10 +-
>   xen/arch/x86/hvm/dm.c                   |  68 ++++
>   xen/arch/x86/hvm/ioreq.c                |  37 +++
>   xen/arch/x86/hvm/irq.c                  |   9 +-
>   xen/arch/x86/hvm/vioapic.c              |  50 ++-
>   xen/arch/x86/hvm/vmsi.c                 |  54 +---
>   xen/arch/x86/include/asm/hvm/domain.h   |   9 +
>   xen/arch/x86/include/asm/hvm/hvm.h      |   4 +-
>   xen/arch/x86/include/asm/hvm/irq.h      |   4 +-
>   xen/arch/x86/include/asm/hvm/vioapic.h  |  12 +
>   xen/arch/x86/include/asm/msi.h          |  18 +-
>   xen/common/ioreq.c                      |  13 +-
>   xen/drivers/passthrough/x86/hvm.c       | 396 ++++++++++++++----------
>   xen/include/public/arch-x86/hvm/save.h  |  21 +-
>   xen/include/public/hvm/dm_op.h          |  53 +++-
>   xen/include/xen/iommu.h                 |   3 +
>   xen/include/xen/ioreq.h                 |  27 ++
>   xen/include/xlat.lst                    |   2 +
>   25 files changed, 703 insertions(+), 244 deletions(-)
> 



--
Julian Vetter | Vates Hypervisor & Kernel Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v4 0/9] x86/hvm: Add Extended MSI destination ID support

[Jan Beulich]

On 02.06.2026 14:08, Julian Vetter wrote:
> Thank you Teddy for your feedback on the patchset. Jan and Roger do you 
> have any additional remarks on the patchset?

I'm pretty sure I will have, once I find time to actually look at the v4
patches. Right now 4.22 is the priority anyway.

Jan

> Is the new structure what you had in mind Roger?
> 
> Thank you.
> 
> Julian