[PATCH 00/11] target/i386/sev: Housekeeping helping using SEV-disabled binaries

[PATCH 00/11] target/i386/sev: Housekeeping helping using SEV-disabled binaries

[Philippe Mathieu-Daudé]

Hi,

I wasted some time trying to figure out how OVMF was supposed to
behave until realizing the binary I was using was built without SEV
support... Then wrote this series to help other developers to not
hit the same problem.
Some SEV patches I was following have been queued on Eduardo's
'x86-next' tree, so I used his tree as base, and included David and
Connor patches to reduce merge conflicts.

Based-on: https://gitlab.com/ehabkost/qemu/-/commits/x86-next/

Connor Kuehl (1):
  MAINTAINERS: Add Connor Kuehl as reviewer for AMD SEV

Dr. David Alan Gilbert (1):
  target/i386/sev: sev_get_attestation_report use g_autofree

Philippe Mathieu-Daudé (9):
  qapi/misc-target: Group SEV QAPI definitions
  target/i386/monitor: Return QMP error when SEV is disabled in build
  target/i386/cpu: Add missing 'qapi/error.h' header
  target/i386/sev_i386.h: Remove unused headers
  target/i386/sev: Remove sev_get_me_mask()
  target/i386/sev: Mark unreachable code with g_assert_not_reached()
  target/i386/sev: Restrict SEV to system emulation
  target/i386/monitor: Move SEV specific commands to sev.c
  monitor: Restrict 'info sev' to x86 targets

 qapi/misc-target.json         |  75 +++++++++++-----------
 include/monitor/hmp-target.h  |   1 +
 include/monitor/hmp.h         |   1 -
 target/i386/sev_i386.h        |   5 --
 target/i386/cpu.c             |   1 +
 target/i386/monitor.c         |  91 --------------------------
 target/i386/sev-stub.c        |  49 +-------------
 target/i386/sev-sysemu-stub.c |  69 ++++++++++++++++++++
 target/i386/sev.c             | 117 +++++++++++++++++++++++++++++-----
 MAINTAINERS                   |   7 ++
 target/i386/meson.build       |   4 +-
 11 files changed, 219 insertions(+), 201 deletions(-)
 create mode 100644 target/i386/sev-sysemu-stub.c

-- 
2.31.1

[PATCH 01/11] MAINTAINERS: Add Connor Kuehl as reviewer for AMD SEV

[Philippe Mathieu-Daudé]

From: Connor Kuehl <ckuehl@redhat.com>

It may not be appropriate for me to take over as a maintainer at this time,
but I would consider myself familiar with AMD SEV and what this code is
meant to be doing as part of a VMM for launching SEV-protected guests.

To that end, I would be happy to volunteer as a reviewer for SEV-related
changes so that I am CC'd on them and can help share the review burden with
whoever does maintain this code.

Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
Message-Id: <20210608192537.103584-1-ckuehl@redhat.com>
[PMD: Cover more files]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 56888121ec8..a93f4ba1861 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2938,6 +2938,13 @@ F: hw/core/clock-vmstate.c
 F: hw/core/qdev-clock.c
 F: docs/devel/clocks.rst
 
+AMD Secure Encrypted Virtualization (SEV)
+R: Connor Kuehl <ckuehl@redhat.com>
+F: docs/amd-memory-encryption.txt
+F: accel/kvm/sev-stub.c
+F: target/i386/sev*
+F: include/sysemu/sev.h
+
 Usermode Emulation
 ------------------
 Overall usermode emulation
-- 
2.31.1

Re: [PATCH 01/11] MAINTAINERS: Add Connor Kuehl as reviewer for AMD SEV

[Philippe Mathieu-Daudé]

On 6/10/21 8:45 AM, Philippe Mathieu-Daudé wrote:
> From: Connor Kuehl <ckuehl@redhat.com>
> 
> It may not be appropriate for me to take over as a maintainer at this time,
> but I would consider myself familiar with AMD SEV and what this code is
> meant to be doing as part of a VMM for launching SEV-protected guests.
> 
> To that end, I would be happy to volunteer as a reviewer for SEV-related
> changes so that I am CC'd on them and can help share the review burden with
> whoever does maintain this code.
> 
> Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
> Message-Id: <20210608192537.103584-1-ckuehl@redhat.com>
> [PMD: Cover more files]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  MAINTAINERS | 7 +++++++
>  1 file changed, 7 insertions(+)

Thanks for stepping in!

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

[PATCH 02/11] qapi/misc-target: Group SEV QAPI definitions

[Philippe Mathieu-Daudé]

There is already a section with various SEV commands / types,
so move the SEV guest attestation together.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 qapi/misc-target.json | 75 +++++++++++++++++++++----------------------
 1 file changed, 37 insertions(+), 38 deletions(-)

diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 5573dcf8f08..1b81f7017d4 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -219,6 +219,43 @@
   'data': { 'packet-header': 'str', 'secret': 'str', '*gpa': 'uint64' },
   'if': 'defined(TARGET_I386)' }
 
+##
+# @SevAttestationReport:
+#
+# The struct describes attestation report for a Secure Encrypted Virtualization
+# feature.
+#
+# @data:  guest attestation report (base64 encoded)
+#
+#
+# Since: 6.1
+##
+{ 'struct': 'SevAttestationReport',
+  'data': { 'data': 'str'},
+  'if': 'defined(TARGET_I386)' }
+
+##
+# @query-sev-attestation-report:
+#
+# This command is used to get the SEV attestation report, and is supported on AMD
+# X86 platforms only.
+#
+# @mnonce: a random 16 bytes value encoded in base64 (it will be included in report)
+#
+# Returns: SevAttestationReport objects.
+#
+# Since: 6.1
+#
+# Example:
+#
+# -> { "execute" : "query-sev-attestation-report", "arguments": { "mnonce": "aaaaaaa" } }
+# <- { "return" : { "data": "aaaaaaaabbbddddd"} }
+#
+##
+{ 'command': 'query-sev-attestation-report', 'data': { 'mnonce': 'str' },
+  'returns': 'SevAttestationReport',
+  'if': 'defined(TARGET_I386)' }
+
 ##
 # @dump-skeys:
 #
@@ -285,41 +322,3 @@
 ##
 { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
   'if': 'defined(TARGET_ARM)' }
-
-
-##
-# @SevAttestationReport:
-#
-# The struct describes attestation report for a Secure Encrypted Virtualization
-# feature.
-#
-# @data:  guest attestation report (base64 encoded)
-#
-#
-# Since: 6.1
-##
-{ 'struct': 'SevAttestationReport',
-  'data': { 'data': 'str'},
-  'if': 'defined(TARGET_I386)' }
-
-##
-# @query-sev-attestation-report:
-#
-# This command is used to get the SEV attestation report, and is supported on AMD
-# X86 platforms only.
-#
-# @mnonce: a random 16 bytes value encoded in base64 (it will be included in report)
-#
-# Returns: SevAttestationReport objects.
-#
-# Since: 6.1
-#
-# Example:
-#
-# -> { "execute" : "query-sev-attestation-report", "arguments": { "mnonce": "aaaaaaa" } }
-# <- { "return" : { "data": "aaaaaaaabbbddddd"} }
-#
-##
-{ 'command': 'query-sev-attestation-report', 'data': { 'mnonce': 'str' },
-  'returns': 'SevAttestationReport',
-  'if': 'defined(TARGET_I386)' }
-- 
2.31.1

Re: [PATCH 02/11] qapi/misc-target: Group SEV QAPI definitions

[Markus Armbruster]

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

> There is already a section with various SEV commands / types,
> so move the SEV guest attestation together.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  qapi/misc-target.json | 75 +++++++++++++++++++++----------------------
>  1 file changed, 37 insertions(+), 38 deletions(-)
>
> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> index 5573dcf8f08..1b81f7017d4 100644
> --- a/qapi/misc-target.json
> +++ b/qapi/misc-target.json
> @@ -219,6 +219,43 @@
>    'data': { 'packet-header': 'str', 'secret': 'str', '*gpa': 'uint64' },
>    'if': 'defined(TARGET_I386)' }
>  
> +##
> +# @SevAttestationReport:
> +#
> +# The struct describes attestation report for a Secure Encrypted Virtualization
> +# feature.
> +#
> +# @data:  guest attestation report (base64 encoded)
> +#
> +#
> +# Since: 6.1
> +##
> +{ 'struct': 'SevAttestationReport',
> +  'data': { 'data': 'str'},
> +  'if': 'defined(TARGET_I386)' }
> +
> +##
> +# @query-sev-attestation-report:
> +#
> +# This command is used to get the SEV attestation report, and is supported on AMD
> +# X86 platforms only.
> +#
> +# @mnonce: a random 16 bytes value encoded in base64 (it will be included in report)
> +#
> +# Returns: SevAttestationReport objects.
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute" : "query-sev-attestation-report", "arguments": { "mnonce": "aaaaaaa" } }
> +# <- { "return" : { "data": "aaaaaaaabbbddddd"} }
> +#
> +##
> +{ 'command': 'query-sev-attestation-report', 'data': { 'mnonce': 'str' },
> +  'returns': 'SevAttestationReport',
> +  'if': 'defined(TARGET_I386)' }
> +
>  ##
>  # @dump-skeys:
>  #

Just code motion, so

Acked-by: Markus Armbruster <armbru@redhat.com>

Opportunity to wrap the long doc comment lines.  Should be kept under 70
or so.

[...]

Re: [PATCH 02/11] qapi/misc-target: Group SEV QAPI definitions

[Philippe Mathieu-Daudé]

On 6/10/21 11:39 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> There is already a section with various SEV commands / types,
>> so move the SEV guest attestation together.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  qapi/misc-target.json | 75 +++++++++++++++++++++----------------------
>>  1 file changed, 37 insertions(+), 38 deletions(-)
>>
>> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
>> index 5573dcf8f08..1b81f7017d4 100644
>> --- a/qapi/misc-target.json
>> +++ b/qapi/misc-target.json
>> @@ -219,6 +219,43 @@
>>    'data': { 'packet-header': 'str', 'secret': 'str', '*gpa': 'uint64' },
>>    'if': 'defined(TARGET_I386)' }
>>  
>> +##
>> +# @SevAttestationReport:
>> +#
>> +# The struct describes attestation report for a Secure Encrypted Virtualization
>> +# feature.
>> +#
>> +# @data:  guest attestation report (base64 encoded)
>> +#
>> +#
>> +# Since: 6.1
>> +##
>> +{ 'struct': 'SevAttestationReport',
>> +  'data': { 'data': 'str'},
>> +  'if': 'defined(TARGET_I386)' }
>> +
>> +##
>> +# @query-sev-attestation-report:
>> +#
>> +# This command is used to get the SEV attestation report, and is supported on AMD
>> +# X86 platforms only.
>> +#
>> +# @mnonce: a random 16 bytes value encoded in base64 (it will be included in report)
>> +#
>> +# Returns: SevAttestationReport objects.
>> +#
>> +# Since: 6.1
>> +#
>> +# Example:
>> +#
>> +# -> { "execute" : "query-sev-attestation-report", "arguments": { "mnonce": "aaaaaaa" } }
>> +# <- { "return" : { "data": "aaaaaaaabbbddddd"} }
>> +#
>> +##
>> +{ 'command': 'query-sev-attestation-report', 'data': { 'mnonce': 'str' },
>> +  'returns': 'SevAttestationReport',
>> +  'if': 'defined(TARGET_I386)' }
>> +
>>  ##
>>  # @dump-skeys:
>>  #
> 
> Just code motion, so
> 
> Acked-by: Markus Armbruster <armbru@redhat.com>
> 
> Opportunity to wrap the long doc comment lines.  Should be kept under 70
> or so.

Hmm is that a QAPI specific requirement? It is not enforced by
checkpatch, and still in the 80-90 grey area:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg763806.html

(I can do if respin required, but I'd rather have this catch earlier,
not at code movement).

Thanks for the A-b!

Re: [PATCH 02/11] qapi/misc-target: Group SEV QAPI definitions

[Markus Armbruster]

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

> On 6/10/21 11:39 AM, Markus Armbruster wrote:
>> Just code motion, so
>> 
>> Acked-by: Markus Armbruster <armbru@redhat.com>
>> 
>> Opportunity to wrap the long doc comment lines.  Should be kept under 70
>> or so.
>
> Hmm is that a QAPI specific requirement? It is not enforced by
> checkpatch, and still in the 80-90 grey area:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg763806.html

Consider why we limit line length at all: it's for *legibility*.  Humans
tend to have trouble following long lines with our eyes (I sure do).
Typographic manuals suggest to limit columns to roughly 60 characters
for exactly that reason[1].  Four levels of indentation plus 60
characters of actual text yields 76.

We add a grey area to provide for the occasional case where deeper
indentation pushes code of reasonable width beyond the "white" area, and
breaking these lines would be less legible than making use of the grey
area.

The lines I referred to are long for no good reason.  Wrapping them will
improve legibility.

> (I can do if respin required, but I'd rather have this catch earlier,
> not at code movement).

Before, during, after, or even not at all *clank*[2], your choice.

> Thanks for the A-b!


[1] https://en.wikipedia.org/wiki/Column_(typography)#Typographic_style

[2] Sad sound of a can being kicked down the road

[PATCH 03/11] target/i386/monitor: Return QMP error when SEV is disabled in build

[Philippe Mathieu-Daudé]

If the management layer tries to inject a secret, it gets an empty
response in case the binary built without SEV:

  { "execute": "sev-inject-launch-secret",
    "arguments": { "packet-header": "mypkt", "secret": "mypass", "gpa": 4294959104 }
  }
  {
      "return": {
      }
  }

Make it clearer by returning an error, mentioning the feature is
disabled:

  { "execute": "sev-inject-launch-secret",
    "arguments": { "packet-header": "mypkt", "secret": "mypass", "gpa": 4294959104 }
  }
  {
      "error": {
          "class": "GenericError",
          "desc": "this feature or command is not currently supported"
      }
  }

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/i386/monitor.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 119211f0b06..c83cca80dc2 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -28,6 +28,7 @@
 #include "monitor/hmp-target.h"
 #include "monitor/hmp.h"
 #include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qerror.h"
 #include "sysemu/kvm.h"
 #include "sysemu/sev.h"
 #include "qapi/error.h"
@@ -742,6 +743,10 @@ void qmp_sev_inject_launch_secret(const char *packet_hdr,
                                   bool has_gpa, uint64_t gpa,
                                   Error **errp)
 {
+    if (!sev_enabled()) {
+        error_setg(errp, QERR_UNSUPPORTED);
+        return;
+    }
     if (!has_gpa) {
         uint8_t *data;
         struct sev_secret_area *area;
-- 
2.31.1

Re: [PATCH 03/11] target/i386/monitor: Return QMP error when SEV is disabled in build

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> If the management layer tries to inject a secret, it gets an empty
> response in case the binary built without SEV:
> 
>   { "execute": "sev-inject-launch-secret",
>     "arguments": { "packet-header": "mypkt", "secret": "mypass", "gpa": 4294959104 }
>   }
>   {
>       "return": {
>       }
>   }
> 
> Make it clearer by returning an error, mentioning the feature is
> disabled:
> 
>   { "execute": "sev-inject-launch-secret",
>     "arguments": { "packet-header": "mypkt", "secret": "mypass", "gpa": 4294959104 }
>   }
>   {
>       "error": {
>           "class": "GenericError",
>           "desc": "this feature or command is not currently supported"
>       }
>   }
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  target/i386/monitor.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index 119211f0b06..c83cca80dc2 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -28,6 +28,7 @@
>  #include "monitor/hmp-target.h"
>  #include "monitor/hmp.h"
>  #include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qerror.h"
>  #include "sysemu/kvm.h"
>  #include "sysemu/sev.h"
>  #include "qapi/error.h"
> @@ -742,6 +743,10 @@ void qmp_sev_inject_launch_secret(const char *packet_hdr,
>                                    bool has_gpa, uint64_t gpa,
>                                    Error **errp)
>  {
> +    if (!sev_enabled()) {
> +        error_setg(errp, QERR_UNSUPPORTED);
> +        return;
> +    }
>      if (!has_gpa) {
>          uint8_t *data;
>          struct sev_secret_area *area;
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 03/11] target/i386/monitor: Return QMP error when SEV is disabled in build

[Connor Kuehl]

On 6/10/21 1:45 AM, Philippe Mathieu-Daudé wrote:
> If the management layer tries to inject a secret, it gets an empty
> response in case the binary built without SEV:
> 
>   { "execute": "sev-inject-launch-secret",
>     "arguments": { "packet-header": "mypkt", "secret": "mypass", "gpa": 4294959104 }
>   }
>   {
>       "return": {
>       }
>   }
> 
> Make it clearer by returning an error, mentioning the feature is
> disabled:
> 
>   { "execute": "sev-inject-launch-secret",
>     "arguments": { "packet-header": "mypkt", "secret": "mypass", "gpa": 4294959104 }
>   }
>   {
>       "error": {
>           "class": "GenericError",
>           "desc": "this feature or command is not currently supported"
>       }
>   }
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Connor Kuehl <ckuehl@redhat.com>

[PATCH 04/11] target/i386/cpu: Add missing 'qapi/error.h' header

[Philippe Mathieu-Daudé]

Commit 00b81053244 ("target-i386: Remove assert_no_error usage")
forgot to add the "qapi/error.h", add it now.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/i386/cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index a9fe1662d39..694031e4aec 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -27,6 +27,7 @@
 #include "sysemu/hvf.h"
 #include "kvm/kvm_i386.h"
 #include "sev_i386.h"
+#include "qapi/error.h"
 #include "qapi/qapi-visit-machine.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qapi-commands-machine-target.h"
-- 
2.31.1

Re: [PATCH 04/11] target/i386/cpu: Add missing 'qapi/error.h' header

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> Commit 00b81053244 ("target-i386: Remove assert_no_error usage")
> forgot to add the "qapi/error.h", add it now.

for the error_abort I guess

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

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  target/i386/cpu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index a9fe1662d39..694031e4aec 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -27,6 +27,7 @@
>  #include "sysemu/hvf.h"
>  #include "kvm/kvm_i386.h"
>  #include "sev_i386.h"
> +#include "qapi/error.h"
>  #include "qapi/qapi-visit-machine.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qapi/qapi-commands-machine-target.h"
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 04/11] target/i386/cpu: Add missing 'qapi/error.h' header

[Connor Kuehl]

On 6/10/21 1:45 AM, Philippe Mathieu-Daudé wrote:
> Commit 00b81053244 ("target-i386: Remove assert_no_error usage")
> forgot to add the "qapi/error.h", add it now.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>


Reviewed-by: Connor Kuehl <ckuehl@redhat.com>

[PATCH 05/11] target/i386/sev_i386.h: Remove unused headers

[Philippe Mathieu-Daudé]

Declarations don't require these headers, remove them.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/i386/sev_i386.h | 4 ----
 target/i386/sev-stub.c | 1 +
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index ae6d8404787..f4223f1febf 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -14,11 +14,7 @@
 #ifndef QEMU_SEV_I386_H
 #define QEMU_SEV_I386_H
 
-#include "qom/object.h"
-#include "qapi/error.h"
-#include "sysemu/kvm.h"
 #include "sysemu/sev.h"
-#include "qemu/error-report.h"
 #include "qapi/qapi-types-misc-target.h"
 
 #define SEV_POLICY_NODBG        0x1
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index 0227cb51778..d91c2ece784 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -12,6 +12,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "sev_i386.h"
 
 SevInfo *sev_get_info(void)
-- 
2.31.1

Re: [PATCH 05/11] target/i386/sev_i386.h: Remove unused headers

[Connor Kuehl]

On 6/10/21 1:45 AM, Philippe Mathieu-Daudé wrote:
> Declarations don't require these headers, remove them.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Connor Kuehl <ckuehl@redhat.com>

[PATCH 06/11] target/i386/sev: Remove sev_get_me_mask()

[Philippe Mathieu-Daudé]

Unused dead code makes review harder, so remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/i386/sev_i386.h | 1 -
 target/i386/sev-stub.c | 5 -----
 target/i386/sev.c      | 9 ---------
 3 files changed, 15 deletions(-)

diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index f4223f1febf..afa19a0a161 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -25,7 +25,6 @@
 #define SEV_POLICY_SEV          0x20
 
 extern bool sev_es_enabled(void);
-extern uint64_t sev_get_me_mask(void);
 extern SevInfo *sev_get_info(void);
 extern uint32_t sev_get_cbit_position(void);
 extern uint32_t sev_get_reduced_phys_bits(void);
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index d91c2ece784..eb0c89bf2be 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -25,11 +25,6 @@ bool sev_enabled(void)
     return false;
 }
 
-uint64_t sev_get_me_mask(void)
-{
-    return ~0;
-}
-
 uint32_t sev_get_cbit_position(void)
 {
     return 0;
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 83df8c09f6a..0a36e81f66c 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -64,7 +64,6 @@ struct SevGuestState {
     uint8_t api_major;
     uint8_t api_minor;
     uint8_t build_id;
-    uint64_t me_mask;
     int sev_fd;
     SevState state;
     gchar *measurement;
@@ -362,12 +361,6 @@ sev_es_enabled(void)
     return sev_enabled() && (sev_guest->policy & SEV_POLICY_ES);
 }
 
-uint64_t
-sev_get_me_mask(void)
-{
-    return sev_guest ? sev_guest->me_mask : ~0;
-}
-
 uint32_t
 sev_get_cbit_position(void)
 {
@@ -810,8 +803,6 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
         goto err;
     }
 
-    sev->me_mask = ~(1UL << sev->cbitpos);
-
     devname = object_property_get_str(OBJECT(sev), "sev-device", NULL);
     sev->sev_fd = open(devname, O_RDWR);
     if (sev->sev_fd < 0) {
-- 
2.31.1

Re: [PATCH 06/11] target/i386/sev: Remove sev_get_me_mask()

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> Unused dead code makes review harder, so remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Yes, it doesn't seem to have been used since it was added.


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  target/i386/sev_i386.h | 1 -
>  target/i386/sev-stub.c | 5 -----
>  target/i386/sev.c      | 9 ---------
>  3 files changed, 15 deletions(-)
> 
> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
> index f4223f1febf..afa19a0a161 100644
> --- a/target/i386/sev_i386.h
> +++ b/target/i386/sev_i386.h
> @@ -25,7 +25,6 @@
>  #define SEV_POLICY_SEV          0x20
>  
>  extern bool sev_es_enabled(void);
> -extern uint64_t sev_get_me_mask(void);
>  extern SevInfo *sev_get_info(void);
>  extern uint32_t sev_get_cbit_position(void);
>  extern uint32_t sev_get_reduced_phys_bits(void);
> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
> index d91c2ece784..eb0c89bf2be 100644
> --- a/target/i386/sev-stub.c
> +++ b/target/i386/sev-stub.c
> @@ -25,11 +25,6 @@ bool sev_enabled(void)
>      return false;
>  }
>  
> -uint64_t sev_get_me_mask(void)
> -{
> -    return ~0;
> -}
> -
>  uint32_t sev_get_cbit_position(void)
>  {
>      return 0;
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 83df8c09f6a..0a36e81f66c 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -64,7 +64,6 @@ struct SevGuestState {
>      uint8_t api_major;
>      uint8_t api_minor;
>      uint8_t build_id;
> -    uint64_t me_mask;
>      int sev_fd;
>      SevState state;
>      gchar *measurement;
> @@ -362,12 +361,6 @@ sev_es_enabled(void)
>      return sev_enabled() && (sev_guest->policy & SEV_POLICY_ES);
>  }
>  
> -uint64_t
> -sev_get_me_mask(void)
> -{
> -    return sev_guest ? sev_guest->me_mask : ~0;
> -}
> -
>  uint32_t
>  sev_get_cbit_position(void)
>  {
> @@ -810,8 +803,6 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>          goto err;
>      }
>  
> -    sev->me_mask = ~(1UL << sev->cbitpos);
> -
>      devname = object_property_get_str(OBJECT(sev), "sev-device", NULL);
>      sev->sev_fd = open(devname, O_RDWR);
>      if (sev->sev_fd < 0) {
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 06/11] target/i386/sev: Remove sev_get_me_mask()

[Connor Kuehl]

On 6/10/21 1:45 AM, Philippe Mathieu-Daudé wrote:
> -uint64_t
> -sev_get_me_mask(void)
> -{
> -    return sev_guest ? sev_guest->me_mask : ~0;
> -}
> -
>  uint32_t
>  sev_get_cbit_position(void)
>  {
> @@ -810,8 +803,6 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>          goto err;
>      }
>  
> -    sev->me_mask = ~(1UL << sev->cbitpos);
> -
>      devname = object_property_get_str(OBJECT(sev), "sev-device", NULL);
>      sev->sev_fd = open(devname, O_RDWR);
>      if (sev->sev_fd < 0) {
> 

Brijesh, do you remember if this was added with the intent that it would
be useful in a future series?

Otherwise:

Reviewed-by: Connor Kuehl <ckuehl@redhat.com>

[PATCH 07/11] target/i386/sev: Mark unreachable code with g_assert_not_reached()

[Philippe Mathieu-Daudé]

The unique sev_encrypt_flash() invocation (in pc_system_flash_map)
is protected by the "if (sev_enabled())" check, so is not
reacheable.
Replace the abort() call in sev_es_save_reset_vector() by
g_assert_not_reached() which meaning is clearer.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/i386/sev-stub.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index eb0c89bf2be..4668365fd3e 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -54,7 +54,7 @@ int sev_inject_launch_secret(const char *hdr, const char *secret,
 
 int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp)
 {
-    return 0;
+    g_assert_not_reached();
 }
 
 bool sev_es_enabled(void)
@@ -68,7 +68,7 @@ void sev_es_set_reset_vector(CPUState *cpu)
 
 int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
 {
-    abort();
+    g_assert_not_reached();
 }
 
 SevAttestationReport *
-- 
2.31.1

Re: [PATCH 07/11] target/i386/sev: Mark unreachable code with g_assert_not_reached()

[Connor Kuehl]

On 6/10/21 1:45 AM, Philippe Mathieu-Daudé wrote:
> The unique sev_encrypt_flash() invocation (in pc_system_flash_map)
> is protected by the "if (sev_enabled())" check, so is not
> reacheable.
> Replace the abort() call in sev_es_save_reset_vector() by
> g_assert_not_reached() which meaning is clearer.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Connor Kuehl <ckuehl@redhat.com>

[PATCH 08/11] target/i386/sev: sev_get_attestation_report use g_autofree

[Philippe Mathieu-Daudé]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Removes a whole bunch of g_free's and a goto.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>
Message-Id: <20210603113017.34922-1-dgilbert@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/i386/sev.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 0a36e81f66c..791804954e9 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -493,8 +493,8 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
     struct kvm_sev_attestation_report input = {};
     SevAttestationReport *report = NULL;
     SevGuestState *sev = sev_guest;
-    guchar *data;
-    guchar *buf;
+    g_autofree guchar *data = NULL;
+    g_autofree guchar *buf = NULL;
     gsize len;
     int err = 0, ret;
 
@@ -514,7 +514,6 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
     if (len != sizeof(input.mnonce)) {
         error_setg(errp, "SEV: mnonce must be %zu bytes (got %" G_GSIZE_FORMAT ")",
                 sizeof(input.mnonce), len);
-        g_free(buf);
         return NULL;
     }
 
@@ -525,7 +524,6 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
         if (err != SEV_RET_INVALID_LEN) {
             error_setg(errp, "failed to query the attestation report length "
                     "ret=%d fw_err=%d (%s)", ret, err, fw_error_to_str(err));
-            g_free(buf);
             return NULL;
         }
     }
@@ -540,7 +538,7 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
     if (ret) {
         error_setg_errno(errp, errno, "Failed to get attestation report"
                 " ret=%d fw_err=%d (%s)", ret, err, fw_error_to_str(err));
-        goto e_free_data;
+        return NULL;
     }
 
     report = g_new0(SevAttestationReport, 1);
@@ -548,9 +546,6 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
 
     trace_kvm_sev_attestation_report(mnonce, report->data);
 
-e_free_data:
-    g_free(data);
-    g_free(buf);
     return report;
 }
 
-- 
2.31.1

[PATCH 09/11] target/i386/sev: Restrict SEV to system emulation

[Philippe Mathieu-Daudé]

SEV is irrelevant on user emulation, so restrict it to sysemu.
Some stubs are still required because used in cpu.c by
x86_register_cpudef_types(), so move the sysemu specific stubs
to sev-sysemu-stub.c instead. This will allow us to simplify
monitor.c (which is not available in user emulation) in the
next commit.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/i386/sev-stub.c        | 43 -------------------------
 target/i386/sev-sysemu-stub.c | 60 +++++++++++++++++++++++++++++++++++
 target/i386/meson.build       |  4 ++-
 3 files changed, 63 insertions(+), 44 deletions(-)
 create mode 100644 target/i386/sev-sysemu-stub.c

diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index 4668365fd3e..8eae5d2fa8d 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -15,11 +15,6 @@
 #include "qapi/error.h"
 #include "sev_i386.h"
 
-SevInfo *sev_get_info(void)
-{
-    return NULL;
-}
-
 bool sev_enabled(void)
 {
     return false;
@@ -35,45 +30,7 @@ uint32_t sev_get_reduced_phys_bits(void)
     return 0;
 }
 
-char *sev_get_launch_measurement(void)
-{
-    return NULL;
-}
-
-SevCapability *sev_get_capabilities(Error **errp)
-{
-    error_setg(errp, "SEV is not available in this QEMU");
-    return NULL;
-}
-
-int sev_inject_launch_secret(const char *hdr, const char *secret,
-                             uint64_t gpa, Error **errp)
-{
-    return 1;
-}
-
-int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp)
-{
-    g_assert_not_reached();
-}
-
 bool sev_es_enabled(void)
 {
     return false;
 }
-
-void sev_es_set_reset_vector(CPUState *cpu)
-{
-}
-
-int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
-{
-    g_assert_not_reached();
-}
-
-SevAttestationReport *
-sev_get_attestation_report(const char *mnonce, Error **errp)
-{
-    error_setg(errp, "SEV is not available in this QEMU");
-    return NULL;
-}
diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
new file mode 100644
index 00000000000..d556b4f091f
--- /dev/null
+++ b/target/i386/sev-sysemu-stub.c
@@ -0,0 +1,60 @@
+/*
+ * QEMU SEV system stub
+ *
+ * Copyright Advanced Micro Devices 2018
+ *
+ * Authors:
+ *      Brijesh Singh <brijesh.singh@amd.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/qapi-commands-misc-target.h"
+#include "qapi/error.h"
+#include "sev_i386.h"
+
+SevInfo *sev_get_info(void)
+{
+    return NULL;
+}
+
+char *sev_get_launch_measurement(void)
+{
+    return NULL;
+}
+
+SevCapability *sev_get_capabilities(Error **errp)
+{
+    error_setg(errp, "SEV is not available in this QEMU");
+    return NULL;
+}
+
+int sev_inject_launch_secret(const char *hdr, const char *secret,
+                             uint64_t gpa, Error **errp)
+{
+    return 1;
+}
+
+int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp)
+{
+    g_assert_not_reached();
+}
+
+void sev_es_set_reset_vector(CPUState *cpu)
+{
+}
+
+int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
+{
+    g_assert_not_reached();
+}
+
+SevAttestationReport *sev_get_attestation_report(const char *mnonce,
+                                                 Error **errp)
+{
+    error_setg(errp, "SEV is not available in this QEMU");
+    return NULL;
+}
diff --git a/target/i386/meson.build b/target/i386/meson.build
index dac19ec00d4..a4f45c3ec1d 100644
--- a/target/i386/meson.build
+++ b/target/i386/meson.build
@@ -6,7 +6,7 @@
   'xsave_helper.c',
   'cpu-dump.c',
 ))
-i386_ss.add(when: 'CONFIG_SEV', if_true: files('host-cpu.c', 'sev.c'), if_false: files('sev-stub.c'))
+i386_ss.add(when: 'CONFIG_SEV', if_true: files('host-cpu.c'), if_false: files('sev-stub.c'))
 
 # x86 cpu type
 i386_ss.add(when: 'CONFIG_KVM', if_true: files('host-cpu.c'))
@@ -20,6 +20,8 @@
   'monitor.c',
   'cpu-sysemu.c',
 ))
+i386_softmmu_ss.add(when: 'CONFIG_SEV', if_true: files('sev.c'), if_false: files('sev-sysemu-stub.c'))
+
 i386_user_ss = ss.source_set()
 
 subdir('kvm')
-- 
2.31.1

[PATCH 10/11] target/i386/monitor: Move SEV specific commands to sev.c

[Philippe Mathieu-Daudé]

Having the HMP/QMP commands defined in monitor.c makes the stubs
rather complicated when SEV is not built in. To simplify, move the
SEV functions to sev.c, and remove a layer of stubs.

Also make it clearer when SEV is not built in, so developers don't
try to enable it when it is not enablable:

 - before:

  (qemu) info sev
  SEV is not enabled

- after:

  (qemu) info sev
  SEV is not available in this QEMU

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/i386/monitor.c         | 96 ----------------------------------
 target/i386/sev-sysemu-stub.c | 29 +++++++----
 target/i386/sev.c             | 97 +++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+), 106 deletions(-)

diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index c83cca80dc2..af3501095e5 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -28,11 +28,8 @@
 #include "monitor/hmp-target.h"
 #include "monitor/hmp.h"
 #include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qerror.h"
 #include "sysemu/kvm.h"
-#include "sysemu/sev.h"
 #include "qapi/error.h"
-#include "sev_i386.h"
 #include "qapi/qapi-commands-misc-target.h"
 #include "qapi/qapi-commands-misc.h"
 #include "hw/i386/pc.h"
@@ -675,96 +672,3 @@ void hmp_info_io_apic(Monitor *mon, const QDict *qdict)
     monitor_printf(mon, "This command is obsolete and will be "
                    "removed soon. Please use 'info pic' instead.\n");
 }
-
-SevInfo *qmp_query_sev(Error **errp)
-{
-    SevInfo *info;
-
-    info = sev_get_info();
-    if (!info) {
-        error_setg(errp, "SEV feature is not available");
-        return NULL;
-    }
-
-    return info;
-}
-
-void hmp_info_sev(Monitor *mon, const QDict *qdict)
-{
-    SevInfo *info = sev_get_info();
-
-    if (info && info->enabled) {
-        monitor_printf(mon, "handle: %d\n", info->handle);
-        monitor_printf(mon, "state: %s\n", SevState_str(info->state));
-        monitor_printf(mon, "build: %d\n", info->build_id);
-        monitor_printf(mon, "api version: %d.%d\n",
-                       info->api_major, info->api_minor);
-        monitor_printf(mon, "debug: %s\n",
-                       info->policy & SEV_POLICY_NODBG ? "off" : "on");
-        monitor_printf(mon, "key-sharing: %s\n",
-                       info->policy & SEV_POLICY_NOKS ? "off" : "on");
-    } else {
-        monitor_printf(mon, "SEV is not enabled\n");
-    }
-
-    qapi_free_SevInfo(info);
-}
-
-SevLaunchMeasureInfo *qmp_query_sev_launch_measure(Error **errp)
-{
-    char *data;
-    SevLaunchMeasureInfo *info;
-
-    data = sev_get_launch_measurement();
-    if (!data) {
-        error_setg(errp, "Measurement is not available");
-        return NULL;
-    }
-
-    info = g_malloc0(sizeof(*info));
-    info->data = data;
-
-    return info;
-}
-
-SevCapability *qmp_query_sev_capabilities(Error **errp)
-{
-    return sev_get_capabilities(errp);
-}
-
-#define SEV_SECRET_GUID "4c2eb361-7d9b-4cc3-8081-127c90d3d294"
-struct sev_secret_area {
-    uint32_t base;
-    uint32_t size;
-};
-
-void qmp_sev_inject_launch_secret(const char *packet_hdr,
-                                  const char *secret,
-                                  bool has_gpa, uint64_t gpa,
-                                  Error **errp)
-{
-    if (!sev_enabled()) {
-        error_setg(errp, QERR_UNSUPPORTED);
-        return;
-    }
-    if (!has_gpa) {
-        uint8_t *data;
-        struct sev_secret_area *area;
-
-        if (!pc_system_ovmf_table_find(SEV_SECRET_GUID, &data, NULL)) {
-            error_setg(errp, "SEV: no secret area found in OVMF,"
-                       " gpa must be specified.");
-            return;
-        }
-        area = (struct sev_secret_area *)data;
-        gpa = area->base;
-    }
-
-    sev_inject_launch_secret(packet_hdr, secret, gpa, errp);
-}
-
-SevAttestationReport *
-qmp_query_sev_attestation_report(const char *mnonce, Error **errp)
-{
-    return sev_get_attestation_report(mnonce, errp);
-}
diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
index d556b4f091f..7a35f0432b2 100644
--- a/target/i386/sev-sysemu-stub.c
+++ b/target/i386/sev-sysemu-stub.c
@@ -12,30 +12,35 @@
  */
 
 #include "qemu/osdep.h"
+#include "monitor/monitor.h"
+#include "monitor/hmp.h"
 #include "qapi/qapi-commands-misc-target.h"
+#include "qapi/qmp/qerror.h"
 #include "qapi/error.h"
 #include "sev_i386.h"
 
-SevInfo *sev_get_info(void)
+SevInfo *qmp_query_sev(Error **errp)
 {
+    error_setg(errp, QERR_UNSUPPORTED);
     return NULL;
 }
 
-char *sev_get_launch_measurement(void)
+SevLaunchMeasureInfo *qmp_query_sev_launch_measure(Error **errp)
 {
+    error_setg(errp, QERR_UNSUPPORTED);
     return NULL;
 }
 
-SevCapability *sev_get_capabilities(Error **errp)
+SevCapability *qmp_query_sev_capabilities(Error **errp)
 {
-    error_setg(errp, "SEV is not available in this QEMU");
+    error_setg(errp, QERR_UNSUPPORTED);
     return NULL;
 }
 
-int sev_inject_launch_secret(const char *hdr, const char *secret,
-                             uint64_t gpa, Error **errp)
+void qmp_sev_inject_launch_secret(const char *packet_header, const char *secret,
+                                  bool has_gpa, uint64_t gpa, Error **errp)
 {
-    return 1;
+    error_setg(errp, QERR_UNSUPPORTED);
 }
 
 int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp)
@@ -52,9 +57,13 @@ int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
     g_assert_not_reached();
 }
 
-SevAttestationReport *sev_get_attestation_report(const char *mnonce,
-                                                 Error **errp)
+SevAttestationReport *qmp_query_sev_attestation_report(const char *mnonce, Error **errp)
 {
-    error_setg(errp, "SEV is not available in this QEMU");
+    error_setg(errp, QERR_UNSUPPORTED);
     return NULL;
 }
+
+void hmp_info_sev(Monitor *mon, const QDict *qdict)
+{
+    monitor_printf(mon, "SEV is not available in this QEMU\n");
+}
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 791804954e9..b4d7c41d3fb 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -27,10 +27,14 @@
 #include "sev_i386.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/runstate.h"
+#include "sysemu/sev.h"
 #include "trace.h"
 #include "migration/blocker.h"
 #include "qom/object.h"
 #include "monitor/monitor.h"
+#include "monitor/hmp.h"
+#include "qapi/qapi-commands-misc-target.h"
+#include "qapi/qmp/qerror.h"
 #include "exec/confidential-guest-support.h"
 #include "hw/i386/pc.h"
 
@@ -1070,3 +1074,96 @@ sev_register_types(void)
 }
 
 type_init(sev_register_types);
+
+SevInfo *qmp_query_sev(Error **errp)
+{
+    SevInfo *info;
+
+    info = sev_get_info();
+    if (!info) {
+        error_setg(errp, "SEV feature is not available");
+        return NULL;
+    }
+
+    return info;
+}
+
+void hmp_info_sev(Monitor *mon, const QDict *qdict)
+{
+    SevInfo *info = sev_get_info();
+
+    if (info && info->enabled) {
+        monitor_printf(mon, "handle: %d\n", info->handle);
+        monitor_printf(mon, "state: %s\n", SevState_str(info->state));
+        monitor_printf(mon, "build: %d\n", info->build_id);
+        monitor_printf(mon, "api version: %d.%d\n",
+                       info->api_major, info->api_minor);
+        monitor_printf(mon, "debug: %s\n",
+                       info->policy & SEV_POLICY_NODBG ? "off" : "on");
+        monitor_printf(mon, "key-sharing: %s\n",
+                       info->policy & SEV_POLICY_NOKS ? "off" : "on");
+    } else {
+        monitor_printf(mon, "SEV is not enabled\n");
+    }
+
+    qapi_free_SevInfo(info);
+}
+
+SevLaunchMeasureInfo *qmp_query_sev_launch_measure(Error **errp)
+{
+    char *data;
+    SevLaunchMeasureInfo *info;
+
+    data = sev_get_launch_measurement();
+    if (!data) {
+        error_setg(errp, "Measurement is not available");
+        return NULL;
+    }
+
+    info = g_malloc0(sizeof(*info));
+    info->data = data;
+
+    return info;
+}
+
+SevCapability *qmp_query_sev_capabilities(Error **errp)
+{
+    return sev_get_capabilities(errp);
+}
+
+#define SEV_SECRET_GUID "4c2eb361-7d9b-4cc3-8081-127c90d3d294"
+struct sev_secret_area {
+    uint32_t base;
+    uint32_t size;
+};
+
+void qmp_sev_inject_launch_secret(const char *packet_hdr,
+                                  const char *secret,
+                                  bool has_gpa, uint64_t gpa,
+                                  Error **errp)
+{
+    if (!sev_enabled()) {
+        error_setg(errp, QERR_UNSUPPORTED);
+        return;
+    }
+    if (!has_gpa) {
+        uint8_t *data;
+        struct sev_secret_area *area;
+
+        if (!pc_system_ovmf_table_find(SEV_SECRET_GUID, &data, NULL)) {
+            error_setg(errp, "SEV: no secret area found in OVMF,"
+                       " gpa must be specified.");
+            return;
+        }
+        area = (struct sev_secret_area *)data;
+        gpa = area->base;
+    }
+
+    sev_inject_launch_secret(packet_hdr, secret, gpa, errp);
+}
+
+SevAttestationReport *qmp_query_sev_attestation_report(const char *mnonce,
+                                                       Error **errp)
+{
+    return sev_get_attestation_report(mnonce, errp);
+}
-- 
2.31.1

Re: [PATCH 10/11] target/i386/monitor: Move SEV specific commands to sev.c

[Connor Kuehl]

On 6/10/21 1:45 AM, Philippe Mathieu-Daudé wrote:
> Having the HMP/QMP commands defined in monitor.c makes the stubs
> rather complicated when SEV is not built in. To simplify, move the
> SEV functions to sev.c, and remove a layer of stubs.
> 
> Also make it clearer when SEV is not built in, so developers don't
> try to enable it when it is not enablable:
> 
>  - before:
> 
>   (qemu) info sev
>   SEV is not enabled
> 
> - after:
> 
>   (qemu) info sev
>   SEV is not available in this QEMU
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  target/i386/monitor.c         | 96 ----------------------------------
>  target/i386/sev-sysemu-stub.c | 29 +++++++----
>  target/i386/sev.c             | 97 +++++++++++++++++++++++++++++++++++

Hi Philippe,

I agree that the split from monitor.c makes it easier to follow. Instead
of putting the QMP entry points in sev-sysemu-stub. and sev.c, what do
you think of placing them in sev-qmp-stub.c and sev-qmp.c, respectively?

I find that appealing from a code organization/module boundary
perspective.

Connor

Re: [PATCH 10/11] target/i386/monitor: Move SEV specific commands to sev.c

[Philippe Mathieu-Daudé]

On 6/10/21 5:19 PM, Connor Kuehl wrote:
> On 6/10/21 1:45 AM, Philippe Mathieu-Daudé wrote:
>> Having the HMP/QMP commands defined in monitor.c makes the stubs
>> rather complicated when SEV is not built in. To simplify, move the
>> SEV functions to sev.c, and remove a layer of stubs.
>>
>> Also make it clearer when SEV is not built in, so developers don't
>> try to enable it when it is not enablable:
>>
>>  - before:
>>
>>   (qemu) info sev
>>   SEV is not enabled
>>
>> - after:
>>
>>   (qemu) info sev
>>   SEV is not available in this QEMU
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  target/i386/monitor.c         | 96 ----------------------------------
>>  target/i386/sev-sysemu-stub.c | 29 +++++++----
>>  target/i386/sev.c             | 97 +++++++++++++++++++++++++++++++++++
> 
> Hi Philippe,
> 
> I agree that the split from monitor.c makes it easier to follow. Instead
> of putting the QMP entry points in sev-sysemu-stub. and sev.c, what do
> you think of placing them in sev-qmp-stub.c and sev-qmp.c, respectively?
> 
> I find that appealing from a code organization/module boundary
> perspective.

I tried but this makes the code more complex since sev-qmp.c calls
functions defined in sev.c, we have to keep them declared in a local
header (sev_i386.h). So for v2 I choose to keep the same organization
but exploded the patch for easier review.

[PATCH 11/11] monitor: Restrict 'info sev' to x86 targets

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/monitor/hmp-target.h  | 1 +
 include/monitor/hmp.h         | 1 -
 target/i386/sev-sysemu-stub.c | 2 +-
 target/i386/sev.c             | 2 +-
 4 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index 60fc92722ae..20adbea5154 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -49,5 +49,6 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict);
 void hmp_mce(Monitor *mon, const QDict *qdict);
 void hmp_info_local_apic(Monitor *mon, const QDict *qdict);
 void hmp_info_io_apic(Monitor *mon, const QDict *qdict);
+void hmp_info_sev(Monitor *mon, const QDict *qdict);
 
 #endif /* MONITOR_HMP_TARGET_H */
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 605d57287ae..ac03adc6f0a 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -124,7 +124,6 @@ void hmp_info_ramblock(Monitor *mon, const QDict *qdict);
 void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict);
 void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict);
 void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict);
-void hmp_info_sev(Monitor *mon, const QDict *qdict);
 void hmp_info_replay(Monitor *mon, const QDict *qdict);
 void hmp_replay_break(Monitor *mon, const QDict *qdict);
 void hmp_replay_delete_break(Monitor *mon, const QDict *qdict);
diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
index 7a35f0432b2..aba02f3c332 100644
--- a/target/i386/sev-sysemu-stub.c
+++ b/target/i386/sev-sysemu-stub.c
@@ -13,7 +13,7 @@
 
 #include "qemu/osdep.h"
 #include "monitor/monitor.h"
-#include "monitor/hmp.h"
+#include "monitor/hmp-target.h"
 #include "qapi/qapi-commands-misc-target.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/error.h"
diff --git a/target/i386/sev.c b/target/i386/sev.c
index b4d7c41d3fb..0103b28c396 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -32,7 +32,7 @@
 #include "migration/blocker.h"
 #include "qom/object.h"
 #include "monitor/monitor.h"
-#include "monitor/hmp.h"
+#include "monitor/hmp-target.h"
 #include "qapi/qapi-commands-misc-target.h"
 #include "qapi/qmp/qerror.h"
 #include "exec/confidential-guest-support.h"
-- 
2.31.1