> -----Original Message-----
> From: Dan Williams (nvidia) <[email protected]>
> Sent: 11 July 2026 03:39
> To: Manish Honap <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Ankit Agrawal
> <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: Neo Jia <[email protected]>; Krishnakant Jaju <[email protected]>;
> Vikram Sethi <[email protected]>; Zhi Wang <[email protected]>; Manish
> Honap <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH v3 08/11] vfio/pci/cxl: Add HDM + COMP_REGS regions
> and DVSEC clipping shim
>
> External email: Use caution opening links or attachments
>
>
> mhonap@ wrote:
> > From: Manish Honap <[email protected]>
> >
> > Complete the vfio-pci-core integration of CXL Type-2 device
> > passthrough by exposing two VFIO regions to userspace, wiring DVSEC
> > config-space accesses through cxl-core's register-virtualization
> > helpers, and reserving the CXL component register block from BAR mmap
> > and BAR resource claim.
> >
> > HDM region (VFIO_REGION_SUBTYPE_CXL):
> > - mmappable view of the device's firmware-committed HPA range
> > - mmap fault handler calls vmf_insert_pfn() from the physical HPA
> > so the guest gets the same backing memory the host sees
> > - pread/pwrite go through the memremap_wb() kva captured at
> > bind time by vfio_cxl_map_hdm()
> >
> > COMP_REGS region (VFIO_REGION_SUBTYPE_CXL_COMP_REGS):
> > - pread/pwrite only, dword-aligned (-EINVAL on misalignment)
> > - thin transport: each dword dispatches by offset to
> > cxl_passthrough_cm_rw() (CM cap-array snapshot) or
> > cxl_passthrough_hdm_rw() (HDM Decoder block). No shadow buffer
> > on the vfio side; all per-field semantics live in cxl-core.
> >
> > DVSEC config-space access:
> > - vfio_pci_cxl_config_boundary() clips a chunk at the CXL Device
> > DVSEC body edge in vfio_pci_config_rw_single() so the generic
> > perm-bits path handles the DVSEC header bytes and the CXL hook
> > handles the body bytes. The clipping shim is used instead of
> > re-pointing the ecap_perms[] readfn/writefn (which would mutate
> > a module-init static and race across multiple CXL devices).
> > - vfio_pci_cxl_config_rw() forwards clipped accesses to
> > cxl_passthrough_dvsec_rw(); cxl-core enforces the per-field
> > write semantics (LOCK/RWO, CONTROL/RWL, STATUS/RW1C,
> > RANGE1/HwInit, RANGE2/RsvdZ).
> >
> > GET_INFO / GET_REGION_INFO:
> > - VFIO_DEVICE_INFO_CAP_CXL advertises the two region indices, the
> > component BAR layout, and HOST_FIRMWARE_COMMITTED.
> > - GET_REGION_INFO on the component BAR returns a sparse-mmap cap
> > that excludes [comp_reg_offset, comp_reg_offset+comp_reg_size).
> >
> > BAR resource handling:
> > - cxl-core holds request_mem_region() on the CXL component
> > register sub-range from devm_cxl_probe_mem(), so vfio_pci-core's
> > pci_request_selected_regions() on the full BAR would collide.
> > map_bars() skips the request for the component BAR (still iomaps
> > it; vfio holds the BAR via driver binding); disable() mirrors
> > the asymmetric skip.
> > - mmap of the component BAR refuses any range overlapping the CXL
> > sub-range via vfio_pci_cxl_mmap_overlaps_comp_regs().
> >
> > vfio_pci_cxl_open() now registers both VFIO regions; close()
> > unregisters them. Raw BAR rw redirect into the CXL sub-range is
> > intentionally not implemented: VMMs use the COMP_REGS region directly.
>
> I jumped ahead in the review to see how passthrough.c was being used.
>
> This patch is doing a lot which raises the risk that individual
> proposals within it are going to raise questions.
>
> > Signed-off-by: Manish Honap <[email protected]>
> > ---
> > drivers/vfio/pci/cxl/vfio_cxl_core.c | 521
> ++++++++++++++++++++++++++-
> > drivers/vfio/pci/vfio_pci_config.c | 31 ++
> > drivers/vfio/pci/vfio_pci_core.c | 44 ++-
> > drivers/vfio/pci/vfio_pci_priv.h | 72 ++++
> > drivers/vfio/pci/vfio_pci_rdwr.c | 17 +
> > 5 files changed, 679 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > b/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > index 42cd00bbe869..8a00b776d7c7 100644
> > --- a/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > +++ b/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > @@ -123,12 +123,24 @@ static int vfio_cxl_probe_regs(struct
> vfio_pci_cxl_state *cxl)
> > if (rc)
> > return rc;
> >
> > + /*
> > + * The CXL Component Register block is a fixed 64 KiB area (CXL
> r4.0
> > + * ยง8.2.3). cxl_pci_setup_regs() records the remaining BAR
> length
> > + * after the regblock offset in reg_map.max_size, which is an
> upper
> > + * bound, not the spec-defined size. Bail if the BAR does not
> have
> > + * room for a full component register block at the recorded
> offset,
> > + * and publish the spec size so the UAPI, sparse-mmap exclusion,
> and
> > + * COMP_REGS region all agree on the same window.
> > + */
> > + if (cxlds->reg_map.max_size < CXL_COMPONENT_REG_BLOCK_SIZE)
> > + return -ENXIO;
>
> This feels like a generic check that should not be vfio specific...
>
> Indeed it is an existing check in cxl_decode_regblock() that already
> makes this assertion.
Agreed - I'll drop the duplicate and rely on cxl-core's existing
assertion in cxl_decode_regblock(), publishing the spec block size from
there rather than re-checking on the vfio side.
>
> > cxl->info.hdm_count = hdm_count;
> > cxl->info.hdm_reg_offset = hdm_off;
> > cxl->info.hdm_reg_size = hdm_size;
> > cxl->info.comp_reg_bir = bir;
> > cxl->info.comp_reg_offset = bar_off;
> > - cxl->info.comp_reg_size = cxlds->reg_map.max_size;
> > + cxl->info.comp_reg_size =
> CXL_COMPONENT_REG_BLOCK_SIZE;
> > cxl->info.host_firmware_committed = true;
> >
> > /*
> > @@ -354,16 +366,515 @@ void vfio_pci_cxl_release(struct
> vfio_pci_core_device *vdev)
> > vdev->cxl = NULL;
> > }
> >
> > +static int vfio_pci_cxl_register_hdm(struct vfio_pci_core_device
> > +*vdev); static int vfio_pci_cxl_register_comp_regs(struct
> > +vfio_pci_core_device *vdev);
> > +
> > int vfio_pci_cxl_open(struct vfio_pci_core_device *vdev) {
> > + struct vfio_pci_cxl_state *cxl = vdev->cxl;
> > + int rc;
> > +
> > + if (!cxl)
> > + return 0; /* plain vfio-pci device */
> > +
> > + rc = vfio_pci_cxl_register_comp_regs(vdev);
> > + if (rc) {
> > + pci_warn(vdev->pdev,
> > + "vfio-cxl: COMP_REGS region register failed
> (%d)\n",
> > + rc);
> > + return rc;
> > + }
> > +
> > + rc = vfio_pci_cxl_register_hdm(vdev);
> > + if (rc) {
> > + pci_warn(vdev->pdev,
> > + "vfio-cxl: HDM region register failed (%d)\n",
> rc);
> > + /*
> > + * COMP_REGS already registered above. vfio core does
> not
> > + * call close_device() when open_device() returns an
> error,
> > + * so roll back the COMP_REGS dynamic region here to
> avoid
> > + * a leaked half-registered open state.
> > + */
> > + vfio_pci_cxl_close(vdev);
> > + return rc;
> > + }
> > + return 0;
> > +}
> > +
> > +void vfio_pci_cxl_close(struct vfio_pci_core_device *vdev) {
> > + struct vfio_pci_cxl_state *cxl = vdev->cxl;
> > + unsigned int i;
> > +
> > + if (!cxl)
> > + return;
> > +
> > + for (i = vdev->num_regions; i > 0; i--) {
> > + struct vfio_pci_region *r = &vdev->region[i - 1];
> > +
> > + if (r->data != cxl)
> > + break;
> > + if (r->ops->release)
> > + r->ops->release(vdev, r);
> > + vdev->num_regions--;
> > + }
> > +}
> > +
> > +/* ------------------------------------------------------------------
> */
> > +/* HDM region: mmappable view of the device's HPA range
> */
> > +/* ------------------------------------------------------------------
> > +*/
> > +
> > +static vm_fault_t hdm_region_fault(struct vm_fault *vmf) {
> > + struct vm_area_struct *vma = vmf->vma;
> > + struct vfio_pci_cxl_state *cxl = vma->vm_private_data;
> > + unsigned long off = (vmf->address - vma->vm_start) +
> > + (vma->vm_pgoff << PAGE_SHIFT);
> > + phys_addr_t pa;
> > +
> > + if (!cxl || !cxl->info.hpa_size)
> > + return VM_FAULT_SIGBUS;
> > + if (off >= cxl->info.hpa_size)
> > + return VM_FAULT_SIGBUS;
> > +
> > + pa = cxl->info.hpa_base + off;
> > + return vmf_insert_pfn(vma, vmf->address, PHYS_PFN(pa));
>
> It seems unfortunate that this reimplements vfio_pci_mmap_ops without
> huge mapping support. If CXL is going to be a first class citizen in
> vfio then why not extend vfio_pci_core_device with the concept that
> devices can have HDM ranges, or some scheme to address large mappings
> from day one.
>
> [..]
> > diff --git a/drivers/vfio/pci/vfio_pci_core.c
> > b/drivers/vfio/pci/vfio_pci_core.c
> > index 05ab4ae59157..2d2dae278d1e 100644
> > --- a/drivers/vfio/pci/vfio_pci_core.c
> > +++ b/drivers/vfio/pci/vfio_pci_core.c
> > @@ -501,6 +501,23 @@ static void vfio_pci_core_map_bars(struct
> vfio_pci_core_device *vdev)
> > if (!pci_resource_len(pdev, i))
> > continue;
> >
> > + /*
> > + * cxl-core already holds request_mem_region() on the
> CXL
> > + * component register sub-range of this BAR. Skip the
> > + * full-BAR request so we do not collide with that
> > + * sub-region; vfio still owns the BAR via the driver
> > + * binding and the iomap below succeeds without a region
> > + * claim.
> > + */
>
> Rather than working around a problem in the CXL core why not fix it?
> This is the second time someone has run across a conflict with
> devm_cxl_iomap_block() when trying to reuse CXL code.
>
> devm_cxl_iomap_block() was a response to folks wanting /dev/mem access
> to CXL BAR space. When VFIO owns the whole BAR it should be blocking
> /dev/mem access to enforce going through VFIO.
>
> I think this looks like a flag on cxl_register_map that only cxl_pci
> sets to indicate the legacy special mode of doing piecemeal resource
> requests of just the component registers. External CXL drivers likely
> just want to own the whole BAR and not workaround an internal detail
> like this.
okay, I will check this part and handle this in v4 as suggested.
