[coreboot-gerrit] [M] Change in coreboot[main]: mb/emulation/qemu-sbsa: Generate PPTT ACPI table

[Arthur Heymans (Code Review)]

Attention is currently required from: David Milosevic.

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79108?usp=email )

Change subject: mb/emulation/qemu-sbsa: Generate PPTT ACPI table
......................................................................

Patch Set 3:

(4 comments)

File src/mainboard/emulation/qemu-sbsa/pptt.c:

https://review.coreboot.org/c/coreboot/+/79108/comment/9ed4e0f4_5022cd3f :
PS3, Line 16: struct
static for all of these caches, resources & topology. Also move these inside the acpi_get_pptt_topology function. A global scope is not needed when static is used as that ensures the lifetime.

https://review.coreboot.org/c/coreboot/+/79108/comment/34280a6b_100264f5 :
PS3, Line 17: 
remove all the newlines when declaring structs.

https://review.coreboot.org/c/coreboot/+/79108/comment/dbf3733a_98f4ad82 :
PS3, Line 65: /*
            : 	 * 'write-policy' and 'allocation type' currently
            : 	 * unsupported.
As a nice example for real hardware, it would be good to set a meaningful value, rather than leave it 0?

https://review.coreboot.org/c/coreboot/+/79108/comment/689eb75b_7d9924d3 :
PS3, Line 130: *it = malloc(sizeof(struct pptt_topology))
There have been quite a few patches finetuning the heap. As you don't give that heap back to the OS and and it needs to be sufficiently large anyway can you just statically allocate a buffer large enough for CONFIG_MAX_CPUS? That avoids this runtime check too.

The following code snippet works btw:

struct device *cur = dev_find_path(NULL, DEVICE_PATH_GICC_V3);
    struct device *next = NULL;
    static struct pptt_topology entries[CONFIG_MAX_CPUS] = {};

root_topology.child = entries;
    for (int i = 0; i < CONFIG_MAX_CPUS; i++) {
    	if (!cur) {
    		break;
    	}
    	struct pptt_topology *entry = &entries[i];
    	entry->processor_id = i;
    	entry->flags.raw    = CORE_FLAGS;
    	entry->resources    = &core_resources;

next = dev_find_path(cur, DEVICE_PATH_GICC_V3);
    	if (next)
    		entry->sibling = &entries[i + 1];
    	cur = next;
    }
    return &root_topology;