Re: [RFC PATCH 0/13] fwctl/cxl: Add CXL feature commands support via fwctl

Dave Jiang
2024-08-06T16:44:49Z

On 7/29/24 5:05 AM, Jonathan Cameron wrote:
> On Thu, 18 Jul 2024 14:32:18 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
>
>> This series add support for CXL feature commands using the FWCTL framework [1].
>> The code is untested and I'm looking for architectural and implementation feedback.
>> While CXL currently has a chardev for user ioctls to send some mailbox
>> commands to a memory device, the fwctl framework provides more security policies
>> that can be a potential vehicle to move CXL ioctl path to that.
>>
>> For this RFC, the mailbox commands "Get Supported Features", "Get Feature", and
>> "Set Feature" commands are implemented. The "get" commands under the
>> FWCTL_RPC_DEBUG_READ_ONLY policy, the "set" command checks the policy depending
>> on the effect of the feature. All mailbox commands for CXL provides an effects
>> table that describes the effects of a command when performed on the device.
>> For CXL features, there is also an effects field that describes the effects
>> a feature write operation has on the device per feature. The security policy
>> is checked against this feature specific effects field. Looking for discussion
>> on matching the CXL spec defined effects with the FWCTL security policy.
>
> Hi Dave,
>
> Great to have this code.
>
> My main concern is that, once a feature is exposed by fwctl, it becomes ABI.
> Even with the taint, does that mean we can't remove it later? We
> may well have userspace code using fwctl that will break kernel driver
> support.
>
> As you probably know, for the scrub control you are using as an example,
> there is an ongoing effort to generalize that across multiple subsystems.
> It's a convenient test as a simple get/set feature in CXL, so maybe
> I'm reading too much into that choice.
>
> One piece of good feedback we got on that generalization effort was that
> this sort of RAS control should all be in one place (we were proposing
> a RAS control subsystem at the times, separate from EDAC). The key reason
> being to ensure it gets sufficient review by experts in the RAS aspects
> (rather than CXL or other implementation).
>
> Shiju's latest series puts it all in /sys/bus/edac/
> https://lore.kernel.org/linux-cxl/20240726160556.2079-1-shiju.jose@huawei.com/T/#t

I didn't see Shiju's patches until I created the RFC. For v1 I just wanted to post upstream to show intent and get some discussion going. I did let Shiju know that I'll pull in his code in a later rev. I expect a lot of churn with this code as we discuss.

It does seem like we may need a per feature (write) list/mask that is exclusive to kernel such as scrub? Also it looks like I need to find a different cxl spec defined feature to use for testing fwctl.

>
> Even if we make the driver safe to someone else messing with the state under it
> (so basically make it stateless) we can't make the same guarantees for any
> user space code on top of that interface. Can we rely on only one path ever
> being used? I'm not sure.
>
> We could in theory decide to expose all the stuff proposed for the EDAC / rascontrol
> as fwctl only, but that's going to get nasty as next set of features we plan to
> implement are memory repair related and in at least some cases those require the
> memory to be offlined if you don't want nasty side effects. I don't think there
> will be appetite for that sort of thing via fwctl.
>
> There are a bunch of other get/set features in the CXL spec and I expect to see
> more over time so I think this will be an ongoing discussion.
>
> Jonathan
>
> +CC Edac maintainers etc, as they may have views on using fwctl for this stuff.
>
>
>
>>
>> The code is based off of the latest FWCTL series [1] posted by Jason on top of v6.10.
>>
>> [1]: https://lore.kernel.org/linux-cxl/20240624161802.1b7c962d@kernel.org/T/#t
>>
>> ---
>>
>> Dave Jiang (13):
>> cxl: Move mailbox related bits to the same context
>> cxl: Fix comment regarding cxl_query_cmd() return data
>> cxl: Refactor user ioctl command path from mds to mailbox
>> cxl: Add Get Supported Features command for kernel usage
>> cxl/test: Add Get Supported Features mailbox command support
>> cxl: Add Get Feature command support
>> cxl: Add Set Feature command support
>> fwctl/cxl: Add driver for CXL mailbox for handling CXL features commands
>> fwctl/cxl: Add support for get driver information
>> fwctl/cxl: Add support for fwctl RPC command to enable CXL feature commands
>> fwctl/cxl: Add query commands software command for ->fw_rpc()
>> cxl/test: Add Get Feature support to cxl_test
>> cxl/test: Add Set Feature support to cxl_test
>>
>> MAINTAINERS | 8 +
>> drivers/cxl/core/core.h | 9 +-
>> drivers/cxl/core/mbox.c | 607 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---------
>> drivers/cxl/core/memdev.c | 78 +++++---
>> drivers/cxl/cxlmem.h | 141 +++------------
>> drivers/cxl/pci.c | 68 ++++---
>> drivers/cxl/pmem.c | 10 +-
>> drivers/cxl/security.c | 18 +-
>> drivers/fwctl/Kconfig | 9 +
>> drivers/fwctl/Makefile | 1 +
>> drivers/fwctl/cxl/Makefile | 4 +
>> drivers/fwctl/cxl/cxl.c | 274 ++++++++++++++++++++++++++++
>> include/linux/cxl/mailbox.h | 175 ++++++++++++++++++
>> include/uapi/fwctl/cxl.h | 92 ++++++++++
>> include/uapi/fwctl/fwctl.h | 1 +
>> include/uapi/linux/cxl_mem.h | 3 +
>> tools/testing/cxl/test/mem.c | 292 +++++++++++++++++++++++++++++-
>> 17 files changed, 1515 insertions(+), 275 deletions(-)
>
>