Changeset View
Standalone View
sys/x86/acpica/srat.c
Show First 20 Lines • Show All 61 Lines • ▼ Show 20 Lines | |||||
static ACPI_TABLE_SRAT *srat; | static ACPI_TABLE_SRAT *srat; | ||||
static vm_paddr_t srat_physaddr; | static vm_paddr_t srat_physaddr; | ||||
static int vm_domains[VM_PHYSSEG_MAX]; | static int vm_domains[VM_PHYSSEG_MAX]; | ||||
static void srat_walk_table(acpi_subtable_handler *handler, void *arg); | static void srat_walk_table(acpi_subtable_handler *handler, void *arg); | ||||
/* | /* | ||||
* SLIT parsing. | |||||
*/ | |||||
static ACPI_TABLE_SLIT *slit; | |||||
jhb: Maybe group these with the SRAT members up above? Generally FreeBSD kernel code puts all the… | |||||
static vm_paddr_t slit_physaddr; | |||||
static uint8_t vm_locality_table[MAXMEMDOM * MAXMEMDOM]; | |||||
static void | |||||
slit_walk_table(ACPI_TABLE_SLIT *s) | |||||
Not Done Inline ActionsPlease call this slit_parse_table() or some such. The name "walk_table" is only used in other ACPI/x86 places for functions that iterate over tables that contain subentries invoking a callback on each subentry (e.g. MPTable, SMBIOS, SRAT). In this case you are just parsing the matrix directly. jhb: Please call this slit_parse_table() or some such. The name "walk_table" is only used in other… | |||||
{ | |||||
int i, j; | |||||
int i_domain, j_domain; | |||||
int offset = 0; | |||||
uint8_t e; | |||||
/* | |||||
* This maps the SLIT data into the VM-domain centric view. | |||||
* There may be sparse entries in the PXM namespace, so | |||||
* remap them to a VM-domain ID and if it doesn't exist, | |||||
* skip it. | |||||
* | |||||
* It should result in a packed 2d array of VM-domain | |||||
* locality information entries. | |||||
*/ | |||||
Not Done Inline ActionsTrim extra newline. jhb: Trim extra newline. | |||||
if (bootverbose) | |||||
printf("SLIT.Localities: %d\n", (int) s->LocalityCount); | |||||
for (i = 0; i < s->LocalityCount; i++) { | |||||
i_domain = acpi_map_pxm_to_vm_domainid(i); | |||||
if (i_domain < 0) | |||||
continue; | |||||
if (bootverbose) | |||||
printf("%d: ", i); | |||||
for (j = 0; j < s->LocalityCount; j++) { | |||||
j_domain = acpi_map_pxm_to_vm_domainid(j); | |||||
if (j_domain < 0) | |||||
continue; | |||||
e = s->Entry[i * s->LocalityCount + j]; | |||||
if (bootverbose) | |||||
printf("%d ", (int) e); | |||||
vm_locality_table[offset] = e; | |||||
offset++; | |||||
} | |||||
if (bootverbose) | |||||
printf("\n"); | |||||
} | |||||
} | |||||
/* | |||||
* Look for an ACPI System Locality Distance Information Table ("SLIT") | |||||
*/ | |||||
static int | |||||
parse_slit(void) | |||||
{ | |||||
int error; | |||||
printf("%s: called\n", __func__); | |||||
rpauloUnsubmitted Not Done Inline ActionsNeeds to be removed. rpaulo: Needs to be removed. | |||||
if (resource_disabled("slit", 0)) { | |||||
rpauloUnsubmitted Not Done Inline ActionsThis isn't how ACPI resources work. Just use if (acpi_disabled("slit")). rpaulo: This isn't how ACPI resources work. Just use if (acpi_disabled("slit")). | |||||
printf("%s: resource disabled?\n", __func__); | |||||
rpauloUnsubmitted Not Done Inline ActionsUsually we don't print messages here. rpaulo: Usually we don't print messages here. | |||||
return (-1); | |||||
} | |||||
slit_physaddr = acpi_find_table(ACPI_SIG_SLIT); | |||||
Not Done Inline ActionsYou can drop these extra braces. jhb: You can drop these extra braces. | |||||
if (slit_physaddr == 0) { | |||||
printf("%s: physaddr=0\n", __func__); | |||||
return (-1); | |||||
} | |||||
/* | |||||
* Make a pass over the table to populate the cpus[] and | |||||
* mem_info[] tables. | |||||
*/ | |||||
slit = acpi_map_table(slit_physaddr, ACPI_SIG_SLIT); | |||||
error = 0; | |||||
rpauloUnsubmitted Not Done Inline ActionsThis looks bogus... rpaulo: This looks bogus... | |||||
slit_walk_table(slit); | |||||
acpi_unmap_table(slit); | |||||
slit = NULL; | |||||
if (error) { | |||||
rpauloUnsubmitted Not Done Inline Actionserror is always 0. rpaulo: error is always 0. | |||||
Not Done Inline ActionsYes, error is unused and should be removed. jhb: Yes, error is unused and should be removed. | |||||
printf("%s: error mapping table\n", __func__); | |||||
slit_physaddr = 0; | |||||
return (-1); | |||||
} | |||||
/* Tell the VM about it! */ | |||||
mem_locality = vm_locality_table; | |||||
rpauloUnsubmitted Not Done Inline ActionsIt might be better to create VM accessor methods to set/get the memory locality. rpaulo: It might be better to create VM accessor methods to set/get the memory locality. | |||||
return (0); | |||||
} | |||||
/* | |||||
* SRAT parsing. | |||||
*/ | |||||
/* | |||||
* Returns true if a memory range overlaps with at least one range in | * Returns true if a memory range overlaps with at least one range in | ||||
* phys_avail[]. | * phys_avail[]. | ||||
*/ | */ | ||||
static int | static int | ||||
overlaps_phys_avail(vm_paddr_t start, vm_paddr_t end) | overlaps_phys_avail(vm_paddr_t start, vm_paddr_t end) | ||||
{ | { | ||||
int i; | int i; | ||||
▲ Show 20 Lines • Show All 218 Lines • ▼ Show 20 Lines | KASSERT(vm_ndomains > 0, | ||||
("renumber_domains: invalid final vm_ndomains setup")); | ("renumber_domains: invalid final vm_ndomains setup")); | ||||
return (0); | return (0); | ||||
} | } | ||||
/* | /* | ||||
* Look for an ACPI System Resource Affinity Table ("SRAT") | * Look for an ACPI System Resource Affinity Table ("SRAT") | ||||
*/ | */ | ||||
static void | static int | ||||
parse_srat(void *dummy) | parse_srat(void) | ||||
{ | { | ||||
int error; | int error; | ||||
if (resource_disabled("srat", 0)) | if (resource_disabled("srat", 0)) | ||||
rpauloUnsubmitted Not Done Inline Actionsacpi_disabled() again. rpaulo: acpi_disabled() again. | |||||
return; | return (-1); | ||||
srat_physaddr = acpi_find_table(ACPI_SIG_SRAT); | srat_physaddr = acpi_find_table(ACPI_SIG_SRAT); | ||||
if (srat_physaddr == 0) | if (srat_physaddr == 0) | ||||
return; | return (-1); | ||||
/* | /* | ||||
Not Done Inline ActionsThat doesn't change that hint.srat.0.disable already exists in 9.x and 10.x. To change now is POLA. I probably chose the non-ACPI route since it is more like the rest of the system, e.g. hint.apic.0.disabled to disable APIC (since that is not ACPI specific). If you want to add acpi_disabled for SRAT and disable SRAT if either is true but only use acpi_disabled for SLIT that is ok I guess. jhb: That doesn't change that hint.srat.0.disable already exists in 9.x and 10.x. To change now is… | |||||
* Make a pass over the table to populate the cpus[] and | * Make a pass over the table to populate the cpus[] and | ||||
* mem_info[] tables. | * mem_info[] tables. | ||||
Not Done Inline ActionsThat's an old bug. :) It's been this way for a long time, so using resource_disabled for "slit" is probably more consistent. jhb: That's an old bug. :) It's been this way for a long time, so using resource_disabled for… | |||||
Not Done Inline ActionsThose have been bugs and need to be fixed. It's definitely not more consistent: rpaulo@akita freebsd/sys/dev % grep -r acpi_disabled acpi* | wc 35 155 2120 rpaulo@akita freebsd/sys/dev % grep -r resource_disabled acpi* | wc 5 21 299 rpaulo: Those have been bugs and need to be fixed. It's definitely not more consistent… | |||||
*/ | */ | ||||
srat = acpi_map_table(srat_physaddr, ACPI_SIG_SRAT); | srat = acpi_map_table(srat_physaddr, ACPI_SIG_SRAT); | ||||
error = 0; | error = 0; | ||||
srat_walk_table(srat_parse_entry, &error); | srat_walk_table(srat_parse_entry, &error); | ||||
acpi_unmap_table(srat); | acpi_unmap_table(srat); | ||||
srat = NULL; | srat = NULL; | ||||
if (error || check_domains() != 0 || check_phys_avail() != 0 || | if (error || check_domains() != 0 || check_phys_avail() != 0 || | ||||
renumber_domains() != 0) { | renumber_domains() != 0) { | ||||
srat_physaddr = 0; | srat_physaddr = 0; | ||||
return; | return (-1); | ||||
} | } | ||||
/* Point vm_phys at our memory affinity table. */ | /* Point vm_phys at our memory affinity table. */ | ||||
mem_affinity = mem_info; | mem_affinity = mem_info; | ||||
return (0); | |||||
} | } | ||||
SYSINIT(parse_srat, SI_SUB_VM - 1, SI_ORDER_FIRST, parse_srat, NULL); | |||||
static void | |||||
init_mem_locality(void) | |||||
{ | |||||
int i; | |||||
/* | |||||
* For now, assume 255 == "no locality information for | |||||
* this pairing. | |||||
*/ | |||||
Not Done Inline ActionsOw. It would be nice if this was a constant, and maybe something like -1. Or is this documented somewhere in the ACPI spec...? ngie: Ow. It would be nice if this was a constant, and maybe something like -1. Or is this documented… | |||||
Not Done Inline ActionsI think he's doing this due to the table being uint8_t. It could perhaps be a matrix of ints instead and use -1 for this (which would also simplify the code in vm_phys.c by removing the 255 special case). jhb: I think he's doing this due to the table being uint8_t. It could perhaps be a matrix of ints… | |||||
for (i = 0; i < (MAXMEMDOM * MAXMEMDOM); i++) | |||||
rpauloUnsubmitted Not Done Inline ActionsUnnecessary parentheses. rpaulo: Unnecessary parentheses. | |||||
vm_locality_table[i] = 255; | |||||
} | |||||
static void | |||||
parse_acpi_tables(void *dummy) | |||||
{ | |||||
if (parse_srat() < 0) | |||||
return; | |||||
init_mem_locality(); | |||||
(void) parse_slit(); | |||||
Not Done Inline ActionsMaybe parse_slit should be a void return type, not int. That would remove the "need" for the (void) cast. ngie: Maybe parse_slit should be a void return type, not int. That would remove the "need" for the… | |||||
Not Done Inline ActionsAgreed. It has an unused error variable that should also be removed. I would also just merge if (parse_srat() == 0) parse_slit(); jhb: Agreed. It has an unused error variable that should also be removed. I would also just merge… | |||||
} | |||||
SYSINIT(parse_acpi_tables, SI_SUB_VM - 1, SI_ORDER_FIRST, parse_acpi_tables, | |||||
NULL); | |||||
Not Done Inline ActionsMove this back up to the same spot as the old code? ngie: Move this back up to the same spot as the old code? | |||||
Not Done Inline ActionsAgreed, just needs the blank line removed. jhb: Agreed, just needs the blank line removed. | |||||
static void | static void | ||||
srat_walk_table(acpi_subtable_handler *handler, void *arg) | srat_walk_table(acpi_subtable_handler *handler, void *arg) | ||||
{ | { | ||||
acpi_walk_subtables(srat + 1, (char *)srat + srat->Header.Length, | acpi_walk_subtables(srat + 1, (char *)srat + srat->Header.Length, | ||||
handler, arg); | handler, arg); | ||||
} | } | ||||
▲ Show 20 Lines • Show All 50 Lines • Show Last 20 Lines |
Maybe group these with the SRAT members up above? Generally FreeBSD kernel code puts all the globals together in one spot in the file.