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

["Dan Williams (nvidia)" <djbw@kernel.org>]

mhonap@ wrote:
> From: Manish Honap <mhonap@nvidia.com>
> 
> 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 <mhonap@nvidia.com>
> ---
>  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.

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