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

> -----Original Message-----
> From: Alex Williamson <[email protected]>
> Sent: 11 July 2026 03:53
> To: Manish Honap <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; Ankit Agrawal <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Neo Jia <[email protected]>; Krishnakant Jaju
> <[email protected]>; Vikram Sethi <[email protected]>; Zhi Wang
> <[email protected]>; [email protected]; [email protected];
> [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
> 
> 
> On Thu, 25 Jun 2026 22:24:04 +0530
> <[email protected]> wrote:
> > diff --git a/drivers/vfio/pci/vfio_pci_config.c
> > b/drivers/vfio/pci/vfio_pci_config.c
> > index a10ed733f0e3..b9f30a33515a 100644
> > --- a/drivers/vfio/pci/vfio_pci_config.c
> > +++ b/drivers/vfio/pci/vfio_pci_config.c
> > @@ -1898,8 +1898,15 @@ ssize_t vfio_pci_config_rw_single(struct
> vfio_pci_core_device *vdev,
> >       /*
> >        * Chop accesses into aligned chunks containing no more than a
> >        * single capability.  Caller increments to the next chunk.
> > +      *
> > +      * For CXL Type-2 devices also clip at the CXL Device DVSEC body
> > +      * boundary so the generic perm-bits path handles the DVSEC
> > +      * header bytes and the CXL hook handles the body bytes; without
> > +      * this clip a 32-bit access at dvsec + 0x08 would span the
> > +      * generic Header2 word and the CXL CAPABILITY word.
> >        */
> >       count = min(count, vfio_pci_cap_remaining_dword(vdev, *ppos));
> > +     count = min(count, vfio_pci_cxl_config_boundary(vdev, *ppos));
> >       if (count >= 4 && !(*ppos % 4))
> >               count = 4;
> >       else if (count >= 2 && !(*ppos % 2)) @@ -1909,6 +1916,30 @@
> > ssize_t vfio_pci_config_rw_single(struct vfio_pci_core_device *vdev,
> >
> >       ret = count;
> >
> > +     /*
> > +      * Give the CXL Type-2 hook first claim on this access: if the
> > +      * range lies inside the CXL Device DVSEC body, forward it to
> > +      * cxl-core's register-virtualization helpers instead of the
> > +      * standard perm-bits path.  -ENOENT means "not for me; use the
> > +      * default path"; any other negative value is a hard error.
> > +      */
> > +     if (vdev->cxl) {
> > +             __le32 le_val = 0;
> > +             ssize_t cxl_ret;
> > +
> > +             if (iswrite && copy_from_user(&le_val, buf, count))
> > +                     return -EFAULT;
> > +             cxl_ret = vfio_pci_cxl_config_rw(vdev, *ppos, count,
> &le_val,
> > +                                              iswrite);
> > +             if (cxl_ret >= 0) {
> > +                     if (!iswrite && copy_to_user(buf, &le_val,
> count))
> > +                             return -EFAULT;
> > +                     return cxl_ret;
> > +             }
> > +             if (cxl_ret != -ENOENT)
> > +                     return cxl_ret;
> > +     }
> > +
> 
> I think the solution here is just to set the .readfn and .writefn for
> PCI_EXT_CAP_ID_DVSEC to dvsec specific handlers, rather than the raw
> write and direct read handlers.  The new handlers would detect whether
> the reference is to the CXL DVSEC body, possibly via ranges stored in
> vdev->cxl, and either call through to CXL handlers via cxl_ops
> (previously suggested), or fall through to the raw/direct handlers.

okay, I will have handling for this in v4.

> 
> >       cap_id = vdev->pci_config_map[*ppos];
> >
> >       if (cap_id == PCI_CAP_ID_INVALID) { 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.
> > +              */
> > +             if (vdev->cxl && bar ==
> vfio_pci_cxl_get_component_reg_bar(vdev)) {
> > +                     vdev->barmap[bar] = pci_iomap(pdev, bar, 0);
> > +                     if (!vdev->barmap[bar]) {
> > +                             pci_dbg(pdev, "Failed to iomap region
> %d\n", bar);
> > +                             vdev->barmap[bar] = IOMEM_ERR_PTR(-
> ENOMEM);
> > +                     }
> > +                     continue;
> > +             }
> > +
> >               if (pci_request_selected_regions(pdev, 1 << bar,
> "vfio")) {
> >                       pci_dbg(pdev, "Failed to reserve region %d\n",
> bar);
> >                       vdev->barmap[bar] = IOMEM_ERR_PTR(-EBUSY); @@
> > -701,7 +718,10 @@ void vfio_pci_core_disable(struct
> vfio_pci_core_device *vdev)
> >               if (IS_ERR_OR_NULL(vdev->barmap[bar]))
> >                       continue;
> >               pci_iounmap(pdev, vdev->barmap[bar]);
> > -             pci_release_selected_regions(pdev, 1 << bar);
> > +             /* Mirror the asymmetric setup-time skip in map_bars().
> */
> > +             if (!(vdev->cxl &&
> > +                   i == vfio_pci_cxl_get_component_reg_bar(vdev)))
> > +                     pci_release_selected_regions(pdev, 1 << bar);
> 
> It would be much less ugly to create
> vfio_pci_{request,release}_selected_region() wrappers that mask whether
> the region is actually requested or released than to disrupt the code
> flow like this.  Likewise below, think about creating wrappers that do
> the right thing for cxl and are no-ops otherwise.  For example, embed
> the vdev->cxl test into the function to cleanup the callers.  Thanks,
> 

Agreed. I will add helper routines for this as suggested.

> Alex
> 
> >               vdev->barmap[bar] = NULL;
> >       }
> >
> > @@ -1051,6 +1071,16 @@ static int vfio_pci_ioctl_get_info(struct
> vfio_pci_core_device *vdev,
> >       info.num_regions = VFIO_PCI_NUM_REGIONS + vdev->num_regions;
> >       info.num_irqs = VFIO_PCI_NUM_IRQS;
> >
> > +     if (vdev->cxl) {
> > +             ret = vfio_pci_cxl_get_info(vdev, &caps);
> > +             if (ret) {
> > +                     pci_warn(vdev->pdev,
> > +                              "Failed to add CXL info capability\n");
> > +                     return ret;
> > +             }
> > +             info.flags |= VFIO_DEVICE_FLAGS_CXL;
> > +     }
> > +
> >       ret = vfio_pci_info_zdev_add_caps(vdev, &caps);
> >       if (ret && ret != -ENODEV) {
> >               pci_warn(vdev->pdev,
> > @@ -1093,6 +1123,12 @@ int vfio_pci_ioctl_get_region_info(struct
> vfio_device *core_vdev,
> >       struct pci_dev *pdev = vdev->pdev;
> >       int i, ret;
> >
> > +     if (vdev->cxl) {
> > +             ret = vfio_pci_cxl_get_region_info(vdev, info, caps);
> > +             if (ret != -ENOTTY)
> > +                     return ret;
> > +     }
> > +
> >       switch (info->index) {
> >       case VFIO_PCI_CONFIG_REGION_INDEX:
> >               info->offset = VFIO_PCI_INDEX_TO_OFFSET(info->index);