> -----Original Message----- > From: Dan Williams <dan.j.williams@xxxxxxxxx> > Sent: Tuesday, May 21, 2024 4:24 PM > To: Vikram Sethi <vsethi@xxxxxxxxxx>; Dan Williams > <dan.j.williams@xxxxxxxxx>; Vishal Aslot <os.vaslot@xxxxxxxxx>; > dave.jiang@xxxxxxxxx > Cc: Jonathan.Cameron@xxxxxxxxxx; alison.schofield@xxxxxxxxx; > bhelgaas@xxxxxxxxxx; dave@xxxxxxxxxxxx; ira.weiny@xxxxxxxxx; linux- > cxl@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; lukas@xxxxxxxxx; > vishal.l.verma@xxxxxxxxx; Vikram Sethi <vikramsethi@xxxxxxxxx> > Subject: RE: [PATCH v2 2/3] PCI: Create new reset method to force SBR for CXL > > > 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. > It could certainly be done that way, but also seems like common functionality, so wouldn't it be better to handle that in the "core/bus" driver, rather than each endpoint driver to be bit banging standard registers for standardized resets? Perhaps minimally some exported functions that could be called by endpoint drivers. Another thing I've been thinking about recently is what the responsibilities of the CXL core/bus driver are around the equivalent of PCIe Bus mastering enable (BME) and shutdown/kexec paths for CXL.cache. It's been a while since I looked at that code, but IIRC for PCIe, Root Port BME gets cleared as part of shutdown/kexec paths. This can prevent crashes due to errant DMA in shutdown/kexec flows, even if the endpoint driver didn't disable its own BME in its shutdown callback. A CXL host bridge would need to disable both BME for CXL.IO, and also CXL.cache for .cache capable devices. Unfortunately, the naming and control of the ".cache disable" is a bit convoluted on the CXL host bridge side and doesn't match the endpoint register naming. The CXL "Root Port n security policy" register in the CXL Extended Security capability structure allows for setting the Device trust Level =2 which results in CXL.cache requests being aborted by the host, which is roughly equivalent to RP BME disable on the PCIe/CXL.IO side. Do you agree this is something the core/bus driver must do since it is controlling the host bridge/RP registers and the host must protect itself against errant DMA from devices? There may be other similar usecases. Just thought I'd bring it up, that one can't purely think of .cache as an endpoint driver thing with no services provided by the core. I can certainly see the point that endpoint drivers must orchestrate their own standard controls by calling common exported services provided by a common layer in their own callbacks, which could include device side .cache disable and BME disable as part of both shutdown and reset_prepare callbacks. > > 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).
