Re: [PATCH v3 2/7] x86/msi: Define extended destination ID masks and IO-APIC RTE fields

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.03.2026 13:31, Julian Vetter wrote:
> --- a/xen/arch/x86/include/asm/hvm/vioapic.h
> +++ b/xen/arch/x86/include/asm/hvm/vioapic.h
> @@ -32,6 +32,19 @@
>  #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_ID_UPPER_BITS  8

The name suggests this is the number of upper bits, which it isn't. You 
shouldn't
need this constant anyway, see below.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -615,12 +615,14 @@ struct xen_domctl_bind_pt_irq {
>          struct {
>              uint8_t gvec;
>              uint32_t gflags;
> -#define XEN_DOMCTL_VMSI_X86_DEST_ID_MASK 0x0000ff
> -#define XEN_DOMCTL_VMSI_X86_RH_MASK      0x000100
> -#define XEN_DOMCTL_VMSI_X86_DM_MASK      0x000200
> -#define XEN_DOMCTL_VMSI_X86_DELIV_MASK   0x007000
> -#define XEN_DOMCTL_VMSI_X86_TRIG_MASK    0x008000
> -#define XEN_DOMCTL_VMSI_X86_UNMASKED     0x010000
> +#define XEN_DOMCTL_VMSI_X86_DEST_ID_MASK        0x0000ff
> +#define XEN_DOMCTL_VMSI_X86_DEST_ID_BITS        8

This constant is redundant with _MASK. It is generally helpful to avoid
such redundancies (especially in the public interface), and derive the wanted
value from the main (most generally usable) constant. IOW here maybe

#define XEN_DOMCTL_VMSI_X86_DEST_ID_BITS        8
#define XEN_DOMCTL_VMSI_X86_DEST_ID_MASK        ((1U << 
XEN_DOMCTL_VMSI_X86_DEST_ID_BITS) - 1)

But of course it could also be done the other way around, albeit in a public
header this may end up more difficult. (Generally the mask wants to be the
main definition, as everything else can be derived from them.)

> @@ -630,6 +632,10 @@ struct xen_domctl_bind_pt_irq {
>      } u;
>  };
>  
> +#define XEN_DOMCTL_VMSI_X86_FULL_DEST(gflags) \
> +        (MASK_EXTR((gflags), XEN_DOMCTL_VMSI_X86_DEST_ID_MASK) | \
> +        (MASK_EXTR((gflags), XEN_DOMCTL_VMSI_X86_EXT_DEST_ID_MASK) << \
> +         XEN_DOMCTL_VMSI_X86_DEST_ID_BITS))

There's no MASK_EXTR() in the public interface.

Also, nit: No need to parenthesize gflags when used in macro invocations like
this one. Iirc there was a similar pattern elsewhere in the patch.

Jan