Change in coreboot[master]: cpu/intel/model_206ax: Replace `generate_cstate_entries`

[Angel Pons (Code Review)]

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/49093 )

Change subject: cpu/intel/model_206ax: Replace `generate_cstate_entries`
......................................................................

cpu/intel/model_206ax: Replace `generate_cstate_entries`

Leverage the existing `acpigen_write_CST_package` function.

Change-Id: Icec5431987d91242930efcea0c8ea4e3df3182fd
Signed-off-by: Angel Pons 
---
M src/cpu/intel/model_206ax/acpi.c
1 file changed, 15 insertions(+), 37 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/49093/1

diff --git a/src/cpu/intel/model_206ax/acpi.c b/src/cpu/intel/model_206ax/acpi.c
index beb2fbc..bd743c7 100644
--- a/src/cpu/intel/model_206ax/acpi.c
+++ b/src/cpu/intel/model_206ax/acpi.c
@@ -19,39 +19,6 @@
    return msr.lo & 0xffff;
 }

-static void generate_cstate_entries(acpi_cstate_t *cstates,
-				    int c1, int c2, int c3)
-{
-	int cstate_count = 0;
-
-	/* Count number of active C-states */
-	if (c1 > 0)
-		++cstate_count;
-	if (c2 > 0)
-		++cstate_count;
-	if (c3 > 0)
-		++cstate_count;
-
-	acpigen_write_package(cstate_count + 1);
-	acpigen_write_byte(cstate_count);
-
-	/* Add an entry if the level is enabled */
-	if (c1 > 0) {
-		cstates[c1].ctype = 1;
-		acpigen_write_CST_package_entry(&cstates[c1]);
-	}
-	if (c2 > 0) {
-		cstates[c2].ctype = 2;
-		acpigen_write_CST_package_entry(&cstates[c2]);
-	}
-	if (c3 > 0) {
-		cstates[c3].ctype = 3;
-		acpigen_write_CST_package_entry(&cstates[c3]);
-	}
-
-	acpigen_pop_len();
-}
-
 static void generate_C_state_entries(void)
 {
    struct cpu_info *info;
@@ -75,10 +42,21 @@
    if (!cpu || !cpu->cstates)
    	return;

-	acpigen_write_method("_CST", 0);
-	acpigen_emit_byte(RETURN_OP);
-	generate_cstate_entries(cpu->cstates, conf->acpi_c1, conf->acpi_c2, conf->acpi_c3);
-	acpigen_pop_len();
+	const int acpi_cstates[3] = { conf->acpi_c1, conf->acpi_c2, conf->acpi_c3 };
+
+	acpi_cstate_t cstate_map[ARRAY_SIZE(acpi_cstates)];
+
+	/* Count number of active C-states */
+	int count = 0;
+
+	for (int i = 0; i < ARRAY_SIZE(acpi_cstates); i++) {
+		if (acpi_cstates[i] > 0) {
+			cstate_map[count] = cpu->cstates[acpi_cstates[i]];
+			cstate_map[count].ctype = i + 1;
+			count++;
+		}
+	}
+	acpigen_write_CST_package(cstate_map, count);
 }

static acpi_tstate_t tss_table_fine[] = {

[Patrick Rudolph (Code Review)]

Attention is currently required from: Nico Huber, Angel Pons, Arthur Heymans.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49093 )

Change subject: cpu/intel/model_206ax: Replace `generate_cstate_entries`
......................................................................

Patch Set 2:

(2 comments)

File src/cpu/intel/model_206ax/acpi.c:

https://review.coreboot.org/c/coreboot/+/49093/comment/968b1205_7d875269 
PS2, Line 54: acpi_cstates
possible buffer overflow.

https://review.coreboot.org/c/coreboot/+/49093/comment/e6d26aa2_55660df9 
PS2, Line 55: i + 1
shouldn't the ctype be already correct here? why do you need to update it?

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber, Arthur Heymans, Patrick Rudolph.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49093 )

Change subject: cpu/intel/model_206ax: Replace `generate_cstate_entries`
......................................................................

Patch Set 2:

(2 comments)

File src/cpu/intel/model_206ax/acpi.c:

https://review.coreboot.org/c/coreboot/+/49093/comment/f234d213_1d3dfe3c 
PS2, Line 54: acpi_cstates
> possible buffer overflow.

Sorry, I don't follow. Do you mean a buffer overflow can happen if a devicetree contains invalid `acpi_cX` values?

https://review.coreboot.org/c/coreboot/+/49093/comment/eda43efe_b54b4186 
PS2, Line 55: i + 1
> shouldn't the ctype be already correct here? why do you need to update it?

I'm just doing what the original code did.

[Patrick Rudolph (Code Review)]

Attention is currently required from: Nico Huber, Angel Pons, Arthur Heymans, Patrick Rudolph.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49093 )

Change subject: cpu/intel/model_206ax: Replace `generate_cstate_entries`
......................................................................

Patch Set 2:

(3 comments)

File src/cpu/intel/model_206ax/acpi.c:

https://review.coreboot.org/c/coreboot/+/49093/comment/ad5b400e_1f0aade5 
PS2, Line 50: 	int count = 0;
Reading this code again and again, I'm still not sure what values to put in as acpi_cX.
According to the ACPI spec you can specify more than 3 C-states in _CST package, so this selection using the devicetree doesn't make any sense. Why not put all (supported) C-states into _CST?

https://review.coreboot.org/c/coreboot/+/49093/comment/32c52afb_01d0fb00 
PS2, Line 54: acpi_cstates
> Sorry, I don't follow. […]

yes, if acpi_cX >= ARRAY_SIZE(cpu->cstates)

https://review.coreboot.org/c/coreboot/+/49093/comment/ca6b2b75_23a1307b 
PS2, Line 55: i + 1
> I'm just doing what the original code did.

Just checked, there's no ctype in the default map. This only works as acpi_cstates contains the cstates C1 - C3.

[Nico Huber (Code Review)]

Attention is currently required from: Patrick Rudolph, Angel Pons, Arthur Heymans.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49093 )

Change subject: cpu/intel/model_206ax: Replace `generate_cstate_entries`
......................................................................

Patch Set 2:

(2 comments)

Patchset:

PS2: 
Sorry, I thought it would be good to review and say something
here, but I might have made it worse...

File src/cpu/intel/model_206ax/acpi.c:

https://review.coreboot.org/c/coreboot/+/49093/comment/747f2725_8da28bf6 
PS2, Line 54: acpi_cstates
> yes, if acpi_cX >= ARRAY_SIZE(cpu->cstates)

Let's call it an out-of-bounds read. You can just add it to the condition

if (acpi_cstates[i] > 0 && acpi_cstates[i] < cpu_cstate_count)

We need to count first, though. `cpu->cstates` is 0 terminated. (Ugh, I don't
understand this indirection. It's a static array, why is it not declared
here?)(Also, why are those acpi_cstates `int` and not `unsigned`?)

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber, Patrick Rudolph, Arthur Heymans.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49093 )

Change subject: cpu/intel/model_206ax: Replace `generate_cstate_entries`
......................................................................

Patch Set 2:

(1 comment)

File src/cpu/intel/model_206ax/acpi.c:

https://review.coreboot.org/c/coreboot/+/49093/comment/ea1bb25b_52f8da94 
PS2, Line 54: acpi_cstates
> Let's call it an out-of-bounds read. You can just add it to the condition […]

Yeah, I don't like these things at all. I'll see what I can do. I should also fix Haswell, but I'm stalled on changes like CB:46924 that touch the C-state code.

[Patrick Rudolph (Code Review)]

Attention is currently required from: Nico Huber, Angel Pons, Arthur Heymans, Patrick Rudolph.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49093 )

Change subject: cpu/intel/model_206ax: Replace `generate_cstate_entries`
......................................................................

Patch Set 2:

(1 comment)

File src/cpu/intel/model_206ax/acpi.c:

https://review.coreboot.org/c/coreboot/+/49093/comment/41134389_22a7ea2c 
PS2, Line 50: 	int count = 0;
> Reading this code again and again, I'm still not sure what values to put in as acpi_cX. […]

Let's fix this in a separate commit.

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber, Patrick Rudolph, Arthur Heymans.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49093 )

Change subject: cpu/intel/model_206ax: Replace `generate_cstate_entries`
......................................................................

Patch Set 2:

(1 comment)

File src/cpu/intel/model_206ax/acpi.c:

https://review.coreboot.org/c/coreboot/+/49093/comment/430744dc_a34f005b 
PS2, Line 54: acpi_cstates
> Yeah, I don't like these things at all. I'll see what I can do. […]

Will need to fix in a follow-up. I first need to drop using the `cstates` pointer completely.

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber, Patrick Rudolph, Arthur Heymans.
Hello build bot (Jenkins), Nico Huber, Patrick Georgi, Arthur Heymans, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/49093

to look at the new patch set (#3).

Change subject: cpu/intel/model_206ax: Replace `generate_cstate_entries`
......................................................................

cpu/intel/model_206ax: Replace `generate_cstate_entries`

Leverage the existing `acpigen_write_CST_package` function.

Yes, bad devicetree values can trigger undefined behavior. The old code
already had this issue, and will be addressed in subsequent commits.

Change-Id: Icec5431987d91242930efcea0c8ea4e3df3182fd
Signed-off-by: Angel Pons 
---
M src/cpu/intel/model_206ax/acpi.c
1 file changed, 15 insertions(+), 37 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/49093/3

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber, Patrick Rudolph, Arthur Heymans.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49093 )

Change subject: cpu/intel/model_206ax: Replace `generate_cstate_entries`
......................................................................

Patch Set 3:

(1 comment)

File src/cpu/intel/model_206ax/acpi.c:

https://review.coreboot.org/c/coreboot/+/49093/comment/f4acc914_86252f6f 
PS2, Line 54: acpi_cstates
> Will need to fix in a follow-up. I first need to drop using the `cstates` pointer completely.

CB:49801

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber, Patrick Rudolph, Arthur Heymans.
Hello build bot (Jenkins), Nico Huber, Patrick Georgi, Arthur Heymans, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/49093

to look at the new patch set (#4).

Change subject: cpu/intel/model_206ax: Replace `generate_cstate_entries`
......................................................................

cpu/intel/model_206ax: Replace `generate_cstate_entries`

Leverage the existing `acpigen_write_CST_package` function.

Yes, bad devicetree values can trigger undefined behavior. The old code
already had this issue, and will be addressed in subsequent commits.

Change-Id: Icec5431987d91242930efcea0c8ea4e3df3182fd
Signed-off-by: Angel Pons 
---
M src/cpu/intel/model_206ax/acpi.c
1 file changed, 15 insertions(+), 37 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/49093/4

[Nico Huber (Code Review)]

Attention is currently required from: Patrick Rudolph, Angel Pons, Arthur Heymans.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49093 )

Change subject: cpu/intel/model_206ax: Replace `generate_cstate_entries`
......................................................................

Patch Set 6: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/49093 )

Change subject: cpu/intel/model_206ax: Replace `generate_cstate_entries`
......................................................................

cpu/intel/model_206ax: Replace `generate_cstate_entries`

Leverage the existing `acpigen_write_CST_package` function.

Yes, bad devicetree values can trigger undefined behavior. The old code
already had this issue, and will be addressed in subsequent commits.

Change-Id: Icec5431987d91242930efcea0c8ea4e3df3182fd
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/49093
Reviewed-by: Nico Huber 
Tested-by: build bot (Jenkins) 
---
M src/cpu/intel/model_206ax/acpi.c
1 file changed, 15 insertions(+), 37 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/src/cpu/intel/model_206ax/acpi.c b/src/cpu/intel/model_206ax/acpi.c
index beb2fbc..6c12261 100644
--- a/src/cpu/intel/model_206ax/acpi.c
+++ b/src/cpu/intel/model_206ax/acpi.c
@@ -19,39 +19,6 @@
    return msr.lo & 0xffff;
 }

-static void generate_cstate_entries(acpi_cstate_t *cstates,
-				    int c1, int c2, int c3)
-{
-	int cstate_count = 0;
-
-	/* Count number of active C-states */
-	if (c1 > 0)
-		++cstate_count;
-	if (c2 > 0)
-		++cstate_count;
-	if (c3 > 0)
-		++cstate_count;
-
-	acpigen_write_package(cstate_count + 1);
-	acpigen_write_byte(cstate_count);
-
-	/* Add an entry if the level is enabled */
-	if (c1 > 0) {
-		cstates[c1].ctype = 1;
-		acpigen_write_CST_package_entry(&cstates[c1]);
-	}
-	if (c2 > 0) {
-		cstates[c2].ctype = 2;
-		acpigen_write_CST_package_entry(&cstates[c2]);
-	}
-	if (c3 > 0) {
-		cstates[c3].ctype = 3;
-		acpigen_write_CST_package_entry(&cstates[c3]);
-	}
-
-	acpigen_pop_len();
-}
-
 static void generate_C_state_entries(void)
 {
    struct cpu_info *info;
@@ -75,10 +42,21 @@
    if (!cpu || !cpu->cstates)
    	return;

-	acpigen_write_method("_CST", 0);
-	acpigen_emit_byte(RETURN_OP);
-	generate_cstate_entries(cpu->cstates, conf->acpi_c1, conf->acpi_c2, conf->acpi_c3);
-	acpigen_pop_len();
+	const int acpi_cstates[3] = { conf->acpi_c1, conf->acpi_c2, conf->acpi_c3 };
+
+	acpi_cstate_t acpi_cstate_map[ARRAY_SIZE(acpi_cstates)] = { 0 };
+
+	/* Count number of active C-states */
+	int count = 0;
+
+	for (int i = 0; i < ARRAY_SIZE(acpi_cstates); i++) {
+		if (acpi_cstates[i] > 0) {
+			acpi_cstate_map[count] = cpu->cstates[acpi_cstates[i]];
+			acpi_cstate_map[count].ctype = i + 1;
+			count++;
+		}
+	}
+	acpigen_write_CST_package(acpi_cstate_map, count);
 }

static acpi_tstate_t tss_table_fine[] = {