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

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