[PATCH v2 0/5] x86: extended destination ID support

[PATCH v2 0/5] x86: extended destination ID support

[Roger Pau Monne]

Hello,

The following series provide a tentative implementation of extended
destination ID support for HVM/PVH guests. A specification for the
feature can be found at:

http://david.woodhou.se/15-bit-msi.pdf

Patch 4 is the one I'm having more doubts about: it's the best thing I
could come up in order for emulators to signal Xen whether they support
parsing the extended destination ID in MSI message address field. This
is only required for device models that support PCI passthrough:
injection of MSI interrupts from emulated devices is done using
XEN_DMOP_inject_msi which already passes the MSI address and data fields
to Xen for processing.

I think we should likely consider patch 1, as it would allow the OS side
of this to make progress (since it's an already present feature in other
hypervisors) independently of the Xen side work.

Thanks, Roger.

Roger Pau Monne (5):
  x86/cpuid: add CPUID flag for Extended Destination ID support
  xen/vioapic: add support for the extended destination ID field
  x86/vmsi: add support for extended destination ID in address field
  x86/ioreq: report extended destination ID support by emulators
  x86/cpuid: expose EXT_DEST_ID feature if supported

 docs/man/xl.cfg.5.pod.in               | 10 ++++++
 tools/include/libxl.h                  |  8 +++++
 tools/libs/light/libxl_create.c        |  6 ++++
 tools/libs/light/libxl_types.idl       |  1 +
 tools/libs/light/libxl_x86.c           | 12 +++++++
 tools/xl/xl_parse.c                    |  3 ++
 xen/arch/arm/ioreq.c                   |  5 +++
 xen/arch/x86/domain.c                  | 10 +++++-
 xen/arch/x86/hvm/ioreq.c               |  7 +++++
 xen/arch/x86/hvm/irq.c                 |  5 ++-
 xen/arch/x86/hvm/vioapic.c             |  3 ++
 xen/arch/x86/hvm/vmsi.c                | 43 +++++++++++++++++++-------
 xen/arch/x86/include/asm/domain.h      |  3 ++
 xen/arch/x86/include/asm/hvm/hvm.h     |  5 +--
 xen/arch/x86/include/asm/msi.h         |  7 +++++
 xen/arch/x86/setup.c                   |  1 +
 xen/arch/x86/traps.c                   |  3 ++
 xen/common/ioreq.c                     |  8 +++--
 xen/drivers/passthrough/x86/hvm.c      | 11 ++++++-
 xen/drivers/vpci/msi.c                 |  2 +-
 xen/include/public/arch-x86/cpuid.h    |  6 ++++
 xen/include/public/arch-x86/hvm/save.h |  4 ++-
 xen/include/public/arch-x86/xen.h      |  2 ++
 xen/include/public/domctl.h            |  3 +-
 xen/include/public/hvm/dm_op.h         |  6 +++-
 xen/include/xen/ioreq.h                |  2 ++
 xen/include/xen/vpci.h                 |  2 +-
 27 files changed, 153 insertions(+), 25 deletions(-)

-- 
2.34.1

[PATCH v2 1/5] x86/cpuid: add CPUID flag for Extended Destination ID support

[Roger Pau Monne]

Introduce the CPUID flag to be used in order to signal the support for
using an extended destination ID in IO-APIC RTEs and MSI address
fields. Such format expands the maximum target APIC ID from 255 to
32768 without requiring the usage of interrupt remapping.

The design document describing the feature can be found at:

http://david.woodhou.se/15-bit-msi.pdf

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - New in this version.
---
 xen/include/public/arch-x86/cpuid.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/include/public/arch-x86/cpuid.h b/xen/include/public/arch-x86/cpuid.h
index ce46305bee..49bcc93b6b 100644
--- a/xen/include/public/arch-x86/cpuid.h
+++ b/xen/include/public/arch-x86/cpuid.h
@@ -102,6 +102,12 @@
 #define XEN_HVM_CPUID_IOMMU_MAPPINGS   (1u << 2)
 #define XEN_HVM_CPUID_VCPU_ID_PRESENT  (1u << 3) /* vcpu id is present in EBX */
 #define XEN_HVM_CPUID_DOMID_PRESENT    (1u << 4) /* domid is present in ECX */
+/*
+ * Bits 55:49 from the IO-APIC RTE and bits 11:5 from the MSI address can be
+ * used to store high bits for the Destination ID. This expands the Destination
+ * ID field from 8 to 15 bits, allowing to target APIC IDs up 32768.
+ */
+#define XEN_HVM_CPUID_EXT_DEST_ID      (1u << 5)
 
 /*
  * Leaf 6 (0x40000x05)
-- 
2.34.1

[PATCH v2 2/5] xen/vioapic: add support for the extended destination ID field

[Roger Pau Monne]

Such field uses bits 55:48, but for the purposes the register will be
used use bits 55:49 instead. Bit 48 is used to signal an RTE entry is
in remappable format which is not supported by the vIO-APIC.

Use the extended destination ID to store the high bits from the
destination ID, thus expanding the size of the destination ID field to
15 bits, allowing an IO-APIC to target APIC IDs up to 32768. A
description of the feature can be found at:

http://david.woodhou.se/15-bit-msi.pdf

Note this is already supported by QEMU/KVM and HyperV.

Introduce an option in xl.cfg to allow switching off the feature.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Expand commit message, add reference document.
 - Add xl option to disable the feature.
---
 docs/man/xl.cfg.5.pod.in               | 10 ++++++++++
 tools/include/libxl.h                  |  8 ++++++++
 tools/libs/light/libxl_create.c        |  6 ++++++
 tools/libs/light/libxl_types.idl       |  1 +
 tools/libs/light/libxl_x86.c           | 12 ++++++++++++
 tools/xl/xl_parse.c                    |  3 +++
 xen/arch/x86/domain.c                  | 10 +++++++++-
 xen/arch/x86/hvm/vioapic.c             |  3 +++
 xen/arch/x86/include/asm/domain.h      |  3 +++
 xen/arch/x86/setup.c                   |  1 +
 xen/include/public/arch-x86/hvm/save.h |  4 +++-
 xen/include/public/arch-x86/xen.h      |  2 ++
 xen/include/public/domctl.h            |  2 +-
 13 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index b98d161398..349e0b432a 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2901,6 +2901,16 @@ option.
 
 If using this option is necessary to fix an issue, please report a bug.
 
+=item B<ext_dest_id=BOOLEAN>
+
+(HVM/PVH only) Signal the hypervisor whether the guest should be allowed to use
+extended destination ID for interrupt messages. Such feature allow expanding
+the target APIC ID from 8 to 15bits without requiring the usage of an emulated
+IOMMU with interrupt remapping. Note this requires guest support and might not
+be exposed to the guest even if explicitly set due to other constrains.
+
+Default: enabled
+
 =back
 
 =head1 SEE ALSO
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index 51a9b6cfac..35329bca50 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -527,6 +527,14 @@
  */
 #define LIBXL_HAVE_MAX_GRANT_VERSION 1
 
+/*
+ * LIBXL_HAVE_X86_EXT_DEST_ID indicates the toolstack can signal to the
+ * hypervisor whether the domain wants to use the extended destination ID mode
+ * for interrupt messages. This is done by setting the libxl_domain_build_info
+ * arch_x86.ext_dest_id field.
+ */
+#define LIBXL_HAVE_X86_EST_DEST_ID 1
+
 /*
  * libxl ABI compatibility
  *
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 7499922088..66ecfbdf4d 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -2330,6 +2330,12 @@ int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config,
      */
     libxl_defbool_setdefault(&d_config->b_info.arch_x86.msr_relaxed, true);
 
+    /*
+     * Do not enable the extended destination ID for restored domains unless
+     * explicitly set.
+     */
+    libxl_defbool_setdefault(&d_config->b_info.arch_x86.ext_dest_id, false);
+
     return do_domain_create(ctx, d_config, domid, restore_fd, send_back_fd,
                             params, ao_how, aop_console_how);
 }
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 2a42da2f7d..bbfe218c48 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -648,6 +648,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                ("vuart", libxl_vuart_type),
                               ])),
     ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
+                               ("ext_dest_id", libxl_defbool),
                               ])),
     # Alternate p2m is not bound to any architecture or guest type, as it is
     # supported by x86 HVM and ARM support is planned.
diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
index 1feadebb18..68f4de7ea9 100644
--- a/tools/libs/light/libxl_x86.c
+++ b/tools/libs/light/libxl_x86.c
@@ -14,6 +14,11 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
         break;
     case LIBXL_DOMAIN_TYPE_PV:
         config->arch.emulation_flags = 0;
+        if (libxl_defbool_val(d_config->b_info.arch_x86.ext_dest_id))
+        {
+            LOG(ERROR, "Extended Destination ID not supported for PV guests");
+            return ERROR_INVAL;
+        }
         break;
     default:
         abort();
@@ -22,6 +27,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
     config->arch.misc_flags = 0;
     if (libxl_defbool_val(d_config->b_info.arch_x86.msr_relaxed))
         config->arch.misc_flags |= XEN_X86_MSR_RELAXED;
+    if (libxl_defbool_val(d_config->b_info.arch_x86.ext_dest_id))
+        config->arch.misc_flags |= XEN_X86_EXT_DEST_ID;
 
     return 0;
 }
@@ -824,6 +831,8 @@ void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
 {
     libxl_defbool_setdefault(&b_info->acpi, true);
     libxl_defbool_setdefault(&b_info->arch_x86.msr_relaxed, false);
+    libxl_defbool_setdefault(&b_info->arch_x86.ext_dest_id,
+                             b_info->type != LIBXL_DOMAIN_TYPE_PV);
 }
 
 int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
@@ -880,6 +889,9 @@ void libxl__arch_update_domain_config(libxl__gc *gc,
      */
     libxl_defbool_setdefault(&dst->b_info.arch_x86.msr_relaxed,
                     libxl_defbool_val(src->b_info.arch_x86.msr_relaxed));
+    /* Force Extended Destination ID so it's part of the migration data. */
+    libxl_defbool_setdefault(&dst->b_info.arch_x86.ext_dest_id,
+                    libxl_defbool_val(src->b_info.arch_x86.ext_dest_id));
 }
 
 /*
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 117fcdcb2b..b609b76779 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2761,6 +2761,9 @@ skip_usbdev:
 
     xlu_cfg_get_defbool(config, "vpmu", &b_info->vpmu, 0);
 
+    xlu_cfg_get_defbool(config, "ext_dest_id", &b_info->arch_x86.ext_dest_id,
+                        0);
+
     xlu_cfg_destroy(config);
 }
 
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index ef1812dc14..9764f42878 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -685,12 +685,19 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
         }
     }
 
-    if ( config->arch.misc_flags & ~XEN_X86_MSR_RELAXED )
+    if ( config->arch.misc_flags &
+         ~(XEN_X86_MSR_RELAXED | XEN_X86_EXT_DEST_ID) )
     {
         dprintk(XENLOG_INFO, "Invalid arch misc flags %#x\n",
                 config->arch.misc_flags);
         return -EINVAL;
     }
+    if ( !hvm && (config->arch.misc_flags & XEN_X86_EXT_DEST_ID) )
+    {
+        dprintk(XENLOG_INFO,
+                "Extended Destination ID only supported for HVM\n");
+        return -EINVAL;
+    }
 
     return 0;
 }
@@ -862,6 +869,7 @@ int arch_domain_create(struct domain *d,
     domain_cpu_policy_changed(d);
 
     d->arch.msr_relaxed = config->arch.misc_flags & XEN_X86_MSR_RELAXED;
+    d->arch.ext_dest_id = config->arch.misc_flags & XEN_X86_EXT_DEST_ID;
 
     return 0;
 
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 553c0f76ef..a02bd16b4b 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -424,6 +424,9 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
 
     ASSERT(spin_is_locked(&d->arch.hvm.irq_lock));
 
+    if ( d->arch.ext_dest_id )
+        dest |= vioapic->redirtbl[pin].fields.virt_ext_dest_id << 8;
+
     HVM_DBG_LOG(DBG_LEVEL_IOAPIC,
                 "dest=%x dest_mode=%x delivery_mode=%x "
                 "vector=%x trig_mode=%x",
diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
index e62e109598..83a1608212 100644
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -440,6 +440,9 @@ struct arch_domain
     /* Don't unconditionally inject #GP for unhandled MSRs. */
     bool msr_relaxed;
 
+    /* Use the Extended Destination ID to expand APIC ID. */
+    bool ext_dest_id;
+
     /* Emulated devices enabled bitmap. */
     uint32_t emulation_flags;
 } __cacheline_aligned;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 115f8f6517..4aaa6ebc07 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -782,6 +782,7 @@ static struct domain *__init create_dom0(const module_t *image,
 
         dom0_cfg.arch.emulation_flags |=
             XEN_X86_EMU_LAPIC | XEN_X86_EMU_IOAPIC | XEN_X86_EMU_VPCI;
+        dom0_cfg.arch.misc_flags |= XEN_X86_EXT_DEST_ID;
     }
 
     if ( iommu_enabled )
diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
index 773a380bc2..253dc89a04 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -376,7 +376,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 :1;
+        uint8_t virt_ext_dest_id:7;
         uint8_t dest_id;
     } fields;
 };
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index 7acd94c8eb..06d64a309f 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -317,6 +317,8 @@ struct xen_arch_domainconfig {
  * doesn't allow the guest to read or write to the underlying MSR.
  */
 #define XEN_X86_MSR_RELAXED (1u << 0)
+/* Select whether to use Extended Destination ID for interrupt messages. */
+#define XEN_X86_EXT_DEST_ID (1u << 1)
     uint32_t misc_flags;
 };
 
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index b85e6170b0..31ec083cb0 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -38,7 +38,7 @@
 #include "hvm/save.h"
 #include "memory.h"
 
-#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014
+#define XEN_DOMCTL_INTERFACE_VERSION 0x00000015
 
 /*
  * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
-- 
2.34.1

[PATCH v2 3/5] x86/vmsi: add support for extended destination ID in address field

[Roger Pau Monne]

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 APIC 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 a modification to the
gflags field in order to pass an APIC destination ID greater than
255.

Take the opportunity to make the domain parameter of
hvm_girq_dest_2_vcpu_id const while modifying the other function
parameters. Also adjust dest_mode when touching related code to make
it bool.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Do not expose extended destination ID support.
 - Use d->arch.ext_dest_id.
 - Add comment clarifying the usage of MSI_ADDR_VIRT_EXT_DEST_ID_MASK.
---
 xen/arch/x86/hvm/irq.c             |  5 +++-
 xen/arch/x86/hvm/vmsi.c            | 43 +++++++++++++++++++++---------
 xen/arch/x86/include/asm/hvm/hvm.h |  5 ++--
 xen/arch/x86/include/asm/msi.h     |  7 +++++
 xen/drivers/passthrough/x86/hvm.c  | 11 +++++++-
 xen/drivers/vpci/msi.c             |  2 +-
 xen/include/public/domctl.h        |  1 +
 xen/include/xen/vpci.h             |  2 +-
 8 files changed, 58 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index 52aae4565f..e10e085a55 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -383,7 +383,7 @@ 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;
+    unsigned int dest = MASK_EXTR(tmp, MSI_ADDR_DEST_ID_MASK);
     uint8_t  dest_mode = !!(tmp & MSI_ADDR_DESTMODE_MASK);
     uint8_t  delivery_mode = (data & MSI_DATA_DELIVERY_MODE_MASK)
         >> MSI_DATA_DELIVERY_MODE_SHIFT;
@@ -391,6 +391,9 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
         >> MSI_DATA_TRIGGER_SHIFT;
     uint8_t vector = data & MSI_DATA_VECTOR_MASK;
 
+    if ( vector && d->arch.ext_dest_id )
+        dest |= MASK_EXTR(tmp, MSI_ADDR_VIRT_EXT_DEST_ID_MASK) << 8;
+
     if ( !vector )
     {
         int pirq = ((addr >> 32) & 0xffffff00) | dest;
diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 13e2a190b4..4af550cc2a 100644
--- 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, bool dest_mode,
     uint8_t delivery_mode, uint8_t trig_mode)
 {
     struct vlapic *target;
@@ -107,11 +107,14 @@ 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;
+    unsigned int dest = MASK_EXTR(flags, XEN_DOMCTL_VMSI_X86_DEST_ID_MASK);
     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;
 
+    if ( d->arch.ext_dest_id )
+        dest |= MASK_EXTR(flags, XEN_DOMCTL_VMSI_X86_EXT_DEST_ID_MASK);
+
     HVM_DBG_LOG(DBG_LEVEL_IOAPIC,
                 "msi: dest=%x dest_mode=%x delivery_mode=%x "
                 "vector=%x trig_mode=%x\n",
@@ -123,7 +126,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(const struct domain *d, unsigned int dest,
+                            bool dest_mode)
 {
     int dest_vcpu_id = -1, w = 0;
     struct vcpu *v;
@@ -636,15 +640,21 @@ void msix_write_completion(struct vcpu *v)
 }
 
 #ifdef CONFIG_HAS_VPCI
-static unsigned int msi_gflags(uint16_t data, uint64_t addr, bool masked)
+static unsigned int msi_gflags(uint16_t data, uint64_t addr, bool masked,
+                               bool ext_dest_id)
 {
+    unsigned int dest = MASK_EXTR(addr, MSI_ADDR_DEST_ID_MASK);
+
+    if ( ext_dest_id )
+        dest |= MASK_EXTR(addr, MSI_ADDR_VIRT_EXT_DEST_ID_MASK) << 8;
+
     /*
      * 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) |
+    return MASK_INSR(dest, XEN_DOMCTL_VMSI_X86_DEST_ID_MASK) |
+           MASK_INSR(dest, XEN_DOMCTL_VMSI_X86_EXT_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),
@@ -698,7 +708,8 @@ static int vpci_msi_update(const struct pci_dev *pdev, uint32_t data,
             .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),
+            .u.msi.gflags = msi_gflags(data, address, (mask >> i) & 1,
+                                       pdev->domain->arch.ext_dest_id),
         };
         int rc = pt_irq_create_bind(pdev->domain, &bind);
 
@@ -826,8 +837,13 @@ void vpci_msi_arch_init(struct vpci_msi *msi)
     msi->arch.pirq = INVALID_PIRQ;
 }
 
-void vpci_msi_arch_print(const struct vpci_msi *msi)
+void vpci_msi_arch_print(const struct vpci_msi *msi, const struct domain *d)
 {
+    unsigned long dest = MASK_EXTR(msi->address, MSI_ADDR_DEST_ID_MASK);
+
+    if ( d->arch.ext_dest_id )
+        dest |= MASK_EXTR(msi->address, MSI_ADDR_VIRT_EXT_DEST_ID_MASK) << 8;
+
     printk("vec=%#02x%7s%6s%3sassert%5s%7s dest_id=%lu pirq: %d\n",
            MASK_EXTR(msi->data, MSI_DATA_VECTOR_MASK),
            msi->data & MSI_DATA_DELIVERY_LOWPRI ? "lowest" : "fixed",
@@ -835,8 +851,7 @@ void vpci_msi_arch_print(const struct vpci_msi *msi)
            msi->data & MSI_DATA_LEVEL_ASSERT ? "" : "de",
            msi->address & MSI_ADDR_DESTMODE_LOGIC ? "log" : "phys",
            msi->address & MSI_ADDR_REDIRECTION_LOWPRI ? "lowest" : "fixed",
-           MASK_EXTR(msi->address, MSI_ADDR_DEST_ID_MASK),
-           msi->arch.pirq);
+           dest, msi->arch.pirq);
 }
 
 void vpci_msix_arch_mask_entry(struct vpci_msix_entry *entry,
@@ -891,11 +906,16 @@ void vpci_msix_arch_init_entry(struct vpci_msix_entry *entry)
 
 int vpci_msix_arch_print(const struct vpci_msix *msix)
 {
+    const struct domain *d = msix->pdev->domain;
     unsigned int i;
 
     for ( i = 0; i < msix->max_entries; i++ )
     {
         const struct vpci_msix_entry *entry = &msix->entries[i];
+        unsigned long dest = MASK_EXTR(entry->addr, MSI_ADDR_DEST_ID_MASK);
+
+        if ( d->arch.ext_dest_id )
+            dest |= MASK_EXTR(entry->addr, MSI_ADDR_VIRT_EXT_DEST_ID_MASK) << 8;
 
         printk("%6u vec=%02x%7s%6s%3sassert%5s%7s dest_id=%lu mask=%u pirq: %d\n",
                i, MASK_EXTR(entry->data, MSI_DATA_VECTOR_MASK),
@@ -904,8 +924,7 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
                entry->data & MSI_DATA_LEVEL_ASSERT ? "" : "de",
                entry->addr & MSI_ADDR_DESTMODE_LOGIC ? "log" : "phys",
                entry->addr & MSI_ADDR_REDIRECTION_LOWPRI ? "lowest" : "fixed",
-               MASK_EXTR(entry->addr, MSI_ADDR_DEST_ID_MASK),
-               entry->masked, entry->arch.pirq);
+               dest, entry->masked, entry->arch.pirq);
         if ( i && !(i % 64) )
         {
             struct pci_dev *pdev = msix->pdev;
diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
index b44bbdeb21..37e9d4c0fc 100644
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -270,11 +270,12 @@ 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,
+    unsigned int dest, bool 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 *);
-int hvm_girq_dest_2_vcpu_id(struct domain *d, uint8_t dest, uint8_t dest_mode);
+int hvm_girq_dest_2_vcpu_id(const struct domain *d, unsigned int dest,
+                            bool dest_mode);
 
 enum hvm_intblk
 hvm_interrupt_blocked(struct vcpu *v, struct hvm_intack intack);
diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
index e228b0f3f3..9d9509a368 100644
--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -54,6 +54,13 @@
 #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)
+/*
+ * Use the reserved bits 11:5 to store the high part of the APIC ID, that
+ * allows expanding the destination field from 8 to 15 bits. Note this is a
+ * feature only present in virtualized hardware and currently only exposed to
+ * guests but not used by the hypervisor itself.
+ */
+#define	 MSI_ADDR_VIRT_EXT_DEST_ID_MASK	0x0000fe0
 
 /* 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 0b37cd145b..9c42ebe17a 100644
--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -269,7 +269,8 @@ int pt_irq_create_bind(
     {
     case PT_IRQ_TYPE_MSI:
     {
-        uint8_t dest, delivery_mode;
+        unsigned int dest;
+        bool delivery_mode;
         bool dest_mode;
         int dest_vcpu_id;
         const struct vcpu *vcpu;
@@ -346,6 +347,10 @@ int pt_irq_create_bind(
         /* Calculate dest_vcpu_id for MSI-type pirq migration. */
         dest = MASK_EXTR(pirq_dpci->gmsi.gflags,
                          XEN_DOMCTL_VMSI_X86_DEST_ID_MASK);
+        if ( d->arch.ext_dest_id )
+            dest |= MASK_EXTR(pirq_dpci->gmsi.gflags,
+                              XEN_DOMCTL_VMSI_X86_EXT_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);
@@ -789,6 +794,10 @@ static int _hvm_dpci_msi_eoi(struct domain *d,
                                       XEN_DOMCTL_VMSI_X86_DEST_ID_MASK);
         bool dest_mode = pirq_dpci->gmsi.gflags & XEN_DOMCTL_VMSI_X86_DM_MASK;
 
+        if ( d->arch.ext_dest_id )
+            dest |= MASK_EXTR(pirq_dpci->gmsi.gflags,
+                              XEN_DOMCTL_VMSI_X86_EXT_DEST_ID_MASK);
+
         if ( vlapic_match_dest(vcpu_vlapic(current), NULL, 0, dest,
                                dest_mode) )
         {
diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
index 5757a7aed2..e1d8c1d6f2 100644
--- a/xen/drivers/vpci/msi.c
+++ b/xen/drivers/vpci/msi.c
@@ -297,7 +297,7 @@ void vpci_dump_msi(void)
                 printk(" vectors max: %u enabled: %u\n",
                        pdev->msi_maxvec, msi->vectors);
 
-                vpci_msi_arch_print(msi);
+                vpci_msi_arch_print(msi, d);
             }
 
             msix = pdev->vpci->msix;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 31ec083cb0..ba71ce1148 100644
--- 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
 
             uint64_aligned_t gtable;
         } msi;
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index e8ac1eb395..354b37ef9c 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -162,7 +162,7 @@ int __must_check vpci_msi_arch_enable(struct vpci_msi *msi,
 void vpci_msi_arch_disable(struct vpci_msi *msi, const struct pci_dev *pdev);
 void vpci_msi_arch_update(struct vpci_msi *msi, const struct pci_dev *pdev);
 void vpci_msi_arch_init(struct vpci_msi *msi);
-void vpci_msi_arch_print(const struct vpci_msi *msi);
+void vpci_msi_arch_print(const struct vpci_msi *msi, const struct domain *d);
 
 /* Arch-specific vPCI MSI-X helpers. */
 void vpci_msix_arch_mask_entry(struct vpci_msix_entry *entry,
-- 
2.34.1

[PATCH v2 RFC 4/5] x86/ioreq: report extended destination ID support by emulators

[Roger Pau Monne]

Introduce a new arch specific field to report whether an emulator
supports the Extended Destination ID field, so that the hypervisor can
refrain from exposing the feature if one of the emulators doesn't
support it.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - New in this version.
---
RFC: I find this kind of clumsy. In fact fully emulated devices
should already support Extended Destination ID without any
modifications, because XEN_DMOP_inject_msi gets passed the address and
data fields, so the hypervisor extracts the extended destination ID
from there.

PCI passthrough devices however use xc_domain_update_msi_irq and that
has leaked the gflags parameter in the API, even worse the position
of the flags are hardcoded in QEMU.

Should the clearing of ext_dest_id be limited to the domain using an
IOMMU?

RFC: Only enable ext_dest_id if max_cpu > 128? So the device model is
aware the domain must use ext_dest_id? (implies device model knows
APIC ID = CPU ID * 2)
---
 xen/arch/arm/ioreq.c           | 5 +++++
 xen/arch/x86/hvm/ioreq.c       | 7 +++++++
 xen/common/ioreq.c             | 8 +++++---
 xen/include/public/hvm/dm_op.h | 6 +++++-
 xen/include/xen/ioreq.h        | 2 ++
 5 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
index 308650b400..7d56d022c8 100644
--- a/xen/arch/arm/ioreq.c
+++ b/xen/arch/arm/ioreq.c
@@ -185,6 +185,11 @@ void arch_ioreq_domain_init(struct domain *d)
 {
 }
 
+void arch_ioreq_server_create(struct domain *d, int bufioreq_handling,
+                              ioservid_t *id, unsigned int arch_flags)
+{
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index 02ad9db565..3276f0360d 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -336,6 +336,13 @@ void arch_ioreq_domain_init(struct domain *d)
     register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
 }
 
+void arch_ioreq_server_create(struct domain *d, int bufioreq_handling,
+                              ioservid_t *id, unsigned int arch_flags)
+{
+    if ( !(arch_flags & X86_SUPPORTS_EXT_DEST_ID) )
+        d->arch.ext_dest_id = false;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
index 689d256544..d4d5c653c7 100644
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -636,7 +636,7 @@ static void ioreq_server_deinit(struct ioreq_server *s)
 }
 
 static int ioreq_server_create(struct domain *d, int bufioreq_handling,
-                               ioservid_t *id)
+                               ioservid_t *id, unsigned int arch_flags)
 {
     struct ioreq_server *s;
     unsigned int i;
@@ -681,6 +681,8 @@ static int ioreq_server_create(struct domain *d, int bufioreq_handling,
     if ( id )
         *id = i;
 
+    arch_ioreq_server_create(d, bufioreq_handling, id, arch_flags);
+
     spin_unlock_recursive(&d->ioreq_server.lock);
     domain_unpause(d);
 
@@ -1340,11 +1342,11 @@ 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->pad[0] || data->pad[1] )
             break;
 
         rc = ioreq_server_create(d, data->handle_bufioreq,
-                                 &data->id);
+                                 &data->id, data->arch_flags);
         break;
     }
 
diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
index fa3f083fed..c6c575328b 100644
--- a/xen/include/public/hvm/dm_op.h
+++ b/xen/include/public/hvm/dm_op.h
@@ -67,7 +67,11 @@ typedef uint16_t ioservid_t;
 struct xen_dm_op_create_ioreq_server {
     /* IN - should server handle buffered ioreqs */
     uint8_t handle_bufioreq;
-    uint8_t pad[3];
+
+/* Signals Xen the emulator supports the Extended Destination ID field. */
+#define X86_SUPPORTS_EXT_DEST_ID (1u << 0)
+    uint8_t arch_flags;
+    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 a26614d331..f4566a1254 100644
--- a/xen/include/xen/ioreq.h
+++ b/xen/include/xen/ioreq.h
@@ -127,6 +127,8 @@ 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);
+void arch_ioreq_server_create(struct domain *d, int bufioreq_handling,
+                              ioservid_t *id, unsigned int arch_flags);
 
 #endif /* __XEN_IOREQ_H__ */
 
-- 
2.34.1

[PATCH v2 5/5] x86/cpuid: expose EXT_DEST_ID feature if supported

[Roger Pau Monne]

Expose the feature if available for the domain.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Note: con not be committed ahead of the rest of the series.
---
Changes since v1:
 - New in this version (split from previous patch).
---
 xen/arch/x86/traps.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 485bd66971..5b24688b07 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1150,6 +1150,9 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
         res->a |= XEN_HVM_CPUID_DOMID_PRESENT;
         res->c = d->domain_id;
 
+        if ( d->arch.ext_dest_id )
+            res->a |= XEN_HVM_CPUID_EXT_DEST_ID;
+
         break;
 
     case 5: /* PV-specific parameters */
-- 
2.34.1

Re: [PATCH v2 RFC 4/5] x86/ioreq: report extended destination ID support by emulators

[Durrant, Paul]

On 16/02/2022 10:30, Roger Pau Monne wrote:
> Introduce a new arch specific field to report whether an emulator
> supports the Extended Destination ID field, so that the hypervisor can
> refrain from exposing the feature if one of the emulators doesn't
> support it.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v1:
>   - New in this version.
> ---
> RFC: I find this kind of clumsy. In fact fully emulated devices
> should already support Extended Destination ID without any
> modifications, because XEN_DMOP_inject_msi gets passed the address and
> data fields, so the hypervisor extracts the extended destination ID
> from there.
> 
> PCI passthrough devices however use xc_domain_update_msi_irq and that
> has leaked the gflags parameter in the API, even worse the position
> of the flags are hardcoded in QEMU.
> 
> Should the clearing of ext_dest_id be limited to the domain using an
> IOMMU?
> 
> RFC: Only enable ext_dest_id if max_cpu > 128? So the device model is
> aware the domain must use ext_dest_id? (implies device model knows
> APIC ID = CPU ID * 2)

There is still only a single sync ioreq server page, so 128 vCPUs is the 
max possible.

   Paul

Re: [PATCH v2 RFC 4/5] x86/ioreq: report extended destination ID support by emulators

[Roger Pau Monné]

On Wed, Feb 16, 2022 at 10:53:58AM +0000, Durrant, Paul wrote:
> On 16/02/2022 10:30, Roger Pau Monne wrote:
> > Introduce a new arch specific field to report whether an emulator
> > supports the Extended Destination ID field, so that the hypervisor can
> > refrain from exposing the feature if one of the emulators doesn't
> > support it.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Changes since v1:
> >   - New in this version.
> > ---
> > RFC: I find this kind of clumsy. In fact fully emulated devices
> > should already support Extended Destination ID without any
> > modifications, because XEN_DMOP_inject_msi gets passed the address and
> > data fields, so the hypervisor extracts the extended destination ID
> > from there.
> > 
> > PCI passthrough devices however use xc_domain_update_msi_irq and that
> > has leaked the gflags parameter in the API, even worse the position
> > of the flags are hardcoded in QEMU.
> > 
> > Should the clearing of ext_dest_id be limited to the domain using an
> > IOMMU?
> > 
> > RFC: Only enable ext_dest_id if max_cpu > 128? So the device model is
> > aware the domain must use ext_dest_id? (implies device model knows
> > APIC ID = CPU ID * 2)
> 
> There is still only a single sync ioreq server page, so 128 vCPUs is the max
> possible.

Right - so device models wanting to support > 128 vCPUs will already
need to be modified, and hence we could assume that any HVM guests
with > 128 vCPUs is using a device model capable of handling extended
destination ID?

Thanks, Roger.

Re: [PATCH v2 1/5] x86/cpuid: add CPUID flag for Extended Destination ID support

[Jan Beulich]

On 16.02.2022 11:30, Roger Pau Monne wrote:
> --- a/xen/include/public/arch-x86/cpuid.h
> +++ b/xen/include/public/arch-x86/cpuid.h
> @@ -102,6 +102,12 @@
>  #define XEN_HVM_CPUID_IOMMU_MAPPINGS   (1u << 2)
>  #define XEN_HVM_CPUID_VCPU_ID_PRESENT  (1u << 3) /* vcpu id is present in EBX */
>  #define XEN_HVM_CPUID_DOMID_PRESENT    (1u << 4) /* domid is present in ECX */
> +/*
> + * Bits 55:49 from the IO-APIC RTE and bits 11:5 from the MSI address can be
> + * used to store high bits for the Destination ID. This expands the Destination
> + * ID field from 8 to 15 bits, allowing to target APIC IDs up 32768.
> + */
> +#define XEN_HVM_CPUID_EXT_DEST_ID      (1u << 5)

Would the comment perhaps better include "in the absence of (guest
visible) interrupt remapping", since otherwise the layout / meaning
changes anyway? Apart from this I'd be fine with this going in
ahead of the rest of this series.

Jan

Re: [PATCH v2 2/5] xen/vioapic: add support for the extended destination ID field

[Jan Beulich]

On 16.02.2022 11:30, Roger Pau Monne wrote:
> Such field uses bits 55:48, but for the purposes the register will be
> used use bits 55:49 instead. Bit 48 is used to signal an RTE entry is
> in remappable format which is not supported by the vIO-APIC.

Nit: The first sentence looks to have some stray words.

> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -527,6 +527,14 @@
>   */
>  #define LIBXL_HAVE_MAX_GRANT_VERSION 1
>  
> +/*
> + * LIBXL_HAVE_X86_EXT_DEST_ID indicates the toolstack can signal to the
> + * hypervisor whether the domain wants to use the extended destination ID mode
> + * for interrupt messages. This is done by setting the libxl_domain_build_info
> + * arch_x86.ext_dest_id field.
> + */
> +#define LIBXL_HAVE_X86_EST_DEST_ID 1

Did you mean LIBXL_HAVE_X86_EXT_DEST_ID, as the comment has it?

> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -648,6 +648,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                 ("vuart", libxl_vuart_type),
>                                ])),
>      ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
> +                               ("ext_dest_id", libxl_defbool),

Let's hope there's not going to appear any other meaning of "dest ID".
I would have suggested to add "apic" to the name, but this would get
it a little longish for my taste.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -782,6 +782,7 @@ static struct domain *__init create_dom0(const module_t *image,
>  
>          dom0_cfg.arch.emulation_flags |=
>              XEN_X86_EMU_LAPIC | XEN_X86_EMU_IOAPIC | XEN_X86_EMU_VPCI;
> +        dom0_cfg.arch.misc_flags |= XEN_X86_EXT_DEST_ID;
>      }

Without any way to suppress this?

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -38,7 +38,7 @@
>  #include "hvm/save.h"
>  #include "memory.h"
>  
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000015

I'm struggling to figure which binary incompatible change in here
requires this bumping. Does this perhaps belong in a later patch?

Jan

Re: [PATCH v2 3/5] x86/vmsi: add support for extended destination ID in address field

[Jan Beulich]

On 16.02.2022 11:30, Roger Pau Monne wrote:
> --- 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 think this is what requires the interface version bump. With that
moved here:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 1/5] x86/cpuid: add CPUID flag for Extended Destination ID support

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1423 bytes --]

On Wed, 2022-02-16 at 16:43 +0100, Jan Beulich wrote:
> On 16.02.2022 11:30, Roger Pau Monne wrote:
> > --- a/xen/include/public/arch-x86/cpuid.h
> > +++ b/xen/include/public/arch-x86/cpuid.h
> > @@ -102,6 +102,12 @@
> >  #define XEN_HVM_CPUID_IOMMU_MAPPINGS   (1u << 2)
> >  #define XEN_HVM_CPUID_VCPU_ID_PRESENT  (1u << 3) /* vcpu id is present in EBX */
> >  #define XEN_HVM_CPUID_DOMID_PRESENT    (1u << 4) /* domid is present in ECX */
> > +/*
> > + * Bits 55:49 from the IO-APIC RTE and bits 11:5 from the MSI address can be
> > + * used to store high bits for the Destination ID. This expands the Destination
> > + * ID field from 8 to 15 bits, allowing to target APIC IDs up 32768.
> > + */
> > +#define XEN_HVM_CPUID_EXT_DEST_ID      (1u << 5)
> 
> Would the comment perhaps better include "in the absence of (guest
> visible) interrupt remapping", since otherwise the layout / meaning
> changes anyway? Apart from this I'd be fine with this going in
> ahead of the rest of this series.

No, this still works even if the guest has a vIOMMU with interrupt
remapping. The Compatibility Format and Remappable Format MSI messages
are distinct because the low bit of the Ext Dest ID is used to indicate
Remappable Format.

If you wanted to add "… without having to use interrupt remapping in
the guest" to the end of the comment then I suppose you could, but I
don't think it adds much.


[-- Attachment #1.2: Type: text/html, Size: 1977 bytes --]

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v2 RFC 4/5] x86/ioreq: report extended destination ID support by emulators

[Durrant, Paul]

On 16/02/2022 11:32, Roger Pau Monné wrote:
> On Wed, Feb 16, 2022 at 10:53:58AM +0000, Durrant, Paul wrote:
>> On 16/02/2022 10:30, Roger Pau Monne wrote:
>>> Introduce a new arch specific field to report whether an emulator
>>> supports the Extended Destination ID field, so that the hypervisor can
>>> refrain from exposing the feature if one of the emulators doesn't
>>> support it.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> Changes since v1:
>>>    - New in this version.
>>> ---
>>> RFC: I find this kind of clumsy. In fact fully emulated devices
>>> should already support Extended Destination ID without any
>>> modifications, because XEN_DMOP_inject_msi gets passed the address and
>>> data fields, so the hypervisor extracts the extended destination ID
>>> from there.
>>>
>>> PCI passthrough devices however use xc_domain_update_msi_irq and that
>>> has leaked the gflags parameter in the API, even worse the position
>>> of the flags are hardcoded in QEMU.
>>>
>>> Should the clearing of ext_dest_id be limited to the domain using an
>>> IOMMU?
>>>
>>> RFC: Only enable ext_dest_id if max_cpu > 128? So the device model is
>>> aware the domain must use ext_dest_id? (implies device model knows
>>> APIC ID = CPU ID * 2)
>>
>> There is still only a single sync ioreq server page, so 128 vCPUs is the max
>> possible.
> 
> Right - so device models wanting to support > 128 vCPUs will already
> need to be modified, and hence we could assume that any HVM guests
> with > 128 vCPUs is using a device model capable of handling extended
> destination ID?
> 

Yes, exactly.

   Paul

Re: [PATCH v2 1/5] x86/cpuid: add CPUID flag for Extended Destination ID support

[Jan Beulich]

On 16.02.2022 17:08, David Woodhouse wrote:
> On Wed, 2022-02-16 at 16:43 +0100, Jan Beulich wrote:
>> On 16.02.2022 11:30, Roger Pau Monne wrote:
>>> --- a/xen/include/public/arch-x86/cpuid.h
>>> +++ b/xen/include/public/arch-x86/cpuid.h
>>> @@ -102,6 +102,12 @@
>>>  #define XEN_HVM_CPUID_IOMMU_MAPPINGS   (1u << 2)
>>>  #define XEN_HVM_CPUID_VCPU_ID_PRESENT  (1u << 3) /* vcpu id is present in EBX */
>>>  #define XEN_HVM_CPUID_DOMID_PRESENT    (1u << 4) /* domid is present in ECX */
>>> +/*
>>> + * Bits 55:49 from the IO-APIC RTE and bits 11:5 from the MSI address can be
>>> + * used to store high bits for the Destination ID. This expands the Destination
>>> + * ID field from 8 to 15 bits, allowing to target APIC IDs up 32768.
>>> + */
>>> +#define XEN_HVM_CPUID_EXT_DEST_ID      (1u << 5)
>>
>> Would the comment perhaps better include "in the absence of (guest
>> visible) interrupt remapping", since otherwise the layout / meaning
>> changes anyway? Apart from this I'd be fine with this going in
>> ahead of the rest of this series.
> 
> No, this still works even if the guest has a vIOMMU with interrupt
> remapping. The Compatibility Format and Remappable Format MSI messages
> are distinct because the low bit of the Ext Dest ID is used to indicate
> Remappable Format.

Well, yes, that was my point: With that bit set bits 55:49 / 11:5 change
meaning. As an alternative to my initial proposal the comment could also
state that bit 48 / 4 needs to be clear for this feature to take effect.

Jan

Re: [PATCH v2 1/5] x86/cpuid: add CPUID flag for Extended Destination ID support

[Roger Pau Monné]

On Thu, Feb 17, 2022 at 09:52:51AM +0100, Jan Beulich wrote:
> On 16.02.2022 17:08, David Woodhouse wrote:
> > On Wed, 2022-02-16 at 16:43 +0100, Jan Beulich wrote:
> >> On 16.02.2022 11:30, Roger Pau Monne wrote:
> >>> --- a/xen/include/public/arch-x86/cpuid.h
> >>> +++ b/xen/include/public/arch-x86/cpuid.h
> >>> @@ -102,6 +102,12 @@
> >>>  #define XEN_HVM_CPUID_IOMMU_MAPPINGS   (1u << 2)
> >>>  #define XEN_HVM_CPUID_VCPU_ID_PRESENT  (1u << 3) /* vcpu id is present in EBX */
> >>>  #define XEN_HVM_CPUID_DOMID_PRESENT    (1u << 4) /* domid is present in ECX */
> >>> +/*
> >>> + * Bits 55:49 from the IO-APIC RTE and bits 11:5 from the MSI address can be
> >>> + * used to store high bits for the Destination ID. This expands the Destination
> >>> + * ID field from 8 to 15 bits, allowing to target APIC IDs up 32768.
> >>> + */
> >>> +#define XEN_HVM_CPUID_EXT_DEST_ID      (1u << 5)
> >>
> >> Would the comment perhaps better include "in the absence of (guest
> >> visible) interrupt remapping", since otherwise the layout / meaning
> >> changes anyway? Apart from this I'd be fine with this going in
> >> ahead of the rest of this series.
> > 
> > No, this still works even if the guest has a vIOMMU with interrupt
> > remapping. The Compatibility Format and Remappable Format MSI messages
> > are distinct because the low bit of the Ext Dest ID is used to indicate
> > Remappable Format.
> 
> Well, yes, that was my point: With that bit set bits 55:49 / 11:5 change
> meaning.

Bits 55:49/11:5 become reserved again with the interrupt format bit
set to remappable.

> As an alternative to my initial proposal the comment could also
> state that bit 48 / 4 needs to be clear for this feature to take effect.

I've always assumed that setting the IF to remappable invalidates
extended destination ID, as the format of the interrupt is different
then and there's no destination ID anymore, just a handle field. Maybe
I could make it more explicit:

/*
 * With interrupt format set to 0 (non-remappable) bits 55:49 from the
 * IO-APIC RTE and bits 11:5 from the MSI address can be used to store
 * high bits for the Destination ID. This expands the Destination ID
 * field from 8 to 15 bits, allowing to target APIC IDs up 32768.
 */

Thanks, Roger.

Re: [PATCH v2 1/5] x86/cpuid: add CPUID flag for Extended Destination ID support

[Jan Beulich]

On 17.02.2022 10:24, Roger Pau Monné wrote:
> On Thu, Feb 17, 2022 at 09:52:51AM +0100, Jan Beulich wrote:
>> On 16.02.2022 17:08, David Woodhouse wrote:
>>> On Wed, 2022-02-16 at 16:43 +0100, Jan Beulich wrote:
>>>> On 16.02.2022 11:30, Roger Pau Monne wrote:
>>>>> --- a/xen/include/public/arch-x86/cpuid.h
>>>>> +++ b/xen/include/public/arch-x86/cpuid.h
>>>>> @@ -102,6 +102,12 @@
>>>>>  #define XEN_HVM_CPUID_IOMMU_MAPPINGS   (1u << 2)
>>>>>  #define XEN_HVM_CPUID_VCPU_ID_PRESENT  (1u << 3) /* vcpu id is present in EBX */
>>>>>  #define XEN_HVM_CPUID_DOMID_PRESENT    (1u << 4) /* domid is present in ECX */
>>>>> +/*
>>>>> + * Bits 55:49 from the IO-APIC RTE and bits 11:5 from the MSI address can be
>>>>> + * used to store high bits for the Destination ID. This expands the Destination
>>>>> + * ID field from 8 to 15 bits, allowing to target APIC IDs up 32768.
>>>>> + */
>>>>> +#define XEN_HVM_CPUID_EXT_DEST_ID      (1u << 5)
>>>>
>>>> Would the comment perhaps better include "in the absence of (guest
>>>> visible) interrupt remapping", since otherwise the layout / meaning
>>>> changes anyway? Apart from this I'd be fine with this going in
>>>> ahead of the rest of this series.
>>>
>>> No, this still works even if the guest has a vIOMMU with interrupt
>>> remapping. The Compatibility Format and Remappable Format MSI messages
>>> are distinct because the low bit of the Ext Dest ID is used to indicate
>>> Remappable Format.
>>
>> Well, yes, that was my point: With that bit set bits 55:49 / 11:5 change
>> meaning.
> 
> Bits 55:49/11:5 become reserved again with the interrupt format bit
> set to remappable.
> 
>> As an alternative to my initial proposal the comment could also
>> state that bit 48 / 4 needs to be clear for this feature to take effect.
> 
> I've always assumed that setting the IF to remappable invalidates
> extended destination ID, as the format of the interrupt is different
> then and there's no destination ID anymore, just a handle field. Maybe
> I could make it more explicit:
> 
> /*
>  * With interrupt format set to 0 (non-remappable) bits 55:49 from the
>  * IO-APIC RTE and bits 11:5 from the MSI address can be used to store
>  * high bits for the Destination ID. This expands the Destination ID
>  * field from 8 to 15 bits, allowing to target APIC IDs up 32768.
>  */

Yes, this disambiguates things enough to address my concern. Then
Reviewed-by: Jan Beulich <jbeulich@suse.com>
and I'd be fine making the adjustment while committing, if no other
concerns arise.

Jan

Re: [PATCH v2 5/5] x86/cpuid: expose EXT_DEST_ID feature if supported

[Jan Beulich]

On 16.02.2022 11:30, Roger Pau Monne wrote:
> Expose the feature if available for the domain.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v2 1/5] x86/cpuid: add CPUID flag for Extended Destination ID support

[David Woodhouse]

> /*
>  * With interrupt format set to 0 (non-remappable) bits 55:49 from the
>  * IO-APIC RTE and bits 11:5 from the MSI address can be used to store
>  * high bits for the Destination ID. This expands the Destination ID
>  * field from 8 to 15 bits, allowing to target APIC IDs up 32768.
>  */

I am not keen on that wording because it doesn't seem to fully reflect the
fact that the I/OAPIC is just a device to turn line interrupts into MSIs.
The values in bits 55:49 of the RTE *are* what go into bits 11:5 of the
resulting MSI address. Perhaps make it more parenthetical to make it
clearer that they are not independent... "bits 11:5 of the MSI address
(which come from bits 55:49 of the I/OAPIC RTE)..."


-- 
dwmw2

Re: [PATCH v2 1/5] x86/cpuid: add CPUID flag for Extended Destination ID support

[Roger Pau Monné]

On Sat, Feb 19, 2022 at 11:24:25AM -0000, David Woodhouse wrote:
> 
> 
> > /*
> >  * With interrupt format set to 0 (non-remappable) bits 55:49 from the
> >  * IO-APIC RTE and bits 11:5 from the MSI address can be used to store
> >  * high bits for the Destination ID. This expands the Destination ID
> >  * field from 8 to 15 bits, allowing to target APIC IDs up 32768.
> >  */
> 
> I am not keen on that wording because it doesn't seem to fully reflect the
> fact that the I/OAPIC is just a device to turn line interrupts into MSIs.

But that's an architecture implementation detail, I'm not sure I've
seen this written down explicitly in any specification.

> The values in bits 55:49 of the RTE *are* what go into bits 11:5 of the
> resulting MSI address. Perhaps make it more parenthetical to make it
> clearer that they are not independent... "bits 11:5 of the MSI address
> (which come from bits 55:49 of the I/OAPIC RTE)..."

That could be an option also, as long as it's clear to which bits of
the IO-APIC RTE this affects.

Thanks, Roger.