On Thu Jan 16, 2025 at 10:32 AM CET, Dust Li wrote: > On 2025-01-15 20:55:20, Alexandra Winter wrote: > > Hi Winter, > > I'm fully supportive of the refactor! > > Interestingly, I developed a similar RFC code about a month ago while > working on enhancing internal communication between guest and host > systems. Here are some of my thoughts on the matter: > > Naming and Structure: I suggest we refer to it as SHD (Shared Memory > Device) instead of ISM (Internal Shared Memory). To my knowledge, a > "Shared Memory Device" better encapsulates the functionality we're > aiming to implement. It might be beneficial to place it under > drivers/shd/ and register it as a new class under /sys/class/shd/. That > said, my initial draft also adopted the ISM terminology for simplicity. I'm not sure if we really want to introduce a new name for the already existing ISM device. For me, having two names for the same thing just adds additional complexity. I would go for /sys/class/ism > > Modular Approach: I've made the ism_loopback an independent kernel > module since dynamic enable/disable functionality is not yet supported > in SMC. Using insmod and rmmod for module management could provide the > flexibility needed in practical scenarios. > > Abstraction of ISM Device Details: I propose we abstract the ISM device > details by providing SMC with helper functions. These functions could > encapsulate ism->ops, making the implementation cleaner and more > intuitive. This way, the struct ism_device would mainly serve its > implementers, while the upper helper functions offer a streamlined > interface for SMC. > > Structuring and Naming: I recommend embedding the structure of ism_ops > directly within ism_dev rather than using a pointer. Additionally, > renaming it to ism_device_ops could enhance clarity and consistency. > > > >This RFC is about providing a generic shim layer between all kinds of > >ism devices and all kinds of ism users. > > > >Benefits: > >- Cleaner separation of ISM and SMC-D functionality > >- simpler and less module dependencies > >- Clear interface definition. > >- Extendable for future devices and clients. > > Fully agree. > > > > >Request for comments: > >--------------------- > >Any comments are welcome, but I am aware that this series needs more work. > >It may not be worth your time to do an in-depth review of the details, I am > >looking for feedback on the general idea. > >I am mostly interested in your thoughts and recommendations about the general > >concept, the location of net/ism, the structure of include/linux/ism.h, the > >KConfig and makefiles. > > > >Status of this RFC: > >------------------- > >This is a very early RFC to ask you for comments on this general idea. > >The RFC does not fullfill all criteria required for a patchset. > >The whole set compiles and runs, but I did not try all combinations of > >module and built-in yet. I did not check for checkpatch or any other checkers. > >Also I have only done very rudimentary quick tests of SMC-D. More testing is > >required. > > > >Background / Status quo: > >------------------------ > >Currently s390 hardware provides virtual PCI ISM devices (ism_vpci). Their > >driver is in drivers/s390/net/ism_drv.c. The main user is SMC-D (net/smc). > >ism_vpci driver offers a client interface so other users/protocols > >can also use them, but it is still heavily intermingled with the smc code. > >Namely, the ISM vPCI module cannot be used without the SMC module, which > >feels artificial. > > > >The ISM concept is being extended: > >[1] proposed an ISM loopback interface (ism_lo), that can be used on non-s390 > >architectures (e.g. between containers or to test SMC-D). A minimal implementation > >went upstream with [2]: ism_lo currently is a part of the smc protocol and rather > >hidden. > > > >[3] proposed a virtio definition of ISM (ism_virtio) that can be used between > >kvm guests. > > > >We will shortly send an RFC for an ISM client that uses ISM as transport for TTY. > > > >Concept: > >-------- > >Create a shim layer in net/ism that contains common definitions and code for > >all ism devices and all ism clients. > >Any device or client module only needs to depend on this ism layer module and > >any device or client code only needs to include the definitions in > >include/linux/ism.h > > > >Ideas for next steps: > >--------------------- > >- sysfs representation? e.g. as /sys/class/ism ? > >- provide a full-fledged ism loopback interface > > (runtime enable/disable, sysfs device, ..) > > I think it's better if we can make this common for all ISM devices. > but yeah, that shoud be the next step. I already have patches based on this series that introduce /sys/class/ism and show ism-loopback as well as s390/ism devices. I can send this soon. Julian
