Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
> > Hi Andrew, > > > > We're waiting for the DPIO driver (which we depend on) to be moved > > out of staging first, it's currently under review: > > https://lkml.org/lkml/2018/3/27/1086 > > That's stalled on my side right now as the merge window is open and I > can't do any new stuff until after 4.17-rc1 is out. So everyone please > be patient a bit... I took a quick look. There are a few inline functions in .c files which is generally frowned upon. Let the compiler decide. e.g: static inline struct dpaa2_io *service_select_by_cpu(struct dpaa2_io *d, int cpu) static inline struct dpaa2_io *service_select(struct dpaa2_io *d) dpaa2_io_down() seems to be too simple. dpaa2_io_create() sets up interrupt triggers, notifications, and adds the new object to the dpio_list. dpaa2_io_down() seems to just free the memory. Do notifications need to be disabled, the object taken off the list? dpaa2_io_store_create() allocates memory using kzalloc() and then uses dma_map_single(,,DMA_FROM_DEVICE). The documentation says: DMA_FROM_DEVICE synchronisation must be done before the driver accesses data that may be changed by the device. This memory should be treated as read-only by the driver. If the driver needs to write to it at any point, it should be DMA_BIDIRECTIONAL (see below). Since it has just been allocated, this seems questionable. I'm also not sure where the correct call to dma_map_single(,,DMA_FROM_DEVICE) is? Should dpaa2_io_store_next() doing this? The DMA API usage might need a closer review. Andrew
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
On Thu, Apr 05, 2018 at 03:35:30PM +, Ruxandra Ioana Ciocoi Radulescu wrote: > > -Original Message- > > From: Andrew Lunn [mailto:and...@lunn.ch] > > Sent: Thursday, April 5, 2018 6:24 PM > > To: Laurentiu Tudor > > Cc: Stuart Yoder ; Arnd Bergmann ; > > Ioana Ciornei ; gregkh > > ; Linux Kernel Mailing List > ker...@vger.kernel.org>; Ruxandra Ioana Ciocoi Radulescu > > ; Razvan Stefanescu > > ; Roy Pledge ; > > Networking > > Subject: Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support > > > > On Thu, Apr 05, 2018 at 02:43:29PM +, Laurentiu Tudor wrote: > > > Hi Andrew, > > > > > > On 04/05/2018 03:48 PM, Andrew Lunn wrote: > > > >>> Hi Laurentiu > > > >>> > > > >>> So i can use switchdev without it? I can modprobe the switchdev > > > >>> driver, all the physical interfaces will appear, and i can use ip addr > > > >>> add etc. I do not need to use a user space tool at all in order to use > > > >>> the network functionality? > > > >> > > > >> Absolutely! > > > > > > > > Great. > > > > > > > > Then the easiest way forwards is to simply drop the IOCTL code for the > > > > moment. Get the basic support for the hardware into the kernel > > > > first. Then come back later to look at dynamic behaviour which needs > > > > some form of configuration. > > > > > > Hmm, not sure I understand. We already have a fully functional ethernet > > > driver [1] and a switch driver [2] ... > > > > In staging, the tree of crap. You want to get it out of there and into > > the main tree. But that effort is being side lined by the discussion > > around this IOCTL call. The best way forward is to to accept Greg is > > not going to take this patchset at the moment, and move on. As you > > said, it is not needed for the Ethernet and switchdev driver. > > > > What needs to happen before the Ethernet driver can be reviewed for > > moving out of staging? > > Hi Andrew, > > We're waiting for the DPIO driver (which we depend on) to be moved > out of staging first, it's currently under review: > https://lkml.org/lkml/2018/3/27/1086 That's stalled on my side right now as the merge window is open and I can't do any new stuff until after 4.17-rc1 is out. So everyone please be patient a bit... thanks, greg k-h
RE: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
> -Original Message- > From: Andrew Lunn [mailto:and...@lunn.ch] > Sent: Thursday, April 5, 2018 6:24 PM > To: Laurentiu Tudor > Cc: Stuart Yoder ; Arnd Bergmann ; > Ioana Ciornei ; gregkh > ; Linux Kernel Mailing List ker...@vger.kernel.org>; Ruxandra Ioana Ciocoi Radulescu > ; Razvan Stefanescu > ; Roy Pledge ; > Networking > Subject: Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support > > On Thu, Apr 05, 2018 at 02:43:29PM +, Laurentiu Tudor wrote: > > Hi Andrew, > > > > On 04/05/2018 03:48 PM, Andrew Lunn wrote: > > >>> Hi Laurentiu > > >>> > > >>> So i can use switchdev without it? I can modprobe the switchdev > > >>> driver, all the physical interfaces will appear, and i can use ip addr > > >>> add etc. I do not need to use a user space tool at all in order to use > > >>> the network functionality? > > >> > > >> Absolutely! > > > > > > Great. > > > > > > Then the easiest way forwards is to simply drop the IOCTL code for the > > > moment. Get the basic support for the hardware into the kernel > > > first. Then come back later to look at dynamic behaviour which needs > > > some form of configuration. > > > > Hmm, not sure I understand. We already have a fully functional ethernet > > driver [1] and a switch driver [2] ... > > In staging, the tree of crap. You want to get it out of there and into > the main tree. But that effort is being side lined by the discussion > around this IOCTL call. The best way forward is to to accept Greg is > not going to take this patchset at the moment, and move on. As you > said, it is not needed for the Ethernet and switchdev driver. > > What needs to happen before the Ethernet driver can be reviewed for > moving out of staging? Hi Andrew, We're waiting for the DPIO driver (which we depend on) to be moved out of staging first, it's currently under review: https://lkml.org/lkml/2018/3/27/1086 Ioana
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
On Thu, Apr 05, 2018 at 02:43:29PM +, Laurentiu Tudor wrote: > Hi Andrew, > > On 04/05/2018 03:48 PM, Andrew Lunn wrote: > >>> Hi Laurentiu > >>> > >>> So i can use switchdev without it? I can modprobe the switchdev > >>> driver, all the physical interfaces will appear, and i can use ip addr > >>> add etc. I do not need to use a user space tool at all in order to use > >>> the network functionality? > >> > >> Absolutely! > > > > Great. > > > > Then the easiest way forwards is to simply drop the IOCTL code for the > > moment. Get the basic support for the hardware into the kernel > > first. Then come back later to look at dynamic behaviour which needs > > some form of configuration. > > Hmm, not sure I understand. We already have a fully functional ethernet > driver [1] and a switch driver [2] ... In staging, the tree of crap. You want to get it out of there and into the main tree. But that effort is being side lined by the discussion around this IOCTL call. The best way forward is to to accept Greg is not going to take this patchset at the moment, and move on. As you said, it is not needed for the Ethernet and switchdev driver. What needs to happen before the Ethernet driver can be reviewed for moving out of staging? Andrew
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
Hi Andrew, On 04/05/2018 03:48 PM, Andrew Lunn wrote: >>> Hi Laurentiu >>> >>> So i can use switchdev without it? I can modprobe the switchdev >>> driver, all the physical interfaces will appear, and i can use ip addr >>> add etc. I do not need to use a user space tool at all in order to use >>> the network functionality? >> >> Absolutely! > > Great. > > Then the easiest way forwards is to simply drop the IOCTL code for the > moment. Get the basic support for the hardware into the kernel > first. Then come back later to look at dynamic behaviour which needs > some form of configuration. Hmm, not sure I understand. We already have a fully functional ethernet driver [1] and a switch driver [2] ... >> In normal use cases the system designer, depending on the requirements, >> configures the various devices that it desires through a firmware >> configuration (think something like a device tree). The devices >> configured are presented on the mc-bus and probed normally by the >> kernel. The standard networking linux tools can be used as expected. > > So what you should probably do is start a discussion on what this > device tree binding looks like. But you need to be careful even > here. Device tree describes the hardware, not how you configure the > hardware. So maybe DT does not actually fit. It's not an actual device tree, but a configuration file that happens to reuse the DTS format. I guess my analogy with a device tree was not the best. Detailed documentation on the syntax can be found here [3], chapter 22. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/fsl-dpaa2/ethernet [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/fsl-dpaa2/ethsw [3] https://www.nxp.com/docs/en/user-guide/DPAA2_UM.pdf --- Best Regards, Laurentiu
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
On Thu, Apr 05, 2018 at 02:09:47PM +, Laurentiu Tudor wrote: > Hi Greg, > > On 04/05/2018 03:30 PM, gregkh wrote: > > On Thu, Apr 05, 2018 at 10:30:01AM +, Laurentiu Tudor wrote: > >> Hello, > >> > >> My 2c below. > >> > >> On 04/04/2018 03:42 PM, Andrew Lunn wrote: > I hear you. It is more complicated this way...having all these > individual > objects vs just a single "bundle" of them that represents a NIC. But, > that's > the way the DPAA2 hardware is, and we're implementing kernel support for > the hardware as it is. > >>> > >>> Hi Stuart > >>> > >>> I see we are not making any progress here. > >>> > >>> So what i suggest is you post the kernel code and configuration tool > >>> concept to netdev for a full review. You want reviews from David > >>> Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc. > >>> > >> > >> I think that the discussion steered too much towards networking related > >> topics, while this ioctl doesn't have much to do with networking. > >> It's just an ioctl for our mc-bus bus driver that is used to manage the > >> devices on this bus through userspace tools. > >> In addition, I'd drop any mention of our reference user space app > >> (restool) to emphasize that this ioctl is not added just for a > >> particular user space app. I think Stuart also mentioned this. > > > > I'm not going to take a "generic device configuration ioctl" patch > > unless it is documented to all exactly what it does, and why it is > > there. > > The ioctl() is just a simple pass-through interface to the firmware. Ah, so a new syscall? :) > It passes commands to the firmware and returns the response back to the > userspace. Thus the ABI used by the firmware applies for this ioctl() > and it is documented in detail here: > > https://www.nxp.com/docs/en/user-guide/DPAA2_UM.pdf Let's wait on this until people all agree that it's ok to expose this directly. thanks, greg k-h
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
Hi Greg, On 04/05/2018 03:30 PM, gregkh wrote: > On Thu, Apr 05, 2018 at 10:30:01AM +, Laurentiu Tudor wrote: >> Hello, >> >> My 2c below. >> >> On 04/04/2018 03:42 PM, Andrew Lunn wrote: I hear you. It is more complicated this way...having all these individual objects vs just a single "bundle" of them that represents a NIC. But, that's the way the DPAA2 hardware is, and we're implementing kernel support for the hardware as it is. >>> >>> Hi Stuart >>> >>> I see we are not making any progress here. >>> >>> So what i suggest is you post the kernel code and configuration tool >>> concept to netdev for a full review. You want reviews from David >>> Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc. >>> >> >> I think that the discussion steered too much towards networking related >> topics, while this ioctl doesn't have much to do with networking. >> It's just an ioctl for our mc-bus bus driver that is used to manage the >> devices on this bus through userspace tools. >> In addition, I'd drop any mention of our reference user space app >> (restool) to emphasize that this ioctl is not added just for a >> particular user space app. I think Stuart also mentioned this. > > I'm not going to take a "generic device configuration ioctl" patch > unless it is documented to all exactly what it does, and why it is > there. The ioctl() is just a simple pass-through interface to the firmware. It passes commands to the firmware and returns the response back to the userspace. Thus the ABI used by the firmware applies for this ioctl() and it is documented in detail here: https://www.nxp.com/docs/en/user-guide/DPAA2_UM.pdf --- Best Regards, Laurentiu
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
> > Hi Laurentiu > > > > So i can use switchdev without it? I can modprobe the switchdev > > driver, all the physical interfaces will appear, and i can use ip addr > > add etc. I do not need to use a user space tool at all in order to use > > the network functionality? > > Absolutely! Great. Then the easiest way forwards is to simply drop the IOCTL code for the moment. Get the basic support for the hardware into the kernel first. Then come back later to look at dynamic behaviour which needs some form of configuration. > In normal use cases the system designer, depending on the requirements, > configures the various devices that it desires through a firmware > configuration (think something like a device tree). The devices > configured are presented on the mc-bus and probed normally by the > kernel. The standard networking linux tools can be used as expected. So what you should probably do is start a discussion on what this device tree binding looks like. But you need to be careful even here. Device tree describes the hardware, not how you configure the hardware. So maybe DT does not actually fit. Andrew
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
On Thu, Apr 05, 2018 at 10:30:01AM +, Laurentiu Tudor wrote: > Hello, > > My 2c below. > > On 04/04/2018 03:42 PM, Andrew Lunn wrote: > >> I hear you. It is more complicated this way...having all these individual > >> objects vs just a single "bundle" of them that represents a NIC. But, > >> that's > >> the way the DPAA2 hardware is, and we're implementing kernel support for > >> the hardware as it is. > > > > Hi Stuart > > > > I see we are not making any progress here. > > > > So what i suggest is you post the kernel code and configuration tool > > concept to netdev for a full review. You want reviews from David > > Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc. > > > > I think that the discussion steered too much towards networking related > topics, while this ioctl doesn't have much to do with networking. > It's just an ioctl for our mc-bus bus driver that is used to manage the > devices on this bus through userspace tools. > In addition, I'd drop any mention of our reference user space app > (restool) to emphasize that this ioctl is not added just for a > particular user space app. I think Stuart also mentioned this. I'm not going to take a "generic device configuration ioctl" patch unless it is documented to all exactly what it does, and why it is there. thanks, greg k-h
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
On 04/05/2018 02:47 PM, Andrew Lunn wrote: > On Thu, Apr 05, 2018 at 10:30:01AM +, Laurentiu Tudor wrote: >> Hello, >> >> My 2c below. >> >> On 04/04/2018 03:42 PM, Andrew Lunn wrote: I hear you. It is more complicated this way...having all these individual objects vs just a single "bundle" of them that represents a NIC. But, that's the way the DPAA2 hardware is, and we're implementing kernel support for the hardware as it is. >>> >>> Hi Stuart >>> >>> I see we are not making any progress here. >>> >>> So what i suggest is you post the kernel code and configuration tool >>> concept to netdev for a full review. You want reviews from David >>> Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc. >>> >> >> I think that the discussion steered too much towards networking related >> topics, while this ioctl doesn't have much to do with networking. > > Hi Laurentiu > > So i can use switchdev without it? I can modprobe the switchdev > driver, all the physical interfaces will appear, and i can use ip addr > add etc. I do not need to use a user space tool at all in order to use > the network functionality? Absolutely! In normal use cases the system designer, depending on the requirements, configures the various devices that it desires through a firmware configuration (think something like a device tree). The devices configured are presented on the mc-bus and probed normally by the kernel. The standard networking linux tools can be used as expected. The ioctl is necessary only for more advanced use cases that are supported by this bus. Think "more dynamic" scenarios that involve linking & unlinking various devices at runtime, maybe some virtualization scenarios. Unfortunately I'm not the architect type of guy so I don't have more specific examples to better illustrate ... --- Best Regards, Laurentiu
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
On Thu, Apr 05, 2018 at 10:30:01AM +, Laurentiu Tudor wrote: > Hello, > > My 2c below. > > On 04/04/2018 03:42 PM, Andrew Lunn wrote: > >> I hear you. It is more complicated this way...having all these individual > >> objects vs just a single "bundle" of them that represents a NIC. But, > >> that's > >> the way the DPAA2 hardware is, and we're implementing kernel support for > >> the hardware as it is. > > > > Hi Stuart > > > > I see we are not making any progress here. > > > > So what i suggest is you post the kernel code and configuration tool > > concept to netdev for a full review. You want reviews from David > > Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc. > > > > I think that the discussion steered too much towards networking related > topics, while this ioctl doesn't have much to do with networking. Hi Laurentiu So i can use switchdev without it? I can modprobe the switchdev driver, all the physical interfaces will appear, and i can use ip addr add etc. I do not need to use a user space tool at all in order to use the network functionality? Andrew
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
Hello, My 2c below. On 04/04/2018 03:42 PM, Andrew Lunn wrote: >> I hear you. It is more complicated this way...having all these individual >> objects vs just a single "bundle" of them that represents a NIC. But, that's >> the way the DPAA2 hardware is, and we're implementing kernel support for >> the hardware as it is. > > Hi Stuart > > I see we are not making any progress here. > > So what i suggest is you post the kernel code and configuration tool > concept to netdev for a full review. You want reviews from David > Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc. > I think that the discussion steered too much towards networking related topics, while this ioctl doesn't have much to do with networking. It's just an ioctl for our mc-bus bus driver that is used to manage the devices on this bus through userspace tools. In addition, I'd drop any mention of our reference user space app (restool) to emphasize that this ioctl is not added just for a particular user space app. I think Stuart also mentioned this. --- Thanks & Best Regards, Laurentiu
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
On Wed, Apr 4, 2018 at 7:42 AM, Andrew Lunn wrote: >> I hear you. It is more complicated this way...having all these individual >> objects vs just a single "bundle" of them that represents a NIC. But, that's >> the way the DPAA2 hardware is, and we're implementing kernel support for >> the hardware as it is. > > Hi Stuart > > I see we are not making any progress here. > > So what i suggest is you post the kernel code and configuration tool > concept to netdev for a full review. You want reviews from David > Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc. I know Ioana has other feedback she is addressing so a respin will be coming soon, and she can include those additional reviewers. Thanks, Stuart
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
> I hear you. It is more complicated this way...having all these individual > objects vs just a single "bundle" of them that represents a NIC. But, that's > the way the DPAA2 hardware is, and we're implementing kernel support for > the hardware as it is. Hi Stuart I see we are not making any progress here. So what i suggest is you post the kernel code and configuration tool concept to netdev for a full review. You want reviews from David Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc. Andrew
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
On Tue, Apr 3, 2018 at 8:05 PM, Andrew Lunn wrote: >> Suppose you want to create and assign a network interface to a KVM >> virtual machine, you would do something like the following using >> a user space tool like restool: >>-create a new (empty) dprc object >>-create a new dpni and assign it to the dprc >>-create a new dpio and assign it to the dprc >>-create a new dpbp and assign it to the dprc >>-create a new dpmcp and assign it to the dprc >>-create a new dpmac and assign it to the dprc >>-connect the dpni to the dpmac > > Hi Stuart > > It this connecting to a physical port at the bottom? Yes. > If so, i would expect that when you probe the device you just create > all these for each physical port. The problem is that there is not just one set of objects to implement a network interface. For the highest throughput packet processing you need one dpio per core. So, it will depend on what the requirements are. You might want multiple dpbp (buffer pools) and set up pools of different size buffers for different packet classifications. You might want to have other objects like a crypto accelerator (dpseci) in the container as well. The dprc is a container holding any combination of those objects. So you have complete flexibility. > You then just need to map one of > them into the KVM, in the same way you map one PCI device into a KVM. > > If these are virtual devices, VF devices you would normally do > > echo 4 > /sys/class/net//device/sriov_numvfs > > on the physical device to create virtual devices. > >> The fsl-mc bus and DPAA2 is very NXP-specific, so there doesn't >> seem to be anything that can be made generic here to provide >> more common benefit. > > Which is why you should try to avoid all of this. The user knows how > to use standard linux commands and concepts. They don't want to have > to learn the inside plumbing of your hardware. I hear you. It is more complicated this way...having all these individual objects vs just a single "bundle" of them that represents a NIC. But, that's the way the DPAA2 hardware is, and we're implementing kernel support for the hardware as it is. Thanks, Stuart
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
> Suppose you want to create and assign a network interface to a KVM > virtual machine, you would do something like the following using > a user space tool like restool: >-create a new (empty) dprc object >-create a new dpni and assign it to the dprc >-create a new dpio and assign it to the dprc >-create a new dpbp and assign it to the dprc >-create a new dpmcp and assign it to the dprc >-create a new dpmac and assign it to the dprc >-connect the dpni to the dpmac Hi Stuart It this connecting to a physical port at the bottom? If so, i would expect that when you probe the device you just create all these for each physical port. You then just need to map one of them into the KVM, in the same way you map one PCI device into a KVM. If these are virtual devices, VF devices you would normally do echo 4 > /sys/class/net//device/sriov_numvfs on the physical device to create virtual devices. > The fsl-mc bus and DPAA2 is very NXP-specific, so there doesn't > seem to be anything that can be made generic here to provide > more common benefit. Which is why you should try to avoid all of this. The user knows how to use standard linux commands and concepts. They don't want to have to learn the inside plumbing of your hardware. Andrew
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
On Wed, Mar 28, 2018 at 10:43 AM, Arnd Bergmann wrote: > On Wed, Mar 28, 2018 at 4:27 PM, Ioana Ciornei wrote: >> Hi, >> >>> >>> Hi Ioana, >>> >>> So this driver is a direct passthrough to your hardware for passing fixed- >>> length command/response pairs. Have you considered using a higher-level >>> interface instead? >>> >>> Can you list some of the commands that are passed here as clarification, and >>> explain what the tradeoffs are that have led to adopting a low-level >>> interface >>> instead of a high-level interface? >>> >>> The main downside of the direct passthrough obviously is that you tie your >>> user space to a particular hardware implementation, while a high-level >>> abstraction could in principle work across a wider range of hardware >>> revisions >>> or even across multiple vendors implementing the same concept by different >>> means. >> >> If by "higher-level" you mean an implementation where commands are created >> by the kernel at userspace's request, then I believe this approach is not >> really viable because of the sheer number of possible commands that would >> bloat the driver. >> >> For example, a DPNI (Data Path Network Interface) can be created using a >> command that has the following structure: >> >> struct dpni_cmd_create { >> uint32_t options; >> uint8_t num_queues; >> uint8_t num_tcs; >> uint8_t mac_filter_entries; >> uint8_t pad1; >> uint8_t vlan_filter_entries; >> uint8_t pad2; >> uint8_t qos_entries; >> uint8_t pad3; >> uint16_t fs_entries; >> }; >> >> In the above structure, each field has a meaning that the end-user might >> want to be able to change according to their particular use-case (not much >> is left at its default value). >> The same level of complexity is encountered for all the commands that >> interact with Data Path objects such as DPBP(buffer pools), DPRC(Resource >> Container) etc. >> You can find more examples of commands in restool's repo: >> https://github.com/qoriq-open-source/restool/tree/integration/mc_v10 >> >> In my opinion, an in-kernel implementation that is equivalent in terms of >> flexibility will turn >> into a giant ioctl parser, all while also exposing an userspace API that is >> not as simple/easy to use. > > (adding the netdev list) > > The command you list there seems to be networking related, so instead of > an ioctl based interface, a high-lever interface would likely use netlink > for consistency with other drivers. Are all commands for networking > or are there some that are talking to the device to do something unrelated? > > Obviously creating a high-level interface would be a lot of work in the > kernel, > and it only pays off if there are multiple independent users, we wouldn't do > that for just one driver. > > I'm still not convinced either way (high-level or low-level > interface), but I think > this needs to be discussed with the networking maintainers. Given the examples > on the github page you linked to, the high-level user space commands > based on these ioctls > >ls-addni # adds a network interface >ls-addmux # adds a dpdmux >ls-addsw # adds an l2switch >ls-listmac # lists MACs and their connections >ls-listni # lists network interfaces and their connections > > and I see that you also support the switchdev interface in > drivers/staging/fsl-dpaa2, which I think does some of the same > things, presumably by implementing the switchdev API using > fsl_mc_command low-level interfaces in the kernel. > > Is that a correct interpretation? If yes, could we extend switchdev > or other networking interfaces to fill in whatever those don't handle > yet? The wrapper scripts you referenced are not sufficient to show the scope of what the proposed user space interface is for. The command list is not just about networking related objects, as there are quite a few other types of objects as well: dprc - container object representing an fsl-mc bus instance...i.e. other objects are attached to this bus dpio - used for queuing operations towards any accelerator or network interface dpbp - buffer pool object dpmcp - command portal interface dpdmai - DMA engine dpseci - crypto accelerator dpdcei - compression/decompression accelerator dpni - network interface dprtc - real time counter dpaiop - heterogenous core complex for packet processing offload dpmac - represents an Ethernet MAC dpsw - network switch dpcon - network concentrator dpci - communication interface The proposed ioctl interface is about: A) creating and destroying all those object types B) managing the dprc containers they live in, including moving objects between containers C) where applicable establishing connections between different objects No _operational_ aspects of these object/devices is being handled through this interface. The network
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
On Tue, Apr 03, 2018 at 11:12:52AM +, Razvan Stefanescu wrote: > DPAA2 offers several object-based abstractions for modeling network > related devices (interfaces, L2 Ethernet switch) or accelerators > (DPSECI - crypto and DPDCEI - compression), the latter not up-streamed yet. > They are modeled using various low-level resources (e.g. queues, > classification tables, physical ports) and have multiple configuration and > interconnectivity options, managed by the Management Complex. > Resources are limited and they are only used when needed by the objects, > to accommodate more configurations and usage scenarios. > > Some of the objects have a 1-to-1 correspondence to physical resources > (e.g. DPMACs to physical ports), while others (like DPNIs and DPSW) > can be seen as a collection of the mentioned resources. The types and > number of such objects are not predetermined. > > When the board boots up, none of them exist yet. Restool allows a user to > define the system topology, by providing a way to dynamically create, destroy > and interconnect these objects. Hi Razvan The core concept with Linux networking and offload is that the hardware is there to accelerate what Linux can already do. Since Linux can already do it, i don't need any additional tools. You have new hardware. It might offer features which we currently don't have offload support for. But all the means is you need to extend the core networking code which implements the software version of that feature to offload to the hardware. The board knows how many physical ports it has. switchdev can then setup the plumbing to create the objects needed to represent the ports. Restool is not needed for that. > In the latter case, the two DPNIs will not be connected to any physical > port, but can be used as a point-to-point connection between two virtual > machines for instance. Can Linux already do this? Isn't that what PCI Virtual Functions are all about? You need to find the current Linux concept for this, and extend it to offload the functionality to hardware. If Linux can do it, it already has the tools to configure it. Restool is not needed for that. > So, it is not possible to connect a DPNI to a DPSW after it was > connected to a DPMAC. The DPNI-DPMAC pair would have to be > disconnected and DPMAC will be reconnected to the switch. DPNI > interface that is no longer connected to a DPMAC will be destroyed > and any new addition/deletion of a DPNI/DPMAC interface to the > switch port will trigger the entire switch re-configuration. Switches and ports connected to switches are dynamic. They come and go. You don't expect it to happen very often, but Linux has no restrictions on this. You need to figure out how best to offload this to your hardware. Maybe when you create the switch object you make a guess as to how many ports you need. Leave some of the ports not connected to anything. You can then add ports to the switch using the free ports. If you run out of ports, you have no choice but to destroy the switch object and create a new one. Hopefully that does not take too long. Restool is not needed for this, it all happens within the switchdev driver. Andrew
RE: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
Hello Andrew, > -Original Message- > From: Andrew Lunn [mailto:and...@lunn.ch] > Sent: Monday, April 2, 2018 4:45 PM > To: Ioana Ciornei > Cc: Arnd Bergmann ; gregkh > ; Laurentiu Tudor > ; Linux Kernel Mailing List ker...@vger.kernel.org>; Stuart Yoder ; Ruxandra > Ioana Ciocoi Radulescu ; Razvan Stefanescu > ; Roy Pledge ; > Networking > Subject: Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support > > Hi Ioana > > > The commands listed above are for creating/destroying DPAA2 objects > > in Management Complex and not for runtime configuration where > > standard userspace tools are used. > > Please can you explain why this is not just plumbing inside a > switchdev driver? > > The hardware has a number of physical ports. So on probe, i would > expect it to create a DPMAC, DPNI, and DPIO for each port, and a linux > netdev. From then on, standard tools are all that are needed. The > switchdev driver can create a l2 switch object when the user uses the > ip link add name br0 type bridge. It can then connect the switch > object to the DPNI when the user adds an interface to the switch, etc. > I'll chime in as you mentioned switchdev driver. DPAA2 offers several object-based abstractions for modeling network related devices (interfaces, L2 Ethernet switch) or accelerators (DPSECI - crypto and DPDCEI - compression), the latter not up-streamed yet. They are modeled using various low-level resources (e.g. queues, classification tables, physical ports) and have multiple configuration and interconnectivity options, managed by the Management Complex. Resources are limited and they are only used when needed by the objects, to accommodate more configurations and usage scenarios. Some of the objects have a 1-to-1 correspondence to physical resources (e.g. DPMACs to physical ports), while others (like DPNIs and DPSW) can be seen as a collection of the mentioned resources. The types and number of such objects are not predetermined. When the board boots up, none of them exist yet. Restool allows a user to define the system topology, by providing a way to dynamically create, destroy and interconnect these objects. After an object is created, it will be presented on the fsl-mc bus. A driver is loaded to implement the required kernel interfaces specific to that object type. Kernel can boot and afterwards the DPAA2 objects are added, as the user requires. As you mentioned DPMACs: objects of this type can be connected only to a DPNI (a network interface like object) or to a DPSW (L2 ethernet switch) port. Likewise, a DPNI can have only one connection (to a DPMAC, a DPSW port or another DPNI object). Here's several examples of valid connection types: * DPMAC <> DPNI (standard network i/f corresponding to a physical port) * DPMAC <> DPSW (physical port in a switch) * DPNI <> DPSW (virtual network interface connected to a switch port) * DPNI <> DPNI In the latter case, the two DPNIs will not be connected to any physical port, but can be used as a point-to-point connection between two virtual machines for instance. So, it is not possible to connect a DPNI to a DPSW after it was connected to a DPMAC. The DPNI-DPMAC pair would have to be disconnected and DPMAC will be reconnected to the switch. DPNI interface that is no longer connected to a DPMAC will be destroyed and any new addition/deletion of a DPNI/DPMAC interface to the switch port will trigger the entire switch re-configuration. Best regards, Razvan Stefanescu
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
Hi Ioana > The commands listed above are for creating/destroying DPAA2 objects > in Management Complex and not for runtime configuration where > standard userspace tools are used. Please can you explain why this is not just plumbing inside a switchdev driver? The hardware has a number of physical ports. So on probe, i would expect it to create a DPMAC, DPNI, and DPIO for each port, and a linux netdev. From then on, standard tools are all that are needed. The switchdev driver can create a l2 switch object when the user uses the ip link add name br0 type bridge. It can then connect the switch object to the DPNI when the user adds an interface to the switch, etc. Andrew
RE: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
> > > I'm still not convinced either way (high-level or low-level > > interface), but I think this needs to be discussed with the networking > > maintainers. Given the examples on the github page you linked to, the > > high-level user space commands based on these ioctls > > > >ls-addni # adds a network interface > >ls-addmux # adds a dpdmux > >ls-addsw # adds an l2switch > >ls-listmac # lists MACs and their connections > >ls-listni # lists network interfaces and their connections > > > > and I see that you also support the switchdev interface in > > drivers/staging/fsl-dpaa2, which I think does some of the same things, > > presumably by implementing the switchdev API using fsl_mc_command > > low-level interfaces in the kernel. > > Hi Arnd > > I agree that switchdev and devlink should be the correct way to handle this. > The > low level plumbing of the hardware should all be hidden. There should not be > any user space commands needed other than the usual network configuration > tools and devlink. > Hi, The commands listed above are for creating/destroying DPAA2 objects in Management Complex and not for runtime configuration where standard userspace tools are used. Restool is responsible for creating objects in Management complex and this process can be seen as the equivalent of hotplugging a peripheral rather than configuring it, thus there is no standard userspace tool to handle that. * The Management Complex is configured to create a specific set of DPAA2 objects dynamically through Restool (by sending create commands) or statically, at boot time, through a configuration file (Data Path Layout file) * The objects are then probed and configured by the corresponding drivers * The objects are controlled at runtime by the user via standard tools (e.g. ethtool for network interfaces). I hope this gives a better understanding on the DPAA2 hardware and software architecture. The fsl-mc bus documentation gives more details on this: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/Documentation/networking/dpaa2/overview.rst?h=staging-next Ioana
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
> I'm still not convinced either way (high-level or low-level > interface), but I think > this needs to be discussed with the networking maintainers. Given the examples > on the github page you linked to, the high-level user space commands > based on these ioctls > >ls-addni # adds a network interface >ls-addmux # adds a dpdmux >ls-addsw # adds an l2switch >ls-listmac # lists MACs and their connections >ls-listni # lists network interfaces and their connections > > and I see that you also support the switchdev interface in > drivers/staging/fsl-dpaa2, which I think does some of the same > things, presumably by implementing the switchdev API using > fsl_mc_command low-level interfaces in the kernel. Hi Arnd I agree that switchdev and devlink should be the correct way to handle this. The low level plumbing of the hardware should all be hidden. There should not be any user space commands needed other than the usual network configuration tools and devlink. Andrew
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
On Wed, Mar 28, 2018 at 4:27 PM, Ioana Ciornei wrote: > Hi, > >> >> Hi Ioana, >> >> So this driver is a direct passthrough to your hardware for passing fixed- >> length command/response pairs. Have you considered using a higher-level >> interface instead? >> >> Can you list some of the commands that are passed here as clarification, and >> explain what the tradeoffs are that have led to adopting a low-level >> interface >> instead of a high-level interface? >> >> The main downside of the direct passthrough obviously is that you tie your >> user space to a particular hardware implementation, while a high-level >> abstraction could in principle work across a wider range of hardware >> revisions >> or even across multiple vendors implementing the same concept by different >> means. > > If by "higher-level" you mean an implementation where commands are created by > the kernel at userspace's request, then I believe this approach is not really > viable because of the sheer number of possible commands that would bloat the > driver. > > For example, a DPNI (Data Path Network Interface) can be created using a > command that has the following structure: > > struct dpni_cmd_create { > uint32_t options; > uint8_t num_queues; > uint8_t num_tcs; > uint8_t mac_filter_entries; > uint8_t pad1; > uint8_t vlan_filter_entries; > uint8_t pad2; > uint8_t qos_entries; > uint8_t pad3; > uint16_t fs_entries; > }; > > In the above structure, each field has a meaning that the end-user might want > to be able to change according to their particular use-case (not much is left > at its default value). > The same level of complexity is encountered for all the commands that > interact with Data Path objects such as DPBP(buffer pools), DPRC(Resource > Container) etc. > You can find more examples of commands in restool's repo: > https://github.com/qoriq-open-source/restool/tree/integration/mc_v10 > > In my opinion, an in-kernel implementation that is equivalent in terms of > flexibility will turn > into a giant ioctl parser, all while also exposing an userspace API that is > not as simple/easy to use. (adding the netdev list) The command you list there seems to be networking related, so instead of an ioctl based interface, a high-lever interface would likely use netlink for consistency with other drivers. Are all commands for networking or are there some that are talking to the device to do something unrelated? Obviously creating a high-level interface would be a lot of work in the kernel, and it only pays off if there are multiple independent users, we wouldn't do that for just one driver. I'm still not convinced either way (high-level or low-level interface), but I think this needs to be discussed with the networking maintainers. Given the examples on the github page you linked to, the high-level user space commands based on these ioctls ls-addni # adds a network interface ls-addmux # adds a dpdmux ls-addsw # adds an l2switch ls-listmac # lists MACs and their connections ls-listni # lists network interfaces and their connections and I see that you also support the switchdev interface in drivers/staging/fsl-dpaa2, which I think does some of the same things, presumably by implementing the switchdev API using fsl_mc_command low-level interfaces in the kernel. Is that a correct interpretation? If yes, could we extend switchdev or other networking interfaces to fill in whatever those don't handle yet? >> > @@ -14,10 +14,18 @@ >> > * struct fsl_mc_command - Management Complex (MC) command >> structure >> > * @header: MC command header >> > * @params: MC command parameters >> > + * >> > + * Used by RESTOOL_SEND_MC_COMMAND >> > */ >> > struct fsl_mc_command { >> > __u64 header; >> > __u64 params[MC_CMD_NUM_OF_PARAMS]; }; >> > >> > +#define RESTOOL_IOCTL_TYPE 'R' >> > +#define RESTOOL_IOCTL_SEQ 0xE0 >> >> I tried to follow the code path into the hardware and am a bit confused about >> the semantics of the 'header' field and the data. Both are accessed passed to >> the hardware using >> >>writeq(le64_to_cpu(cmd->header)) >> >> which would indicate a fixed byte layout on the user structure and that it >> should really be a '__le64' instead of '__u64', or possibly should be >> represented as '__u8 header[8]' to clarify that the byte ordering is supposed >> to match the order of the byte addresses of the register. >> > > Indeed, the struct fsl_mc_command should use '__le64' instead of '__u64', so > that the UAPI header file clearly states what endianness should the userspace > prepare. > The writeq appeared as a consequence of a previous mail thread > https://lkml.org/lkml/2017/7/17/415, because the endianness of the command is > handled by the user and the bus should leave the binary blob intact. Ok. However, with the dpni_cmd_create structure you list above, it seems that it doesn
RE: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
Hi, > > Hi Ioana, > > So this driver is a direct passthrough to your hardware for passing fixed- > length command/response pairs. Have you considered using a higher-level > interface instead? > > Can you list some of the commands that are passed here as clarification, and > explain what the tradeoffs are that have led to adopting a low-level interface > instead of a high-level interface? > > The main downside of the direct passthrough obviously is that you tie your > user space to a particular hardware implementation, while a high-level > abstraction could in principle work across a wider range of hardware revisions > or even across multiple vendors implementing the same concept by different > means. If by "higher-level" you mean an implementation where commands are created by the kernel at userspace's request, then I believe this approach is not really viable because of the sheer number of possible commands that would bloat the driver. For example, a DPNI (Data Path Network Interface) can be created using a command that has the following structure: struct dpni_cmd_create { uint32_t options; uint8_t num_queues; uint8_t num_tcs; uint8_t mac_filter_entries; uint8_t pad1; uint8_t vlan_filter_entries; uint8_t pad2; uint8_t qos_entries; uint8_t pad3; uint16_t fs_entries; }; In the above structure, each field has a meaning that the end-user might want to be able to change according to their particular use-case (not much is left at its default value). The same level of complexity is encountered for all the commands that interact with Data Path objects such as DPBP(buffer pools), DPRC(Resource Container) etc. You can find more examples of commands in restool's repo: https://github.com/qoriq-open-source/restool/tree/integration/mc_v10 In my opinion, an in-kernel implementation that is equivalent in terms of flexibility will turn into a giant ioctl parser, all while also exposing an userspace API that is not as simple/easy to use. > > @@ -14,10 +14,18 @@ > > * struct fsl_mc_command - Management Complex (MC) command > structure > > * @header: MC command header > > * @params: MC command parameters > > + * > > + * Used by RESTOOL_SEND_MC_COMMAND > > */ > > struct fsl_mc_command { > > __u64 header; > > __u64 params[MC_CMD_NUM_OF_PARAMS]; }; > > > > +#define RESTOOL_IOCTL_TYPE 'R' > > +#define RESTOOL_IOCTL_SEQ 0xE0 > > I tried to follow the code path into the hardware and am a bit confused about > the semantics of the 'header' field and the data. Both are accessed passed to > the hardware using > >writeq(le64_to_cpu(cmd->header)) > > which would indicate a fixed byte layout on the user structure and that it > should really be a '__le64' instead of '__u64', or possibly should be > represented as '__u8 header[8]' to clarify that the byte ordering is supposed > to match the order of the byte addresses of the register. > Indeed, the struct fsl_mc_command should use '__le64' instead of '__u64', so that the UAPI header file clearly states what endianness should the userspace prepare. The writeq appeared as a consequence of a previous mail thread https://lkml.org/lkml/2017/7/17/415, because the endianness of the command is handled by the user and the bus should leave the binary blob intact. > However, the in-kernel usage of that field suggests that we treat it as a > 64-bit > cpu-endian number, for which the hardware needs to know the endianess of > the currently running kernel and user space. > I might have not understood what you are trying to say, but the in-kernel code treats the fsl_mc_command as little endian by using cpu_to_leXX and leXX_to_cpu in order to setup the command structure. Ioana > Can you have a look at where the mistake is and what the byteorder for the > fsl_mc_command structure is supposed to be? Obviously, this is one thing > that would be simplified by using a high-level interface, but it's possible > to do > it like this as long as it's completely clear how the structure layout is > meant to > be used in the uapi header. > > Arnd
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
On Fri, Mar 23, 2018 at 11:38 PM, Ioana Ciornei wrote: > Adding kernel support for restool, a userspace tool for resource > management, means exporting an ioctl capable device file representing > the root resource container. > This new functionality in the fsl-mc bus driver intends to provide > restool an interface to interact with the MC firmware. > Commands that are composed in userspace are sent to the MC firmware > through the RESTOOL_SEND_MC_COMMAND ioctl. > By default the implicit MC I/O portal is used for this operation, > but if the implicit one is busy, a dynamic portal is allocated and then > freed upon execution. Hi Ioana, So this driver is a direct passthrough to your hardware for passing fixed-length command/response pairs. Have you considered using a higher-level interface instead? Can you list some of the commands that are passed here as clarification, and explain what the tradeoffs are that have led to adopting a low-level interface instead of a high-level interface? The main downside of the direct passthrough obviously is that you tie your user space to a particular hardware implementation, while a high-level abstraction could in principle work across a wider range of hardware revisions or even across multiple vendors implementing the same concept by different means. > +static long fsl_mc_restool_dev_ioctl(struct file *file, > +unsigned int cmd, > +unsigned long arg) > +{ > + int error; > + > + switch (cmd) { > + case RESTOOL_SEND_MC_COMMAND: > + error = fsl_mc_restool_send_command(arg, file->private_data); > + break; > @@ -14,10 +14,18 @@ > * struct fsl_mc_command - Management Complex (MC) command structure > * @header: MC command header > * @params: MC command parameters > + * > + * Used by RESTOOL_SEND_MC_COMMAND > */ > struct fsl_mc_command { > __u64 header; > __u64 params[MC_CMD_NUM_OF_PARAMS]; > }; > > +#define RESTOOL_IOCTL_TYPE 'R' > +#define RESTOOL_IOCTL_SEQ 0xE0 I tried to follow the code path into the hardware and am a bit confused about the semantics of the 'header' field and the data. Both are accessed passed to the hardware using writeq(le64_to_cpu(cmd->header)) which would indicate a fixed byte layout on the user structure and that it should really be a '__le64' instead of '__u64', or possibly should be represented as '__u8 header[8]' to clarify that the byte ordering is supposed to match the order of the byte addresses of the register. However, the in-kernel usage of that field suggests that we treat it as a 64-bit cpu-endian number, for which the hardware needs to know the endianess of the currently running kernel and user space. Can you have a look at where the mistake is and what the byteorder for the fsl_mc_command structure is supposed to be? Obviously, this is one thing that would be simplified by using a high-level interface, but it's possible to do it like this as long as it's completely clear how the structure layout is meant to be used in the uapi header. Arnd
RE: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
> > > > > > > > On Fri, Mar 23, 2018 at 10:38:56AM -0500, Ioana Ciornei wrote: > > > > +#include "fsl-mc-private.h" > > > > + > > > > +#define FSL_MC_BUS_MAX_MINORS 1 > > > > > > As you only need/want one character device here, why not just use > > > the misc device api? It's much simpler, and handles all of the > > > housekeeping for you correctly. It also means I don't have to audit > > > all of your chardev code to verify it is correct :) > > > > I have considered the misc device api but the fsl-mc bus, since it is > > a platform driver, is probing before the misc char driver. > > How? The misc code is just "core code" there's nothing to "probe" here. > Is this an init-call ordering issue somehow? A platform driver probe should > be > after the misc core is initialized. And if not, you can always defer your > probe. Sorry for the previous improper wording. I have tested again using the misc device api and indeed the devices on the fsl-mc bus are probing before the misc core is initialized but I am able to complete the misc registration deferring the probe the first time. I will update the patch set and send a new version. Ioana C > > thanks, > > greg k-h
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
On Fri, Mar 23, 2018 at 03:56:24PM +, Ioana Ciornei wrote: > > > > > On Fri, Mar 23, 2018 at 10:38:56AM -0500, Ioana Ciornei wrote: > > > +#include "fsl-mc-private.h" > > > + > > > +#define FSL_MC_BUS_MAX_MINORS1 > > > > As you only need/want one character device here, why not just use the misc > > device api? It's much simpler, and handles all of the housekeeping for you > > correctly. It also means I don't have to audit all of your chardev code to > > verify it > > is correct :) > > I have considered the misc device api but the fsl-mc bus, since it is > a platform driver, is probing before the misc char driver. How? The misc code is just "core code" there's nothing to "probe" here. Is this an init-call ordering issue somehow? A platform driver probe should be after the misc core is initialized. And if not, you can always defer your probe. thanks, greg k-h
RE: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
> > On Fri, Mar 23, 2018 at 10:38:56AM -0500, Ioana Ciornei wrote: > > +#include "fsl-mc-private.h" > > + > > +#define FSL_MC_BUS_MAX_MINORS 1 > > As you only need/want one character device here, why not just use the misc > device api? It's much simpler, and handles all of the housekeeping for you > correctly. It also means I don't have to audit all of your chardev code to > verify it > is correct :) I have considered the misc device api but the fsl-mc bus, since it is a platform driver, is probing before the misc char driver. Ioana > > And it will save you lines-of-code, always a good thing. > > thanks, > > greg k-h
Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
On Fri, Mar 23, 2018 at 10:38:56AM -0500, Ioana Ciornei wrote: > +#include "fsl-mc-private.h" > + > +#define FSL_MC_BUS_MAX_MINORS1 As you only need/want one character device here, why not just use the misc device api? It's much simpler, and handles all of the housekeeping for you correctly. It also means I don't have to audit all of your chardev code to verify it is correct :) And it will save you lines-of-code, always a good thing. thanks, greg k-h