crypto: ccp: Load the firmware twice when SEV API version < 1.43
Subsystems (3)
PATCH 1/1 crypto: ccp: Load the firmware twice when SEV API version < 1.43
From: Jarkko Sakkinen <[email protected]>
SEV-SNP does not initialize to a legit state, unless the firmware is loaded twice, when SEP API version < 1.43, and the firmware is updated to a later version. Because of this user space needs to work around this with "rmmod && modprobe" combo. Fix this by implementing the workaround to the driver.
--- drivers/crypto/ccp/sev-dev.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)
drivers/crypto/ccp/sev-dev.c +19 -3
@@ -76,6 +76,9 @@ static void *sev_es_tmr;#define NV_LENGTH (32 * 1024)static void *sev_init_ex_buffer; +/* +* SEV API version >= maj.min? +*/ static inline bool sev_version_greater_or_equal (u8 maj,u8 min) {struct sev_device *sev = psp_master->sev_data;
@@ -89,6 +92,14 @@ static inline bool sev_version_greater_or_equal(u8 maj, u8 min)return false; } +/* +* SEV API version < maj.min? +*/ +static inline bool sev_version_less (u8 maj,u8 min) +{ +return !sev_version_greater_or_equal (maj, min); +} +static void sev_irq_handler (int irq,void *data,unsigned int status) {struct sev_device *sev = data;
@@ -1274,6 +1285,7 @@ void sev_pci_init(void) {struct sev_device *sev = psp_master->sev_data;int error, rc; +int i;if (!sev)return ;
@@ -1283,9 +1295,13 @@ void sev_pci_init(void)if (sev_get_api_version ()) goto err; -if (sev_version_greater_or_equal (0 ,15 ) && -sev_update_firmware (sev->dev) ==0 ) -sev_get_api_version (); +/* +* SEV-SNP does not work properly before loading the FW twice in the API +* versions older than SEV 1.43. +*/ +for (i =0 ; i <sev_version_greater_or_equal (0 ,15 ) +sev_version_less (1 ,43 ); i++) +if (sev_update_firmware (sev->dev) ==0 ) +sev_get_api_version ();/* If an init_ex_path is provided rely on INIT_EX for PSP initialization * instead of INIT.
Signature · 1 line
2.37.1
Tom Lendacky
maintainerQuoted 7 lines · Jarkko Sakkinen
From: Jarkko Sakkinen <[email protected]>
SEV-SNP does not initialize to a legit state, unless the firmware is loaded twice, when SEP API version < 1.43, and the firmware is updated to a later version. Because of this user space needs to work around this with "rmmod && modprobe" combo. Fix this by implementing the workaround to the driver.
The SNP hypervisor patches are placing a minimum supported version requirement for the SEV firmware that exceeds the specified version above [1] (for the reason above, as well as some others), so this patch is not needed, NAK.
Thanks,
Tom
Quoted 61 lines
Reported-by Harald Hoyer <[email protected]>Signed-off-by Jarkko Sakkinen <[email protected]>--- drivers/crypto/ccp/sev-dev.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)drivers/crypto/ccp/sev-dev.c +19
-3@@ -76,6 +76,9 @@ static void *sev_es_tmr;#define NV_LENGTH (32 * 1024)static void *sev_init_ex_buffer; +/* +* SEV API version >= maj.min? +*/ static inline bool sev_version_greater_or_equal (u8 maj,u8 min) {struct sev_device *sev = psp_master->sev_data;@@ -89,6 +92,14 @@ static inline bool sev_version_greater_or_equal(u8 maj, u8 min)return false; } +/* +* SEV API version < maj.min? +*/ +static inline bool sev_version_less (u8 maj,u8 min) +{ +return !sev_version_greater_or_equal (maj, min); +} +static void sev_irq_handler (int irq,void *data,unsigned int status) {struct sev_device *sev = data;@@ -1274,6 +1285,7 @@ void sev_pci_init(void) {struct sev_device *sev = psp_master->sev_data;int error, rc; +int i;if (!sev)return ;@@ -1283,9 +1295,13 @@ void sev_pci_init(void)if (sev_get_api_version ()) goto err; -if (sev_version_greater_or_equal (0 ,15 ) && -sev_update_firmware (sev->dev) ==0 ) -sev_get_api_version (); +/* +* SEV-SNP does not work properly before loading the FW twice in the API +* versions older than SEV 1.43. +*/ +for (i =0 ; i <sev_version_greater_or_equal (0 ,15 ) +sev_version_less (1 ,43 ); i++) +if (sev_update_firmware (sev->dev) ==0 ) +sev_get_api_version ();/* If an init_ex_path is provided rely on INIT_EX for PSP initialization * instead of INIT.
Am 04.08.22 um 15:13 schrieb Tom Lendacky:
Quoted 9 lines · Jarkko Sakkinen
Quoted 7 lines
From: Jarkko Sakkinen <[email protected]>
SEV-SNP does not initialize to a legit state, unless the firmware is loaded twice, when SEP API version < 1.43, and the firmware is updated to a later version. Because of this user space needs to work around this with "rmmod && modprobe" combo. Fix this by implementing the workaround to the driver.
Quoted 4 lines
The SNP hypervisor patches are placing a minimum supported version requirement for the SEV firmware that exceeds the specified version above [1] (for the reason above, as well as some others), so this patch is not needed, NAK.
As described in the "Milan Release Notes.txt" of the AMD firmware update package amd_sev_fam19h_model0xh_1.33.03.zip.
"If upgrading to 1.33.01 or later from something older (picking up CSF-1201), it is required that two Download Firmware commands be run to fix the "Committed Version" across the firmware. CSF-1201 fixed a bug where the committed version in the attestation report was incorrect. Performing a single Download Firmware will upgrade the firmware, but performing a second one will correct the committed version. This is a one-time upgrade issue. "
Note that 1.33.01 is not the same version number as "1.51" in [1]. One is the firmware version, the other is the SEV-SNP API version.
I am definitely seeing a wrong TCB version, if the firmware is only updated once to 1.33.01 aka "1.51". Reloading the ccp module, which triggers another firmware load, cures the problem.
The patch might be wrong, as it might not do the right thing, but the problem and the solution exist.
What is your suggestion then to fix the wrong committed TCB version?
Quoted 62 lines
Thanks,
TomQuoted 56 lines
Reported-by Harald Hoyer <[email protected]>Signed-off-by Jarkko Sakkinen <[email protected]>---
drivers/crypto/ccp/sev-dev.c| 22 +++++++++++++++++++---1 file changed, 19 insertions(+), 3 deletions(-)drivers/crypto/ccp/sev-dev.c +19
-3@@ -76,6 +76,9 @@ static void *sev_es_tmr; #define NV_LENGTH (32 * 1024) static void *sev_init_ex_buffer; +/* +* SEV API version >= maj.min? +*/ static inline bool sev_version_greater_or_equal(u8 maj, u8 min) { struct sev_device *sev = psp_master->sev_data;@@ -89,6 +92,14 @@ static inline bool sev_version_greater_or_equal(u8 maj, u8 min) return false; } +/* +* SEV API version < maj.min? +*/ +static inline bool sev_version_less (u8 maj,u8 min) +{ + return !sev_version_greater_or_equal(maj, min); +} + static void sev_irq_handler(int irq, void *data, unsigned int status) { struct sev_device *sev = data;@@ -1274,6 +1285,7 @@ void sev_pci_init(void) { struct sev_device *sev = psp_master->sev_data; int error, rc; + int i; if (!sev) return;@@ -1283,9 +1295,13 @@ void sev_pci_init(void) if (sev_get_api_version()) goto err; - if (sev_version_greater_or_equal(0, 15) && - sev_update_firmware(sev->dev) == 0) - sev_get_api_version(); + /* + * SEV-SNP does not work properly before loading the FW twice in the API + * versions older than SEV 1.43. + */ + for (i = 0; i < sev_version_greater_or_equal(0, 15) + sev_version_less(1, 43); i++) + if (sev_update_firmware(sev->dev) == 0) + sev_get_api_version(); /* If an init_ex_path is provided rely on INIT_EX for PSP initialization* instead of INIT.
Tom Lendacky
maintainerQuoted 15 lines · Harald Hoyer
Am 04.08.22 um 15:13 schrieb Tom Lendacky:
Quoted 9 lines · Jarkko Sakkinen
Quoted 7 lines
From: Jarkko Sakkinen <[email protected]>
SEV-SNP does not initialize to a legit state, unless the firmware is loaded twice, when SEP API version < 1.43, and the firmware is updated to a later version. Because of this user space needs to work around this with "rmmod && modprobe" combo. Fix this by implementing the workaround to the driver.
Quoted 4 lines
The SNP hypervisor patches are placing a minimum supported version requirement for the SEV firmware that exceeds the specified version above [1] (for the reason above, as well as some others), so this patch is not needed, NAK.
Quoted 6 lines
As described in the "Milan Release Notes.txt" of the AMD firmware update package amd_sev_fam19h_model0xh_1.33.03.zip.
"If upgrading to 1.33.01 or later from something older (picking up CSF-1201), it is required that two Download Firmware commands be run to fix the "Committed Version" across the firmware. CSF-1201 fixed a bug where the committed version in the attestation report was incorrect. Performing a single Download Firmware will upgrade the firmware, but performing a second one will correct the committed version. This is a one-time upgrade issue. "
Note that
1.33.01is not the same version number as "1.51" in [1]. One is the firmware version, the other is the SEV-SNP API version.
It is the same and are meant to correlate, the 33 is hex => 51.
Quoted 7 lines
I am definitely seeing a wrong TCB version, if the firmware is only updated once to
1.33.01aka "1.51". Reloading theccpmodule, which triggers another firmware load, cures the problem.The patch might be wrong, as it might not do the right thing, but the problem and the solution exist.
What is your suggestion then to fix the wrong committed TCB version?
Hmmm... ok, I see what you're saying. We don't want to have to make everyone update their BIOS/firmware to get to a starting level above 1.43 to begin with.
Ok, let me review/comment on the patch.
Thanks,
Tom
Quoted 68 lines
Quoted 67 lines
Thanks,
TomQuoted 59 lines
Reported-by Harald Hoyer <[email protected]>Signed-off-by Jarkko Sakkinen <[email protected]>---
drivers/crypto/ccp/sev-dev.c| 22 +++++++++++++++++++---1 file changed, 19 insertions(+), 3 deletions(-)drivers/crypto/ccp/sev-dev.c +14
-3@@ -76,6 +76,9 @@ static void *sev_es_tmr; #define NV_LENGTH (32 * 1024) static void *sev_init_ex_buffer; +/* +* SEV API version >= maj.min? +*/ static inline bool sev_version_greater_or_equal(u8 maj, u8 min) { struct sev_device *sev = psp_master->sev_data;@@ -89,6 +92,14 @@ static inline bool sev_version_greater_or_equal(u8 maj, u8 min) return false; } +/* +* SEV API version < maj.min? +*/ +static inline bool sev_version_less (u8 maj,u8 min) +{ + return !sev_version_greater_or_equal(maj, min); +} + static void sev_irq_handler(int irq, void *data, unsigned int status) { struct sev_device *sev = data;@@ -1274,6 +1285,7 @@ void sev_pci_init(void) { struct sev_device *sev = psp_master->sev_data; int error, rc; + int i; if (!sev) return;@@ -1283,9 +1295,13 @@ void sev_pci_init(void) if (sev_get_api_version()) goto err; - if (sev_version_greater_or_equal(0, 15) && - sev_update_firmware(sev->dev) == 0) - sev_get_api_version(); + /* +* SEV-SNP does not work properly before loading the FW twice in the API
+ * versions older than SEV 1.43.
+ */
+ for (i = 0; i <sev_version_greater_or_equal(0, 15)+sev_version_less(1, 43); i++)
+ if (sev_update_firmware(sev->dev)== 0)
+sev_get_api_version();
/* If aninit_ex_pathis provided rely onINIT_EXfor PSP initialization
* instead of INIT.
Quoted 42 lines · Tom Lendacky
Quoted 27 lines · Harald Hoyer
Am 04.08.22 um 15:13 schrieb Tom Lendacky:
Quoted 9 lines · Jarkko Sakkinen
Quoted 7 lines
From: Jarkko Sakkinen <[email protected]>
SEV-SNP does not initialize to a legit state, unless the firmware is loaded twice, when SEP API version < 1.43, and the firmware is updated to a later version. Because of this user space needs to work around this with "rmmod && modprobe" combo. Fix this by implementing the workaround to the driver.
Quoted 4 lines
The SNP hypervisor patches are placing a minimum supported version requirement for the SEV firmware that exceeds the specified version above [1] (for the reason above, as well as some others), so this patch is not needed, NAK.
As described in the "Milan Release Notes.txt" of the AMD firmware update package amd_sev_fam19h_model0xh_1.33.03.zip.
"If upgrading to 1.33.01 or later from something older (picking up CSF-1201), it is required that two Download Firmware commands be run to fix the "Committed Version" across the firmware. CSF-1201 fixed a bug where the committed version in the attestation report was incorrect. Performing a single Download Firmware will upgrade the firmware, but performing a second one will correct the committed version. This is a one-time upgrade issue.
"Quoted 2 lines
Note that
1.33.01is not the same version number as "1.51" in [1]. One is the firmware version, the other is the SEV-SNP API version.It is the same and are meant to correlate, the 33 is hex => 51.
Quoted 9 lines
I am definitely seeing a wrong TCB version, if the firmware is only updated once to
1.33.01aka "1.51".
Reloading theccpmodule, which triggers another firmware load, cures the problem.The patch might be wrong, as it might not do the right thing, but the problem and the solution exist.
Quoted 7 lines
Quoted 1 line
What is your suggestion then to fix the wrong committed TCB version?
Hmmm... ok, I see what you're saying. We don't want to have to make everyone update their BIOS/firmware to get to a starting level above 1.43 to begin with.
Ok, let me review/comment on the patch.
This was has a bug, and the reference to what Harald denoted is missing. Hold on for v2. I'll put it out soon.
BR, Jarkko
Quoted 17 lines · Harald Hoyer
Am 04.08.22 um 15:13 schrieb Tom Lendacky:
Quoted 9 lines · Jarkko Sakkinen
Quoted 7 lines
From: Jarkko Sakkinen <[email protected]>
SEV-SNP does not initialize to a legit state, unless the firmware is loaded twice, when SEP API version < 1.43, and the firmware is updated to a later version. Because of this user space needs to work around this with "rmmod && modprobe" combo. Fix this by implementing the workaround to the driver.
Quoted 4 lines
The SNP hypervisor patches are placing a minimum supported version requirement for the SEV firmware that exceeds the specified version above [1] (for the reason above, as well as some others), so this patch is not needed, NAK.
As described in the "Milan Release Notes.txt" of the AMD firmware update package amd_sev_fam19h_model0xh_1.33.03.zip.
Quoted 7 lines
"If upgrading to 1.33.01 or later from something older (picking up CSF-1201), it is required that two Download Firmware commands be run to fix the "Committed Version" across the firmware. CSF-1201 fixed a bug where the committed version in the attestation report was incorrect. Performing a single Download Firmware will upgrade the firmware, but performing a second one will correct the committed version. This is a one-time upgrade issue. "
Reference should be part of the commit message. I'll update for the next iteration. Thanks for the remark.
BR, Jarkko
Quoted 14 lines · Tom Lendacky
Quoted 7 lines · Jarkko Sakkinen
From: Jarkko Sakkinen <[email protected]>
SEV-SNP does not initialize to a legit state, unless the firmware is loaded twice, when SEP API version < 1.43, and the firmware is updated to a later version. Because of this user space needs to work around this with "rmmod && modprobe" combo. Fix this by implementing the workaround to the driver.
The SNP hypervisor patches are placing a minimum supported version requirement for the SEV firmware that exceeds the specified version above [1] (for the reason above, as well as some others), so this patch is not needed, NAK.
Quoted 4 lines
Thanks,
Tom
Hi Tom,
Is there any particular reason for this restriction? Does SNP not work with API version <v1.51? Do you have a link to a changelog that describes the changes between different firmware versions?
Thanks,
Jeremi
Quoted 57 lines
Quoted 57 lines
Reported-by Harald Hoyer <[email protected]>Signed-off-by Jarkko Sakkinen <[email protected]>--- drivers/crypto/ccp/sev-dev.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)drivers/crypto/ccp/sev-dev.c +19
-3@@ -76,6 +76,9 @@ static void *sev_es_tmr;#define NV_LENGTH (32 * 1024)static void *sev_init_ex_buffer; +/* +* SEV API version >= maj.min? +*/ static inline bool sev_version_greater_or_equal (u8 maj,u8 min) {struct sev_device *sev = psp_master->sev_data;@@ -89,6 +92,14 @@ static inline bool sev_version_greater_or_equal(u8 maj, u8 min)return false; } +/* +* SEV API version < maj.min? +*/ +static inline bool sev_version_less (u8 maj,u8 min) +{ +return !sev_version_greater_or_equal (maj, min); +} +static void sev_irq_handler (int irq,void *data,unsigned int status) {struct sev_device *sev = data;@@ -1274,6 +1285,7 @@ void sev_pci_init(void) {struct sev_device *sev = psp_master->sev_data;int error, rc; +int i;if (!sev)return ;@@ -1283,9 +1295,13 @@ void sev_pci_init(void)if (sev_get_api_version ()) goto err; -if (sev_version_greater_or_equal (0 ,15 ) && -sev_update_firmware (sev->dev) ==0 ) -sev_get_api_version (); +/* +* SEV-SNP does not work properly before loading the FW twice in the API +* versions older than SEV 1.43. +*/ +for (i =0 ; i <sev_version_greater_or_equal (0 ,15 ) +sev_version_less (1 ,43 ); i++) +if (sev_update_firmware (sev->dev) ==0 ) +sev_get_api_version ();/* If an init_ex_path is provided rely on INIT_EX for PSP initialization * instead of INIT.
Tom Lendacky
maintainerQuoted 31 lines · Jarkko Sakkinen
From: Jarkko Sakkinen <[email protected]>
SEV-SNP does not initialize to a legit state, unless the firmware is loaded twice, when SEP API version < 1.43, and the firmware is updated to a later version. Because of this user space needs to work around this with "rmmod && modprobe" combo. Fix this by implementing the workaround to the driver.
Reported-by Harald Hoyer <[email protected]>Signed-off-by Jarkko Sakkinen <[email protected]>--- drivers/crypto/ccp/sev-dev.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)drivers/crypto/ccp/sev-dev.c +1
-0@@ -1274,6 +1285,7 @@ void sev_pci_init(void) {struct sev_device *sev = psp_master->sev_data;int error, rc; +int i;if (!sev)return ;@@ -1283,9 +1295,13 @@ void sev_pci_init(void)if (sev_get_api_version ()) goto err;
drivers/crypto/ccp/sev-dev.c void sev_pci_init(void) · line 1304
Quoted 10 lines
drivers/crypto/ccp/sev-dev.c +7
-3-if (sev_version_greater_or_equal (0 ,15 ) && -sev_update_firmware (sev->dev) ==0 ) -sev_get_api_version (); +/* +* SEV-SNP does not work properly before loading the FW twice in the API +* versions older than SEV 1.43. +*/ +for (i =0 ; i <sev_version_greater_or_equal (0 ,15 ) +sev_version_less (1 ,43 ); i++) +if (sev_update_firmware (sev->dev) ==0 ) +sev_get_api_version ();
I prefer to have this logic in the sev_update_firmware() function.
And while the loop is correct, it isn't obvious. Something in sev_update_firmware() that just does:
ret = sev_do_cmd(SEV_CMD_DOWNLOAD_FIRMWARE, data, &error); if (!ret && sev_version_greater_or_equal(0, 15) && sev_version_less(1, 43)) ret = sev_do_cmd(SEV_CMD_DOWNLOAD_FIRMWARE, data, &error);
And place a comment before the second command that references the reason for the second download.
Thanks,
Tom
Quoted 1 line
Quoted 46 lines · Tom Lendacky
Quoted 29 lines · Jarkko Sakkinen
From: Jarkko Sakkinen <[email protected]>
SEV-SNP does not initialize to a legit state, unless the firmware is loaded twice, when SEP API version < 1.43, and the firmware is updated to a later version. Because of this user space needs to work around this with "rmmod && modprobe" combo. Fix this by implementing the workaround to the driver.
Reported-by Harald Hoyer <[email protected]>Signed-off-by Jarkko Sakkinen <[email protected]>--- drivers/crypto/ccp/sev-dev.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)drivers/crypto/ccp/sev-dev.c +1
-0@@ -1274,6 +1285,7 @@ void sev_pci_init(void) {struct sev_device *sev = psp_master->sev_data;int error, rc; +int i;if (!sev)return ;@@ -1283,9 +1295,13 @@ void sev_pci_init(void)if (sev_get_api_version ()) goto err;drivers/crypto/ccp/sev-dev.c void sev_pci_init(void) · line 1303
Quoted 10 lines
drivers/crypto/ccp/sev-dev.c +7
-3-if (sev_version_greater_or_equal (0 ,15 ) && -sev_update_firmware (sev->dev) ==0 ) -sev_get_api_version (); +/* +* SEV-SNP does not work properly before loading the FW twice in the API +* versions older than SEV 1.43. +*/ +for (i =0 ; i <sev_version_greater_or_equal (0 ,15 ) +sev_version_less (1 ,43 ); i++) +if (sev_update_firmware (sev->dev) ==0 ) +sev_get_api_version ();I prefer to have this logic in the
sev_update_firmware()function.And while the loop is correct, it isn't obvious. Something in
sev_update_firmware()that just does:
Quoted 7 lines
ret = sev_do_cmd(SEV_CMD_DOWNLOAD_FIRMWARE, data, &error); if (!ret && sev_version_greater_or_equal(0, 15) && sev_version_less(1, 43)) ret = sev_do_cmd(SEV_CMD_DOWNLOAD_FIRMWARE, data, &error);And place a comment before the second command that references the reason for the second download.
OK, I'll do that. Thank you.
BR, Jarkko