| Message ID | 20240718213446.1750135-1-dave.jiang@intel.com |
|---|---|
| Headers | show |
| Series | fwctl/cxl: Add CXL feature commands support via fwctl | expand |
On 7/18/24 22:32, Dave Jiang 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. > > 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(-) > I can not see any documentation change in that file list. I would say for something like what this patchset does, it is mandatory. Isn't it?
On 7/18/24 11:23 PM, Alejandro Lucero Palau wrote: > > On 7/18/24 22:32, Dave Jiang 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. >> >> 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(-) >> > > I can not see any documentation change in that file list. > > I would say for something like what this patchset does, it is mandatory. Isn't it? Yeah I will write some documentation to detail the new ioctl API. Thanks for the reminder. >
On Thu, Jul 18, 2024 at 02:32:18PM -0700, Dave Jiang 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. This looks pretty good, I think it shows my general feeling that it might be better to have these kinds of things in a structured framework with a documented security policy then ad-hoc in ioctls with every subsystem.. Thanks, Jason
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 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(-)
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(-) > >
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. Maybe we can sanitize the return of the get features command on the kernel side and block what we don't want the user to see. That way we can theoretically "remove" things that's within the kernel's control right? DJ > > 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 > > 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(-) >
On Tue, 12 Nov 2024 17:17:11 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > 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. > > Maybe we can sanitize the return of the get features command on the kernel side and block what we don't want the user to see. That way we can theoretically "remove" things that's within the kernel's control right? Hmm. That sort of works, though I think it still would conventionally be ABI breakage if we made a feature disappear between kernel versions. Hopefully we are find doing that in the corner case that we don't have visibility of a future need to remove a feature from what get feature reports. J > > DJ > > > > > > 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 > > > > 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(-) > > > >
On 11/19/24 4:43 AM, Jonathan Cameron wrote: > On Tue, 12 Nov 2024 17:17:11 -0700 > Dave Jiang <dave.jiang@intel.com> wrote: > >> 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. >> >> Maybe we can sanitize the return of the get features command on the kernel side and block what we don't want the user to see. That way we can theoretically "remove" things that's within the kernel's control right? > > Hmm. That sort of works, though I think it still would conventionally be > ABI breakage if we made a feature disappear between kernel versions. > > Hopefully we are find doing that in the corner case that we don't have > visibility of a future need to remove a feature from what get feature > reports. Please take a look at the implementation in v2 and see if that would work. To me given that the features are discovery based and we filter the output of "get supported features" command via the kernel, user app should only issue command after verified via discovery and not hard code support. > > J >> >> DJ >> >> >>> >>> 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 >>> >>> 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(-) >>> >> >> >
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. 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(-)