Add an intrng GICv3 ITS driver

[andrew]

Summary

Add a new GICv3 ITS driver to handle intrng. As many of the interfaces have changed and to not break the existing driver the driver has been moved to a new file, however much of the code has been moved and been updated from the existing ITS driver.

This driver is intended to reduce the interdependence between it and the GICv3. The ITS driver is able to know about the GICv3 driver, but the GICv3 driver needs to pass through intrng to interact with the ITS driver.

[andrew]

Finish the intrng driver for review

[zbb]

I will refrain from making further comments for separate lines or code blocks, I think what we all see here is clear.
Basically I don't understand this methodology to create a new file while adding support for INTRNG :-).
As far as I know (please correct me if I'm wrong), for INTRNG we need a set of callbacks such as enable interrupt, disable interrupt, post-filter, pre-ithread, etc. These are basically what we do now in GIC/ITS, if_pic.m so minor changes should be required to modify existing code and add missing routines required by INTRNG.
So how would you justify a need for a new file, moving the code around, changing names and definitions, etc.?

sys/arm64/arm64/gic_v3.c

189

Why do we need to create new accessors? Old ones will not work with INTRNG?

377

How this is needed for INTRNG?

sys/arm64/arm64/gicv3_its.c

153

Is renaming fields and their order in the softc structure required for INTRNG?

160

redist_lpis was the wrong place for these in terms of INTRNG?

174

This looks familiar. What is wrong with (1UL << 0) that we need to redefine it? How this is needed for INTRNG?

510

Block below was originally part of lpi_config_cpu() called from here. This would not work with INTRNG?

521

gicr_xbaser needed to be changed?

1096

adding htole64() is in no way related to supporting INTRNG. This should be added in a separate commit and is not necessary as long as we don't use Big Endian.

1531

But we already have FDT attachment in the separate file

[andrew]

Use vmem to alloc interrupts

[andrew]

I wanted to change many of the internal functions to handle the differences between the old interrupt handling code and intrng, e.g. the its_cmd_ now take a struct gicv3_its_irqsrc * as this fully describes the interrupt.

I have also used this a chance to reduce the interdependence between the GICv3 and ITS drivers, e.g. we could build a kernel without ITS support if needed. We should also be able to change the GICv3 driver around if needed as the ITS driver should only know about a few public interfaces.

sys/arm64/arm64/gic_v3.c

189

I am trying to reduce the knowledge the ITS needs for the GICv3 driver. To help with this these take the gic_v3 device_t object and find the softc from that as the ITS driver shouldn't need to know about this.

377

It's used in the ITS driver when it calls gicv3_get_redist_vaddr and gicv3_get_nirqs. I don't like the latter use, but seems to be needed for now by intrng.

sys/arm64/arm64/gicv3_its.c

153

I like to prefix the names in a struct so, when there are a few levels of indirection, I can quickly see I didn't miss a level. It's not uncommon within the kernel to do this.

1531

It didn't seem worth creating a separate file for so few functions. If this changes we can pull them out later.

[zbb]

I get that we have new API, new callbacks to provide, new interrupt description, etc. but this doesn't explain all other modifications that do not change the functionality but rather naming, code location, etc. I can see the differences such as using

desc.cmd_desc_movi.id = girq->gi_irq - girq->gi_its_dev->lpis.lpi_base;

in movi command instead of i.e.

irq = irq - its_dev->lpis.lpi_base;
its_cmd_movi(sc, its_dev, col, irq);

but that is not something that cannot be applied on-top of the current ITS implementation, don't you think?

sys/arm64/arm64/gic_v3.c

189

Although ITS will always be related to GICv3 (so there is no such need as we will not have ITS dependent on something else than GICv3) this seems like a reasonable change. Why not to apply it as a separate commit to the current ITS?

sys/arm64/arm64/gicv3_its.c

153

So why create a new file when you can change that in the current file if needed?

1531

But there already is a separate file for them...

[andrew]

I'm intending on pushing the struct gicv3_its_irqsrc * further into the code, but am also trying to touch the common headers as little as possible so some further cleanups will need to wait until we only have a single driver.

sys/arm64/arm64/gicv3_its.c

1096

It was part of a cleanup I did at the same time, it didn't feel like there was much need to call htole64 on each 64-bit block of data after the fact.

1531

The ITS attachment in gic_v3_fdt.c just has a probe function so is mostly for the gic_v3 driver. As I'm trying to keep the drivers a little separate I'd prefer to keep new ITS code out of gic_v3 driver code if possible. The headers will need some work, but will have to wait for there to be a single interrupt framework.

[zbb]

Again, I would prefer a patch against existing ITS code not instead of it. This would allow us to track the diff properly and here it's hardly possible. Most of my previous remarks still apply ;-).

sys/arm64/arm64/gicv3_its.c

1531

Still, creating a new file for FDT attachment is at most 10 mins of work don't you think? In addition, creating a new file for the FDT attachment is much more reasonable than creating a new file for the whole ITS driver.

[andrew]

In D6437#140235, @zbb wrote:

Again, I would prefer a patch against existing ITS code not instead of it. This would allow us to track the diff properly and here it's hardly possible. Most of my previous remarks still apply ;-).

Many of the remarks are how I've done things different than the existing driver. This is expected when one of the goals was to reduce the knowledge of the internals of the GICv3 driver.

[zbb]

In D6437#140473, @andrew wrote:

In D6437#140235, @zbb wrote:

Again, I would prefer a patch against existing ITS code not instead of it. This would allow us to track the diff properly and here it's hardly possible. Most of my previous remarks still apply ;-).

Many of the remarks are how I've done things different than the existing driver. This is expected when one of the goals was to reduce the knowledge of the internals of the GICv3 driver.

I would say few remarks were to the new code and they were explained. Most of the remarks however are for the existing code modifications that were not explained.
For example how does that reduces knowledge of GICv3 driver:

- #define	 ITS_FLAGS_CMDQ_FLUSH           (1UL << 0)
+ #define        ITS_FLAGS_CMDQ_FLUSH           0x00000001

There is no sense to make such remarks line by line but I think it is pretty obvious what I'm asking you to explain, isn't it?
Why can't we have a patch adding INTRNG support for the existing GICv3/ITS drivers instead we move the code around, change names of the variables and redefine macros in order to create a new file?

For example when you get to the point when you would like to add superpages support to pmap I presume you will not create a new file, let's say "p-map.c" with the old code modified to support superpages and containing other modifications such as redefinition of bit masks, macros, variable names and code moved from one function to the upper level function, etc.? I suspect you would add the patch for the existing pmap.c file that will allow it to utilize superpages. That is why I'm curious why the approach here is different.

[andrew]

In D6437#140787, @zbb wrote:

For example when you get to the point when you would like to add superpages support to pmap I presume you will not create a new file, let's say "p-map.c" with the old code modified to support superpages and containing other modifications such as redefinition of bit masks, macros, variable names and code moved from one function to the upper level function, etc.? I suspect you would add the patch for the existing pmap.c file that will allow it to utilize superpages. That is why I'm curious why the approach here is different.

Adding supperpages is different, there is no change in the interface to add support. If the pmap interface was to be significantly changed I would expect to create a new file to handle the change. This is not the case with superpages.