[PATCH v5 00/11] x86/hvm: Add Extended MSI destination ID support

[Julian Vetter <julian.vetter@xxxxxxxxxx>]

Thank you Teddy and Jan for the detailed review on v4! Jan you're right,
When recently asking I didn't understand the issue with only having a
ENABLED/DISABLED for the ext_dest_id flag. Now it's clear why we need a
tri-state, but also why a tri-state is sufficient. I have converted it
to UNSET, DISABLED, or ENABLED. So, after a migration we know that if
it's still UNSET the domain is unaware of the feature and we will not
announce it no matter how many ioreq servers have opt'ed in on the new
host.

I have also split the parts that "just" move code around with no
functional changes into smaller chunks so each of them is easier to
diff:
Patch 1  x86/vioapic: check of the IOAPIC save record.
Path 2-6 Refactor pt_irq_create_bind(): brace the retry block, turn
         "goto restart" into a loop, extract pt_irq_dpci_setup(),
         extract the PT_IRQ_TYPE_MSI body into pt_irq_bind_msi() as just
         a code move, then re-indent. No functional change.
Patch 7  Switch pt_irq_bind_msi() and struct hvm_gmsi_info to the raw
         MSI address/data words, add MSI_ADDR_DEST(), reject a stored
         address that is not in 0xfeexxxxx MSI format. domctl callers
         keep working via a gflags -> message rebuild. No functional
         change for existing 8-bit-destination guests.
Patch 8  Add the tri-state d->arch.hvm.ext_dest_id and fold the extended
         bits into every call site, guarded by hvmext_dest_id_active().
         Still never enabled, so still no functional change.
Patch 9  Add XEN_DMOP_{,un}bind_pt_msi_irq (raw address/data) and remove
         PT_IRQ_TYPE_MSI from XEN_DOMCTL_{,un}bind_pt_irq. This is an
         incompatible hypercall-ABI change, recorded in CHANGELOG.md and
         public/domctl.h.
Patch 10 Negotiate the feature per ioreq server (a flags byte on
         XEN_DMOP_create_ioreq_server), level it across all servers,
         latch it at arch_domain_creation_finished(), and migrate it in
         a new HVM_SAVE_TYPE(EXT_DEST_ID) record.
Patch 11 Advertise XEN_HVM_CPUID_EXT_DEST_ID from the latched value.

I only have two remaining questions:
- As pointed out by Jan: The new DM ops now only relies on xsm_dm_op()
  for access control. Daniel Smith can you comment on, whether a
  xsm_bind_pt_irq hook is also wanted here?
- In dm.c we still use the read_lock(&d->pci_lock) around the
  bind/unbind calls, as was the case in v4. Jan you suspected no lock is
  needed there, but for now I left it as-is.

Signed-off-by: Julian Vetter <julian.vetter@xxxxxxxxxx>
---
Changes in v5:
- The domctl MSI path is no longer rejected (that broke older DMs and
  left dead code). PT_IRQ_TYPE_MSI keeps working until patch 9 removes
  it alltogether, with the now-dead pt_irq_create_bind() case deleted in
  the same step.
- pt_irq_bind_msi() rejects an address not in MSI format (0xfeexxxxx),
  so every caller storing into gmsi.addr is guaranteed a well-formed
  message.
- The ext_dest_id "bool" state is now a tri-state (UNSET / DISABLED /
  ENABLED). Every decode site folds in the extended bits only when the
  feature is active and treats them as reserved otherwise, rather than
  rejecting a guest that left values there. Removed the ioapic_check()
  extended-bit rejection loop.
- The creation_finished latch only fires while the state is UNSET, so a
  migrated value wins.
- hvm_load() resolves a record-less stream (from an "old" Xen) to
  DISABLED.
- v4's single "extract pt_irq_bind_msi()" patch is split into
  - The goto -> loop conversion is its own patch (Patch 3) and does a
    "for ( ; ; )" instead of do/while
  - A code move (Patch 4)
  - A re-indent (Patches 5, 6)
- Moved hvm_ext_dest_id_enabled() into common/ioreq.c as a function that
  iterates over ARRAY_SIZE(d->ioreq_server.server). Flag-bit validation
  is now arch specific. The Arm stub rejects any non-zero flag. Added a
  check handler for the EXT_DEST_ID save record.
- Fixed a bug in _hvm_dpci_msi_eoi(). Function tested
  XEN_DOMCTL_VMSI_X86_DM_MASK against a raw address and not against
  MSI_ADDR_DESTMODE_MASK.
- Added the missing libxendevicemodel.map VERS_1.5 entry and Makefile
  MINOR bump. The new parameter for xendevicemodel_create_ioreq_server()
  is a compatibility break for DMs, now mentioned in CHANGELOG.md.
- ioapic_check() validates base_address alignment, hap_paddr_bits and
  bounds the APIC ID by the IO_APIC_reg_02 field and the ioregsel check
  is gone, since vioapic_write() has no such restriction.
- Addressed general feedback from Jan:
  - machine_irq / gflags and the flags parameters are unsigned int
  - MSI_ADDR_DEST() derives its shift from a new MSI_ADDR_DEST_ID_WIDTH
    and leaves the existing MSI_ADDR_DEST_ID_* lines alone
  - Removed the "Intel convention" in a comment
  - MASK_EXTR / MASK_INSR used consistently
  - Removed redundant nr_pirqs check and "!!"
  - DM op debug print shortened to "%pd: fn() failed: %ld"
  - DM op struct fields are pirq / msg_addr / msg_data, the flag
    is XEN_DMOP_MSI_BIND_UNMASKED
  - gtable is documented as a guest-physical address
  - x86-specific wording is out of the public header
  - needless typedefs / blank line removed
  - The unbind op no longer requires current-domain IRQ permission.
---
Julian Vetter (11):
  x86/vioapic: Add ioapic_check() to validate IO-APIC state before
    restore
  x86/passthrough: Wrap pt_irq_create_bind() restart block in braces
  x86/passthrough: Replace pt_irq_create_bind() goto restart with a loop
  x86/passthrough: Extract pt_irq_dpci_setup() from pt_irq_create_bind()
  x86/passthrough: Extract PT_IRQ_TYPE_MSI body into pt_irq_bind_msi()
  x86/passthrough: Re-indent pt_irq_bind_msi() body
  x86/passthrough: Switch pt_irq_bind_msi() to raw MSI address/data
  x86/hvm: Decode extended MSI / IO-APIC destination IDs when opted in
  x86/dmop: Add XEN_DMOP_{,un}bind_pt_msi_irq
  hvm/ioreq: Negotiate extended destination ID support per ioreq server
  x86/cpuid: Advertise XEN_HVM_CPUID_EXT_DEST_ID

 CHANGELOG.md                                 |   6 +
 tools/include/xendevicemodel.h               |  33 +-
 tools/libs/ctrl/xc_devicemodel_compat.c      |   2 +-
 tools/libs/ctrl/xc_domain.c                  |  51 ++-
 tools/libs/devicemodel/Makefile              |   2 +-
 tools/libs/devicemodel/core.c                |  41 ++-
 tools/libs/devicemodel/libxendevicemodel.map |   6 +
 xen/arch/arm/ioreq.c                         |   6 +
 xen/arch/x86/cpuid.c                         |   8 +
 xen/arch/x86/domain.c                        |  13 +
 xen/arch/x86/domctl.c                        |  11 +-
 xen/arch/x86/hvm/dm.c                        |  66 ++++
 xen/arch/x86/hvm/ioreq.c                     |  57 +++
 xen/arch/x86/hvm/irq.c                       |   6 +-
 xen/arch/x86/hvm/save.c                      |   9 +
 xen/arch/x86/hvm/vioapic.c                   |  49 ++-
 xen/arch/x86/hvm/vmsi.c                      |  59 ++-
 xen/arch/x86/include/asm/hvm/domain.h        |  13 +
 xen/arch/x86/include/asm/hvm/hvm.h           |  12 +-
 xen/arch/x86/include/asm/hvm/irq.h           |   4 +-
 xen/arch/x86/include/asm/hvm/vioapic.h       |  10 +
 xen/arch/x86/include/asm/msi.h               |  19 +
 xen/common/ioreq.c                           |  31 +-
 xen/drivers/passthrough/x86/hvm.c            | 366 +++++++++++--------
 xen/include/public/arch-x86/hvm/save.h       |  20 +-
 xen/include/public/hvm/dm_op.h               |  51 ++-
 xen/include/xen/iommu.h                      |   3 +
 xen/include/xen/ioreq.h                      |  11 +
 xen/include/xlat.lst                         |   2 +
 29 files changed, 723 insertions(+), 244 deletions(-)

-- 
2.53.0



--
Julian Vetter | Vates Hypervisor & Kernel Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech