Change in coreboot[master]: amd/microcode: Change equivalant ID width as 16bit

[Bao Zheng (Code Review)]

Hello Zheng Bao,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: amd/microcode: Change equivalant ID width as 16bit
......................................................................

amd/microcode: Change equivalant ID width as 16bit

Change-Id: Iacabee7e571bd37f3aca106d515d755969daf8f3
Signed-off-by: Zheng Bao 
---
M src/cpu/amd/pi/00730F01/microcode_fam16h.c
M src/cpu/amd/pi/00730F01/update_microcode.c
M src/include/cpu/amd/microcode.h
3 files changed, 4 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/41869/1

diff --git a/src/cpu/amd/pi/00730F01/microcode_fam16h.c b/src/cpu/amd/pi/00730F01/microcode_fam16h.c
index 496f2ec..7d47258 100644
--- a/src/cpu/amd/pi/00730F01/microcode_fam16h.c
+++ b/src/cpu/amd/pi/00730F01/microcode_fam16h.c
@@ -88,7 +88,7 @@
 }

static void amd_update_microcode(const void *ucode,  size_t ucode_len,
-				uint32_t equivalent_processor_rev_id)
+				uint16_t equivalent_processor_rev_id)
 {
    const struct microcode *m;
    const uint8_t *c = ucode;
@@ -99,7 +99,7 @@
    	apply_microcode_patch(m);
 }

-void amd_update_microcode_from_cbfs(uint32_t equivalent_processor_rev_id)
+void amd_update_microcode_from_cbfs(uint16_t equivalent_processor_rev_id)
 {
    const void *ucode;
    size_t ucode_len;
diff --git a/src/cpu/amd/pi/00730F01/update_microcode.c b/src/cpu/amd/pi/00730F01/update_microcode.c
index 9142681..e3f0c3b 100644
--- a/src/cpu/amd/pi/00730F01/update_microcode.c
+++ b/src/cpu/amd/pi/00730F01/update_microcode.c
@@ -37,7 +37,7 @@

void update_microcode(u32 cpu_deviceid)
 {
-	u32 equivalent_processor_rev_id =
+	u16 equivalent_processor_rev_id =
    			get_equivalent_processor_rev_id(cpu_deviceid);
    amd_update_microcode_from_cbfs(equivalent_processor_rev_id);
 }
diff --git a/src/include/cpu/amd/microcode.h b/src/include/cpu/amd/microcode.h
index 8ebe675..29b5576 100644
--- a/src/include/cpu/amd/microcode.h
+++ b/src/include/cpu/amd/microcode.h
@@ -2,6 +2,6 @@
 #define CPU_AMD_MICROCODE_H

void update_microcode(u32 cpu_deviceid);
-void amd_update_microcode_from_cbfs(u32 equivalent_processor_rev_id);
+void amd_update_microcode_from_cbfs(u16 equivalent_processor_rev_id);

#endif /* CPU_AMD_MICROCODE_H */

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41869 )

Change subject: amd/microcode: Change equivalant ID width as 16bit
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/41869/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/41869/2//COMMIT_MSG@7 
PS2, Line 7: as
to

https://review.coreboot.org/c/coreboot/+/41869/2//COMMIT_MSG@8 
PS2, Line 8: 
Please add an explanation to the commit message body.

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41869 )

Change subject: amd/microcode: Change equivalant ID width as 16bit
......................................................................

Patch Set 2:

I've only looked at the internal fam17h RV1/PCO PPR, but that one disagrees with this. was there a change between fam16h and fam17h?

[Bao Zheng (Code Review)]

Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41869 )

Change subject: amd/microcode: Change equivalant ID width as 16bit
......................................................................

Patch Set 2:
> Patch Set 2:
>
> I've only looked at the internal fam17h RV1/PCO PPR, but that one disagrees with this. was there a change between fam16h and fam17h?

struct microcode {
    uint32_t date_code;
    uint32_t patch_id;

uint16_t mc_patch_data_id;
    uint8_t reserved1[6];

uint32_t chipset1_dev_id;
    uint32_t chipset2_dev_id;

uint16_t processor_rev_id;  //This field is 16bit.

uint8_t chipset1_rev_id;
    uint8_t chipset2_rev_id;

uint8_t reserved2[4];

uint8_t m_patch_data[F17H_MPB_MAX_SIZE-F17H_MPB_DATA_OFFSET];

};

Change in coreboot[master]: amd/microcode: Change equivalant ID width to 16bit

[Bao Zheng (Code Review)]

Hello build bot (Jenkins), Zheng Bao,

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

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

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

Change subject: amd/microcode: Change equivalant ID width to 16bit
......................................................................

amd/microcode: Change equivalant ID width to 16bit

The definition of processor_rev_id in struct microcode
is 16 bits. So we need to change the a series of parameters
passing to 16 bits.

Change-Id: Iacabee7e571bd37f3aca106d515d755969daf8f3
Signed-off-by: Zheng Bao 
---
M src/cpu/amd/pi/00730F01/microcode_fam16h.c
M src/cpu/amd/pi/00730F01/update_microcode.c
M src/include/cpu/amd/microcode.h
3 files changed, 4 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/41869/3

Change in coreboot[master]: amd/microcode: Change equivalant ID width to 16bit

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41869 )

Change subject: amd/microcode: Change equivalant ID width to 16bit
......................................................................

Patch Set 3: Code-Review-1
> uint16_t processor_rev_id;  //This field is 16bit.
>    uint8_t chipset1_rev_id;
>    uint8_t chipset2_rev_id;

I've looked at the fam 16h models 30-3fh BKDG and that matches the existing struct.

In fam17h RV1/PCO PPR the microcode patch equivalent processor ID is 4 bytes long and the chipset revision ID fields don't seem to exist any more. See the PPR section "Microcode Patch Block (MPB)".

So the struct needs to be split into two different versions for pre-zen and zen. in both cases the processor revision ID starts at offset 18h == 24

Change in coreboot[master]: amd/microcode: Change equivalant ID width to 16bit

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41869 )

Change subject: amd/microcode: Change equivalant ID width to 16bit
......................................................................

Patch Set 3:

I remember I had to update the microcode structures for the family 16h, because earlier structures were incompatible...

Probably it will require a separate directory with family 17h definitions...

Change in coreboot[master]: amd/microcode: Change equivalant ID width to 16bit

[Bao Zheng (Code Review)]

Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41869 )

Change subject: amd/microcode: Change equivalant ID width to 16bit
......................................................................

Patch Set 3:
> Patch Set 3: Code-Review-1
> > uint16_t processor_rev_id;  //This field is 16bit.
> >    uint8_t chipset1_rev_id;
> >    uint8_t chipset2_rev_id;
>
> I've looked at the fam 16h models 30-3fh BKDG and that matches the existing struct.
>
> In fam17h RV1/PCO PPR the microcode patch equivalent processor ID is 4 bytes long and the chipset revision ID fields don't seem to exist any more. See the PPR section "Microcode Patch Block (MPB)".
>
> So the struct needs to be split into two different versions for pre-zen and zen. in both cases the processor revision ID starts at offset 18h == 24

MPB_NB_ID         0010h 4 
MPB_SB_ID         0014h 4 
MPB_REVISION      0018h 4 
MPB_BIOS_REVISION 001Ch 1
(Reserved)        001Dh 3 
Patch Data        0020h 2272

Here MPB_BIOS_REVISION has the same offset as equivalent processor ID. But this field combines the
equivalent processor ID, Chipset1 RevisionID and Chipset2 RevisionID.
The AGESA source code also compares the 16bit data to decide if the patch is going to be loaded.

Change in coreboot[master]: amd/microcode: Change equivalant ID width to 16bit

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41869 )

Change subject: amd/microcode: Change equivalant ID width to 16bit
......................................................................

Patch Set 3: Code-Review+2
> Here MPB_BIOS_REVISION has the same offset as equivalent processor ID. But this field combines the
> equivalent processor ID, Chipset1 RevisionID and Chipset2 RevisionID.
> The AGESA source code also compares the 16bit data to decide if the patch is going to be loaded.

Ok, that explains things. The reference code does indeed only use 16 bit for this field; only looked at the PPR before. I wonder why the documentation combines these 3 fields into one 32 bit one though.

Change in coreboot[master]: amd/microcode: Change equivalant ID width to 16bit

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41869 )

Change subject: amd/microcode: Change equivalant ID width to 16bit
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/41869/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/41869/2//COMMIT_MSG@7 
PS2, Line 7: as
> to

Done

https://review.coreboot.org/c/coreboot/+/41869/2//COMMIT_MSG@8 
PS2, Line 8:
> Please add an explanation to the commit message body.

Done

Change in coreboot[master]: amd/microcode: Change equivalant ID width to 16bit

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41869 )

Change subject: amd/microcode: Change equivalant ID width to 16bit
......................................................................

amd/microcode: Change equivalant ID width to 16bit

The definition of processor_rev_id in struct microcode
is 16 bits. So we need to change the a series of parameters
passing to 16 bits.

Change-Id: Iacabee7e571bd37f3aca106d515d755969daf8f3
Signed-off-by: Zheng Bao 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/41869
Tested-by: build bot (Jenkins) 
Reviewed-by: Felix Held 
---
M src/cpu/amd/pi/00730F01/microcode_fam16h.c
M src/cpu/amd/pi/00730F01/update_microcode.c
M src/include/cpu/amd/microcode.h
3 files changed, 4 insertions(+), 4 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Felix Held: Looks good to me, approved

diff --git a/src/cpu/amd/pi/00730F01/microcode_fam16h.c b/src/cpu/amd/pi/00730F01/microcode_fam16h.c
index 496f2ec..7d47258 100644
--- a/src/cpu/amd/pi/00730F01/microcode_fam16h.c
+++ b/src/cpu/amd/pi/00730F01/microcode_fam16h.c
@@ -88,7 +88,7 @@
 }

static void amd_update_microcode(const void *ucode,  size_t ucode_len,
-				uint32_t equivalent_processor_rev_id)
+				uint16_t equivalent_processor_rev_id)
 {
    const struct microcode *m;
    const uint8_t *c = ucode;
@@ -99,7 +99,7 @@
    	apply_microcode_patch(m);
 }

-void amd_update_microcode_from_cbfs(uint32_t equivalent_processor_rev_id)
+void amd_update_microcode_from_cbfs(uint16_t equivalent_processor_rev_id)
 {
    const void *ucode;
    size_t ucode_len;
diff --git a/src/cpu/amd/pi/00730F01/update_microcode.c b/src/cpu/amd/pi/00730F01/update_microcode.c
index 9142681..e3f0c3b 100644
--- a/src/cpu/amd/pi/00730F01/update_microcode.c
+++ b/src/cpu/amd/pi/00730F01/update_microcode.c
@@ -37,7 +37,7 @@

void update_microcode(u32 cpu_deviceid)
 {
-	u32 equivalent_processor_rev_id =
+	u16 equivalent_processor_rev_id =
    			get_equivalent_processor_rev_id(cpu_deviceid);
    amd_update_microcode_from_cbfs(equivalent_processor_rev_id);
 }
diff --git a/src/include/cpu/amd/microcode.h b/src/include/cpu/amd/microcode.h
index 8ebe675..29b5576 100644
--- a/src/include/cpu/amd/microcode.h
+++ b/src/include/cpu/amd/microcode.h
@@ -2,6 +2,6 @@
 #define CPU_AMD_MICROCODE_H

void update_microcode(u32 cpu_deviceid);
-void amd_update_microcode_from_cbfs(u32 equivalent_processor_rev_id);
+void amd_update_microcode_from_cbfs(u16 equivalent_processor_rev_id);

#endif /* CPU_AMD_MICROCODE_H */