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

[Jan Beulich <jbeulich@xxxxxxxx>]

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