> -----Original Message-----
> From: Cédric Le Goater <[email protected]>
> Sent: Thursday, September 17, 2026 9:30 PM
> To: Manish Honap <[email protected]>; [email protected]; Ankit Agrawal
> <[email protected]>; [email protected]; [email protected];
> [email protected]; Srirangan Madhavan
> <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Nitesh Narayan Lal
> <[email protected]>
> Cc: Krishnakant Jaju <[email protected]>; Vikram Sethi <[email protected]>;
> Zhi Wang <[email protected]>; [email protected]; qemu-
> [email protected]; [email protected]
> Subject: Re: [PATCH v2 06/10] hw/vfio/pci: Bind a CXL device to its fixed
> memory window
>
> External email: Use caution opening links or attachments
>
>
> 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.
>
I Agree this patch is doing too much. I will break this patch into topology
check, window match, size validation, and the suggested fixes in this
thread. I will start the commit message with the problem description and define
the relevant terms (e.g. CFMWS etc.), so a reviewer without CXL background can
follow it.
> 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 {
