RE: [PATCH 10/20] vfio/cxl: CXL region management

[Manish Honap]

> -----Original Message-----
> From: Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>
> Sent: 13 March 2026 18:23
> To: Dave Jiang <dave.jiang@xxxxxxxxx>
> Cc: Manish Honap <mhonap@xxxxxxxxxx>; Aniket Agashe <aniketa@xxxxxxxxxx>;
> Ankit Agrawal <ankita@xxxxxxxxxx>; Alex Williamson
> <alwilliamson@xxxxxxxxxx>; Vikram Sethi <vsethi@xxxxxxxxxx>; Jason
> Gunthorpe <jgg@xxxxxxxxxx>; Matt Ochs <mochs@xxxxxxxxxx>; Shameer Kolothum
> Thodi <skolothumtho@xxxxxxxxxx>; alejandro.lucero-palau@xxxxxxx;
> dave@xxxxxxxxxxxx; alison.schofield@xxxxxxxxx; vishal.l.verma@xxxxxxxxx;
> ira.weiny@xxxxxxxxx; dan.j.williams@xxxxxxxxx; jgg@xxxxxxxx; Yishai Hadas
> <yishaih@xxxxxxxxxx>; kevin.tian@xxxxxxxxx; Neo Jia <cjia@xxxxxxxxxx>;
> Tarun Gupta (SW-GPU) <targupta@xxxxxxxxxx>; Zhi Wang <zhiw@xxxxxxxxxx>;
> Krishnakant Jaju <kjaju@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; linux-
> cxl@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 10/20] vfio/cxl: CXL region management
> 
> External email: Use caution opening links or attachments
> 
> 
> On Thu, 12 Mar 2026 15:55:32 -0700
> Dave Jiang <dave.jiang@xxxxxxxxx> wrote:
> 
> > On 3/11/26 1:34 PM, mhonap@xxxxxxxxxx wrote:
> > > From: Manish Honap <mhonap@xxxxxxxxxx>
> > >
> > > Add CXL region management for future guest access.
> > >
> > > Region Management makes use of APIs provided by CXL_CORE as below:
> > >
> > > CREATE_REGION flow:
> > > 1. Validate request (size, decoder availability) 2. Allocate HPA via
> > > cxl_get_hpa_freespace() 3. Allocate DPA via cxl_request_dpa() 4.
> > > Create region via cxl_create_region() - commits HDM decoder!
> > > 5. Get HPA range via cxl_get_region_range()
> > >
> > > DESTROY_REGION flow:
> > > 1. Detach decoder via cxl_decoder_detach() 2. Free DPA via
> > > cxl_dpa_free() 3. Release root decoder via cxl_put_root_decoder()
> > >
> > > Signed-off-by: Manish Honap <mhonap@xxxxxxxxxx>
> A few additional comments from me.
> 
> > > ---
> > >  drivers/vfio/pci/cxl/vfio_cxl_core.c | 118
> ++++++++++++++++++++++++++-
> > >  drivers/vfio/pci/cxl/vfio_cxl_priv.h |   5 ++
> > >  drivers/vfio/pci/vfio_pci_priv.h     |   8 ++
> > >  3 files changed, 130 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > > b/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > > index 2da6da1c0605..9c71f592e74e 100644
> > > --- a/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > > +++ b/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > > @@ -126,6 +126,112 @@ static int vfio_cxl_setup_regs(struct
> vfio_pci_core_device *vdev)
> > >     return 0;
> > >  }
> > >
> > > +int vfio_cxl_create_cxl_region(struct vfio_pci_core_device *vdev,
> > > +resource_size_t size) {
> > > +   struct vfio_pci_cxl_state *cxl = vdev->cxl;
> > > +   resource_size_t max_size;
> > > +   int ret;
> > > +
> > > +   if (cxl->precommitted)
> > > +           return 0;
> > > +
> > > +   cxl->cxlrd = cxl_get_hpa_freespace(cxl->cxlmd, 1,
> > > +                                      CXL_DECODER_F_RAM |
> > > +                                      CXL_DECODER_F_TYPE2,
> > > +                                      &max_size);
> >
> > Not sure what VFIO subsystem's policy is on scoped base resource
> cleanup, but a __free() here can get you out of managing put() of the root
> decoder.

After having a word with Alex for this, I have added __free macros.

> >
> > > +   if (IS_ERR(cxl->cxlrd))
> > > +           return PTR_ERR(cxl->cxlrd);
> > > +
> > > +   /* Insufficient HPA space */
> > > +   if (max_size < size) {
> > > +           cxl_put_root_decoder(cxl->cxlrd);
> > > +           cxl->cxlrd = NULL;
> Similar to other cases, I'd keep assigning stuff in cxl to the point where
> there are no more error paths.  Use local variables until then.
> (that would fit with using __free() as well which I'd also favor if
> accepted in VFIO).
> 

Yes, addressed.

> > > +           return -ENOSPC;
> > > +   }
> > > +
> > > +   cxl->cxled = cxl_request_dpa(cxl->cxlmd, CXL_PARTMODE_RAM,
> > > + size);
> >
> > Same comment here about __free().
> >
> > > +   if (IS_ERR(cxl->cxled)) {
> > > +           ret = PTR_ERR(cxl->cxled);
> > > +           goto err_free_hpa;
> > > +   }
> > > +
> > > +   cxl->region = cxl_create_region(cxl->cxlrd, &cxl->cxled, 1);
> > > +   if (IS_ERR(cxl->region)) {
> > > +           ret = PTR_ERR(cxl->region);
> 
> You carefully NULL this in vfio_cxl_destroy_region, but if you fail here
> you end up with it containing an ERR_PTR(). I'd avoid that by using a
> local variable and only assigning cxl->region after this suceeds.

Agreed.

> 
> > > +           goto err_free_dpa;
> > > +   }
> > > +
> > > +   return 0;
> > > +
> > > +err_free_dpa:
> > > +   cxl_dpa_free(cxl->cxled);
> > > +err_free_hpa:
> > > +   if (cxl->cxlrd)
> > > +           cxl_put_root_decoder(cxl->cxlrd);
> > > +
> > > +   return ret;
> > > +}
> > > +
> > > +void vfio_cxl_destroy_cxl_region(struct vfio_pci_core_device *vdev)
> > > +{
> > > +   struct vfio_pci_cxl_state *cxl = vdev->cxl;
> > > +
> > > +   if (!cxl->region)
> > > +           return;
> > > +
> > > +   cxl_unregister_region(cxl->region);
> > > +   cxl->region = NULL;
> > > +
> > > +   if (cxl->precommitted)
> > > +           return;
> > > +
> > > +   cxl_dpa_free(cxl->cxled);
> > > +   cxl_put_root_decoder(cxl->cxlrd); }
> > > +
> > > +static int vfio_cxl_create_region_helper(struct vfio_pci_core_device
> *vdev,
> > > +                                    resource_size_t capacity) {
> > > +   struct vfio_pci_cxl_state *cxl = vdev->cxl;
> > > +   struct pci_dev *pdev = vdev->pdev;
> > > +   int ret;
> > > +
> > > +   if (cxl->precommitted) {
> > > +           cxl->cxled = cxl_get_committed_decoder(cxl->cxlmd,
> > > +                                                  &cxl->region);
> > > +           if (IS_ERR(cxl->cxled))
> > > +                   return PTR_ERR(cxl->cxled);
> > > +   } else {
> > > +           ret = vfio_cxl_create_cxl_region(vdev, capacity);
> > > +           if (ret)
> > > +                   return ret;
> > > +   }
> > > +
> > > +   if (cxl->region) {
> >
> > Maybe if you do 'if (!cxl->region)' first and just exit, then you don't
> need to indent the normal code path.

Okay, I will change this.

> >
> > > +           struct range range;
> > > +
> > > +           ret = cxl_get_region_range(cxl->region, &range);
> > > +           if (ret)
> > > +                   goto failed;
> > > +
> > > +           cxl->region_hpa = range.start;
> > > +           cxl->region_size = range_len(&range);
> > > +
> > > +           pci_dbg(pdev, "Precommitted decoder: HPA 0x%llx size %lu
> MB\n",
> > > +                   cxl->region_hpa, cxl->region_size >> 20);
> > > +   } else {
> > > +           pci_err(pdev, "Failed to create CXL region\n");
> > > +           ret = -ENODEV;
> > > +           goto failed;
> > > +   }
> > > +
> > > +   return 0;
> > > +
> > > +failed:
> > > +   vfio_cxl_destroy_cxl_region(vdev);
> 
> Little bit of refactoring and this could be replaced with __free() magic.
> 
> > > +   return ret;
> > > +}
> > > +
> > >  /**
> > >   * vfio_pci_cxl_detect_and_init - Detect and initialize CXL Type-2
> device
> > >   * @vdev: VFIO PCI device
> > > @@ -172,6 +278,12 @@ void vfio_pci_cxl_detect_and_init(struct
> > > vfio_pci_core_device *vdev)
> > >
> > >     pci_disable_device(pdev);
> > >
> > > +   ret = vfio_cxl_create_region_helper(vdev, SZ_256M);
> >
> > Maybe a comment on why this size?
> :)  I wondered that as well.  I'm guessing your bios isn't always
> providing the decoder and this lets you test.

This was added for handling the patch organization so that the patches
are cut at a short reviewable boundary. It seems this patch is not 
cut correctly.

I have removed this temporary size and refactored the patches to
incorporate firmware committed decoder size here.

> 
> 
> >
> > DJ
> >
> > > +   if (ret)
> > > +           goto failed;
> > > +
> > > +   cxl->precommitted = true;
> > > +
> > >     return;
> > >
> > >  failed:
> > > @@ -181,6 +293,10 @@ void vfio_pci_cxl_detect_and_init(struct
> > > vfio_pci_core_device *vdev)
> > >
> > >  void vfio_pci_cxl_cleanup(struct vfio_pci_core_device *vdev)  {
> > > -   if (!vdev->cxl)
> > > +   struct vfio_pci_cxl_state *cxl = vdev->cxl;
> 
> Do that in the earlier patch to reduce churn a tiny bit.

Okay, agreed.

> 
> > > +
> > > +   if (!cxl || !cxl->region)
> > >             return;
> > > +
> > > +   vfio_cxl_destroy_cxl_region(vdev);
> > >  }