RE: [PATCH v3 07/11] vfio/pci: Add CONFIG_VFIO_PCI_CXL with bind-time CXL Type-2 acquisition

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

Manish Honap wrote:
[..]
> > > vfio_pci_cxl_acquire() implements the bind sequence:
> > >
> > >   - pcie_is_cxl() and CXL Device DVSEC discovery (-ENODEV if absent
> > >     or if MEM_CAPABLE clear — caller falls back to plain vfio-pci)
> > >   - devm_cxl_dev_state_create() with struct vfio_pci_cxl_state
> > >     embedding cxl_dev_state at offset 0 (required by the 7-arg
> > >     macro's static_assert in include/cxl/cxl.h)
> > >   - pci_enable_device_mem(), cxl_pci_setup_regs(), cxl_get_hdm_info()
> > >     (rejecting hdm_count != 1), cxl_regblock_get_bar_info(),
> > >     cxl_await_range_active()
> > 
> > The cover letter claims otherwise:
> > 
> >   "- cxl_await_range_active stays in cxl-core probe; not exported, vfio
> >   does not call it."
> > 
> > It's exported in 2/ and called below.

This feels like a gap on the CXL side. devm_cxl_probe_mem() should take
care to only succeed after memory is active, or fail with a timeout.

Recall that the current cxl_await_range_active() export is for the
"internal" cxl_pci driver. Now that there are external consumers
devm_cxl_probe_mem() should return a fully validated and awaited memory
range. Do not make every driver remember this part of the setup flow.

> > >   - devm_cxl_passthrough_create() to snapshot the DVSEC body, HDM
> > >     block, and CM cap-array shadows owned by cxl-core
> > >   - pci_disable_device() — clears PCI_COMMAND_MASTER but NOT
> > >     PCI_COMMAND_MEMORY, so cxl-core MMIO accesses from the next step
> > >     still succeed
> > >   - devm_cxl_probe_mem() to register the cxl_memdev, enumerate the
> > >     endpoint port, and attach the firmware-committed autoregion
> > >   - request_mem_region() + memremap_wb() of the autoregion's HPA so
> > >     the HDM VFIO region can serve guest accesses through it
> > 
> > How does this interact with:
> > 
> >  - The device making use of low power states while idle
> >  - Repeatability per tenant instance
> >  - Protection of tenant data per instance
> > 
> > The culmination of all of these, plus the basic housekeeping of
> > maintaining the lightest touch on the device, including keeping the
> > device in the minimum state of functionality outside of an actual user,
> > is why I would expect to perform acquire/release as part of open/close.
> 
> The cover letter description is wrong. During my earlier thought process,
> creating a region during probe seemed a correct option as any later requirement
> where region needs to be already created at probe emerged, we will have easier
> way to handle it. I will give some more thought to update v4 to acquire
> CXL state in open_device() and release it in close_device(), matching zpci.

This is going to run up against a current design wart of
devm_cxl_probe_mem() that does not have a teardown flow implemented
outside of the typical ->remove() to devres_release_all() flow. Even if
that was fixed I do not see it affecting the listed concerns.
devm_cxl_probe_mem() for firmware committed decoders is mostly just
reading resource values.

For power state management the power management that vfio-pci performs
only informs the CXL.io portion of device power. CXL.cache and CXL.mem
are dynamically managed by the device.

For repeatability I do think discarding and reinitializing the shadow
configuration makes sense, but the base HDM configuration does not
require teardown.

For tenant data protection that arises from the data retention after
reset policy, not impacted by unloading the HDM configuration.

I assume VFIO likely would want to build a capability to zero HDM memory
if the device leaves the state ambiguous / implementation defined?

That is a good note for the CXL Reset series as well. If VFIO issued
resets are for tenant handoff the make sure that CXL Reset requests
memory clearing.