Re: [RFC PATCH] cxl/region: Allow out of order assembly of autodiscovered regions

[Alison Schofield <alison.schofield@intel.com>]

On Thu, Feb 08, 2024 at 02:57:44PM -0800, Dan Williams wrote:
> Alison Schofield wrote:
> > 
> > On Thu, Jan 18, 2024 at 11:46:44AM -0800, Dan Williams wrote:
> > 
> > - snip to decoder replay patch -
> > 
> > > 
> > > So one of the largest roadblocks for creating arbitrary region assembly
> > > scenarios with cxl_test is the inability to save and restore decoder
> > > settings.
> > > 
> > > The patch below adds support for recording decoder settings and skipping
> > > the reset of those values when unloading the driver. Then, when the
> > > driver is reloaded, it simulates the case of BIOS created CXL regions
> > > prior to OS boot.
> > > 
> > > We can go after even finer grained cases once the uunit effort settles,
> > > but in the meantime cxl_test can add auto-assembly regression tests with
> > > the current topology.
> > > 
> > > With the below you can simply trigger "cxl {en,dis}able-memdev" in the
> > > proper order to cause the violation.
> > > 
> > > -- >8 --
> > > From 5839392a3dbb64dbbac0630d930b1f48a8ef7ea6 Mon Sep 17 00:00:00 2001
> > > From: Dan Williams <dan.j.williams@intel.com>
> > > Date: Wed, 17 Jan 2024 20:56:20 -0800
> > > Subject: [PATCH] tools/testing/cxl: Add decoder save/restore support
> > > 
> > > Record decoder values at init and mock_decoder_commit() time, and
> > > restore them at the next invocation of mock_init_hdm_decoder(). Add 2
> > > attributes to the cxl_test "cxl_acpi" device to optionally flush the
> > > cache of topology decoder values, or disable updating the decoder at
> > > mock_decoder_reset() time.
> > > 
> > > This enables replaying a saved decoder configuration when re-triggering
> > > a topology scan by re-binding the cxl_acpi driver to "cxl_acpi.0" (the
> > > cxl_test emulation of an ACPI0017 instance).
> > 
> > Hi Dan,
> > 
> > Sorry it's taken a while to come back on this. I did find it useful
> > in testing the auto-assembly order issue as you suggested. 
> > 
> > I didn't use this one: &dev_attr_decoder_registry_invalidate.attr,
> > I just reloaded the cxl-test module to do same.
> 
> Makes sense, that can go.
> 
> > This I used:  &dev_attr_decoder_registry_reset_disable.attr,
> > with your decoders state fixup to set CXL_DECODER_STATE_AUTO,
> > and a work-around to avoid pmem_probe failures on pmem region
> > auto create. 
> > 
> > More generally, I'm wondering about the implementation of the
> > 'registry_save'. Here it continuously updates during all
> > cxl-test usage. Did you consider only creating the registry upon
> > user request and then at the next mock_init_hmd_decoder() look
> > for and use that registry if it exists.
> > 
> > Usage would be: 
> > # echo 1 > /sys/bus/platform/devices/cxl_acpi.0/decoder_registry_create
> 
> You mean wait to snapshot decoder state when triggered?
Yes.

> 
> > # echo cxl_acpi.0 > /sys/bus/platform/drivers/cxl_acpi/unbind
> > # echo cxl_acpi.0 > /sys/bus/platform/drivers/cxl_acpi/bind
> > 
> > And then maybe, for folks who like to acpi/unbind,bind, rather
> > than reload module, we could offer a decoder_registry_remove attr.
> > 
> > Am I missing something regarding the need to keep it updated
> > on the fly? 
> 
> I can see it being an alternate way, but not sure how to weigh one
> approach vs the other. Is the dynamic update getting in the way of a
> test case you are thinking about? Otherwise it seemed easy to reason
> that the registry is always on, but only takes effect when
> registry_reset_disable is set, and cxl_acpi rebinds the test device.

The constant work of updating the registry caught my attention, but
no impact that I know of. The dynamic approach is more intrusive and
impacts the normal path needlessly. A snapshot approach limits much
of the impact to users of the new feature.