[PATCH 0/2] SEV firmware error list touchups

[PATCH 0/2] SEV firmware error list touchups

[Connor Kuehl]

Connor Kuehl (2):
  sev: use explicit indices for mapping firmware error codes to strings
  sev: add missing firmware error conditions

 target/i386/sev.c | 48 ++++++++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

-- 
2.29.2

[PATCH 1/2] sev: use explicit indices for mapping firmware error codes to strings

[Connor Kuehl]

This can help lower any margin for error when making future additions to
the list, especially if they're made out of order.

While doing so, make capitalization of ASID consistent with its usage in
the SEV firmware spec (Asid -> ASID).

Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
---
 target/i386/sev.c | 46 +++++++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 0f414df02f..0de690058e 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -88,29 +88,29 @@ static SevGuestState *sev_guest;
 static Error *sev_mig_blocker;
 
 static const char *const sev_fw_errlist[] = {
-    "",
-    "Platform state is invalid",
-    "Guest state is invalid",
-    "Platform configuration is invalid",
-    "Buffer too small",
-    "Platform is already owned",
-    "Certificate is invalid",
-    "Policy is not allowed",
-    "Guest is not active",
-    "Invalid address",
-    "Bad signature",
-    "Bad measurement",
-    "Asid is already owned",
-    "Invalid ASID",
-    "WBINVD is required",
-    "DF_FLUSH is required",
-    "Guest handle is invalid",
-    "Invalid command",
-    "Guest is active",
-    "Hardware error",
-    "Hardware unsafe",
-    "Feature not supported",
-    "Invalid parameter"
+    [SEV_RET_SUCCESS]                = "",
+    [SEV_RET_INVALID_PLATFORM_STATE] = "Platform state is invalid",
+    [SEV_RET_INVALID_GUEST_STATE]    = "Guest state is invalid",
+    [SEV_RET_INAVLID_CONFIG]         = "Platform configuration is invalid",
+    [SEV_RET_INVALID_LEN]            = "Buffer too small",
+    [SEV_RET_ALREADY_OWNED]          = "Platform is already owned",
+    [SEV_RET_INVALID_CERTIFICATE]    = "Certificate is invalid",
+    [SEV_RET_POLICY_FAILURE]         = "Policy is not allowed",
+    [SEV_RET_INACTIVE]               = "Guest is not active",
+    [SEV_RET_INVALID_ADDRESS]        = "Invalid address",
+    [SEV_RET_BAD_SIGNATURE]          = "Bad signature",
+    [SEV_RET_BAD_MEASUREMENT]        = "Bad measurement",
+    [SEV_RET_ASID_OWNED]             = "ASID is already owned",
+    [SEV_RET_INVALID_ASID]           = "Invalid ASID",
+    [SEV_RET_WBINVD_REQUIRED]        = "WBINVD is required",
+    [SEV_RET_DFFLUSH_REQUIRED]       = "DF_FLUSH is required",
+    [SEV_RET_INVALID_GUEST]          = "Guest handle is invalid",
+    [SEV_RET_INVALID_COMMAND]        = "Invalid command",
+    [SEV_RET_ACTIVE]                 = "Guest is active",
+    [SEV_RET_HWSEV_RET_PLATFORM]     = "Hardware error",
+    [SEV_RET_HWSEV_RET_UNSAFE]       = "Hardware unsafe",
+    [SEV_RET_UNSUPPORTED]            = "Feature not supported",
+    [SEV_RET_INVALID_PARAM]          = "Invalid parameter",
 };
 
 #define SEV_FW_MAX_ERROR      ARRAY_SIZE(sev_fw_errlist)
-- 
2.29.2

[PATCH 2/2] sev: add missing firmware error conditions

[Connor Kuehl]

The SEV userspace header[1] exports a couple of other error conditions that
aren't listed in QEMU's SEV implementation, so let's just round out the
list.

[1] linux-headers/linux/psp-sev.h

Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
---
 target/i386/sev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 0de690058e..37690ae809 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -111,6 +111,8 @@ static const char *const sev_fw_errlist[] = {
     [SEV_RET_HWSEV_RET_UNSAFE]       = "Hardware unsafe",
     [SEV_RET_UNSUPPORTED]            = "Feature not supported",
     [SEV_RET_INVALID_PARAM]          = "Invalid parameter",
+    [SEV_RET_RESOURCE_LIMIT]         = "Required firmware resource depleted",
+    [SEV_RET_SECURE_DATA_INVALID]    = "Part-specific integrity check failure",
 };
 
 #define SEV_FW_MAX_ERROR      ARRAY_SIZE(sev_fw_errlist)
-- 
2.29.2

Re: [PATCH 0/2] SEV firmware error list touchups

[Philippe Mathieu-Daudé]

On 2/18/21 4:16 PM, Connor Kuehl wrote:
> Connor Kuehl (2):
>   sev: use explicit indices for mapping firmware error codes to strings
>   sev: add missing firmware error conditions
> 
>  target/i386/sev.c | 48 ++++++++++++++++++++++++-----------------------
>  1 file changed, 25 insertions(+), 23 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>


To avoid this problem in future (new error code added on the Linux
kernel side) would it be acceptable to add a 3rd patch as:

-- >8 --
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 0f414df02f3..e086d3198e8 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -155,9 +155,12 @@ sev_platform_ioctl(int fd, int cmd, void *data, int
*error)
 static const char *
 fw_error_to_str(int code)
 {
+    QEMU_BUILD_BUG_ON(SEV_RET_SECURE_DATA_INVALID + 1 == SEV_RET_MAX);
+
     if (code < 0 || code >= SEV_FW_MAX_ERROR) {
         return "unknown error";
     }
+    assert(sev_fw_errlist[code]);

     return sev_fw_errlist[code];
 }
---

which triggers a build error if scripts/update-linux-headers.sh
added another sev_ret_code entry?

Re: [PATCH 0/2] SEV firmware error list touchups

[Connor Kuehl]

On 2/18/21 9:48 AM, Philippe Mathieu-Daudé wrote:
> On 2/18/21 4:16 PM, Connor Kuehl wrote:
>> Connor Kuehl (2):
>>    sev: use explicit indices for mapping firmware error codes to strings
>>    sev: add missing firmware error conditions
>>
>>   target/i386/sev.c | 48 ++++++++++++++++++++++++-----------------------
>>   1 file changed, 25 insertions(+), 23 deletions(-)
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thank you! :-)

> To avoid this problem in future (new error code added on the Linux
> kernel side) would it be acceptable to add a 3rd patch as:
> 
> -- >8 --
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 0f414df02f3..e086d3198e8 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -155,9 +155,12 @@ sev_platform_ioctl(int fd, int cmd, void *data, int
> *error)
>   static const char *
>   fw_error_to_str(int code)
>   {
> +    QEMU_BUILD_BUG_ON(SEV_RET_SECURE_DATA_INVALID + 1 == SEV_RET_MAX);
> +
>       if (code < 0 || code >= SEV_FW_MAX_ERROR) {
>           return "unknown error";
>       }
> +    assert(sev_fw_errlist[code]);
> 
>       return sev_fw_errlist[code];
>   }
> ---
> 
> which triggers a build error if scripts/update-linux-headers.sh
> added another sev_ret_code entry?
> 

I like this a lot. Should I send a v2 of the series with a third patch 
like this or shall I wait to see if these patches get applied then send 
something like this as a follow up patch?

Thank you,

Connor

Re: [PATCH 0/2] SEV firmware error list touchups

[Philippe Mathieu-Daudé]

On 2/19/21 3:46 PM, Connor Kuehl wrote:
> On 2/18/21 9:48 AM, Philippe Mathieu-Daudé wrote:
>> On 2/18/21 4:16 PM, Connor Kuehl wrote:
>>> Connor Kuehl (2):
>>>    sev: use explicit indices for mapping firmware error codes to strings
>>>    sev: add missing firmware error conditions
>>>
>>>   target/i386/sev.c | 48 ++++++++++++++++++++++++-----------------------
>>>   1 file changed, 25 insertions(+), 23 deletions(-)
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Thank you! :-)
> 
>> To avoid this problem in future (new error code added on the Linux
>> kernel side) would it be acceptable to add a 3rd patch as:
>>
>> -- >8 --
>> diff --git a/target/i386/sev.c b/target/i386/sev.c
>> index 0f414df02f3..e086d3198e8 100644
>> --- a/target/i386/sev.c
>> +++ b/target/i386/sev.c
>> @@ -155,9 +155,12 @@ sev_platform_ioctl(int fd, int cmd, void *data, int
>> *error)
>>   static const char *
>>   fw_error_to_str(int code)
>>   {
>> +    QEMU_BUILD_BUG_ON(SEV_RET_SECURE_DATA_INVALID + 1 == SEV_RET_MAX);
>> +
>>       if (code < 0 || code >= SEV_FW_MAX_ERROR) {
>>           return "unknown error";
>>       }
>> +    assert(sev_fw_errlist[code]);
>>
>>       return sev_fw_errlist[code];
>>   }
>> ---
>>
>> which triggers a build error if scripts/update-linux-headers.sh
>> added another sev_ret_code entry?
>>
> 
> I like this a lot. Should I send a v2 of the series with a third patch
> like this or shall I wait to see if these patches get applied then send
> something like this as a follow up patch?

Since I've the patch locally I'll simply send it.

Re: [PATCH 0/2] SEV firmware error list touchups

[Connor Kuehl]

On 2/18/21 9:16 AM, Connor Kuehl wrote:
> Connor Kuehl (2):
>    sev: use explicit indices for mapping firmware error codes to strings
>    sev: add missing firmware error conditions
> 
>   target/i386/sev.c | 48 ++++++++++++++++++++++++-----------------------
>   1 file changed, 25 insertions(+), 23 deletions(-)
> 

Ping. Also, I am not sure whose tree these are best suited for.

Connor

Re: [PATCH 0/2] SEV firmware error list touchups

[Connor Kuehl]

On 2/18/21 9:16 AM, Connor Kuehl wrote:
> Connor Kuehl (2):
>    sev: use explicit indices for mapping firmware error codes to strings
>    sev: add missing firmware error conditions
> 
>   target/i386/sev.c | 48 ++++++++++++++++++++++++-----------------------
>   1 file changed, 25 insertions(+), 23 deletions(-)
> 

Eduardo, Paolo, Richard: ping

Re: [PATCH 0/2] SEV firmware error list touchups

[Philippe Mathieu-Daudé]

Hi Connor,

On 3/15/21 3:08 PM, Connor Kuehl wrote:
> On 2/18/21 9:16 AM, Connor Kuehl wrote:
>> Connor Kuehl (2):
>>    sev: use explicit indices for mapping firmware error codes to strings
>>    sev: add missing firmware error conditions
>>
>>   target/i386/sev.c | 48 ++++++++++++++++++++++++-----------------------
>>   1 file changed, 25 insertions(+), 23 deletions(-)
>>
> 
> Eduardo, Paolo, Richard: ping

Looks too late for 6.0 now.

Can you repost/ping after QEMU 6.0 is release?

Thanks,

Phil.

Re: [PATCH 0/2] SEV firmware error list touchups

[Connor Kuehl]

On 3/22/21 5:18 AM, Philippe Mathieu-Daudé wrote:
> Hi Connor,
> 
> On 3/15/21 3:08 PM, Connor Kuehl wrote:
>> On 2/18/21 9:16 AM, Connor Kuehl wrote:
>>> Connor Kuehl (2):
>>>     sev: use explicit indices for mapping firmware error codes to strings
>>>     sev: add missing firmware error conditions
>>>
>>>    target/i386/sev.c | 48 ++++++++++++++++++++++++-----------------------
>>>    1 file changed, 25 insertions(+), 23 deletions(-)
>>>
>>
>> Eduardo, Paolo, Richard: ping
> 
> Looks too late for 6.0 now.
> 
> Can you repost/ping after QEMU 6.0 is release?

Sure.

Connor

Re: [PATCH 0/2] SEV firmware error list touchups

[Eduardo Habkost]

On Mon, Mar 22, 2021 at 09:09:44AM -0500, Connor Kuehl wrote:
> On 3/22/21 5:18 AM, Philippe Mathieu-Daudé wrote:
> > Hi Connor,
> > 
> > On 3/15/21 3:08 PM, Connor Kuehl wrote:
> > > On 2/18/21 9:16 AM, Connor Kuehl wrote:
> > > > Connor Kuehl (2):
> > > >     sev: use explicit indices for mapping firmware error codes to strings
> > > >     sev: add missing firmware error conditions
> > > > 
> > > >    target/i386/sev.c | 48 ++++++++++++++++++++++++-----------------------
> > > >    1 file changed, 25 insertions(+), 23 deletions(-)
> > > > 
> > > 
> > > Eduardo, Paolo, Richard: ping
> > 
> > Looks too late for 6.0 now.
> > 
> > Can you repost/ping after QEMU 6.0 is release?
> 
> Sure.

My apologies for not replying before and not reviewing or merging
this in time for 6.0.  I'm seriously behind on all my upstream
maintainer work.

-- 
Eduardo

Re: [PATCH 0/2] SEV firmware error list touchups

[Philippe Mathieu-Daudé]

On Mon, Mar 22, 2021 at 5:43 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Mar 22, 2021 at 09:09:44AM -0500, Connor Kuehl wrote:
> > On 3/22/21 5:18 AM, Philippe Mathieu-Daudé wrote:
> > > On 3/15/21 3:08 PM, Connor Kuehl wrote:
> > > > On 2/18/21 9:16 AM, Connor Kuehl wrote:
> > > > > Connor Kuehl (2):
> > > > >     sev: use explicit indices for mapping firmware error codes to strings
> > > > >     sev: add missing firmware error conditions
> > > > >
> > > > >    target/i386/sev.c | 48 ++++++++++++++++++++++++-----------------------
> > > > >    1 file changed, 25 insertions(+), 23 deletions(-)
> > > > >
> > > >
> > > > Eduardo, Paolo, Richard: ping
> > >
> > > Looks too late for 6.0 now.
> > >
> > > Can you repost/ping after QEMU 6.0 is release?
> >
> > Sure.
>
> My apologies for not replying before and not reviewing or merging
> this in time for 6.0.

FYI there is a RESEND:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg803017.html