On 9/16/26 20:44, [email protected] wrote:
From: Manish Honap <[email protected]>
The guest programs a GPA inside a CXL fixed memory window and QEMU maps
the HDM memory there, so the device must sit under exactly one
single-target CFMWS.
CFMWS (CXL Fixed Memory Window Structure). Pity that the fields are named
fmws_base, fmws_size.
Reject any ambiguous, interleaved, unsized, or
missing window rather than guess. Match windows by target name, since the
resolved target pointers are only filled in by a machine-done notifier
that may run after this one.
Require exactly one endpoint function below the root port: two functions
in the same slot would each match that single-target CFMWS and alias their
HDM memory at one base, so count every present function rather than one per
slot. The CFMWS windows are placed at machine-init-done, so a cold-plugged
device can only be validated from that notifier, where a bad topology is
fatal to startup. Validate a hotplugged device inline from realize instead,
reporting through errp so a bad device_add fails cleanly rather than
aborting the running VM, and arm the notifier only for a cold-plugged
device.
The endpoint's HDM size comes from the kernel region. cxl_fmws_set_memmap()
places the window in the guest PA map before this device is realized, so
the window cannot yet be sized from the device; check the configured window
against the device geometry instead. Reject a window too small to hold the
endpoint and name the size to set, and warn on an oversized window, since
the padding becomes guest CEDT that migration has to preserve.
I am lost ... Too much stuff there.
Each paragraph introduces a new topic/problem, and implementation details,
all mixed up, without first explaining what problem is being solved:
A passed-through CXL Type-2 device needs to know where its memory
window lives in the guest PA space so QEMU can map the device memory
there.
Is that it ?
A maintainer reviewing this patch shouldn't need to know what a CFMWS
is to understand the commit. CXL is still an emerging technology.
Most VFIO and QEMU reviewers will not have CXL spec background and
CXL-specific mechanic knowledge.
AI-used-for: code (prototype)
The code still reads like a proof-of-concept "prototype". This patch
in particular is very hard to follow. The problem isn't clearly described,
and likely isn't fully understood yet. The series should be broken into
smaller patches.
Signed-off-by: Manish Honap <[email protected]>
---
hw/pci-bridge/pci_expander_bridge_stubs.c | 6 +
hw/vfio/pci.c | 192 ++++++++++++++++++++++
hw/vfio/pci.h | 3 +
3 files changed, 201 insertions(+)
diff --git a/hw/pci-bridge/pci_expander_bridge_stubs.c
b/hw/pci-bridge/pci_expander_bridge_stubs.c
index b35180311f..c44ad7fab6 100644
--- a/hw/pci-bridge/pci_expander_bridge_stubs.c
+++ b/hw/pci-bridge/pci_expander_bridge_stubs.c
@@ -10,5 +10,11 @@
#include "hw/pci/pci_bus.h"
#include "hw/pci-bridge/pci_expander_bridge.h"
#include "hw/cxl/cxl.h"
+#include "hw/cxl/cxl_component.h"
void pxb_cxl_hook_up_registers(CXLState *state, PCIBus *bus, Error **errp) {};
+
+bool cxl_get_hb_passthrough(PCIHostState *hb)
+{
+ return false;
+}
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 4716266595..670e0d1da4 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -27,7 +27,12 @@
#include "hw/pci/msi.h"
#include "hw/pci/msix.h"
#include "hw/pci/pci_bridge.h"
+#include "hw/pci/pci_host.h"
+#include "hw/pci/pcie_port.h"
#include "hw/cxl/cxl.h"
+#include "hw/cxl/cxl_host.h"
+#include "hw/cxl/cxl_component.h"
+#include "system/system.h"
#include "hw/core/qdev-properties.h"
#include "hw/core/qdev-properties-system.h"
#include "hw/vfio/vfio-cpr.h"
@@ -3641,6 +3646,171 @@ static bool vfio_cxl_check_topology(VFIOPCIDevice
*vdev, Error **errp)
return true;
}
+/*
+ * Count the CXL fixed memory windows that target this device's pxb-cxl and
+ * report the matched window's base, size and target count. Match on the target
+ * names: cxl_fmws_link_targets() resolves target_hbs[] only at machine_done,
+ * which may run after this, so the resolved pointers can still be NULL here.
+ */
+static int vfio_cxl_match_fmws(PXBCXLDev *pxb, hwaddr *base, uint64_t *size,
+ int *nwindows, int *ntargets)
I think this should be in the CXL subsystem. That's a *lot* of out
parameters ...
+{
+ GSList *list = cxl_fmws_get_all_sorted();
+ GSList *iter;
+ int matches = 0;
+
+ *nwindows = g_slist_length(list);
+ *base = 0;
+ *size = 0;
+ *ntargets = 0;
+
+ for (iter = list; iter; iter = iter->next) {
+ CXLFixedWindow *fw = CXL_FMW(iter->data);
+ int i;
+
+ for (i = 0; i < fw->num_targets; i++) {
+ bool ambiguous = false;
+ Object *t = object_resolve_path_type(fw->targets[i],
+ TYPE_PXB_CXL_DEV, &ambiguous);
+
+ if (t && !ambiguous && PXB_CXL_DEV(t) == pxb) {
+ *base = fw->base;
+ *size = fw->size;
+ *ntargets = fw->num_targets;
+ matches++;
+ break;
+ }
+ }
+ }
+ g_slist_free(list);
+ return matches;
+}
+
+/*
+ * Fix the bounds of the device's memory window once the topology has settled.
+ * Exactly one single-target CFMWS, with an assigned base and room for the HDM
+ * memory, is required; anything else is a misconfiguration the guest cannot
+ * recover from, so reject it rather than guess a window.
+ */
+static bool vfio_cxl_do_bind_fmws(VFIOPCIDevice *vdev, Error **errp)
+{
This does too much :
topology validation
CFMWS window matching
size validation
It looks like vfio_cxl_check_topology() and it should be in the CXL subsystem.
+ VFIOCXL *cxl = &vdev->cxl;
+ const char *name = vdev->vbasedev.name;
+ int nwindows = 0, ntargets = 0, matches;
+ hwaddr base = 0;
+ uint64_t size = 0, need;
+ PXBCXLDev *pxb;
+
+ pxb = vfio_cxl_find_pxb(vdev, errp);
+ if (!pxb) {
+ return false;
+ }
+
+ if (pcie_count_ds_ports(PCI_HOST_BRIDGE(pxb->cxl_host_bridge)->bus) != 1) {
This won't build on all platforms.
+ error_setg(errp,
+ "vfio-cxl: %s: pxb-cxl not in HDM passthrough mode "
+ "(use a single cxl-rp)", name);
+ return false;
+ }
+
+ /*
+ * Count every present function, not one per slot, and require exactly one
+ * endpoint below the root port.
+ */> + {
+ PCIBus *ep_bus = pci_get_bus(&vdev->parent_obj);
+ int slot, fn, nendpoints = 0;
This smells like a function.
+ for (slot = 0; slot < PCI_SLOT_MAX; slot++) {
+ for (fn = 0; fn < PCI_FUNC_MAX; fn++) {
+ if (ep_bus->devices[PCI_DEVFN(slot, fn)]) {
+ nendpoints++;
+ }
+ }
+ }
+ if (nendpoints != 1) {
+ error_setg(errp,
+ "vfio-cxl: %s: %d devices below the cxl-rp; a passed "
+ "through CXL endpoint must be alone below its root
port",
+ name, nendpoints);
+ return false;
+ }
+ }
+
+ matches = vfio_cxl_match_fmws(pxb, &base, &size, &nwindows, &ntargets);
+ if (matches == 1 && ntargets == 1) {
+ if (!base) {
+ error_setg(errp,
+ "vfio-cxl: %s: matched CFMWS has no base; reduce its "
+ "size or grow the guest PA space", name);
+ return false;
+ }
+ /*
+ * QEMU reads the endpoint's HDM size from the kernel region, so the
+ * window size is really device information, not a value to make the
+ * caller guess. Sizing the CFMWS from the device the way firmware does
+ * is not possible here: the window is placed in the guest PA map by
+ * cxl_fmws_set_memmap() before this device is realized, so its size is
+ * fixed before dpa_size is known. Until a core change can size the
+ * window from the device, take the device geometry as the source of
+ * truth: reject a window too small to hold the endpoint, and warn when
+ * it is larger than needed, since the padding becomes guest CEDT that
+ * migration has to preserve.
+ */
+ need = ROUND_UP(cxl->dpa_size, 256 * MiB);
+ if (size < need) {
+ error_setg(errp,
+ "vfio-cxl: %s: CFMWS size 0x%" PRIx64 " cannot hold the
"
+ "endpoint; set the cxl-fmw size to 0x%" PRIx64,
+ name, size, need);
+ return false;
+ }
+ if (size > need) {
+ warn_report("vfio-cxl: %s: CFMWS size 0x%" PRIx64 " exceeds the "
+ "endpoint's 0x%" PRIx64 "; set the cxl-fmw size to 0x%"
+ PRIx64 " to keep the guest memory map stable across "
+ "migration", name, size, cxl->dpa_size, need);
+ }
+ cxl->fmws_base = base;
+ cxl->fmws_size = size;
+ return true;
+ }
+
+ if (matches == 1) {
+ error_setg(errp,
+ "vfio-cxl: %s: its CFMWS interleaves %d targets; use a "
+ "single-target window", name, ntargets);
+ } else if (matches > 1) {
+ error_setg(errp,
+ "vfio-cxl: %s: pxb-cxl is targeted by %d CFMWS; use one",
+ name, matches);
+ } else {
+ error_setg(errp,
+ "vfio-cxl: %s: no CFMWS targets this device (%d present)",
+ name, nwindows);
+ }
+ return false;
+}
+
+/*
+ * Cold-plug path: the CFMWS windows are placed at machine init done, so the
are you sure of that ? I think fw->base is already set when vfio_cxl_setup()
runs for cold-plug.
Normal vfio-pci has no hotplug-specific code, vfio_pci_realize() runs
the same path regardless of DEVICE(vdev)->hotplugged. The CXL case
should be the same.
+ * binding can only be validated from this notifier. The VM has not run yet, so
+ * a configuration error is fatal to startup. A hotplugged device is validated
+ * in realize instead (see vfio_cxl_setup), where the failure fails device_add
+ * without taking down the running VM.
+ */
+static void vfio_cxl_bind_fmws(Notifier *n, void *data)
+{
+ VFIOCXL *cxl = container_of(n, VFIOCXL, machine_done);
+ VFIOPCIDevice *vdev = container_of(cxl, VFIOPCIDevice, cxl);
+ Error *err = NULL;
+
+ if (!vfio_cxl_do_bind_fmws(vdev, &err)) {
+ error_report_err(err);
+ exit(1);
+ }
+}
+
/*
* Learn the CXL geometry the kernel reports: the HPA-backed HDM memory region
* and the trapped HDM decoder block (which BAR carries it and at what
offset).
@@ -3717,6 +3887,22 @@ static bool vfio_cxl_setup(VFIOPCIDevice *vdev, Error
**errp)
"performance may be slow", vbasedev->name);
}
+ if (DEVICE(vdev)->hotplugged) {
+ /*
+ * The machine is already up, so the CFMWS windows are placed and the
+ * binding can be validated now. Report a failure through errp so a bad
+ * device_add fails cleanly instead of aborting the running VM.
+ */
+ if (!vfio_cxl_do_bind_fmws(vdev, errp)) {
+ vfio_region_exit(&cxl->mem_region);
+ vfio_region_finalize(&cxl->mem_region);
as said before, the tear down has 2 parts. Anyhow, I don't think we need a
CXL-specific hotplug patch.
C.
+ return false;
+ }
+ } else {
+ cxl->machine_done.notify = vfio_cxl_bind_fmws;
+ qemu_add_machine_init_done_notifier(&cxl->machine_done);
+ }
+
cxl->enabled = true;
return true;
@@ -3729,6 +3915,12 @@ static void vfio_cxl_teardown(VFIOPCIDevice *vdev)
if (!cxl->enabled) {
return;
}
+
+ if (cxl->machine_done.notify) {
+ qemu_remove_machine_init_done_notifier(&cxl->machine_done);
+ cxl->machine_done.notify = NULL;
+ }
+
if (cxl->mem_region.mem) {
vfio_region_exit(&cxl->mem_region);
vfio_region_finalize(&cxl->mem_region);
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 06d15e807d..22fe8d7ff7 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -135,6 +135,9 @@ typedef struct VFIOCXL {
uint64_t hdm_offset; /* block offset within the component BAR
*/
uint64_t dpa_size; /* size of the HDM memory region */
VFIORegion mem_region; /* HDM memory, mapped at committed GPA */
+ Notifier machine_done; /* CFMWS validated at machine_done */
+ hwaddr fmws_base; /* base of the memory window */
+ uint64_t fmws_size; /* size of the memory window */
} VFIOCXL;
struct VFIOPCIDevice {
