ARM64 GICv3: Fix device table size calculation

[phk]

Summary

The calculation of table sizes for the GICv3 was wrong "Indiana Jones" style: The width of the bit field is reported one less than it's value.

This patch makes interrupt work on my T41s G6 SnapDragon, but I have no idea if/how it works on any other hardware.

The cache bits cannot be written on this platform, and it is not obvious to me if we should even be messing with them in the first place, or trust the default offered by the hardware.

I cannot tell if the ITS_FLAGS_ERRATA_CAVIUM_22375 workaround is still necessary, to me the comment sounds a little bit like it works around the wrong math ?

I renamed the "page_size" variable to "gic_page_size" for clarity.

[emaste]

Please generate patches with git diff -U99999 or git show -U99999 so that Phab will show context

[phk]

git diff -U999999

[phk]

Ping ?

This is a showstopper for Snapdragon Elite support...

[jrtc27]

sys/arm64/arm64/gicv3_its.c

579

tmp is rather non-descriptive, why not hoist its_tbl_size and use that (with suitable changes to the if)? Then you also don't need the else.

586

gic_page_size, surely? It's about determining whether making the data structure sparse can ever save space, and a two-level structure has at least two GIC pages in use (assuming you're using any entries at all), so you need at least three pages in the linear table to make it worth adding an extra level.

597

This doesn't seem to match line 638's use, nor 601's assignment? Previously it was bytes, now it's number of pages.

619

Inconsistent with line 601.

683

This hunk should be a completely separate change

683

Also, style(9) says this should be (...) != 0

686

100 character line and two random hex numbers in parens doesn't make it obvious what they mean

687

Uh?

[andrew]

The gic_page_size change should be split out to a new review.

sys/arm64/arm64/gicv3_its.c

543

GITS_TYPER_DEVB already includes a + 1

544

This could be changed to GITS_BASER_CACHE_RAWAWB and the cache type check change below shouldn't be needed.

568

This probably just needs to be (1ul << devbits)

683

It's probably unneeded if we switch the cache to GITS_BASER_CACHE_RAWAWB

[phk]

sys/arm64/arm64/gicv3_its.c

543

Ok, I'm used to _reg.h files being "transparent" with the hardware, so it never even occured to me to look for the +1 over there and at least I would have expected a comment about it.

I can either move the +1 to the .c or the doc-reference to _reg.h., which do you prefer ?

586

It can be either gic_page_size or PAGE_SIZE, and the value of two is highly debatable as well.

At most you can save one gic_page_size worth of RAM, so that is not the bargain behind this bit of architecture: Fractionally faster interrupts is the game.

As I read the ARM spec (pg 12-835) the Devbits field may be R/W, so the boot-code can, maybe, ratchet the number down from the silicon default, to make direct maps an option.

So if the advertised devbits is "small" we should use direct maps, but what is "small" ? 64k ? 1MB? 64MB?

The CAVIUM ERRATA comment at lines ~520 say that chip comes in at 21bits, the T14s/Elite is about the same, so neither of those are "small".

I feel uncomfortable picking some random number, given only two data points on the high side to guide me, and risk future systems failing into our unused and untested "direct" code, so I deliberately set the limit so low, that only if the controller is truly tiny, will we attempt direct maps.

683

I will tackle this first because it is probably the gnarliest part of the change.

First, there are two cache fields: Inner [61:59] and Outer [55:53].

ARM's spec says Outer may be W/O, and neither we nor Linux makes any attempt to control it, but should we make an attempt for RAWAWB, just in case ?

From ARM's specs, it is not obvious to me that we should touch the Inner cache field, or leave what we find there in place.

The Linux code writes RAWAWB , and reads back, but does not check if the desired value stuck.

I'm fine with writing RAWAWB, but can we agree that we should not complain if it does not "take" ?