[PATCH v3 0/3] Fix MSI-X handling for Xen HVM

[PATCH v3 0/3] Fix MSI-X handling for Xen HVM

[Marek Marczykowski-Górecki]

This series fixes handling MSI-X when device model is running in a stubdomain.
The main part is to avoid accessing /dev/mem, which also fixes running dom0
with lockdown enabled.

It depends on a behavior change of Xen that was just comitted, and signaled
with a feature flag. If Xen is too old (and XENFEAT_dm_msix_all_writes flag is
not set), fallback to the old behavior.

The other part is a fix to enforce read-only registers in the config space.
This fixes MSI-X setup for iwlwifi Linux driver, as it happen to write to MSI-X
capability id reg (as a workaround for some older device which has another
register there). It should be no-op, but due to a bug in xen_pt code,
it broke MSI-X detection.

All those patches have been shipped in Qubes OS 4.2 already, and prove to fix
the issue.

See individual commit messages for details.

Marek Marczykowski-Górecki (3):
  hw/xen/xen_pt: Save back data only for declared registers
  Update Xen's features.h header
  Do not access /dev/mem in MSI-X PCI passthrough on Xen

 hw/xen/xen_pt.c                     |  7 +-
 hw/xen/xen_pt_msi.c                 | 94 ++++++++++++++++++------------
 include/hw/xen/interface/features.h | 17 +++++-
 3 files changed, 82 insertions(+), 36 deletions(-)

base-commit: 2358f1b60f73287fe606c7ff48043b4f9e1c2d0f
-- 
git-series 0.9.1

[PATCH v3 1/3] hw/xen/xen_pt: Save back data only for declared registers

[Marek Marczykowski-Górecki]

Call pci_default_write_config() only after resolving any handlers from
XenPTRegInfo structures, and only with a value updated with those
handlers. This is important for two reasons:
1. XenPTRegInfo has ro_mask which needs to be enforced - Xen-specific
   hooks do that on their own (especially xen_pt_*_reg_write()).
2. Not setting value early allows hooks to see the old value too.

If it would be only about the first point, setting PCIDevice.wmask would
probably be sufficient, but given the second point, change those
writes.

Relevant handlers already save data back to the emulated registers
space, call the pci_default_write_config() only for its side effects.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
v3:
 - use emulated register value for pci_default_write_config() call, not
   the one for writting back to the hardware
 - greatly simplify the patch by calling pci_default_write_config() on
   the whole value
v2:
 - rewrite commit message, previous one was very misleading
 - fix loop saving register values
 - fix int overflow when calculating write mask
---
 hw/xen/xen_pt.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 3635d1b..5f12d3c 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -311,7 +311,6 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr,
     }
 
     memory_region_transaction_begin();
-    pci_default_write_config(d, addr, val, len);
 
     /* adjust the read and write value to appropriate CFC-CFF window */
     read_val <<= (addr & 3) << 3;
@@ -397,6 +396,12 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr,
     /* need to shift back before passing them to xen_host_pci_set_block. */
     val >>= (addr & 3) << 3;
 
+    /* Call default handler for its side effects only, with value already
+     * written by specific handlers. */
+    pci_default_write_config(d, addr,
+                             pci_default_read_config(d, addr, len),
+                             len);
+
     memory_region_transaction_commit();
 
 out:
-- 
git-series 0.9.1

[PATCH v3 2/3] Update Xen's features.h header

[Marek Marczykowski-Górecki]

Update it to get XENFEAT_dm_msix_all_writes for the next patch.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 include/hw/xen/interface/features.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/hw/xen/interface/features.h b/include/hw/xen/interface/features.h
index d2a9175..8801930 100644
--- a/include/hw/xen/interface/features.h
+++ b/include/hw/xen/interface/features.h
@@ -111,6 +111,23 @@
 #define XENFEAT_not_direct_mapped         16
 #define XENFEAT_direct_mapped             17
 
+/*
+ * Signal whether the domain is able to use the following hypercalls:
+ *
+ * VCPUOP_register_runstate_phys_area
+ * VCPUOP_register_vcpu_time_phys_area
+ */
+#define XENFEAT_runstate_phys_area        18
+#define XENFEAT_vcpu_time_phys_area       19
+
+/*
+ * If set, Xen will passthrough all MSI-X vector ctrl writes to device model,
+ * not only those unmasking an entry. This allows device model to properly keep
+ * track of the MSI-X table without having to read it from the device behind
+ * Xen's backs. This information is relevant only for device models.
+ */
+#define XENFEAT_dm_msix_all_writes        20
+
 #define XENFEAT_NR_SUBMAPS 1
 
 #endif /* __XEN_PUBLIC_FEATURES_H__ */
-- 
git-series 0.9.1

[PATCH v3 3/3] Do not access /dev/mem in MSI-X PCI passthrough on Xen

[Marek Marczykowski-Górecki]

The /dev/mem is used for two purposes:
 - reading PCI_MSIX_ENTRY_CTRL_MASKBIT
 - reading Pending Bit Array (PBA)

The first one was originally done because when Xen did not send all
vector ctrl writes to the device model, so QEMU might have outdated old
register value. If Xen is new enough, this has been changed, so QEMU can
now use its cached value of the register instead. Detect the "new
enough" based on XENFEAT_dm_msix_all_writes bit in XENVER_get_features.

The Pending Bit Array (PBA) handling is for the case where it lives on
the same page as the MSI-X table itself. Xen has been extended to handle
this case too (as well as other registers that may live on those pages),
so QEMU handling is not necessary anymore.

Additionally, reading from /dev/mem is trapped and emulated by Xen, so
QEMU doesn't see real values anyway. And if it did, this method is prone
to race conditions. Removing /dev/mem access is useful to work within
stubdomain (avoids emulated reads and potential races), and necessary
when dom0 kernel runs in lockdown mode (where /dev/mem is unavailable at
all).

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v2:
- Make change conditional on new Xen version (tested via
  XENFEAT_dm_msix_all_writes)
- add few comments
---
 hw/xen/xen_pt_msi.c | 94 ++++++++++++++++++++++++++++------------------
 1 file changed, 59 insertions(+), 35 deletions(-)

diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index 09cca4e..836cc9c 100644
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -460,15 +460,23 @@ static void pci_msix_write(void *opaque, hwaddr addr,
         entry->updated = true;
     } else if (msix->enabled && entry->updated &&
                !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
-        const volatile uint32_t *vec_ctrl;
-
         /*
-         * If Xen intercepts the mask bit access, entry->vec_ctrl may not be
-         * up-to-date. Read from hardware directly.
+         * Reading mask bit from hardware directly is needed on older Xen only.
          */
-        vec_ctrl = s->msix->phys_iomem_base + entry_nr * PCI_MSIX_ENTRY_SIZE
-            + PCI_MSIX_ENTRY_VECTOR_CTRL;
-        xen_pt_msix_update_one(s, entry_nr, *vec_ctrl);
+        if (s->msix->phys_iomem_base) {
+            /* Memory mapped registers */
+            const volatile uint32_t *vec_ctrl;
+
+            /*
+             * If Xen intercepts the mask bit access, entry->vec_ctrl may not be
+             * up-to-date. Read from hardware directly.
+             */
+            vec_ctrl = s->msix->phys_iomem_base + entry_nr * PCI_MSIX_ENTRY_SIZE
+                + PCI_MSIX_ENTRY_VECTOR_CTRL;
+            xen_pt_msix_update_one(s, entry_nr, *vec_ctrl);
+        } else {
+            xen_pt_msix_update_one(s, entry_nr, entry->latch(VECTOR_CTRL));
+        }
     }
 
     set_entry_value(entry, offset, val);
@@ -493,7 +501,12 @@ static uint64_t pci_msix_read(void *opaque, hwaddr addr,
         return get_entry_value(&msix->msix_entry[entry_nr], offset);
     } else {
         /* Pending Bit Array (PBA) */
-        return *(uint32_t *)(msix->phys_iomem_base + addr);
+        if (s->msix->phys_iomem_base) {
+            return *(uint32_t *)(msix->phys_iomem_base + addr);
+        }
+        XEN_PT_LOG(&s->dev, "reading PBA, addr 0x%lx, offset 0x%lx\n",
+                   addr, addr - msix->total_entries * PCI_MSIX_ENTRY_SIZE);
+        return 0xFFFFFFFF;
     }
 }
 
@@ -528,8 +541,8 @@ int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base)
     uint32_t table_off = 0;
     int i, total_entries, bar_index;
     XenHostPCIDevice *hd = &s->real_device;
+    xen_feature_info_t xc_version_info = { 0 };
     PCIDevice *d = &s->dev;
-    int fd = -1;
     XenPTMSIX *msix = NULL;
     int rc = 0;
 
@@ -543,6 +556,10 @@ int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base)
         return -1;
     }
 
+    if (xc_version(xen_xc, XENVER_get_features, &xc_version_info) < 0) {
+        return -1;
+    }
+
     rc = xen_host_pci_get_word(hd, base + PCI_MSIX_FLAGS, &control);
     if (rc) {
         XEN_PT_ERR(d, "Failed to read PCI_MSIX_FLAGS field\n");
@@ -576,33 +593,40 @@ int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base)
     msix->table_base = s->real_device.io_regions[bar_index].base_addr;
     XEN_PT_LOG(d, "get MSI-X table BAR base 0x%"PRIx64"\n", msix->table_base);
 
-    fd = open("/dev/mem", O_RDWR);
-    if (fd == -1) {
-        rc = -errno;
-        XEN_PT_ERR(d, "Can't open /dev/mem: %s\n", strerror(errno));
-        goto error_out;
-    }
-    XEN_PT_LOG(d, "table_off = 0x%x, total_entries = %d\n",
-               table_off, total_entries);
-    msix->table_offset_adjust = table_off & 0x0fff;
-    msix->phys_iomem_base =
-        mmap(NULL,
-             total_entries * PCI_MSIX_ENTRY_SIZE + msix->table_offset_adjust,
-             PROT_READ,
-             MAP_SHARED | MAP_LOCKED,
-             fd,
-             msix->table_base + table_off - msix->table_offset_adjust);
-    close(fd);
-    if (msix->phys_iomem_base == MAP_FAILED) {
-        rc = -errno;
-        XEN_PT_ERR(d, "Can't map physical MSI-X table: %s\n", strerror(errno));
-        goto error_out;
-    }
-    msix->phys_iomem_base = (char *)msix->phys_iomem_base
-        + msix->table_offset_adjust;
+    /* Accessing /dev/mem is needed only on older Xen. */
+    if (!(xc_version_info.submap & (1U << XENFEAT_dm_msix_all_writes))) {
+        int fd = -1;
+
+        fd = open("/dev/mem", O_RDWR);
+        if (fd == -1) {
+            rc = -errno;
+            XEN_PT_ERR(d, "Can't open /dev/mem: %s\n", strerror(errno));
+            goto error_out;
+        }
+        XEN_PT_LOG(d, "table_off = 0x%x, total_entries = %d\n",
+                   table_off, total_entries);
+        msix->table_offset_adjust = table_off & 0x0fff;
+        msix->phys_iomem_base =
+            mmap(NULL,
+                 total_entries * PCI_MSIX_ENTRY_SIZE
+                 + msix->table_offset_adjust,
+                 PROT_READ,
+                 MAP_SHARED | MAP_LOCKED,
+                 fd,
+                 msix->table_base + table_off - msix->table_offset_adjust);
+        close(fd);
+        if (msix->phys_iomem_base == MAP_FAILED) {
+            rc = -errno;
+            XEN_PT_ERR(d, "Can't map physical MSI-X table: %s\n",
+                       strerror(errno));
+            goto error_out;
+        }
+        msix->phys_iomem_base = (char *)msix->phys_iomem_base
+            + msix->table_offset_adjust;
 
-    XEN_PT_LOG(d, "mapping physical MSI-X table to %p\n",
-               msix->phys_iomem_base);
+        XEN_PT_LOG(d, "mapping physical MSI-X table to %p\n",
+                   msix->phys_iomem_base);
+    }
 
     memory_region_add_subregion_overlap(&s->bar[bar_index], table_off,
                                         &msix->mmio,
-- 
git-series 0.9.1

Re: [PATCH v3 0/3] Fix MSI-X handling for Xen HVM

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1682 bytes --]

On Mon, May 06, 2024 at 02:33:19AM +0200, Marek Marczykowski-Górecki wrote:
> This series fixes handling MSI-X when device model is running in a stubdomain.
> The main part is to avoid accessing /dev/mem, which also fixes running dom0
> with lockdown enabled.
> 
> It depends on a behavior change of Xen that was just comitted, and signaled
> with a feature flag. If Xen is too old (and XENFEAT_dm_msix_all_writes flag is
> not set), fallback to the old behavior.
> 
> The other part is a fix to enforce read-only registers in the config space.
> This fixes MSI-X setup for iwlwifi Linux driver, as it happen to write to MSI-X
> capability id reg (as a workaround for some older device which has another
> register there). It should be no-op, but due to a bug in xen_pt code,
> it broke MSI-X detection.
> 
> All those patches have been shipped in Qubes OS 4.2 already, and prove to fix
> the issue.
> 
> See individual commit messages for details.

Initially I sent the series with the old Anthony's address, but just in
case I forwarded it to his new address too.

> Marek Marczykowski-Górecki (3):
>   hw/xen/xen_pt: Save back data only for declared registers
>   Update Xen's features.h header
>   Do not access /dev/mem in MSI-X PCI passthrough on Xen
> 
>  hw/xen/xen_pt.c                     |  7 +-
>  hw/xen/xen_pt_msi.c                 | 94 ++++++++++++++++++------------
>  include/hw/xen/interface/features.h | 17 +++++-
>  3 files changed, 82 insertions(+), 36 deletions(-)
> 
> base-commit: 2358f1b60f73287fe606c7ff48043b4f9e1c2d0f
> -- 
> git-series 0.9.1

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/3] hw/xen/xen_pt: Save back data only for declared registers

[Anthony PERARD]

On Mon, May 06, 2024 at 02:33:20AM +0200, Marek Marczykowski-Górecki wrote:
> Call pci_default_write_config() only after resolving any handlers from
> XenPTRegInfo structures, and only with a value updated with those
> handlers. This is important for two reasons:
> 1. XenPTRegInfo has ro_mask which needs to be enforced - Xen-specific
>    hooks do that on their own (especially xen_pt_*_reg_write()).
> 2. Not setting value early allows hooks to see the old value too.
>
> If it would be only about the first point, setting PCIDevice.wmask would
> probably be sufficient, but given the second point, change those
> writes.
>
> Relevant handlers already save data back to the emulated registers
> space, call the pci_default_write_config() only for its side effects.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> v3:
>  - use emulated register value for pci_default_write_config() call, not
>    the one for writting back to the hardware
>  - greatly simplify the patch by calling pci_default_write_config() on
>    the whole value
> v2:
>  - rewrite commit message, previous one was very misleading
>  - fix loop saving register values
>  - fix int overflow when calculating write mask

Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

--

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v3 2/3] Update Xen's features.h header

[Anthony PERARD]

On Mon, May 06, 2024 at 02:33:21AM +0200, Marek Marczykowski-Górecki wrote:
> Update it to get XENFEAT_dm_msix_all_writes for the next patch.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

--

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v3 3/3] Do not access /dev/mem in MSI-X PCI passthrough on Xen

[Anthony PERARD]

On Mon, May 06, 2024 at 02:33:22AM +0200, Marek Marczykowski-Górecki wrote:
> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> index 09cca4e..836cc9c 100644
> --- a/hw/xen/xen_pt_msi.c
> +++ b/hw/xen/xen_pt_msi.c
> @@ -493,7 +501,12 @@ static uint64_t pci_msix_read(void *opaque, hwaddr addr,
>          return get_entry_value(&msix->msix_entry[entry_nr], offset);
>      } else {
>          /* Pending Bit Array (PBA) */
> -        return *(uint32_t *)(msix->phys_iomem_base + addr);
> +        if (s->msix->phys_iomem_base) {
> +            return *(uint32_t *)(msix->phys_iomem_base + addr);
> +        }
> +        XEN_PT_LOG(&s->dev, "reading PBA, addr 0x%lx, offset 0x%lx\n",
> +                   addr, addr - msix->total_entries * PCI_MSIX_ENTRY_SIZE);
> +        return 0xFFFFFFFF;

If Xen advertise XENFEAT_dm_msix_all_writes, we are not expecting QEMU
to reach this code, right? A comment might be useful.

>      }
>  }
>
> @@ -576,33 +593,40 @@ int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base)
>      msix->table_base = s->real_device.io_regions[bar_index].base_addr;
>      XEN_PT_LOG(d, "get MSI-X table BAR base 0x%"PRIx64"\n", msix->table_base);
>
> +    /* Accessing /dev/mem is needed only on older Xen. */
> +    if (!(xc_version_info.submap & (1U << XENFEAT_dm_msix_all_writes))) {

Would it be ok to use test_bit() instead?

Thanks,

--

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech