x86/acpica/srat.c: Add API for parsing proximity tables

[jchandra]

Summary

The SLIT/SRAT tables needs to be parsed for on arm64 platforms that use UEFI/ACPI firmware and support NUMA. To do this, we need to move most of the logic of x86/acpica/srat.c to dev/acpica and provide an API which other architectures can use to parse and configure NUMA proximity information.

This commit adds the API to srat.c as a first step, without making any functional changes. We will move the common code as the next step.

The functions added are:

• int acpi_pxm_init(int ncpus, vm_paddr_t maxphys) - to allocate and initialize data structures used.
• void acpi_pxm_parse_tables(void) - parse SRAT/SLIT, save the cpu and memory proximity information
• void acpi_pxm_set_mem_locality(void) - the use saved data to set memory locality
• void acpi_pxm_set_cpu_locality(void) - use saved data to set cpu locality
• void acpi_pxm_free(void) - free data structs allocated by init

On arm64, we do not have an cpu APIC id that can be used as index to store CPU data, we need to use the Processor Uid. To help with this, define internal functions cpus_add, cpus_find, cpus_get_info to store and get CPU info.

Test Plan

Should have no effect on x86 UEFI/ACPI boot with or without NUMA

[jhb]

sys/x86/acpica/srat.c

60 ↗

(On Diff #50275)

style(9) wants periods at the end of sentences in comments here and in various other places (I know there's at least one existing place that is missing a period that this patch touches as well.)

73 ↗

(On Diff #50275)

This new member doesn't seem to be used in this patch? Maybe it is used in one of the later patches in the series, but if so it might be cleaner to only add it in the patch that uses it.

197 ↗

(On Diff #50275)

Maybe "Find CPU by processor ID (APIC ID on x86)."

200 ↗

(On Diff #50275)

I would perhaps call this 'cpu_find_id'. cpus_ is a bit of an odd prefix vs cpu_ as we normally don't use plural prefixes.

210 ↗

(On Diff #50275)

Maybe "Find CPU by pcpu pointer."

213 ↗

(On Diff #50275)

I would perhaps call this "cpu_find_pcpu".

226 ↗

(On Diff #50275)

Maybe "Add new CPU proximity information."

229 ↗

(On Diff #50275)

I would call this "cpu_add"

238 ↗

(On Diff #50275)

Can probably just drop this comment as it mostly mirrors the code.

481 ↗

(On Diff #50275)

Maybe: "Look for an ACPI System Resource Affinity Table ("SRAT"), allocate space for CPU information, and initialize globals."

[jchandra]

Thanks for the review, will update the diff with the changes (and a fix related use of max).

sys/x86/acpica/srat.c

60 ↗

(On Diff #50275)

Ok, will fix here and other places.

229 ↗

(On Diff #50275)

Ok - I went with cpus_add since the array is called 'cpus'. Agree that it does not really sound right, will fix.

[jchandra]

Addressed comments from jhb's review:

• fixed comment stye & reworded as suggested.
• removed 'id' field until it is needed (in a later patch)

Fixed a bug - was using max instead of imax (noted by markj in a later review and also found when testing on qemu)

[markj]

This mostly LGTM, a couple of nits inline.

I find the "acpi_pxm" prefix a bit confusing since memory locality info is not obtained using _PXM. Is "pxm" generally understood to be a generic abbreviation for "proximity info"?

sys/x86/acpica/srat.c

580 ↗

(On Diff #50577)

I'd find it more natural to check this in parse_srat() and add a return value to acpi_pxm_parse_tables(). Then the machdep code would do:

acpi_pxm_init();
if (acpi_pxm_parse_tables() == 0)
    acpi_pxm_set_mem_locality();

Or even combine parse_tables() and set_mem_locality()?

637 ↗

(On Diff #50577)

Why not just conditionalize on cpus == NULL?

[jchandra]

markj: please see the inline comment, and let me know if think there is further changes needed. Thanks.

sys/x86/acpica/srat.c

580 ↗

(On Diff #50577)

We need to keep track of whether parse_tables succeeded in this file using a global anyway. This is needed later in when setting cpu locality info and freeing. I followed the existing convention of using srat_physaddr being valid. Once we have the global state tracking this, having a return value is unnecessary.

Regarding combining functions: init/parse_tables/set_mem_locality is called together and so is set_cpu_locality/free, I had considered multiple ways of combining these, but ended up with these functions to get the functionality supported by the file into logical pieces(even if it adds a couple of extra function calls).

An alternative would be to combine acpi_pxm_init() and acpi_pxm_parse_tables(), use 'cpus' to keep global state instead of srat_physaddr. I don't think would be much better than the current approach...

637 ↗

(On Diff #50577)

please see above

[jchandra]

markj: Regarding the question on pxm prefix. I used the convention starting with the filename. I kind of settled for acpi_pxm since I could not think of a better filename and prefix (which covers SRAT for resource affinity and SLIT for locality).

An alternative would be to have acpi_proximity.c containing:
int acpi_parse_proximity_tables(int cpus, vm_paddr_t maxmem);
int acpi_set_mem_locality(void);
int acpi_set_cpu_locality(void);
void acpi_free_proximity_data(void);

Let me know if you have any suggestion on this.

[markj]

In D17940#392523, @jchandra wrote:

markj: Regarding the question on pxm prefix. I used the convention starting with the filename. I kind of settled for acpi_pxm since I could not think of a better filename and prefix (which covers SRAT for resource affinity and SLIT for locality).

An alternative would be to have acpi_proximity.c containing:
int acpi_parse_proximity_tables(int cpus, vm_paddr_t maxmem);
int acpi_set_mem_locality(void);
int acpi_set_cpu_locality(void);
void acpi_free_proximity_data(void);

Let me know if you have any suggestion on this.

Thanks. I think acpi_pxm is a reasonable prefix, I just wasn't sure if that was an already-established convention.