On Wed, Feb 13, 2019 at 10:54:49AM -0700, Logan Gunthorpe wrote:

Hi

When using multi-ports each port uses resources (dbs, msgs, mws, etc) on every other port. Creating a mapping for these resources such that each port has a corresponding resource on every other port is a bit tricky.

Introduce the ntb_peer_resource_idx() function for this purpose. It returns the peer resource number that will correspond with the local peer index on the remote peer.

Also, introduce ntb_peer_highest_mw_idx() which will use ntb_peer_resource_idx() but return the MW index starting with the highest index and working down.

Signed-off-by: Logan Gunthorpe logang@deltatee.com Cc: Jon Mason jdmason@kudzu.us Cc: Dave Jiang dave.jiang@intel.com Cc: Allen Hubbe allenbh@gmail.com

---

include/linux/ntb.h | 70 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+)

diff --git a/include/linux/ntb.h b/include/linux/ntb.h index 181d16601dd9..f5c69d853489 100644 --- a/include/linux/ntb.h +++ b/include/linux/ntb.h @@ -1502,4 +1502,74 @@ static inline int ntb_peer_msg_write(struct ntb_dev *ntb, int pidx, int midx, return ntb->ops->peer_msg_write(ntb, pidx, midx, msg); } +/**

• • ntb_peer_resource_idx() - get a resource index for a given peer idx
• • @ntb: NTB device context.
• • @pidx: Peer port index.
• • When constructing a graph of peers, each remote peer must use a different
• • resource index (mw, doorbell, etc) to communicate with each other
• • peer.
• • In a two peer system, this function should always return 0 such that
• • resource 0 points to the remote peer on both ports.
• • In a 5 peer system, this function will return the following matrix
• • pidx \ port 0 1 2 3 4
• • 0 0 0 1 2 3
• • 1 0 1 2 3 4
• • 2 0 1 2 3 4
• • 3 0 1 2 3 4

This table is too simplified to represent a generic case of port-index mapping table. In particular the IDT PCIe switch got it ports numbered with uneven integers like: 0 2 4 6 8 12 16 20 or 0 8 16, and so on. Moreover some of the ports might be disabled or may have NTB functions deactivated, in which case these ports shouldn't be considered by NTB subsystem at all. Basically we may have any increasing subset of that port numbers depending on the current IDT PCIe-switch ports setup.

What I am trying to say by this is that if in real life the NTB MSI library or some other library used that matrix to calculate the MW index, then most likely it would either consume nearly all the MWs leaving holes in the space of MWs or just run out of them since depending on the configuration there might be from 0 to 24 MWs enabled in a IDT NTB function. In order to solve the problem we either need the ntb_peer_resource_idx() method somehow fixed so it would use the pidx instead of the pure port number or we should properly alter the current NTB API port-index table implementation, so the ntb_pingpong, NTB MSI library and others wouldn't need to a have some other table to distribute the NTB resources.

A short preamble of why we introduced the ports-index NTB API. Since each NTB MW and NTB message registers can be setup to be related with any NTB device port, we needed to have the port attribute accepted by NTB API functions. But it turned out the IDT PCIe switch ports are unevenly numbered, which may cause difficulties in using their numbers for bulk configurations. So in order to simplify a client code utilizing the multi-ports NTB API (for example for simpler looping around the device ports), we decided to create the ports-index table in the NTB API internals. The table is defined by a set of ntb_port_*()/ntb_peer_port_*() functions. So now all the NTB API methods accept a port index instead of an irregular port number.

Here is the port-index table currently defined for IDT 89HPES32NT8 PCIe-switch with all 8 ports having NTB functions enabled:

peer port \ local port 0 2 4 6 8 12 16 20 0 x 0 0 0 0 0 0 0 2 0 x 1 1 1 1 1 1 4 1 1 x 2 2 2 2 2 6 2 2 2 x 3 3 3 3 8 3 3 3 3 x 4 4 4 12 4 4 4 4 4 x 5 5 16 5 5 5 5 5 5 x 6 20 6 6 6 6 6 6 6 x

As you can see currently each NTB device side's got its own port-index mapping, so each port can access another ports, but not itself. I implemented it this way intentionally for two reasons: 1) I don't think anyone ever would need to have MW or Message registers setup pointing to the local port. 2) IDT PCIe switch will report "Unsupported TLP" error in case if there is an attempt to touch a MW frame initialized with port number matching the local port number. (Internally IDT PCIe-switch is working with port partitions. But I didn't want to complicate the API description, so we just utilize the port numbers naming, which is essentially the same as partitions for NTB.)

The biggest drawback of the selected table representation is that the port-index mapping is unique for each local port, which makes the indexes unsuitable to distribute shared resources like outbound MWs, Doorbells and Scratchpads (if one ever implemented for multi-port NTB devices). For instance each port can access a port with index 0, which for local port 0 is port 2, while for the rest of the local ports it's port 0, and so on.

This drawback already caused complications in the ntb_pingpong driver, where after your patchset acceptance we use port numbers to distribute the doorbell bits. The same problem is popping up here, but this time it can't be solved by the port numbers utilization due to the limited number of MWs available on IDT.

As I already said there can be two ways to get rid of the complication. One of them is to redefine the port-index table, so to map the port numbers to global indexes instead of the local ones, like this:

peer port \ local port 0 2 4 6 8 12 16 20 0 0 0 0 0 0 0 0 0 2 1 1 1 1 1 1 1 1 4 2 2 2 2 2 2 2 2 6 3 3 3 3 3 3 3 3 8 4 4 4 4 4 4 4 4 12 5 5 5 5 5 5 5 5 16 6 6 6 6 6 6 6 6 20 7 7 7 7 7 7 7 7

By doing so we'll be able to use the port indexes to distribute the shared resources like MWs, Doorbells and Scratchpads, but we may get the problem with "Unsupported TLP" error I described before for IDT devices, if a NTB API client code tries to define a MW pointing to the local port on IDT NTB hardware. I don't know whether the same problem exists for Switchtec multi-ports NTB devices, but this approach shall definitely lead to cleaner and easier NTB API as well as simplify the IDT NTB hw driver. We also won't need to have any additional tables like ntb_peer_resource_idx().

(the second solution is described further in the next comment)

• • For example, if this function is used to program peer's memory
• • windows, port 0 will program MW 0 on all it's peers to point to itself.
• • port 1 will program MW 0 in port 0 to point to itself and MW 1 on all
• • other ports. etc.
• • For the legacy two host case, ntb_port_number() and ntb_peer_port_number()
• • both return zero and therefore this function will always return zero.
• • So MW 0 on each host would be programmed to point to the other host.
• • Return: the resource index to use for that peer.
• */

+static inline int ntb_peer_resource_idx(struct ntb_dev *ntb, int pidx) +{

• int local_port, peer_port;
• if (pidx >= ntb_peer_port_count(ntb))

• return -EINVAL;
  

• local_port = ntb_port_number(ntb);
• peer_port = ntb_peer_port_number(ntb, pidx);
• if (peer_port < local_port)

• return local_port - 1;
  

• else

• return local_port;
  

+}

Instead of redefining the port-index table we can just fix the ntb_peer_resource_idx() method, so it would return a global port index instead of some number based on the port number. It can be done just by the next modification:

+ if (peer_port <= local_port) + return pidx; + else + return pidx + 1;

Personally I'd prefer the first solution even though it may lead to the "Unsupported TLP" errors and cause a greater code changes. Here is why: 1) the error might be IDT-specific, so we shouldn't limit the API due to one particular hardware peculiarity, 2) port-index table with global indexes implementation shall simplify the IDT NTB hw driver and provide a cleaner NTB API with simpler shared resources utilization code.

The final decision is after the NTB subsystem maintainers. If they agree with solution #1 I'll send a corresponding patchset on this week, so you can alter this patchset being based on it.

+/**

• • ntb_peer_highest_mw_idx() - get a memory window index for a given peer idx
• • using the highest index memory windows first
• • @ntb: NTB device context.
• • @pidx: Peer port index.
• • Like ntb_peer_resource_idx(), except it returns indexes starting with
• • last memory window index.
• • Return: the resource index to use for that peer.
• */

+static inline int ntb_peer_highest_mw_idx(struct ntb_dev *ntb, int pidx) +{

• int ret;
• ret = ntb_peer_resource_idx(ntb, pidx);
• if (ret < 0)

• return ret;
  

• return ntb_mw_count(ntb, pidx) - ret - 1;

+}

#endif

2.19.0

-- You received this message because you are subscribed to the Google Groups "linux-ntb" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+unsubscribe@googlegroups.com. To post to this group, send email to linux-ntb@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/20190213175454.7506-8-logang%40d.... For more options, visit https://groups.google.com/d/optout.

-Sergey

On Wed, Mar 06, 2019 at 04:22:33PM -0700, Logan Gunthorpe wrote:

On 2019-03-06 3:45 p.m., Serge Semin wrote: [Snip]

Pretty sure everything above is just agreement...

...

So your current approach is inbound MW-centralized, while mine is developed around the outbound MWs.

I don't think this has anything to do with inbound vs outbound. The problem is the same no matter from which direction you assign things.

...

...

Physical Port Number: 0 2 4 6 8 12 16 20 Logical Port Number: 0 1 2 3 4 5 6 7 Peer Index (Port 0): x 0 1 2 3 4 5 6 Port Index (Port 8): 0 1 2 3 x 4 5 6 (etc)

That's what I suggested in the two possible solutions: 1st solution: replace current pidx with Logical Port Number, 2nd solution: alter ntb_peer_resource_idx() so it would return the Logical Port Number.

Well my solution wasn't to change pidx and no, we don't want to change ntb_peer_resource_idx() to return the logical port number because that's not how it's being used and I have no use for a ntb_peer_port_global_idx() function. Functions are supposed to be added by code that needs to call them, not by some grand design. Part of the reason we have this confusing mess is because the API was reviewed and merged before any users of the API were presented. Usually this is not accepted in kernel development.

My suggestion is to simply say that the existing port numbers are the logical port number and have the drivers handle the physical port number mapping internally. That requires the fewest changes.

Now I see what you were suggesting. The solution you propose doesn't require any changes in your patches, but requires to update IDT driver. It also makes a change in the ports numbering semantics, which you said you didn't want to introduce. (read all further comments to get my conclusion regarding it)

...

IMO In case of the 2nd solution I'd also suggest to rename the ntb_peer_resource_idx() method into ntb_peer_port_global_idx(), and then consider the current port indexes used in the NTB API as local port indexes. The resource indexing can be abstracted by a macro like this: #define ntb_peer_resource_idx ntb_peer_port_global_idx

That define would not be useful.

...

Finally in order to close the space up we'd also need to define a method: ntb_port_global_idx(), which would return a Logical (global) index of local port.

Again, I'd rather not add a bunch of large semantic and infrastructure changes at this point. It's confusing enough as it is and we don't need to introduce yet another indexing scheme API to the clients that really do not need it. What the clients need is a simple API to decide which resources to use for which peers, and to figure out which peers used which resources. ntb_peer_port_idx() and ntb_peer_resource_idx() suit these purposes. Nothing else really needs to exist.

If you don't want to add a large semantic and infrastructure change at this point, then it would be better to leave NTB port API the way it is now, and use logical port indexes in the ntb_peer_resource_idx() method. You'd also need to use this method to access both outbound and inbound memory windows. In this case we wouldn't need to change anything in current API and drivers. Yes, this approach would cause one resource being wasted, but it would lead to simpler semantic of the port numbers API.

Don't get me wrong. I am not completely against the approach you suggest. It just has its pros and cons. The same way as mine does. I'll explain my opinion later in this email.

...

...

Where the Physical Port Number is whatever the hardware uses and the logical port number is a numbering scheme starting with zero with no gaps. Then the port indexes are still as we currently have them. If we say that the port numbers we have now are the Logical Port Number, then ntb_peer_resource_idx() is correct.

Current port numbers are the physical port numbers with gaps.

I think that's up for interpretation as, based on the existing code, I naturally interpreted it the other way and therefore it's pretty simple to say that it's the logical port number and fix the one driver that needs to change.

...

That's why we introduced the port-index NTB API abstraction in the first place, to have these gaps eliminated and to provide a simple way of bulk setup. Although that abstraction turned out not that suitable to distribute the shared resources. So the Logical (Global) indexing is needed to do it (that's what ntb_pingpong used to do and ntb_perf still does now).

My interpretation of the port-index was simply to match what was done in the two port case seeing code like ntb_transport simply uses the default 0 as the port index. There was no reason to believe, based on the code, that there would be gaps.

Sorry man, but how could you base your interpretation on a code, which didn't support multi-port case in the first place and just couldn't provide you a full impression by definition? You knew ntb_transport doesn't support the multi-port NTB devices, right? Moreover as far as I remember we already concerned similar problem in a discussion of your patches submitted for ntb_pingpong driver. You knew ntb_pingpong and ntb_perf driver support multi-port devices. So you could get your interpretation from there.

BTW I didn't figure out it at that time, but you could fix the ntb_pingpong driver just by replacing the strict inequality in the conditional statement: --- a/drivers/ntb/test/ntb_pingpong.c +++ b/drivers/ntb/test/ntb_pingpong.c @@ -299,7 +299,7 @@ static void pp_init_flds(struct pp_ctx *pp) lport = ntb_port_number(pp->ntb); pcnt = ntb_peer_port_count(pp->ntb); for (pidx = 0; pidx < pcnt; pidx++) { - if (lport < ntb_peer_port_number(pp->ntb, pidx)) + if (lport <= ntb_peer_port_number(pp->ntb, pidx)) break; }

This loop just finds out the logical index (as you named it) of the local port, which then is used to select a doorbell bit from the shared doorbell register. The similar algo is used in the ntb_perf driver.

...

...

I would strongly argue that the clients don't need to know anything about the Physical Port Number and these should be handled strictly inside the drivers. If multiple drivers need to do something similar to map the logical to physical port numbers then we should introduce helper functions to allow them to do so. If the Physical Numbers are not contained in the driver than the API would need to be expanded to expose which numbers are actually used to avoid needing to constantly loop through all the indexes to find this out.

Absolutely agree with you. The main idea of NTB API was to provide a set of methods to access the NTB hardware without any abstractions but with possible useful helpers, like your NTB MSI library, or transport library, or anything else. So the physical port numbers must be available for the client drivers.

Huh? How can you say you absolutely agree with me? I said the clients should not need to know about physical port numbers and you said the physical port numbers *must* be available to clients. I think this statement needs to be justified. Why should the clients need to know about the physical port numbers?

Ah, I misunderstood your statement. I thought you implied the other way around. I disagree then. Client drivers should be somehow able to retrieve the real physical port number. Physical port numbers can be connected with some ports-specific functionality (and they are in our projects), so they are used to enable/disable corresponding code of the client drivers.

Additionally IDT PCIe NTB functions can be disabled/enabled by the device firmware, so one device may have for instance ports 2 4 6 with NTB, while another - ports 2 6 12, and so on. Both of these ports-set would be mapped to the same logical indexes space - 0 1 2. In case of having just the logical indexes, the code would have an impression of working with the same hardware, while in fact it isn't.

You said: "Part of the reason we have this confusing mess is because the API was reviewed and merged before any users of the API were presented. Usually this is not accepted in kernel development." A source code of my project is using current port API and happy with it, so there was at least one user of the API at the time of the API submission. I bet there are others now, since I constantly get private questions regarding the IDT hardware configurations. So please don't be so judgemental. If you see some confusing from your point of view things it doesn't always mean it is a mess, you just may misunderstand something. I am sure a pro with experience like yours doesn't need this to be explained.

...

...

On a similar vein, I'd suggest that most clients shouldn't even really need to do anything with the Logical Port Number and should deal largely with Port Indexes. Ideally, Logical Port Numbers should only be used by helper code in the common layer to help assign resources used by the clients (like ntb_peer_resource_idx()).

This is the main question. Do we really need the current port indexes implementation at all? After all these years of NTB API usage I don't really see it useful in any case except to loop over the outbound MW resources automatically skipping the local port (usefulness of this is also questionable). As I already said I created the port-index table this way due to the IDT NTB MWs peculiarity, which doesn't seem to me a big problem now comparing to all these additional complications we intend to introduce.

To me, it seems like the port indexes are used everywhere. A client almost always wants to deal with every port except itself. That seems entirely natural to me. Using the port index as the resource index makes perfect sense (which is why I implemented the inverse ntb_peer_resource_idx()).

...

The rest of the drivers code really need to have the Logical (global) port indexes, at least to distribute the shared resources, and don't use the current pidx that much.

Drivers don't care about port indexes or how the resources are distributed. I would expect that all they'd do is map the port index (which all existing APIs take) to the physical address and program hardware as necessary.

...

Wouldn't it be better to just redefine the current port-index table in the following way? ntb_port_number() - local physical port number, ntb_port_idx() - local port logical (global) index, ntb_peer_port_count() - total number of ports NTB device provide (including the local one), ntb_peer_port_number() - physical port number of the peer with passed logical port index, ntb_peer_port_idx - logical port index of the passed physical port number.

No, from the perspective of the client, the physical port numbers are useless. I don't want the count to include the local one because that means I'd always have to subtract one every time I want to loop through all peers (I never want to loop through all ports including the local one). I have no idea what your distinction between logical (global) index and logical port index is. In fact I find this all confusing.

...

while currently we have: ntb_port_number() - local physical port number, ntb_peer_port_count() - total number of ports NTB device provide (excluding the local one), ntb_peer_port_number() - physical port number of the peer with passed port index, ntb_peer_port_idx - port index of the passed physical port number;

What we currently have all makes perfect sense to me and is exactly what I want for the clients, except I think ntb_port_number() needs to be the logical port number (with the gaps removed), so that we can more easily implement ntb_peer_resource_idx(). If ntb_port_number() corresponds to the physical port number, then the correct implementation of ntb_peer_resource_idx() is going to be crazy, essentially needing to generate a logical port number for every port, every time it's called.

Logan

To sum the discussion up we now have two possible solutions of the problem discovered in this patch:

1) Leave current NTB port API as is, but fix this patch so the ntb_peer_resource_idx() method would return a port logical index: Physical port numbers: 0 2 4 6 8 12 16 20 (ntb_peer_port_number()/ntb_port_number()) Logical port numbers: 0 1 2 3 4 5 6 7 (ntb_peer_resource_idx()) Port indexes (Port 0): x 0 1 2 3 4 5 6 (ntb_peer_port_idx()) Port indexes (Port 8): 0 1 2 3 x 4 5 6 (ntb_peer_port_idx()) + pros: no need to change current NTB port API and available drivers; no need to apply the gap-less limitation on the NTB port numbers sequence; - cons: use ntb_peer_resource_idx() method to distribute a shared resource on each side of NTB (although it might be neutral or pros from some point of view); waste one memory window (no problem with shared doorbell register).

2) Apply the gap-less limitation on the NTB port numbers sequence, so the ntb_peer_port_number()/ntb_port_number() would return the logical port index: Physical port numbers: 0 2 4 6 8 12 16 20 (no NTB API method to get these numbers) Logical port numbers: 0 1 2 3 4 5 6 7 (ntb_peer_port_number()/ntb_port_number()) Port indexes (Port 0): x 0 1 2 3 4 5 6 (ntb_peer_port_idx()) Port indexes (Port 8): 0 1 2 3 x 4 5 6 (ntb_peer_port_idx()) + pros: no waste of one memory window; use port numbers returned from ntb_peer_port_number()//ntb_port_number() to distribute doorbell bits and outbound MWs; use ntb_peer_resource_idx() just at the peer side to select a proper inbound MW; - cons: need to change the port-index callbacks of the IDT hardware driver; introduces a change in the port numbers semantic; NTB port API gets to be an abstraction over the real physical port numbers, while the later ones won't de directly available to retrieve.

The rest of the solutions would lead to overcomplications in the NTB port API, which we don't want to introduce.

Personally after all the considerations I am now more inclined to the (2)-nd solution. Even though it causes more changes and makes the ports API more abstract, it provides a way to create a simpler shared resources distribution code as well as to exactly distribute the necessary number of memory windows. While the physical port number still can be found by client drivers directly from pci_dev descriptor.

The final decision regarding the solution is after the subsystem maintainers. But although the provided by this patchset NTB MSI library consists some part with multi-port API utilization like MWs distribution, as I said in comments to the other patches, it doesn't really support the only multi-ports NTB device currently available - IDT (which I only interested in). So I don't see a reason why I should bother with providing a patch with alterations to the IDT hardware driver unless this patchset provides a portable NTB MSI library.

-Sergey

-- You received this message because you are subscribed to the Google Groups "linux-ntb" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+unsubscribe@googlegroups.com. To post to this group, send email to linux-ntb@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/4acf79c1-766b-1db6-45c1-4cdd4cbb.... For more options, visit https://groups.google.com/d/optout.

On Wed, Feb 13, 2019 at 10:54:51AM -0700, Logan Gunthorpe wrote:

Hi

The NTB MSI library allows passing MSI interrupts across a memory window. This offers similar functionality to doorbells or messages except will often have much better latency and the client can potentially use significantly more remote interrupts than typical hardware provides for doorbells. (Which can be important in high-multiport setups.)

The library utilizes one memory window per peer and uses the highest index memory windows. Before any ntb_msi function may be used, the user must call ntb_msi_init(). It may then setup and tear down the memory windows when the link state changes using ntb_msi_setup_mws() and ntb_msi_clear_mws().

The peer which receives the interrupt must call ntb_msim_request_irq() to assign the interrupt handler (this function is functionally similar to devm_request_irq()) and the returned descriptor must be transferred to the peer which can use it to trigger the interrupt. The triggering peer, once having received the descriptor, can trigger the interrupt by calling ntb_msi_peer_trigger().

The library is very useful, thanks for sharing it with us.

Here are my two general concerns regarding the implementation. (More specific comments are further in the letter.)

First of all, It might be unsafe to have some resources consumed by NTB MSI or some other library without a simple way to warn NTB client drivers about their attempts to access that resources, since it might lead to random errors. When I thought about implementing a transport library based on the Message/Spad+Doorbell registers, I had in mind to create an internal bits-field array with the resources busy-flags. If, for instance, some message or scratchpad register is occupied by the library (MSI, transport or some else), then it would be impossible to access these resources directly through NTB API methods. So NTB client driver shall retrieve an error in an attempt to write/read data to/from busy message or scratchpad register, or in an attempt to set some occupied doorbell bit. The same thing can be done for memory windows.

Second tiny concern is about documentation. Since there is a special file for all NTB-related doc, it would be good to have some description about the NTB MSI library there as well: Documentation/ntb.txt

Signed-off-by: Logan Gunthorpe logang@deltatee.com Cc: Jon Mason jdmason@kudzu.us Cc: Dave Jiang dave.jiang@intel.com Cc: Allen Hubbe allenbh@gmail.com

---

drivers/ntb/Kconfig | 11 ++ drivers/ntb/Makefile | 3 +- drivers/ntb/msi.c | 415 +++++++++++++++++++++++++++++++++++++++++++ include/linux/ntb.h | 73 ++++++++ 4 files changed, 501 insertions(+), 1 deletion(-) create mode 100644 drivers/ntb/msi.c

diff --git a/drivers/ntb/Kconfig b/drivers/ntb/Kconfig index 95944e52fa36..5760764052be 100644 --- a/drivers/ntb/Kconfig +++ b/drivers/ntb/Kconfig @@ -12,6 +12,17 @@ menuconfig NTB if NTB +config NTB_MSI

• bool "MSI Interrupt Support"
• depends on PCI_MSI
• help

• Support using MSI interrupt forwarding instead of (or in addition to)
  

• hardware doorbells. MSI interrupts typically offer lower latency
  

• than doorbells and more MSI interrupts can be made available to
  

• clients. However this requires an extra memory window and support
  

• in the hardware driver for creating the MSI interrupts.
  

• If unsure, say N.
  

source "drivers/ntb/hw/Kconfig" source "drivers/ntb/test/Kconfig" diff --git a/drivers/ntb/Makefile b/drivers/ntb/Makefile index 537226f8e78d..cc27ad2ef150 100644 --- a/drivers/ntb/Makefile +++ b/drivers/ntb/Makefile @@ -1,4 +1,5 @@ obj-$(CONFIG_NTB) += ntb.o hw/ test/ obj-$(CONFIG_NTB_TRANSPORT) += ntb_transport.o -ntb-y := core.o +ntb-y := core.o +ntb-$(CONFIG_NTB_MSI) += msi.o diff --git a/drivers/ntb/msi.c b/drivers/ntb/msi.c new file mode 100644 index 000000000000..5d4bd7a63924 --- /dev/null +++ b/drivers/ntb/msi.c @@ -0,0 +1,415 @@ +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)

+#include <linux/irq.h> +#include <linux/module.h> +#include <linux/ntb.h> +#include <linux/msi.h> +#include <linux/pci.h>

+MODULE_LICENSE("Dual BSD/GPL"); +MODULE_VERSION("0.1"); +MODULE_AUTHOR("Logan Gunthorpe logang@deltatee.com"); +MODULE_DESCRIPTION("NTB MSI Interrupt Library");

+struct ntb_msi {

• u64 base_addr;
• u64 end_addr;
• void (*desc_changed)(void *ctx);
• u32 *peer_mws[];

Shouldn't we use the __iomem attribute here since later the devm_ioremap() is used to map MWs at these pointers?

+};

+/**

• • ntb_msi_init() - Initialize the MSI context
• • @ntb: NTB device context
• • This function must be called before any other ntb_msi function.
• • It initializes the context for MSI operations and maps
• • the peer memory windows.
• • This function reserves the last N outbound memory windows (where N
• • is the number of peers).
• • Return: Zero on success, otherwise a negative error number.
• */

+int ntb_msi_init(struct ntb_dev *ntb,

•  void (*desc_changed)(void *ctx))
  

+{

• phys_addr_t mw_phys_addr;
• resource_size_t mw_size;
• size_t struct_size;
• int peer_widx;
• int peers;
• int ret;
• int i;
• peers = ntb_peer_port_count(ntb);
• if (peers <= 0)

• return -EINVAL;
  

• struct_size = sizeof(*ntb->msi) + sizeof(*ntb->msi->peer_mws) * peers;
• ntb->msi = devm_kzalloc(&ntb->dev, struct_size, GFP_KERNEL);
• if (!ntb->msi)

• return -ENOMEM;
  

• ntb->msi->desc_changed = desc_changed;
• for (i = 0; i < peers; i++) {

• peer_widx = ntb_peer_mw_count(ntb) - 1 - i;
  

• ret = ntb_peer_mw_get_addr(ntb, peer_widx, &mw_phys_addr,
  

• 			   &mw_size);
  

• if (ret)
  

• 	goto unroll;
  

• ntb->msi->peer_mws[i] = devm_ioremap(&ntb->dev, mw_phys_addr,
  

• 				     mw_size);
  

• if (!ntb->msi->peer_mws[i]) {
  

• 	ret = -EFAULT;
  

• 	goto unroll;
  

• }
  

• }
• return 0;

+unroll:

• for (i = 0; i < peers; i++)

• if (ntb->msi->peer_mws[i])
  

• 	devm_iounmap(&ntb->dev, ntb->msi->peer_mws[i]);
  

Simpler and faster cleanup-code would be:

+ unroll: + for (--i; i >= 0; --i) + devm_iounmap(&ntb->dev, ntb->msi->peer_mws[i]);

• devm_kfree(&ntb->dev, ntb->msi);
• ntb->msi = NULL;
• return ret;

+} +EXPORT_SYMBOL(ntb_msi_init);

+/**

• • ntb_msi_setup_mws() - Initialize the MSI inbound memory windows
• • @ntb: NTB device context
• • This function sets up the required inbound memory windows. It should be
• • called from a work function after a link up event.
• • Over the entire network, this function will reserves the last N
• • inbound memory windows for each peer (where N is the number of peers).
• • ntb_msi_init() must be called before this function.
• • Return: Zero on success, otherwise a negative error number.
• */

+int ntb_msi_setup_mws(struct ntb_dev *ntb) +{

• struct msi_desc *desc;
• u64 addr;
• int peer, peer_widx;
• resource_size_t addr_align, size_align, size_max;
• resource_size_t mw_size = SZ_32K;
• resource_size_t mw_min_size = mw_size;
• int i;
• int ret;
• if (!ntb->msi)

• return -EINVAL;
  

• desc = first_msi_entry(&ntb->pdev->dev);
• addr = desc->msg.address_lo + ((uint64_t)desc->msg.address_hi << 32);
• for (peer = 0; peer < ntb_peer_port_count(ntb); peer++) {

• peer_widx = ntb_peer_highest_mw_idx(ntb, peer);
  

• if (peer_widx < 0)
  

• 	return peer_widx;
  

• ret = ntb_mw_get_align(ntb, peer, peer_widx, &addr_align,
  

• 		       NULL, NULL);
  

• if (ret)
  

• 	return ret;
  

• addr &= ~(addr_align - 1);
  

• }
• for (peer = 0; peer < ntb_peer_port_count(ntb); peer++) {

• peer_widx = ntb_peer_highest_mw_idx(ntb, peer);
  

• if (peer_widx < 0) {
  

• 	ret = peer_widx;
  

• 	goto error_out;
  

• }
  

• ret = ntb_mw_get_align(ntb, peer, peer_widx, NULL,
  

• 		       &size_align, &size_max);
  

• if (ret)
  

• 	goto error_out;
  

• mw_size = round_up(mw_size, size_align);
  

• mw_size = max(mw_size, size_max);
  

• if (mw_size < mw_min_size)
  

• 	mw_min_size = mw_size;
  

• ret = ntb_mw_set_trans(ntb, peer, peer_widx,
  

• 		       addr, mw_size);
  

• if (ret)
  

• 	goto error_out;
  

Alas calling the ntb_mw_set_trans() method isn't enough to fully initialize NTB Memory Windows. Yes, the library will work for Intel/AMD/Switchtec (two-ports legacy configuration), but will fail for IDT due to being based on the outbound MW xlat interface. So the library at this stage isn't portable across all NTB hardware. In order to make it working the translation address is supposed to be transferred to the peer side, where a peer code should call ntb_peer_mw_set_trans() method with the retrieved xlat address. See documentation for details:

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Docume...

ntb_perf driver can be also used as a reference of the portable NTB MWs setup.

So I'd suggest to add some method like ntb_msi_peer_setup_mws() or similar which is supposed to be called on the peer side with a translation address or some common descriptor containing the address passed to the function argument.

It seems to me the test driver should be also altered to support this case.

• }
• ntb->msi->base_addr = addr;
• ntb->msi->end_addr = addr + mw_min_size;
• return 0;

+error_out:

• for (i = 0; i < peer; i++) {

• peer_widx = ntb_peer_highest_mw_idx(ntb, peer);
  

• if (peer_widx < 0)
  

• 	continue;
  

• ntb_mw_clear_trans(ntb, i, peer_widx);
  

• }

The same cleanup pattern can be utilized here: +error_out: + for (--peer; peer >= 0; --peer) { + peer_widx = ntb_peer_highest_mw_idx(ntb, peer); + ntb_mw_clear_trans(ntb, i, peer_widx); + }

So you won't need "i" variable here anymore. You also don't need to check the return value of ntb_peer_highest_mw_idx() in the cleanup loop because it was already checked in the main algo code.

• return ret;

+} +EXPORT_SYMBOL(ntb_msi_setup_mws);

+/**

• • ntb_msi_clear_mws() - Clear all inbound memory windows
• • @ntb: NTB device context
• • This function tears down the resources used by ntb_msi_setup_mws().
• */

+void ntb_msi_clear_mws(struct ntb_dev *ntb) +{

• int peer;
• int peer_widx;
• for (peer = 0; peer < ntb_peer_port_count(ntb); peer++) {

• peer_widx = ntb_peer_highest_mw_idx(ntb, peer);
  

• if (peer_widx < 0)
  

• 	continue;
  

• ntb_mw_clear_trans(ntb, peer, peer_widx);
  

• }

+} +EXPORT_SYMBOL(ntb_msi_clear_mws);

Similarly something like ntb_msi_peer_clear_mws() should be added to unset a translation address on the peer side.

+struct ntb_msi_devres {

• struct ntb_dev *ntb;
• struct msi_desc *entry;
• struct ntb_msi_desc *msi_desc;

+};

+static int ntb_msi_set_desc(struct ntb_dev *ntb, struct msi_desc *entry,

• 	    struct ntb_msi_desc *msi_desc)
  

+{

• u64 addr;
• addr = entry->msg.address_lo +

• ((uint64_t)entry->msg.address_hi << 32);
  

• if (addr < ntb->msi->base_addr || addr >= ntb->msi->end_addr) {

• dev_warn_once(&ntb->dev,
  

• 	      "IRQ %d: MSI Address not within the memory window (%llx, [%llx %llx])\n",
  

• 	      entry->irq, addr, ntb->msi->base_addr,
  

• 	      ntb->msi->end_addr);
  

• return -EFAULT;
  

• }
• msi_desc->addr_offset = addr - ntb->msi->base_addr;
• msi_desc->data = entry->msg.data;
• return 0;

+}

+static void ntb_msi_write_msg(struct msi_desc *entry, void *data) +{

• struct ntb_msi_devres *dr = data;
• WARN_ON(ntb_msi_set_desc(dr->ntb, entry, dr->msi_desc));
• if (dr->ntb->msi->desc_changed)

• dr->ntb->msi->desc_changed(dr->ntb->ctx);
  

+}

+static void ntbm_msi_callback_release(struct device *dev, void *res) +{

• struct ntb_msi_devres *dr = res;
• dr->entry->write_msi_msg = NULL;
• dr->entry->write_msi_msg_data = NULL;

+}

+static int ntbm_msi_setup_callback(struct ntb_dev *ntb, struct msi_desc *entry,

• 		   struct ntb_msi_desc *msi_desc)
  

+{

• struct ntb_msi_devres *dr;
• dr = devres_alloc(ntbm_msi_callback_release,

• 	  sizeof(struct ntb_msi_devres), GFP_KERNEL);
  

• if (!dr)

• return -ENOMEM;
  

• dr->ntb = ntb;
• dr->entry = entry;
• dr->msi_desc = msi_desc;
• devres_add(&ntb->dev, dr);
• dr->entry->write_msi_msg = ntb_msi_write_msg;
• dr->entry->write_msi_msg_data = dr;
• return 0;

+}

+/**

• • ntbm_msi_request_threaded_irq() - allocate an MSI interrupt
• • @ntb: NTB device context
• • @handler: Function to be called when the IRQ occurs
• • @thread_fn: Function to be called in a threaded interrupt context. NULL

• •          for clients which handle everything in @handler
    

• • @devname: An ascii name for the claiming device, dev_name(dev) if NULL
• • @dev_id: A cookie passed back to the handler function
• • This function assigns an interrupt handler to an unused
• • MSI interrupt and returns the descriptor used to trigger
• • it. The descriptor can then be sent to a peer to trigger
• • the interrupt.
• • The interrupt resource is managed with devres so it will
• • be automatically freed when the NTB device is torn down.
• • If an IRQ allocated with this function needs to be freed
• • separately, ntbm_free_irq() must be used.
• • Return: IRQ number assigned on success, otherwise a negative error number.
• */

+int ntbm_msi_request_threaded_irq(struct ntb_dev *ntb, irq_handler_t handler,

• 		  irq_handler_t thread_fn,
  

• 		  const char *name, void *dev_id,
  

• 		  struct ntb_msi_desc *msi_desc)
  

+{

• struct msi_desc *entry;
• struct irq_desc *desc;
• int ret;
• if (!ntb->msi)

• return -EINVAL;
  

• for_each_pci_msi_entry(entry, ntb->pdev) {

• desc = irq_to_desc(entry->irq);
  

• if (desc->action)
  

• 	continue;
  

• ret = devm_request_threaded_irq(&ntb->dev, entry->irq, handler,
  

• 				thread_fn, 0, name, dev_id);
  

• if (ret)
  

• 	continue;
  

• if (ntb_msi_set_desc(ntb, entry, msi_desc)) {
  

• 	devm_free_irq(&ntb->dev, entry->irq, dev_id);
  

• 	continue;
  

• }
  

• ret = ntbm_msi_setup_callback(ntb, entry, msi_desc);
  

• if (ret) {
  

• 	devm_free_irq(&ntb->dev, entry->irq, dev_id);
  

• 	return ret;
  

• }
  

• return entry->irq;
  

• }
• return -ENODEV;

+} +EXPORT_SYMBOL(ntbm_msi_request_threaded_irq);

+static int ntbm_msi_callback_match(struct device *dev, void *res, void *data) +{

• struct ntb_dev *ntb = dev_ntb(dev);
• struct ntb_msi_devres *dr = res;
• return dr->ntb == ntb && dr->entry == data;

+}

+/**

• • ntbm_msi_free_irq() - free an interrupt
• • @ntb: NTB device context
• • @irq: Interrupt line to free
• • @dev_id: Device identity to free
• • This function should be used to manually free IRQs allocated with
• • ntbm_request_[threaded_]irq().
• */

+void ntbm_msi_free_irq(struct ntb_dev *ntb, unsigned int irq, void *dev_id) +{

• struct msi_desc *entry = irq_get_msi_desc(irq);
• entry->write_msi_msg = NULL;
• entry->write_msi_msg_data = NULL;
• WARN_ON(devres_destroy(&ntb->dev, ntbm_msi_callback_release,

• 	       ntbm_msi_callback_match, entry));
  

• devm_free_irq(&ntb->dev, irq, dev_id);

+} +EXPORT_SYMBOL(ntbm_msi_free_irq);

+/**

• • ntb_msi_peer_trigger() - Trigger an interrupt handler on a peer
• • @ntb: NTB device context
• • @peer: Peer index
• • @desc: MSI descriptor data which triggers the interrupt
• • This function triggers an interrupt on a peer. It requires
• • the descriptor structure to have been passed from that peer
• • by some other means.
• • Return: Zero on success, otherwise a negative error number.
• */

+int ntb_msi_peer_trigger(struct ntb_dev *ntb, int peer,

• 	 struct ntb_msi_desc *desc)
  

+{

• int idx;
• if (!ntb->msi)

• return -EINVAL;
  

• idx = desc->addr_offset / sizeof(*ntb->msi->peer_mws[peer]);
• ntb->msi->peer_mws[peer][idx] = desc->data;

Shouldn't we use iowrite32() here instead of direct access to the IO-memory?

• return 0;

+} +EXPORT_SYMBOL(ntb_msi_peer_trigger);

+/**

• • ntb_msi_peer_addr() - Get the DMA address to trigger a peer's MSI interrupt
• • @ntb: NTB device context
• • @peer: Peer index
• • @desc: MSI descriptor data which triggers the interrupt
• • @msi_addr: Physical address to trigger the interrupt
• • This function allows using DMA engines to trigger an interrupt
• • (for example, trigger an interrupt to process the data after
• • sending it). To trigger the interrupt, write @desc.data to the address
• • returned in @msi_addr
• • Return: Zero on success, otherwise a negative error number.
• */

+int ntb_msi_peer_addr(struct ntb_dev *ntb, int peer,

•       struct ntb_msi_desc *desc,
  

•       phys_addr_t *msi_addr)
  

+{

• int peer_widx = ntb_peer_mw_count(ntb) - 1 - peer;
• phys_addr_t mw_phys_addr;
• int ret;
• ret = ntb_peer_mw_get_addr(ntb, peer_widx, &mw_phys_addr, NULL);
• if (ret)

• return ret;
  

• if (msi_addr)

• *msi_addr = mw_phys_addr + desc->addr_offset;
  

• return 0;

+} +EXPORT_SYMBOL(ntb_msi_peer_addr); diff --git a/include/linux/ntb.h b/include/linux/ntb.h index f5c69d853489..b9c61ee3c734 100644 --- a/include/linux/ntb.h +++ b/include/linux/ntb.h @@ -58,9 +58,11 @@ #include <linux/completion.h> #include <linux/device.h> +#include <linux/interrupt.h> struct ntb_client; struct ntb_dev; +struct ntb_msi; struct pci_dev; /** @@ -425,6 +427,10 @@ struct ntb_dev { spinlock_t ctx_lock; /* block unregister until device is fully released */ struct completion released;

• #ifdef CONFIG_NTB_MSI
• struct ntb_msi *msi;
• #endif

I'd align the macro-condition to the most left position: +#ifdef CONFIG_NTB_MSI + struct ntb_msi *msi; +#endif

}; #define dev_ntb(__dev) container_of((__dev), struct ntb_dev, dev) @@ -1572,4 +1578,71 @@ static inline int ntb_peer_highest_mw_idx(struct ntb_dev *ntb, int pidx) return ntb_mw_count(ntb, pidx) - ret - 1; } +struct ntb_msi_desc {

• u32 addr_offset;
• u32 data;

+};

+#ifdef CONFIG_NTB_MSI

+int ntb_msi_init(struct ntb_dev *ntb, void (*desc_changed)(void *ctx)); +int ntb_msi_setup_mws(struct ntb_dev *ntb); +void ntb_msi_clear_mws(struct ntb_dev *ntb); +int ntbm_msi_request_threaded_irq(struct ntb_dev *ntb, irq_handler_t handler,

• 		  irq_handler_t thread_fn,
  

• 		  const char *name, void *dev_id,
  

• 		  struct ntb_msi_desc *msi_desc);
  

+void ntbm_msi_free_irq(struct ntb_dev *ntb, unsigned int irq, void *dev_id); +int ntb_msi_peer_trigger(struct ntb_dev *ntb, int peer,

• 	 struct ntb_msi_desc *desc);
  

+int ntb_msi_peer_addr(struct ntb_dev *ntb, int peer,

•       struct ntb_msi_desc *desc,
  

•       phys_addr_t *msi_addr);
  

+#else /* not CONFIG_NTB_MSI */

+static inline int ntb_msi_init(struct ntb_dev *ntb,

• 	       void (*desc_changed)(void *ctx))
  

+{

• return -EOPNOTSUPP;

+} +static inline int ntb_msi_setup_mws(struct ntb_dev *ntb) +{

• return -EOPNOTSUPP;

+} +static inline void ntb_msi_clear_mws(struct ntb_dev *ntb) {} +static inline int ntbm_msi_request_threaded_irq(struct ntb_dev *ntb,

• 				irq_handler_t handler,
  

• 				irq_handler_t thread_fn,
  

• 				const char *name, void *dev_id,
  

• 				struct ntb_msi_desc *msi_desc)
  

+{

• return -EOPNOTSUPP;

+} +static inline void ntbm_msi_free_irq(struct ntb_dev *ntb, unsigned int irq,

• 		     void *dev_id) {}
  

+static inline int ntb_msi_peer_trigger(struct ntb_dev *ntb, int peer,

• 		       struct ntb_msi_desc *desc)
  

+{

• return -EOPNOTSUPP;

+} +static inline int ntb_msi_peer_addr(struct ntb_dev *ntb, int peer,

• 		    struct ntb_msi_desc *desc,
  

• 		    phys_addr_t *msi_addr)
  

+{

• return -EOPNOTSUPP;

+}

+#endif /* CONFIG_NTB_MSI */

+static inline int ntbm_msi_request_irq(struct ntb_dev *ntb,

• 		       irq_handler_t handler,
  

• 		       const char *name, void *dev_id,
  

• 		       struct ntb_msi_desc *msi_desc)
  

+{

• return ntbm_msi_request_threaded_irq(ntb, handler, NULL, name,

• 			     dev_id, msi_desc);
  

+}

#endif

2.19.0

-- You received this message because you are subscribed to the Google Groups "linux-ntb" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+unsubscribe@googlegroups.com. To post to this group, send email to linux-ntb@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/20190213175454.7506-10-logang%40.... For more options, visit https://groups.google.com/d/optout.

On Wed, Mar 06, 2019 at 02:35:53PM -0700, Logan Gunthorpe wrote:

On 2019-03-06 1:26 p.m., Serge Semin wrote:

...

First of all, It might be unsafe to have some resources consumed by NTB MSI or some other library without a simple way to warn NTB client drivers about their attempts to access that resources, since it might lead to random errors. When I thought about implementing a transport library based on the Message/Spad+Doorbell registers, I had in mind to create an internal bits-field array with the resources busy-flags. If, for instance, some message or scratchpad register is occupied by the library (MSI, transport or some else), then it would be impossible to access these resources directly through NTB API methods. So NTB client driver shall retrieve an error in an attempt to write/read data to/from busy message or scratchpad register, or in an attempt to set some occupied doorbell bit. The same thing can be done for memory windows.

Yes, it would be nice to have a generic library to manage all the resources, but right now we don't and it's unfair to expect us to take on this work to get the features we care about merged. Right now, it's not at all unsafe as the client is quite capable of ensuring it has the resources for the MSI library. The changes for ntb_transport to ensure this are quite reasonable.

...

Second tiny concern is about documentation. Since there is a special file for all NTB-related doc, it would be good to have some description about the NTB MSI library there as well: Documentation/ntb.txt

Sure, I'll add a short blurb for v3. Though, I noticed it's quite out of date since your changes. Especially in the ntb_tool section...

Ok. Thanks. If you want you can add some info to the ntb_tool section as well. If you don't have time, I'll update it next time I submit anything new to the subsystem.

-Sergey

...

...

• u32 *peer_mws[];

Shouldn't we use the __iomem attribute here since later the devm_ioremap() is used to map MWs at these pointers?

Yes, will change for v3.

...

Simpler and faster cleanup-code would be:

...

• unroll:
• for (--i; i >= 0; --i)

• devm_iounmap(&ntb->dev, ntb->msi->peer_mws[i]);
  

Faster, maybe, but I would not consider this simpler. It's much more complicated to reason about and ensure it's correct. I prefer my way because I don't care about speed, but I do care about readability.

...

Alas calling the ntb_mw_set_trans() method isn't enough to fully initialize NTB Memory Windows. Yes, the library will work for Intel/AMD/Switchtec (two-ports legacy configuration), but will fail for IDT due to being based on the outbound MW xlat interface. So the library at this stage isn't portable across all NTB hardware. In order to make it working the translation address is supposed to be transferred to the peer side, where a peer code should call ntb_peer_mw_set_trans() method with the retrieved xlat address. See documentation for details:

...

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Docume...

ntb_perf driver can be also used as a reference of the portable NTB MWs setup.

Gross. Well, given that ntb_transport doesn't even support this and we don't really have a sensible library to transfer this information, I'm going to leave it as is for now. Someone can update ntb_msi when they update ntb_transport, preferably after we have a nice library to handle the transfers for us seeing I absolutely do not want to replicate the mess in ntb_perf.

Actually, if we had a generic spad/msg communication library, it would probably be better to have a common ntb_mw_set_trans() function that uses the communications library to send the data and automatically call ntb_peer_mw_set_trans() on the peer. That way we don't have to push this mess into the clients.

...

The same cleanup pattern can be utilized here: +error_out:

• for (--peer; peer >= 0; --peer) {

• peer_widx = ntb_peer_highest_mw_idx(ntb, peer);
  

• ntb_mw_clear_trans(ntb, i, peer_widx);
  

• }

So you won't need "i" variable here anymore. You also don't need to check the return value of ntb_peer_highest_mw_idx() in the cleanup loop because it was already checked in the main algo code.

See above.

...

...

+EXPORT_SYMBOL(ntb_msi_clear_mws);

Similarly something like ntb_msi_peer_clear_mws() should be added to unset a translation address on the peer side.

Well, we can table that for when ntb_msi supports the peer MW setting functions.

...

...

+int ntb_msi_peer_trigger(struct ntb_dev *ntb, int peer,

• 	 struct ntb_msi_desc *desc)
  

+{

• int idx;
• if (!ntb->msi)

• return -EINVAL;
  

• idx = desc->addr_offset / sizeof(*ntb->msi->peer_mws[peer]);
• ntb->msi->peer_mws[peer][idx] = desc->data;

Shouldn't we use iowrite32() here instead of direct access to the IO-memory?

Yes, as above I'll fix it for v3.

...

...

@@ -425,6 +427,10 @@ struct ntb_dev { spinlock_t ctx_lock; /* block unregister until device is fully released */ struct completion released;

• #ifdef CONFIG_NTB_MSI
• struct ntb_msi *msi;
• #endif

I'd align the macro-condition to the most left position: +#ifdef CONFIG_NTB_MSI

• struct ntb_msi *msi;

+#endif

Fixed for v3.

Logan

-- You received this message because you are subscribed to the Google Groups "linux-ntb" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+unsubscribe@googlegroups.com. To post to this group, send email to linux-ntb@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/5b420eb7-5010-aae3-e9bd-1c612af4.... For more options, visit https://groups.google.com/d/optout.

Hi

On Wed, Feb 13, 2019 at 10:54:52AM -0700, Logan Gunthorpe wrote:

Introduce a tool to test NTB MSI interrupts similar to the other NTB test tools. This tool creates a debugfs directory for each NTB device with the following files:

port irqX_occurrences peerX/port peerX/count peerX/trigger

The 'port' file tells the user the local port number and the 'occurrences' files tell the number of local interrupts that have been received for each interrupt.

For each peer, the 'port' file and the 'count' file tell you the peer's port number and number of interrupts respectively. Writing the interrupt number to the 'trigger' file triggers the interrupt handler for the peer which should increment their corresponding 'occurrences' file. The 'ready' file indicates if a peer is ready, writing to this file blocks until it is ready.

The module parameter num_irqs can be used to set the number of local interrupts. By default this is 4. This is only limited by the number of unused MSI interrupts registered by the hardware (this will require support of the hardware driver) and there must be at least 2*num_irqs + 1 spads registers available.

Alas the test driver is not going to work with IDT NTB hardware, since it uses Spad only, which aren't supported by IDT devices. IDT NTB PCIe functions provide message registers to communicate with peers. See ntb_perf driver code for reference of portable driver design.

In order to make the test driver portable the following alterations are needed: 1) Add NTB Message register API usage together with Scratchpad+Doorbell. 2) When NTB MSI library is updated with functions like ntb_msi_peer_setup_mws()/ntb_msi_peer_clear_mws() they are needed to be utilized in the test driver as well to set a translation address on the peer side.

Signed-off-by: Logan Gunthorpe logang@deltatee.com Cc: Jon Mason jdmason@kudzu.us Cc: Dave Jiang dave.jiang@intel.com Cc: Allen Hubbe allenbh@gmail.com

---

drivers/ntb/test/Kconfig | 9 + drivers/ntb/test/Makefile | 1 + drivers/ntb/test/ntb_msi_test.c | 433 ++++++++++++++++++++++++++++++++ 3 files changed, 443 insertions(+) create mode 100644 drivers/ntb/test/ntb_msi_test.c

diff --git a/drivers/ntb/test/Kconfig b/drivers/ntb/test/Kconfig index a5d0eda44438..a3f3e2638935 100644 --- a/drivers/ntb/test/Kconfig +++ b/drivers/ntb/test/Kconfig @@ -25,3 +25,12 @@ config NTB_PERF to and from the window without additional software interaction. If unsure, say N.

+config NTB_MSI_TEST

• tristate "NTB MSI Test Client"
• depends on NTB_MSI
• help

•  This tool demonstrates the use of the NTB MSI library to
  

•  send MSI interrupts between peers.
  

•  If unsure, say N.
  

diff --git a/drivers/ntb/test/Makefile b/drivers/ntb/test/Makefile index 9e77e0b761c2..d2895ca995e4 100644 --- a/drivers/ntb/test/Makefile +++ b/drivers/ntb/test/Makefile @@ -1,3 +1,4 @@ obj-$(CONFIG_NTB_PINGPONG) += ntb_pingpong.o obj-$(CONFIG_NTB_TOOL) += ntb_tool.o obj-$(CONFIG_NTB_PERF) += ntb_perf.o +obj-$(CONFIG_NTB_MSI_TEST) += ntb_msi_test.o diff --git a/drivers/ntb/test/ntb_msi_test.c b/drivers/ntb/test/ntb_msi_test.c new file mode 100644 index 000000000000..99d826ed9c34 --- /dev/null +++ b/drivers/ntb/test/ntb_msi_test.c @@ -0,0 +1,433 @@ +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)

+#include <linux/module.h> +#include <linux/debugfs.h> +#include <linux/ntb.h> +#include <linux/pci.h> +#include <linux/radix-tree.h> +#include <linux/workqueue.h>

+MODULE_LICENSE("Dual BSD/GPL"); +MODULE_VERSION("0.1"); +MODULE_AUTHOR("Logan Gunthorpe logang@deltatee.com"); +MODULE_DESCRIPTION("Test for sending MSI interrupts over an NTB memory window");

+static int num_irqs = 4; +module_param(num_irqs, int, 0644); +MODULE_PARM_DESC(num_irqs, "number of irqs to use");

+struct ntb_msit_ctx {

• struct ntb_dev *ntb;
• struct dentry *dbgfs_dir;
• struct work_struct setup_work;
• struct ntb_msit_isr_ctx {

• int irq_idx;
  

• int irq_num;
  

• int occurrences;
  

• struct ntb_msit_ctx *nm;
  

• struct ntb_msi_desc desc;
  

• } *isr_ctx;
• struct ntb_msit_peer {

• struct ntb_msit_ctx *nm;
  

• int pidx;
  

• int num_irqs;
  

• struct completion init_comp;
  

• struct ntb_msi_desc *msi_desc;
  

• } peers[];

+};

+static struct dentry *ntb_msit_dbgfs_topdir;

+static irqreturn_t ntb_msit_isr(int irq, void *dev) +{

• struct ntb_msit_isr_ctx *isr_ctx = dev;
• struct ntb_msit_ctx *nm = isr_ctx->nm;
• dev_dbg(&nm->ntb->dev, "Interrupt Occurred: %d",

• isr_ctx->irq_idx);
  

• isr_ctx->occurrences++;
• return IRQ_HANDLED;

+}

+static void ntb_msit_setup_work(struct work_struct *work) +{

• struct ntb_msit_ctx *nm = container_of(work, struct ntb_msit_ctx,

• 			       setup_work);
  

• int irq_count = 0;
• int irq;
• int ret;
• uintptr_t i;
• ret = ntb_msi_setup_mws(nm->ntb);
• if (ret) {

• dev_err(&nm->ntb->dev, "Unable to setup MSI windows: %d\n",
  

• 	ret);
  

• return;
  

• }
• for (i = 0; i < num_irqs; i++) {

• nm->isr_ctx[i].irq_idx = i;
  

• nm->isr_ctx[i].nm = nm;
  

• if (!nm->isr_ctx[i].irq_num) {
  

• 	irq = ntbm_msi_request_irq(nm->ntb, ntb_msit_isr,
  

• 				   KBUILD_MODNAME,
  

• 				   &nm->isr_ctx[i],
  

• 				   &nm->isr_ctx[i].desc);
  

• 	if (irq < 0)
  

• 		break;
  

• 	nm->isr_ctx[i].irq_num = irq;
  

• }
  

• ret = ntb_spad_write(nm->ntb, 2 * i + 1,
  

• 		     nm->isr_ctx[i].desc.addr_offset);
  

• if (ret)
  

• 	break;
  

• ret = ntb_spad_write(nm->ntb, 2 * i + 2,
  

• 		     nm->isr_ctx[i].desc.data);
  

• if (ret)
  

• 	break;
  

• irq_count++;
  

• }
• ntb_spad_write(nm->ntb, 0, irq_count);
• ntb_peer_db_set(nm->ntb, BIT(ntb_port_number(nm->ntb)));

+}

+static void ntb_msit_desc_changed(void *ctx) +{

• struct ntb_msit_ctx *nm = ctx;
• int i;
• dev_dbg(&nm->ntb->dev, "MSI Descriptors Changed\n");
• for (i = 0; i < num_irqs; i++) {

• ntb_spad_write(nm->ntb, 2 * i + 1,
  

• 	       nm->isr_ctx[i].desc.addr_offset);
  

• ntb_spad_write(nm->ntb, 2 * i + 2,
  

• 	       nm->isr_ctx[i].desc.data);
  

• }
• ntb_peer_db_set(nm->ntb, BIT(ntb_port_number(nm->ntb)));

+}

+static void ntb_msit_link_event(void *ctx) +{

• struct ntb_msit_ctx *nm = ctx;
• if (!ntb_link_is_up(nm->ntb, NULL, NULL))

• return;
  

• schedule_work(&nm->setup_work);

+}

+static void ntb_msit_copy_peer_desc(struct ntb_msit_ctx *nm, int peer) +{

• int i;
• struct ntb_msi_desc *desc = nm->peers[peer].msi_desc;
• int irq_count = nm->peers[peer].num_irqs;
• for (i = 0; i < irq_count; i++) {

• desc[i].addr_offset = ntb_peer_spad_read(nm->ntb, peer,
  

• 					 2 * i + 1);
  

• desc[i].data = ntb_peer_spad_read(nm->ntb, peer, 2 * i + 2);
  

• }
• dev_info(&nm->ntb->dev, "Found %d interrupts on peer %d\n",

•  irq_count, peer);
  

• complete_all(&nm->peers[peer].init_comp);

+}

+static void ntb_msit_db_event(void *ctx, int vec) +{

• struct ntb_msit_ctx *nm = ctx;
• struct ntb_msi_desc *desc;
• u64 peer_mask = ntb_db_read(nm->ntb);
• u32 irq_count;
• int peer;
• ntb_db_clear(nm->ntb, peer_mask);
• for (peer = 0; peer < sizeof(peer_mask) * 8; peer++) {

• if (!(peer_mask & BIT(peer)))
  

• 	continue;
  

• irq_count = ntb_peer_spad_read(nm->ntb, peer, 0);
  

• if (irq_count == -1)
  

• 	continue;
  

• desc = kcalloc(irq_count, sizeof(*desc), GFP_ATOMIC);
  

• if (!desc)
  

• 	continue;
  

• kfree(nm->peers[peer].msi_desc);
  

• nm->peers[peer].msi_desc = desc;
  

• nm->peers[peer].num_irqs = irq_count;
  

• ntb_msit_copy_peer_desc(nm, peer);
  

• }

+}

+static const struct ntb_ctx_ops ntb_msit_ops = {

• .link_event = ntb_msit_link_event,
• .db_event = ntb_msit_db_event,

+};

+static int ntb_msit_dbgfs_trigger(void *data, u64 idx) +{

• struct ntb_msit_peer *peer = data;
• if (idx >= peer->num_irqs)

• return -EINVAL;
  

• dev_dbg(&peer->nm->ntb->dev, "trigger irq %llu on peer %u\n",

• idx, peer->pidx);
  

• return ntb_msi_peer_trigger(peer->nm->ntb, peer->pidx,

• 		    &peer->msi_desc[idx]);
  

+}

+DEFINE_DEBUGFS_ATTRIBUTE(ntb_msit_trigger_fops, NULL,

• 	 ntb_msit_dbgfs_trigger, "%llu\n");
  

+static int ntb_msit_dbgfs_port_get(void *data, u64 *port) +{

• struct ntb_msit_peer *peer = data;
• *port = ntb_peer_port_number(peer->nm->ntb, peer->pidx);
• return 0;

+}

+DEFINE_DEBUGFS_ATTRIBUTE(ntb_msit_port_fops, ntb_msit_dbgfs_port_get,

• 	 NULL, "%llu\n");
  

+static int ntb_msit_dbgfs_count_get(void *data, u64 *count) +{

• struct ntb_msit_peer *peer = data;
• *count = peer->num_irqs;
• return 0;

+}

+DEFINE_DEBUGFS_ATTRIBUTE(ntb_msit_count_fops, ntb_msit_dbgfs_count_get,

• 	 NULL, "%llu\n");
  

+static int ntb_msit_dbgfs_ready_get(void *data, u64 *ready) +{

• struct ntb_msit_peer *peer = data;
• *ready = try_wait_for_completion(&peer->init_comp);
• return 0;

+}

+static int ntb_msit_dbgfs_ready_set(void *data, u64 ready) +{

• struct ntb_msit_peer *peer = data;
• return wait_for_completion_interruptible(&peer->init_comp);

+}

+DEFINE_DEBUGFS_ATTRIBUTE(ntb_msit_ready_fops, ntb_msit_dbgfs_ready_get,

• 	 ntb_msit_dbgfs_ready_set, "%llu\n");
  

+static int ntb_msit_dbgfs_occurrences_get(void *data, u64 *occurrences) +{

• struct ntb_msit_isr_ctx *isr_ctx = data;
• *occurrences = isr_ctx->occurrences;
• return 0;

+}

+DEFINE_DEBUGFS_ATTRIBUTE(ntb_msit_occurrences_fops,

• 	 ntb_msit_dbgfs_occurrences_get,
  

• 	 NULL, "%llu\n");
  

+static int ntb_msit_dbgfs_local_port_get(void *data, u64 *port) +{

• struct ntb_msit_ctx *nm = data;
• *port = ntb_port_number(nm->ntb);
• return 0;

+}

+DEFINE_DEBUGFS_ATTRIBUTE(ntb_msit_local_port_fops,

• 	 ntb_msit_dbgfs_local_port_get,
  

• 	 NULL, "%llu\n");
  

+static void ntb_msit_create_dbgfs(struct ntb_msit_ctx *nm) +{

• struct pci_dev *pdev = nm->ntb->pdev;
• char buf[32];
• int i;
• struct dentry *peer_dir;
• nm->dbgfs_dir = debugfs_create_dir(pci_name(pdev),

• 			   ntb_msit_dbgfs_topdir);
  

• debugfs_create_file("port", 0400, nm->dbgfs_dir, nm,

• 	    &ntb_msit_local_port_fops);
  

• for (i = 0; i < ntb_peer_port_count(nm->ntb); i++) {

• nm->peers[i].pidx = i;
  

• nm->peers[i].nm = nm;
  

• init_completion(&nm->peers[i].init_comp);
  

• snprintf(buf, sizeof(buf), "peer%d", i);
  

• peer_dir = debugfs_create_dir(buf, nm->dbgfs_dir);
  

• debugfs_create_file_unsafe("trigger", 0200, peer_dir,
  

• 			   &nm->peers[i],
  

• 			   &ntb_msit_trigger_fops);
  

• debugfs_create_file_unsafe("port", 0400, peer_dir,
  

• 			   &nm->peers[i], &ntb_msit_port_fops);
  

• debugfs_create_file_unsafe("count", 0400, peer_dir,
  

• 			   &nm->peers[i],
  

• 			   &ntb_msit_count_fops);
  

• debugfs_create_file_unsafe("ready", 0600, peer_dir,
  

• 			   &nm->peers[i],
  

• 			   &ntb_msit_ready_fops);
  

• }
• for (i = 0; i < num_irqs; i++) {

• snprintf(buf, sizeof(buf), "irq%d_occurrences", i);
  

• debugfs_create_file_unsafe(buf, 0400, nm->dbgfs_dir,
  

• 			   &nm->isr_ctx[i],
  

• 			   &ntb_msit_occurrences_fops);
  

• }

+}

+static void ntb_msit_remove_dbgfs(struct ntb_msit_ctx *nm) +{

• debugfs_remove_recursive(nm->dbgfs_dir);

+}

+static int ntb_msit_probe(struct ntb_client *client, struct ntb_dev *ntb) +{

• struct ntb_msit_ctx *nm;
• size_t struct_size;
• int peers;
• int ret;
• peers = ntb_peer_port_count(ntb);
• if (peers <= 0)

• return -EINVAL;
  

• if (ntb_spad_is_unsafe(ntb) || ntb_spad_count(ntb) < 2 * num_irqs + 1) {

• dev_err(&ntb->dev, "NTB MSI test requires at least %d spads for %d irqs\n",
  

• 	2 * num_irqs + 1, num_irqs);
  

• return -EFAULT;
  

• }
• ret = ntb_spad_write(ntb, 0, -1);
• if (ret) {

• dev_err(&ntb->dev, "Unable to write spads: %d\n", ret);
  

• return ret;
  

• }
• ret = ntb_db_clear_mask(ntb, GENMASK(peers - 1, 0));
• if (ret) {

• dev_err(&ntb->dev, "Unable to clear doorbell mask: %d\n", ret);
  

• return ret;
  

• }
• ret = ntb_msi_init(ntb, ntb_msit_desc_changed);
• if (ret) {

• dev_err(&ntb->dev, "Unable to initialize MSI library: %d\n",
  

• 	ret);
  

• return ret;
  

• }
• struct_size = sizeof(*nm) + sizeof(*nm->peers) * peers;
• nm = devm_kzalloc(&ntb->dev, struct_size, GFP_KERNEL);
• if (!nm)

• return -ENOMEM;
  

• nm->isr_ctx = devm_kcalloc(&ntb->dev, num_irqs, sizeof(*nm->isr_ctx),

• 		   GFP_KERNEL);
  

• if (!nm->isr_ctx)

• return -ENOMEM;
  

• INIT_WORK(&nm->setup_work, ntb_msit_setup_work);
• nm->ntb = ntb;
• ntb_msit_create_dbgfs(nm);
• ret = ntb_set_ctx(ntb, nm, &ntb_msit_ops);
• if (ret)

• goto remove_dbgfs;
  

• if (!nm->isr_ctx)

• goto remove_dbgfs;
  

• ntb_link_enable(ntb, NTB_SPEED_AUTO, NTB_WIDTH_AUTO);
• return 0;

+remove_dbgfs:

• ntb_msit_remove_dbgfs(nm);
• devm_kfree(&ntb->dev, nm->isr_ctx);
• devm_kfree(&ntb->dev, nm);
• return ret;

+}

+static void ntb_msit_remove(struct ntb_client *client, struct ntb_dev *ntb) +{

• struct ntb_msit_ctx *nm = ntb->ctx;
• int i;
• ntb_link_disable(ntb);
• ntb_db_set_mask(ntb, ntb_db_valid_mask(ntb));
• ntb_msi_clear_mws(ntb);
• for (i = 0; i < ntb_peer_port_count(ntb); i++)

• kfree(nm->peers[i].msi_desc);
  

• ntb_clear_ctx(ntb);
• ntb_msit_remove_dbgfs(nm);

+}

+static struct ntb_client ntb_msit_client = {

• .ops = {

• .probe = ntb_msit_probe,
  

• .remove = ntb_msit_remove
  

• }

+};

+static int __init ntb_msit_init(void) +{

• int ret;
• if (debugfs_initialized())

• ntb_msit_dbgfs_topdir = debugfs_create_dir(KBUILD_MODNAME,
  

• 					   NULL);
  

• ret = ntb_register_client(&ntb_msit_client);
• if (ret)

• debugfs_remove_recursive(ntb_msit_dbgfs_topdir);
  

• return ret;

+} +module_init(ntb_msit_init);

+static void __exit ntb_msit_exit(void) +{

• ntb_unregister_client(&ntb_msit_client);
• debugfs_remove_recursive(ntb_msit_dbgfs_topdir);

+}

+module_exit(ntb_msit_exit);

2.19.0

-- You received this message because you are subscribed to the Google Groups "linux-ntb" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+unsubscribe@googlegroups.com. To post to this group, send email to linux-ntb@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/20190213175454.7506-11-logang%40.... For more options, visit https://groups.google.com/d/optout.