On 20.01.2022 16:23, Roger Pau Monne wrote:
> Both QEMU/KVM and HyperV support using bits 11:5 from the MSI address
> field in order to store the high part of the target APIC ID. This
> allows expanding the maximum APID ID usable without interrupt
> remapping support from 255 to 32768.
>
> Note the interface used by QEMU for emulated devices (via the
> XEN_DMOP_inject_msi hypercall) already passes both the address and
> data fields into Xen for processing, so there's no need for any change
> to QEMU there.
>
> However for PCI passthrough devices QEMU uses the
> XEN_DOMCTL_bind_pt_irq hypercall which does need an addition to the
> gflags field in order to pass the high bits of the APIC destination
> ID.
>
> Introduce a new CPUID flag to signal the support for the feature. The
> introduced flag covers both the support for extended ID for the
> IO-APIC RTE and the MSI address registers. Such flag is currently only
> exposed when the domain is using vPCI (ie: a PVH dom0).
Because of also covering the IO-APIC side, I think the CPUID aspect of
this really wants splitting into a 3rd patch. That way the MSI and
IO-APIC parts could in principle go in independently, and only the
CPUID one needs to remain at the tail.
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -66,7 +66,7 @@ static void vmsi_inj_irq(
>
> int vmsi_deliver(
> struct domain *d, int vector,
> - uint8_t dest, uint8_t dest_mode,
> + unsigned int dest, unsigned int dest_mode,
If you change the type of dest_mode, then to "bool" please - see its
only call site.
> @@ -123,7 +125,8 @@ 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, unsigned int dest,
> + unsigned int dest_mode)
Same here then.
> --- a/xen/arch/x86/include/asm/msi.h
> +++ b/xen/arch/x86/include/asm/msi.h
> @@ -54,6 +54,7 @@
> #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_EXT_DEST_ID_MASK 0x0000fe0
Especially the immediately preceding macro now becomes kind of stale.
> --- a/xen/drivers/passthrough/x86/hvm.c
> +++ b/xen/drivers/passthrough/x86/hvm.c
> @@ -269,7 +269,7 @@ int pt_irq_create_bind(
> {
> case PT_IRQ_TYPE_MSI:
> {
> - uint8_t dest, delivery_mode;
> + unsigned int dest, delivery_mode;
> bool dest_mode;
If you touch delivery_mode's type, wouldn't that better become bool?
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -588,6 +588,7 @@ struct xen_domctl_bind_pt_irq {
> #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_EXT_DEST_ID_MASK 0xfe0000
I'm not convinced it is a good idea to limit the overall destination
ID width to 15 bits here - at the interface level we could as well
permit more bits right away; the implementation would reject too high
a value, of course. Not only with this I further wonder whether the
field shouldn't be unsplit while extending it. You won't get away
without bumping XEN_DOMCTL_INTERFACE_VERSION anyway (unless it was
bumped already for 4.17) since afaics the unused bits of this field
previously weren't checked for being zero. We could easily have 8
bits vector, 16 bits flags, and 32 bits destination ID in the struct.
And there would then still be 8 unused bits (which from now on we
ought to check for being zero).
Jan
