Page MenuHomeFreeBSD
Differential D47819

ARM64 GICv3: Fix device table size calculation
Needs ReviewPublic
Actions

Authored by phk on Wed, Nov 27, 7:34 PM.
Tags
Referenced Files
F104979922: D47819.id147082.diff
Wed, Dec 11, 8:03 AM
Unknown Object (File)
Mon, Dec 2, 2:21 AM
Unknown Object (File)
Sun, Dec 1, 4:01 AM
Subscribers
emaste

Details

Reviewers
imp
kevans
scottph
andrew
manu
jrtc27
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.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

phk created this revision.Wed, Nov 27, 7:34 PM
Herald added a reviewer: andrew. · View Herald TranscriptWed, Nov 27, 7:34 PM
Herald added a reviewer: andrew. · View Herald Transcript
Herald added a reviewer: manu. · View Herald Transcript
Herald added a subscriber: emaste. · View Herald Transcript
phk requested review of this revision.Wed, Nov 27, 7:34 PM
phk added a reviewer: jrtc27.
emaste added a comment.Wed, Nov 27, 7:41 PM

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

phk updated this revision to Diff 147083.Wed, Nov 27, 7:53 PM

git diff -U999999

phk added a comment.Mon, Dec 9, 9:50 PM

Ping ?

This is a showstopper for Snapdragon Elite support...

jrtc27 added inline comments.Mon, Dec 9, 10:10 PM
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 added a comment.Mon, Dec 9, 10:26 PM

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

Revision Contents
Changeset List

PathSize
sys/
arm64/
arm64/
77 lines

Diff 147082

View Options

sys/arm64/arm64/gicv3_its.c

Loading...