Add MSI/MSI-X support to intrng

[andrew]

Summary

We do this by adding a new interface MSI controllers need to implement.
The intrng framework manages these in a similar way to the existing
interrupt controllers so the code to manage lists of controllers has
been generalised.

The MSI controller is expected to work with an interrupt controller.
As such the interface will return the PIC that will handle the interrupt.

This has been tested with the generic PCIe controller driver, and the
GICv2m MSI controller.

[andrew]

sys/arm/arm/gic.c

1629–1631 ↗

(On Diff #15286)

@jhb Is this correct to align the interrupt allocation?

[emaste]

sys/arm/arm/gic.c

131–132 ↗

(On Diff #15286)

Could these two have a comment to explain the relationship?

[andrew]

Add a comment explaining the MSI flags

[skra]

sys/arm/arm/gic.c

593 ↗

(On Diff #15380)

All MSI and MSI-X interrupts use INTR_TRIGGER_EDGE and INTR_POLARITY_HIGH in gic. IMO, they may be set only once here and never cleared. There is already GI_FLAG_MSI flag in ISRC, so it should be save.

1061 ↗

(On Diff #15380)

Related to note in intr_internal_map_irq().

1110 ↗

(On Diff #15380)

Hmm, this is my bad. The idea is that if an interrupt does not need to be configured explicitly, the data may be NULL. Thus, all methods which takes struct intr_map_data as an argument should be changed to deal with it now, when it may be a case.

It's related to note in arm_gic_reserve_msi_range().

1142 ↗

(On Diff #15380)

Related to note in arm_gic_reserve_msi_range().

1669 ↗

(On Diff #15380)

Related to note in arm_gic_reserve_msi_range().

1701 ↗

(On Diff #15380)

Related to note in arm_gic_reserve_msi_range().

1738 ↗

(On Diff #15380)

Related to note in arm_gic_reserve_msi_range().

1762 ↗

(On Diff #15380)

Related to note in arm_gic_reserve_msi_range().

sys/kern/subr_intr.c

109 ↗

(On Diff #15380)

I'm not sure that two (or more if it comes) lists is good option. What about to add a flag to struct intr_pic and to related functions? How many PICs will be in a system? If 10 or 20, one list should be enough for them.

602 ↗

(On Diff #15380)

IMO, this is not needed at all as there is no configuration data saved in the struct for this type. Global interrupt number is stored in ISRC. Note that all stuff around struct intr_dev_data is temporary and should not be used for new cases. It will be removed when OFW buses will be fixed for INTRNG.

1284 ↗

(On Diff #15380)

Hmm, pic_dev is never NULL here. It should be fixed in intr_map_irq() too. My bad.

1291 ↗

(On Diff #15380)

irqs[i] = isrc[i]->isrc_irq;

Related to note in intr_internal_map_irq().

1309 ↗

(On Diff #15380)

Same note as in intr_alloc_msi().

1315 ↗

(On Diff #15380)

isrc[i] = isrc_lookup(irq[i]);

Related to note in intr_internal_map_irq().

1336 ↗

(On Diff #15380)

Same note as in intr_alloc_msi().

1343 ↗

(On Diff #15380)

*irq = isrc->isrc_irq;

Related to note in intr_internal_map_irq().

1355 ↗

(On Diff #15380)

Same note as in intr_alloc_msi().

1358 ↗

(On Diff #15380)

isrc = isrc_lookup(irq);

Related to note in intr_internal_map_irq().

1375 ↗

(On Diff #15380)

Same note as in intr_alloc_msi().

1378 ↗

(On Diff #15380)

isrc = isrc_lookup(irq);

Related to note in intr_internal_map_irq().

[skra]

sys/arm/arm/gic.c

592 ↗

(On Diff #15380)

What is the reason for these two KASSERTs?

[jhb]

sys/arm/arm/gic.c

1629–1631 ↗

(On Diff #15286)

For MSI (but not MSI-X), an allocation of multiple messages is required to have the 'data' value (the value written to the 'data' register in the MSI capability in PCI config space) aligned such that the low N bits are zero. The PCI function stores the specific message number in the low N bits to generate the 'data' value in the MSI message it sends out to the bus.

If you are storing the IRQ value as the 'data' value (x86 stores the IDT vector in the low 8 bits for example), then, yes, the IRQ must be aligned.

[andrew]

Update based on comments

sys/kern/subr_intr.c

109 ↗

(On Diff #15380)

They are lists of different things. Having two lists help ensure we can only get the correct device type when looking it up.

1315 ↗

(On Diff #15380)

How do you propose handling the unknown number of interrupt ids we will need to handle with the GICv3? The MSI space starts at 8192 and is an implementation defined size. From my quick reading of the GICv3 spec it could be up to 2^24 interrupt IDs.

[skra]

sys/arm/arm/gic.c

592 ↗

(On Diff #15380)

Well, I have a reason for the question. For example, testing isrc_handlers could be better if you are checking that the interrupt is not used by anyone.

sys/kern/subr_intr.c

109 ↗

(On Diff #15380)

Well, I disagree with you. For me, they are all PIC. Will we start to have one list for gpio PICs, one list for i2c PICs, another list for pci PICs?

Tell me, the device_t pointers which are registered into INTRNG are different for gic and gicv2m? The xrefs are different too? Maybe, no flag is needed at all to distinguish these devices.

1315 ↗

(On Diff #15380)

System has some limitions always. A maximal number of processes, threads, opened files. So, NIRQS says how many interrupts can be serviced by system. This defines a size of global (system scope) interrupt number (name)space. If you have enough memory then it can be 2^24.

Maybe, the question was about a size of local (gic scope) interrupt IDs (name)space. Then it's up to gic implementation how it wants to manage its interrupts IDs. If gic wants to make public some interrupt ID, it creates ISRC and registers it to INTRNG. No one dictates that gic must keep its ISRCs in a table. It could be a list, some kind of tree, whatever. However, when an interrupt comes, gic must be able to find corresponding ISRC quickly. It's the only limitation. Of course, gic also should save an interrupt ID in its extension of ISRC as INTRNG communicates with gic by ISRCs.

However, I think that even gic could make some limitation of how many interrupts it can service. So ISRCs in a table is good for me. There could be holes in interrupts IDs which are used. Then only thing you must know is how to process an interrupt ID to get an index to ISRC table.

Or are you saying that gic has no control over how interrupt IDs are allocated by consumers? Then the main problem is still how to find ISRC quickly when an interrupt comes. Ideally, it would be nice to be able to save some cookie to "hardware" so when an interrupt comes, associated ISRC could be find by this cookie. But now, I probably ran far away from your question.

BTW, do you have any reason why you did not replace all appearance of intr_ddata_lookup() by isrc_lookup() in your diff? Of course, it works this way too as isrc_lookup() is called inside of intr_ddata_lookup(), but using intr_ddata_lookup() is wrong now.

[andrew]

sys/kern/subr_intr.c

109 ↗

(On Diff #15380)

All your examples implement the PIC interface. the MSI interface is different and may or may not implement any of the PIC functions. The gic implements the PIC interface, the gicv2m implements the MSI interface.

1315 ↗

(On Diff #15380)

On the GICv3 interrupts are mapped as:

0  - 15: SGI
16 - 31: PPI
32 - 1019: SPI
1020 - 1023: Special interrupts
1024 - 8191: Reserved
8192 - n: LPI -- a message based interrupt, e.g. Used to handle MSI and MSI-X

We don't control the mapping within the PPI or SPI space. The size of n is implementation defined, but if LPIs are implemented it will be a power of 2 between 2^14 and 2^24, but we do have some control over its allocation for MSI interrupts.

I would prefer to not have a static array of pointers as I would expect having 128MB of BSS will cause the early boot to fail.

The LPI space will be a little separate as there will be children each managing a contiguous blocks in the LPI space.

[andrew]

Remove a few missed calls to intr_ddata_lookup.

[andrew]

KASSERT isrc_handlers is zero

[skra]

sys/kern/subr_intr.c

109 ↗

(On Diff #15380)

Hmm, I see your point. But when your argument is two different interfaces - PIC and MSI - then there should be two different structs - struct pic and struct msi. So I suggest to split that totally. Further, when isrc_lookup() will be made global, the msi stuff can be in separate file.

BTW, I plan to change the way how the struct pic is kept in list, so there will be the possibility to register more xrefs for one device. I.e., there will be triplet - device_t, xref, struct pic * - in the list. When the triplet will be - device_t, xref, void* - it would be used for both struct pic and struct msi.

1315 ↗

(On Diff #15380)

Interrupts on which we don't control how they are allocated by consumers, the corresponding ISRCs must be allocated and registered in advance. Otherwise any request to setup such interrupt will end with EINVAL.

In MSI/MSI-X case, where we assign some interrupt to a consumer, the corresponding ISRC may be registered in time of assignment. So even allocation of ISRC may be done at that time. But PIC must know about its ISRCs, so it must keep pointers to them somewhere. For example, for interrupts 0 - 1019, it will be table, while for interrupts 8192 - n, it will be a list, a tree, or a set of small dynamically allocated tables, whatever. BTW, the way how the ISRCs are stored and lookup for MSI/MSI-X interrupts may be general and in MSI specific file. ;)

But remember what I said. When an interrupt comes, PIC must be able to find corresponding ISRC quickly.