RE: [PATCH v2 2/3] PCI: Create new reset method to force SBR for CXL

[Dan Williams <dan.j.williams@intel.com>]

Vikram Sethi wrote:
> Hi Dan,
> 
> > Vishal Aslot wrote:
> > > Hi,
> > >
> > > For T2 and T3 persistent memory devices, wouldn’t we also need a way
> > > to trigger device cache flush and then disable out of
> > > cxl_reest_bus_function()?
> > >
> > > CXL Spec 3.1 (Aug ’23), Section 9.3 which refers to system reset flow
> > > has RESETPREP VDMs to trigger device cache flush, put memory in safe
> > > state, etc. These devices would benefit from this in case of SBR as
> > > well, but it is root port specific so may be an ACPI method could be
> > > involved out of cxl_reset_bus_function()?
> > 
> > In short, no, OS initiated device-cache-flush is not indicated, nor possible (GPF
> > has no mechanism for system-software trigger) for this case.
> > 
> > Specifically that section states:
> > 
> > "...it is expected that the CXL devices are already in an Inactive State with their
> > contexts flushed to the system memory or CXL-attached memory before the
> > platform reset flow is triggered"
> > 
> > ...so if reset is triggered while the device is mapped and active then the
> > administrator gets to keep all the pieces. This SBR enabling is all about making
> > sure the kernel log reflects when the administrator messed up and triggered
> > reset while the device had active decoders.
> 
> For a .cache capable device, shouldn't the kernel be writing to the
> device CXL Control2 register " Initiate cache writeback and
> Invalidation", as part of the "OS orchestrated reset flow"?

For a CXL.cache capable initiator, since there is no generic driver
model for that I would expect that responsibility to fall to endpoint
drivers to implement in their reset_prepare callbacks. Otherwise I would
expect the device to be already "Inactive" prior to reset.

> CXL reset, the link is going down in SBR case, so the device has no
> chance of doing the writeback of dirty system memory lines it holds.

For suprise reset, sure, but drivers can always trap reset_prepare.

> Hence OS must do it prior to the SBR issuance.

"OS" is one of userspace device idling, accelerator driver, or PCI core.
I think if userspace fails to idle the device, then it is up to the
accelerator driver to handle reset while the device is not idle, the PCI
core should likely not be burdended with this per-device / optional
CXL-ism around reset.

> that the only 'supported'/workable SBR for such a device would include
> previously offlining its memory and unloading its driver, and part of
> that step would be driver code doing the device cache WB+invalidate?

That certainly is the expectation for CXL-memory-expanders, so when
accelerator drivers arrive they need to consider that this will not be
done automatically on their behalf.

> I think that additionally, kernel should also be doing a host cache
> flush here to WB+invalidate dirty Device owned/homed lines cached in
> the host CPU, to handle the previously discussed scenario of device
> snoop filter being reset as part of reset, but not expecting future
> WBs from host, and raising errors if that were to happen.

Again that is an accelerator specific responsibility in my mind, and
ideally the device handles this with its own back-invalidate given the
difficulties of wielding instructions like wbinvd (on x86 at least).