RE: [PATCH v3 08/11] vfio/pci/cxl: Add HDM + COMP_REGS regions and DVSEC clipping shim

> -----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.