Page MenuHomeFreeBSD
Differential D47819 Diff 147083 sys/arm64/arm64/gicv3_its.c

Changeset View

Standalone View

View Options

sys/arm64/arm64/gicv3_its.c

Show First 20 Lines • Show All 499 Lines • ▼ Show 20 Lines
static int static int
gicv3_its_table_init(device_t dev, struct gicv3_its_softc *sc) gicv3_its_table_init(device_t dev, struct gicv3_its_softc *sc)
{ {
void *table; void *table;
vm_paddr_t paddr; vm_paddr_t paddr;
uint64_t cache, reg, share, tmp, type; uint64_t cache, reg, share, tmp, type;
size_t its_tbl_size, nitspages, npages; size_t its_tbl_size, nitspages, npages;
size_t l1_esize, l2_esize, l1_nidents, l2_nidents; size_t l1_esize, l2_esize, l1_nidents, l2_nidents;
int i, page_size; int i, gic_page_size;
int devbits; int devbits;
bool indirect; bool indirect;
if ((sc->sc_its_flags & ITS_FLAGS_ERRATA_CAVIUM_22375) != 0) { if ((sc->sc_its_flags & ITS_FLAGS_ERRATA_CAVIUM_22375) != 0) {
/* /*
* GITS_TYPER[17:13] of ThunderX reports that device IDs * GITS_TYPER[17:13] of ThunderX reports that device IDs
* are to be 21 bits in length. The entry size of the ITS * are to be 21 bits in length. The entry size of the ITS
* table can be read from GITS_BASERn[52:48] and on ThunderX * table can be read from GITS_BASERn[52:48] and on ThunderX
Show All 9 Lines if ((sc->sc_its_flags & ITS_FLAGS_ERRATA_CAVIUM_22375) != 0) {
* *
* Set an arbitrary number of device ID bits to 20 in order * Set an arbitrary number of device ID bits to 20 in order
* to limit the number of entries in ITS device table to * to limit the number of entries in ITS device table to
* 0x100000 and the table size to 8MB. * 0x100000 and the table size to 8MB.
*/ */
devbits = 20; devbits = 20;
cache = 0; cache = 0;
} else { } else {
devbits = GITS_TYPER_DEVB(gic_its_read_8(sc, GITS_TYPER)); /*
* ARM IHI 0069G page 12-835:
*
* Devbits, bits [17:13]
* The number of DeviceID bits implemented, minus one.
*
* A page break before "minus one" would have been a great
* homage to the Staff or Ra in Raiders of the lost Ark.
*/
devbits = GITS_TYPER_DEVB(gic_its_read_8(sc, GITS_TYPER)) + 1;
andrewUnsubmitted
Not Done
Inline Actions

GITS_TYPER_DEVB already includes a + 1

andrew: `GITS_TYPER_DEVB` already includes a `+ 1`
phkAuthorUnsubmitted
Done
Inline Actions

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 ?

phk: Ok, I'm used to _reg.h files being "transparent" with the hardware, so it never even occured to…
cache = GITS_BASER_CACHE_WAWB; cache = GITS_BASER_CACHE_WAWB;
andrewUnsubmitted
Not Done
Inline Actions

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

andrew: This could be changed to `GITS_BASER_CACHE_RAWAWB` and the cache type check change below…
} }
sc->sc_devbits = devbits; sc->sc_devbits = devbits;
share = GITS_BASER_SHARE_IS; share = GITS_BASER_SHARE_IS;
for (i = 0; i < GITS_BASER_NUM; i++) { for (i = 0; i < GITS_BASER_NUM; i++) {
reg = gic_its_read_8(sc, GITS_BASER(i)); reg = gic_its_read_8(sc, GITS_BASER(i));
/* The type of table */ /* The type of table */
type = GITS_BASER_TYPE(reg); type = GITS_BASER_TYPE(reg);
if (type == GITS_BASER_TYPE_UNIMPL) if (type == GITS_BASER_TYPE_UNIMPL)
continue; continue;
/* The table entry size */ /* The table entry size */
l1_esize = GITS_BASER_ESIZE(reg); l1_esize = GITS_BASER_ESIZE(reg);
/* Find the tables page size */ /* Find the tables page size */
page_size = gicv3_its_table_page_size(sc, i); gic_page_size = gicv3_its_table_page_size(sc, i);
if (page_size == -1) { if (gic_page_size == -1) {
device_printf(dev, "No valid page size for table %d\n", device_printf(dev, "No valid page size for table %d\n",
i); i);
return (EINVAL); return (EINVAL);
} }
indirect = false; indirect = false;
l2_nidents = 0; l2_nidents = 0;
l2_esize = 0; l2_esize = 0;
switch(type) { switch(type) {
case GITS_BASER_TYPE_DEV: case GITS_BASER_TYPE_DEV:
if (sc->sc_dev_table_idx != -1) if (sc->sc_dev_table_idx != -1)
device_printf(dev, device_printf(dev,
"Warning: Multiple device tables found\n"); "Warning: Multiple device tables found\n");
sc->sc_dev_table_idx = i; sc->sc_dev_table_idx = i;
l1_nidents = (1 << devbits);
andrewUnsubmitted
Not Done
Inline Actions

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

andrew: This probably just needs to be `(1ul << devbits)`
if ((l1_esize * l1_nidents) > (page_size * 2)) { /* Total address space required */
indirect = tmp = l1_esize << devbits;
jrtc27Unsubmitted
Not Done
Inline Actions

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.

jrtc27: tmp is rather non-descriptive, why not hoist its_tbl_size and use that (with suitable changes…
gicv3_its_table_supports_indirect(sc, i);
if (indirect) {
/* /*
* Each l1 entry is 8 bytes and points * It is hard to imagine a system where the total
* to an l2 table of size page_size. * size would come out to just one or two VM pages,
* Calculate how many entries this is * but just in case...
* and use this to find how many
* 8 byte l1 idents we need.
*/ */
if (howmany(tmp, PAGE_SIZE) > 2 &&
jrtc27Unsubmitted
Not Done
Inline Actions

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.

jrtc27: gic_page_size, surely? It's about determining whether making the data structure sparse can ever…
phkAuthorUnsubmitted
Done
Inline Actions

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.

phk: It can be either gic_page_size or PAGE_SIZE, and the value of two is highly debatable as well.
gicv3_its_table_supports_indirect(sc, i)) {
indirect = true;
l2_esize = l1_esize; l2_esize = l1_esize;
l2_nidents = page_size / l2_esize; l2_nidents = gic_page_size / l2_esize;
l1_nidents = l1_nidents / l2_nidents; l1_nidents = tmp / gic_page_size;
l1_esize = GITS_INDIRECT_L1_ESIZE; l1_esize = GITS_INDIRECT_L1_ESIZE;
} else {
l1_nidents = (1ULL << devbits);
} }
}
its_tbl_size = l1_esize * l1_nidents; its_tbl_size = l1_esize * l1_nidents;
its_tbl_size = roundup2(its_tbl_size, page_size); its_tbl_size = howmany(its_tbl_size, gic_page_size);
jrtc27Unsubmitted
Not Done
Inline Actions

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

jrtc27: This doesn't seem to match line 638's use, nor 601's assignment? Previously it was bytes, now…
break; break;
case GITS_BASER_TYPE_PP: /* Undocumented? */ case GITS_BASER_TYPE_PP: /* Undocumented? */
case GITS_BASER_TYPE_IC: case GITS_BASER_TYPE_IC:
its_tbl_size = page_size; its_tbl_size = gic_page_size;
break; break;
case GITS_BASER_TYPE_VP: case GITS_BASER_TYPE_VP:
/* /*
* If GITS_TYPER.SVPET != 0, the pending table is * If GITS_TYPER.SVPET != 0, the pending table is
* shared amongst the redistibutors and ther other * shared amongst the redistibutors and ther other
* ITSes. Requiring sharing across the ITSes when none * ITSes. Requiring sharing across the ITSes when none
* of the redistributors have GICR_VPROPBASER.Valid==1 * of the redistributors have GICR_VPROPBASER.Valid==1
* isn't specified in the architecture, but that's how * isn't specified in the architecture, but that's how
* the GIC-700 behaves. We don't handle vPE tables at * the GIC-700 behaves. We don't handle vPE tables at
* all yet, so just skip this base register. * all yet, so just skip this base register.
*/ */
default: default:
if (bootverbose) if (bootverbose)
device_printf(dev, "Unhandled table type %lx\n", device_printf(dev, "Unhandled table type %lx\n",
type); type);
continue; continue;
} }
npages = howmany(its_tbl_size, PAGE_SIZE); npages = howmany(its_tbl_size * gic_page_size, PAGE_SIZE);
jrtc27Unsubmitted
Not Done
Inline Actions

Inconsistent with line 601.

jrtc27: Inconsistent with line 601.
/* Allocate the table */ /* Allocate the table */
table = contigmalloc_domainset(npages * PAGE_SIZE, table = contigmalloc_domainset(npages * PAGE_SIZE,
M_GICV3_ITS, sc->sc_ds, M_WAITOK | M_ZERO, 0, M_GICV3_ITS, sc->sc_ds, M_WAITOK | M_ZERO, 0,
(1ul << 48) - 1, PAGE_SIZE_64K, 0); (1ul << 48) - 1, gic_page_size, 0);
sc->sc_its_ptab[i].ptab_vaddr = table; sc->sc_its_ptab[i].ptab_vaddr = table;
sc->sc_its_ptab[i].ptab_l1_size = its_tbl_size; sc->sc_its_ptab[i].ptab_l1_size = its_tbl_size;
sc->sc_its_ptab[i].ptab_l1_nidents = l1_nidents; sc->sc_its_ptab[i].ptab_l1_nidents = l1_nidents;
sc->sc_its_ptab[i].ptab_l2_size = page_size; sc->sc_its_ptab[i].ptab_l2_size = gic_page_size;
sc->sc_its_ptab[i].ptab_l2_nidents = l2_nidents; sc->sc_its_ptab[i].ptab_l2_nidents = l2_nidents;
sc->sc_its_ptab[i].ptab_indirect = indirect; sc->sc_its_ptab[i].ptab_indirect = indirect;
sc->sc_its_ptab[i].ptab_page_size = page_size; sc->sc_its_ptab[i].ptab_page_size = gic_page_size;
paddr = vtophys(table); paddr = vtophys(table);
while (1) { while (1) {
nitspages = howmany(its_tbl_size, page_size); nitspages = howmany(its_tbl_size, gic_page_size);
/* Clear the fields we will be setting */ /* Clear the fields we will be setting */
reg &= ~(GITS_BASER_VALID | GITS_BASER_INDIRECT | reg &= ~(GITS_BASER_VALID | GITS_BASER_INDIRECT |
GITS_BASER_CACHE_MASK | GITS_BASER_TYPE_MASK | GITS_BASER_CACHE_MASK | GITS_BASER_TYPE_MASK |
GITS_BASER_PA_MASK | GITS_BASER_PA_MASK |
GITS_BASER_SHARE_MASK | GITS_BASER_PSZ_MASK | GITS_BASER_SHARE_MASK | GITS_BASER_PSZ_MASK |
GITS_BASER_SIZE_MASK); GITS_BASER_SIZE_MASK);
/* Set the new values */ /* Set the new values */
reg |= GITS_BASER_VALID | reg |= GITS_BASER_VALID |
(indirect ? GITS_BASER_INDIRECT : 0) | (indirect ? GITS_BASER_INDIRECT : 0) |
(cache << GITS_BASER_CACHE_SHIFT) | (cache << GITS_BASER_CACHE_SHIFT) |
(type << GITS_BASER_TYPE_SHIFT) | (type << GITS_BASER_TYPE_SHIFT) |
paddr | (share << GITS_BASER_SHARE_SHIFT) | paddr | (share << GITS_BASER_SHARE_SHIFT) |
(nitspages - 1); (nitspages - 1);
switch (page_size) { switch (gic_page_size) {
case PAGE_SIZE_4K: /* 4KB */ case PAGE_SIZE_4K: /* 4KB */
reg |= reg |=
GITS_BASER_PSZ_4K << GITS_BASER_PSZ_SHIFT; GITS_BASER_PSZ_4K << GITS_BASER_PSZ_SHIFT;
break; break;
case PAGE_SIZE_16K: /* 16KB */ case PAGE_SIZE_16K: /* 16KB */
reg |= reg |=
GITS_BASER_PSZ_16K << GITS_BASER_PSZ_SHIFT; GITS_BASER_PSZ_16K << GITS_BASER_PSZ_SHIFT;
break; break;
Show All 11 Lines while (1) {
/* Do the shareability masks line up? */ /* Do the shareability masks line up? */
if ((tmp & GITS_BASER_SHARE_MASK) != if ((tmp & GITS_BASER_SHARE_MASK) !=
(reg & GITS_BASER_SHARE_MASK)) { (reg & GITS_BASER_SHARE_MASK)) {
share = (tmp & GITS_BASER_SHARE_MASK) >> share = (tmp & GITS_BASER_SHARE_MASK) >>
GITS_BASER_SHARE_SHIFT; GITS_BASER_SHARE_SHIFT;
continue; continue;
} }
if (tmp != reg) { /* The cache bits may not be writable */
if ((tmp ^ reg) & ~GITS_BASER_CACHE_MASK) {
jrtc27Unsubmitted
Not Done
Inline Actions

This hunk should be a completely separate change

jrtc27: This hunk should be a completely separate change
jrtc27Unsubmitted
Not Done
Inline Actions

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

jrtc27: Also, style(9) says this should be `(...) != 0`
andrewUnsubmitted
Not Done
Inline Actions

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

andrew: It's probably unneeded if we switch the cache to `GITS_BASER_CACHE_RAWAWB`
phkAuthorUnsubmitted
Not Done
Inline Actions

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" ?

phk: I will tackle this first because it is probably the gnarliest part of the change. First, there…
device_printf(dev, "GITS_BASER%d: " device_printf(dev, "GITS_BASER%d: "
"unable to be updated: %lx != %lx\n", "unable to be updated: %lx != %lx (%lx %lx)\n",
i, reg, tmp); i, reg, tmp, (tmp ^ reg), (tmp ^ reg) & ~GITS_BASER_CACHE_MASK);
jrtc27Unsubmitted
Not Done
Inline Actions

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

jrtc27: 100 character line and two random hex numbers in parens doesn't make it obvious what they mean
return (ENXIO); // return (ENXIO);
jrtc27Unsubmitted
Not Done
Inline Actions

Uh?

jrtc27: Uh?
} }
sc->sc_its_ptab[i].ptab_share = share; sc->sc_its_ptab[i].ptab_share = share;
/* We should have made all needed changes */ /* We should have made all needed changes */
break; break;
} }
} }
▲ Show 20 Lines • Show All 1,645 Lines • Show Last 20 Lines