[PATCH v3 00/22] target/i386/sev: Housekeeping SEV + measured Linux SEV guest

[PATCH v3 00/22] target/i386/sev: Housekeeping SEV + measured Linux SEV guest

[Philippe Mathieu-Daudé]

Hi,

While testing James & Dov patch:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg810571.html
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.

Since v2:
- Rebased on top of SGX
- Addressed review comments from Markus / David
- Included/rebased 'Measured Linux SEV guest' from Dov [1]
- Added orphean MAINTAINERS section

[1] https://lore.kernel.org/qemu-devel/20210825073538.959525-1-dovmurik@linux.ibm.com/

Supersedes: <20210616204328.2611406-1-philmd@redhat.com>

Dov Murik (2):
  sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux
    boot
  x86/sev: generate SEV kernel loader hashes in x86_load_linux

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

Philippe Mathieu-Daudé (19):
  qapi/misc-target: Wrap long 'SEV Attestation Report' long lines
  qapi/misc-target: Group SEV QAPI definitions
  target/i386/kvm: Introduce i386_softmmu_kvm Meson source set
  target/i386/kvm: Restrict SEV stubs to x86 architecture
  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/sev: Declare system-specific functions in 'sev_i386.h'
  target/i386/sev: Remove stubs by using code elision
  target/i386/sev: Move qmp_query_sev_attestation_report() to sev.c
  target/i386/sev: Move qmp_sev_inject_launch_secret() to sev.c
  target/i386/sev: Move qmp_query_sev_capabilities() to sev.c
  target/i386/sev: Move qmp_query_sev_launch_measure() to sev.c
  target/i386/sev: Move qmp_query_sev() & hmp_info_sev() to sev.c
  monitor: Restrict 'info sev' to x86 targets
  MAINTAINERS: Cover AMD SEV files

 qapi/misc-target.json                 |  77 ++++----
 include/monitor/hmp-target.h          |   1 +
 include/monitor/hmp.h                 |   1 -
 include/sysemu/sev.h                  |  20 +-
 target/i386/sev_i386.h                |  32 +--
 hw/i386/pc_sysfw.c                    |   2 +-
 hw/i386/x86.c                         |  25 ++-
 target/i386/cpu.c                     |  17 +-
 {accel => target/i386}/kvm/sev-stub.c |   0
 target/i386/monitor.c                 |  92 +--------
 target/i386/sev-stub.c                |  83 --------
 target/i386/sev-sysemu-stub.c         |  70 +++++++
 target/i386/sev.c                     | 268 +++++++++++++++++++++++---
 MAINTAINERS                           |   7 +
 accel/kvm/meson.build                 |   1 -
 target/i386/kvm/meson.build           |   8 +-
 target/i386/meson.build               |   4 +-
 17 files changed, 438 insertions(+), 270 deletions(-)
 rename {accel => target/i386}/kvm/sev-stub.c (100%)
 delete mode 100644 target/i386/sev-stub.c
 create mode 100644 target/i386/sev-sysemu-stub.c

-- 
2.31.1

[PATCH v3 01/22] qapi/misc-target: Wrap long 'SEV Attestation Report' long lines

[Philippe Mathieu-Daudé]

Wrap long lines before 70 characters for legibility.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 qapi/misc-target.json | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 594fbd1577f..ae5577e0390 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -300,8 +300,8 @@
 ##
 # @SevAttestationReport:
 #
-# The struct describes attestation report for a Secure Encrypted Virtualization
-# feature.
+# The struct describes attestation report for a Secure Encrypted
+# Virtualization feature.
 #
 # @data:  guest attestation report (base64 encoded)
 #
@@ -315,10 +315,11 @@
 ##
 # @query-sev-attestation-report:
 #
-# This command is used to get the SEV attestation report, and is supported on AMD
-# X86 platforms only.
+# 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)
+# @mnonce: a random 16 bytes value encoded in base64 (it will be
+#          included in report)
 #
 # Returns: SevAttestationReport objects.
 #
@@ -326,11 +327,13 @@
 #
 # Example:
 #
-# -> { "execute" : "query-sev-attestation-report", "arguments": { "mnonce": "aaaaaaa" } }
+# -> { "execute" : "query-sev-attestation-report",
+#                  "arguments": { "mnonce": "aaaaaaa" } }
 # <- { "return" : { "data": "aaaaaaaabbbddddd"} }
 #
 ##
-{ 'command': 'query-sev-attestation-report', 'data': { 'mnonce': 'str' },
+{ 'command': 'query-sev-attestation-report',
+  'data': { 'mnonce': 'str' },
   'returns': 'SevAttestationReport',
   'if': 'TARGET_I386' }
 
-- 
2.31.1

Re: [PATCH v3 01/22] qapi/misc-target: Wrap long 'SEV Attestation Report' long lines

[Paolo Bonzini]

On 02/10/21 14:52, Philippe Mathieu-Daudé wrote:
> Wrap long lines before 70 characters for legibility.
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   qapi/misc-target.json | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> index 594fbd1577f..ae5577e0390 100644
> --- a/qapi/misc-target.json
> +++ b/qapi/misc-target.json
> @@ -300,8 +300,8 @@
>   ##
>   # @SevAttestationReport:
>   #
> -# The struct describes attestation report for a Secure Encrypted Virtualization
> -# feature.
> +# The struct describes attestation report for a Secure Encrypted
> +# Virtualization feature.
>   #
>   # @data:  guest attestation report (base64 encoded)
>   #
> @@ -315,10 +315,11 @@
>   ##
>   # @query-sev-attestation-report:
>   #
> -# This command is used to get the SEV attestation report, and is supported on AMD
> -# X86 platforms only.
> +# 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)
> +# @mnonce: a random 16 bytes value encoded in base64 (it will be
> +#          included in report)
>   #
>   # Returns: SevAttestationReport objects.
>   #
> @@ -326,11 +327,13 @@
>   #
>   # Example:
>   #
> -# -> { "execute" : "query-sev-attestation-report", "arguments": { "mnonce": "aaaaaaa" } }
> +# -> { "execute" : "query-sev-attestation-report",
> +#                  "arguments": { "mnonce": "aaaaaaa" } }
>   # <- { "return" : { "data": "aaaaaaaabbbddddd"} }
>   #
>   ##
> -{ 'command': 'query-sev-attestation-report', 'data': { 'mnonce': 'str' },
> +{ 'command': 'query-sev-attestation-report',
> +  'data': { 'mnonce': 'str' },
>     'returns': 'SevAttestationReport',
>     'if': 'TARGET_I386' }
>   
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[PATCH v3 02/22] 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 | 80 +++++++++++++++++++++----------------------
 1 file changed, 40 insertions(+), 40 deletions(-)

diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index ae5577e0390..5aa2b95b7d4 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -229,6 +229,46 @@
   'data': { 'packet-header': 'str', 'secret': 'str', '*gpa': 'uint64' },
   'if': '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': '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': 'TARGET_I386' }
+
 ##
 # @dump-skeys:
 #
@@ -297,46 +337,6 @@
   'if': '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': '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': 'TARGET_I386' }
-
 ##
 # @SGXInfo:
 #
-- 
2.31.1

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

[Paolo Bonzini]

On 02/10/21 14:52, Philippe Mathieu-Daudé wrote:
> 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 | 80 +++++++++++++++++++++----------------------
>   1 file changed, 40 insertions(+), 40 deletions(-)
> 
> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> index ae5577e0390..5aa2b95b7d4 100644
> --- a/qapi/misc-target.json
> +++ b/qapi/misc-target.json
> @@ -229,6 +229,46 @@
>     'data': { 'packet-header': 'str', 'secret': 'str', '*gpa': 'uint64' },
>     'if': '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': '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': 'TARGET_I386' }
> +
>   ##
>   # @dump-skeys:
>   #
> @@ -297,46 +337,6 @@
>     'if': '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': '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': 'TARGET_I386' }
> -
>   ##
>   # @SGXInfo:
>   #
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[PATCH v3 03/22] target/i386/kvm: Introduce i386_softmmu_kvm Meson source set

[Philippe Mathieu-Daudé]

Introduce the i386_softmmu_kvm Meson source set to be able to
add features dependent on CONFIG_KVM.

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

diff --git a/target/i386/kvm/meson.build b/target/i386/kvm/meson.build
index 0a533411cab..b1c76957c76 100644
--- a/target/i386/kvm/meson.build
+++ b/target/i386/kvm/meson.build
@@ -1,8 +1,12 @@
 i386_ss.add(when: 'CONFIG_KVM', if_false: files('kvm-stub.c'))
 
-i386_softmmu_ss.add(when: 'CONFIG_KVM', if_true: files(
+i386_softmmu_kvm_ss = ss.source_set()
+
+i386_softmmu_kvm_ss.add(files(
   'kvm.c',
   'kvm-cpu.c',
 ))
 
 i386_softmmu_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'), if_false: files('hyperv-stub.c'))
+
+i386_softmmu_ss.add_all(when: 'CONFIG_KVM', if_true: i386_softmmu_kvm_ss)
-- 
2.31.1

Re: [PATCH v3 03/22] target/i386/kvm: Introduce i386_softmmu_kvm Meson source set

[Paolo Bonzini]

On 02/10/21 14:52, Philippe Mathieu-Daudé wrote:
> Introduce the i386_softmmu_kvm Meson source set to be able to
> add features dependent on CONFIG_KVM.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   target/i386/kvm/meson.build | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/kvm/meson.build b/target/i386/kvm/meson.build
> index 0a533411cab..b1c76957c76 100644
> --- a/target/i386/kvm/meson.build
> +++ b/target/i386/kvm/meson.build
> @@ -1,8 +1,12 @@
>   i386_ss.add(when: 'CONFIG_KVM', if_false: files('kvm-stub.c'))
>   
> -i386_softmmu_ss.add(when: 'CONFIG_KVM', if_true: files(
> +i386_softmmu_kvm_ss = ss.source_set()
> +
> +i386_softmmu_kvm_ss.add(files(
>     'kvm.c',
>     'kvm-cpu.c',
>   ))
>   
>   i386_softmmu_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'), if_false: files('hyperv-stub.c'))
> +
> +i386_softmmu_ss.add_all(when: 'CONFIG_KVM', if_true: i386_softmmu_kvm_ss)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[PATCH v3 04/22] target/i386/kvm: Restrict SEV stubs to x86 architecture

[Philippe Mathieu-Daudé]

SEV is x86-specific, no need to add its stub to other
architectures. Move the stub file to target/i386/kvm/.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 {accel => target/i386}/kvm/sev-stub.c | 0
 accel/kvm/meson.build                 | 1 -
 target/i386/kvm/meson.build           | 2 ++
 3 files changed, 2 insertions(+), 1 deletion(-)
 rename {accel => target/i386}/kvm/sev-stub.c (100%)

diff --git a/accel/kvm/sev-stub.c b/target/i386/kvm/sev-stub.c
similarity index 100%
rename from accel/kvm/sev-stub.c
rename to target/i386/kvm/sev-stub.c
diff --git a/accel/kvm/meson.build b/accel/kvm/meson.build
index 8d219bea507..397a1fe1fd1 100644
--- a/accel/kvm/meson.build
+++ b/accel/kvm/meson.build
@@ -3,6 +3,5 @@
   'kvm-all.c',
   'kvm-accel-ops.c',
 ))
-kvm_ss.add(when: 'CONFIG_SEV', if_false: files('sev-stub.c'))
 
 specific_ss.add_all(when: 'CONFIG_KVM', if_true: kvm_ss)
diff --git a/target/i386/kvm/meson.build b/target/i386/kvm/meson.build
index b1c76957c76..736df8b72e3 100644
--- a/target/i386/kvm/meson.build
+++ b/target/i386/kvm/meson.build
@@ -7,6 +7,8 @@
   'kvm-cpu.c',
 ))
 
+i386_softmmu_kvm_ss.add(when: 'CONFIG_SEV', if_false: files('sev-stub.c'))
+
 i386_softmmu_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'), if_false: files('hyperv-stub.c'))
 
 i386_softmmu_ss.add_all(when: 'CONFIG_KVM', if_true: i386_softmmu_kvm_ss)
-- 
2.31.1

Re: [PATCH v3 04/22] target/i386/kvm: Restrict SEV stubs to x86 architecture

[Paolo Bonzini]

On 02/10/21 14:52, Philippe Mathieu-Daudé wrote:
> SEV is x86-specific, no need to add its stub to other
> architectures. Move the stub file to target/i386/kvm/.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   {accel => target/i386}/kvm/sev-stub.c | 0
>   accel/kvm/meson.build                 | 1 -
>   target/i386/kvm/meson.build           | 2 ++
>   3 files changed, 2 insertions(+), 1 deletion(-)
>   rename {accel => target/i386}/kvm/sev-stub.c (100%)
> 
> diff --git a/accel/kvm/sev-stub.c b/target/i386/kvm/sev-stub.c
> similarity index 100%
> rename from accel/kvm/sev-stub.c
> rename to target/i386/kvm/sev-stub.c
> diff --git a/accel/kvm/meson.build b/accel/kvm/meson.build
> index 8d219bea507..397a1fe1fd1 100644
> --- a/accel/kvm/meson.build
> +++ b/accel/kvm/meson.build
> @@ -3,6 +3,5 @@
>     'kvm-all.c',
>     'kvm-accel-ops.c',
>   ))
> -kvm_ss.add(when: 'CONFIG_SEV', if_false: files('sev-stub.c'))
>   
>   specific_ss.add_all(when: 'CONFIG_KVM', if_true: kvm_ss)
> diff --git a/target/i386/kvm/meson.build b/target/i386/kvm/meson.build
> index b1c76957c76..736df8b72e3 100644
> --- a/target/i386/kvm/meson.build
> +++ b/target/i386/kvm/meson.build
> @@ -7,6 +7,8 @@
>     'kvm-cpu.c',
>   ))
>   
> +i386_softmmu_kvm_ss.add(when: 'CONFIG_SEV', if_false: files('sev-stub.c'))
> +
>   i386_softmmu_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'), if_false: files('hyperv-stub.c'))
>   
>   i386_softmmu_ss.add_all(when: 'CONFIG_KVM', if_true: i386_softmmu_kvm_ss)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[PATCH v3 05/22] 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"
      }
  }

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
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 196c1c9e77f..a9f85acd473 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"
@@ -743,6 +744,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 v3 05/22] target/i386/monitor: Return QMP error when SEV is disabled in build

[Paolo Bonzini]

On 02/10/21 14:53, 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"
>        }
>    }
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
> 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 196c1c9e77f..a9f85acd473 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"
> @@ -743,6 +744,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;
> 

This should be done in the sev_inject_launch_secret stub instead, I 
think.  Or if you do it here, you can remove the "if (!sev_guest)" 
conditional in the non-stub version.

Paolo

Re: [PATCH v3 05/22] target/i386/monitor: Return QMP error when SEV is disabled in build

[Philippe Mathieu-Daudé]

On 10/4/21 10:11, Paolo Bonzini wrote:
> On 02/10/21 14:53, 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"
>>        }
>>    }
>>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
>> 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 196c1c9e77f..a9f85acd473 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"
>> @@ -743,6 +744,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;
>>
> 
> This should be done in the sev_inject_launch_secret stub instead, I
> think.  Or if you do it here, you can remove the "if (!sev_guest)"
> conditional in the non-stub version.

This part is not related to SEV builtin; what we want to avoid here
is management layer to try to inject secret while the guest hasn't
been started with SEV (IOW 'no memory encryption requested for KVM).

Maybe this error message is more explicit?

  error_setg(errp, "Guest is not using memory encryption");

Or:

  error_setg(errp, "Guest is not using SEV");

Re: [PATCH v3 05/22] target/i386/monitor: Return QMP error when SEV is disabled in build

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> On 10/4/21 10:11, Paolo Bonzini wrote:
> > On 02/10/21 14:53, 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"
> >>        }
> >>    }
> >>
> >> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
> >> 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 196c1c9e77f..a9f85acd473 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"
> >> @@ -743,6 +744,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;
> >>
> > 
> > This should be done in the sev_inject_launch_secret stub instead, I
> > think.  Or if you do it here, you can remove the "if (!sev_guest)"
> > conditional in the non-stub version.
> 
> This part is not related to SEV builtin; what we want to avoid here
> is management layer to try to inject secret while the guest hasn't
> been started with SEV (IOW 'no memory encryption requested for KVM).
> 
> Maybe this error message is more explicit?
> 
>   error_setg(errp, "Guest is not using memory encryption");
> 
> Or:
> 
>   error_setg(errp, "Guest is not using SEV");

This is better; there's a separate feature called memory encryption, so
we don't want to confuse things.

Dave

> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH v3 06/22] 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" for &error_abort, add it now.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
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 cacec605bf1..e169a01713d 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 v3 06/22] target/i386/cpu: Add missing 'qapi/error.h' header

[Paolo Bonzini]

On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
> Commit 00b81053244 ("target-i386: Remove assert_no_error usage")
> forgot to add the "qapi/error.h" for &error_abort, add it now.
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
> 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 cacec605bf1..e169a01713d 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"
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[PATCH v3 07/22] target/i386/sev_i386.h: Remove unused headers

[Philippe Mathieu-Daudé]

Declarations don't require these headers, remove them.

Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
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 v3 07/22] target/i386/sev_i386.h: Remove unused headers

[Paolo Bonzini]

On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
> Declarations don't require these headers, remove them.
> 
> Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
> 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)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[PATCH v3 08/22] target/i386/sev: Remove sev_get_me_mask()

[Philippe Mathieu-Daudé]

Unused dead code makes review harder, so remove it.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
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 fa7210473a6..c88cd808410 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)
 {
@@ -804,8 +797,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 v3 08/22] target/i386/sev: Remove sev_get_me_mask()

[Paolo Bonzini]

On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
> Unused dead code makes review harder, so remove it.
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
> 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 fa7210473a6..c88cd808410 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)
>   {
> @@ -804,8 +797,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) {
> 

RB

Re: [PATCH v3 08/22] target/i386/sev: Remove sev_get_me_mask()

[Paolo Bonzini]

On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
> Unused dead code makes review harder, so remove it.
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
> 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 fa7210473a6..c88cd808410 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)
>   {
> @@ -804,8 +797,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) {
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[PATCH v3 09/22] 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.

Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
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 v3 09/22] target/i386/sev: Mark unreachable code with g_assert_not_reached()

[Paolo Bonzini]

On 02/10/21 14:53, 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.
> 
> Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
> 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 *
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[PATCH v3 10/22] 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 c88cd808410..aefbef4bb63 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

Re: [PATCH v3 10/22] target/i386/sev: sev_get_attestation_report use g_autofree

[Paolo Bonzini]

On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
> 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 c88cd808410..aefbef4bb63 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;
>   }
>   
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[PATCH v3 11/22] 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

Re: [PATCH v3 11/22] target/i386/sev: Restrict SEV to system emulation

[Paolo Bonzini]

On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
> 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')
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[PATCH v3 12/22] target/i386/sev: Declare system-specific functions in 'sev_i386.h'

[Philippe Mathieu-Daudé]

While prefixed with sysemu, 'sysemu/sev.h' contains the architecture
specific declarations. The system specific parts are declared in
'sev_i386.h'.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/sysemu/sev.h   | 6 ------
 target/i386/sev_i386.h | 7 +++++++
 hw/i386/pc_sysfw.c     | 2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index 94d821d737c..a329ed75c1c 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -18,11 +18,5 @@
 
 bool sev_enabled(void);
 int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
-int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
-int sev_inject_launch_secret(const char *hdr, const char *secret,
-                             uint64_t gpa, Error **errp);
-
-int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size);
-void sev_es_set_reset_vector(CPUState *cpu);
 
 #endif
diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index afa19a0a161..0798ab3519a 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -33,4 +33,11 @@ extern SevCapability *sev_get_capabilities(Error **errp);
 extern SevAttestationReport *
 sev_get_attestation_report(const char *mnonce, Error **errp);
 
+int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
+int sev_inject_launch_secret(const char *hdr, const char *secret,
+                             uint64_t gpa, Error **errp);
+
+int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size);
+void sev_es_set_reset_vector(CPUState *cpu);
+
 #endif
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 68d6b1f783e..0b202138b66 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -37,7 +37,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/block/flash.h"
 #include "sysemu/kvm.h"
-#include "sysemu/sev.h"
+#include "sev_i386.h"
 
 #define FLASH_SECTOR_SIZE 4096
 
-- 
2.31.1

Re: [PATCH v3 12/22] target/i386/sev: Declare system-specific functions in 'sev_i386.h'

[Paolo Bonzini]

On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
> While prefixed with sysemu, 'sysemu/sev.h' contains the architecture
> specific declarations. The system specific parts are declared in
> 'sev_i386.h'.

While outside target/i386, 'sysemu/sev.h' contains some architecture 
specific declarations. Move them to 'sev_i386.h'.

Otherwise,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   include/sysemu/sev.h   | 6 ------
>   target/i386/sev_i386.h | 7 +++++++
>   hw/i386/pc_sysfw.c     | 2 +-
>   3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
> index 94d821d737c..a329ed75c1c 100644
> --- a/include/sysemu/sev.h
> +++ b/include/sysemu/sev.h
> @@ -18,11 +18,5 @@
>   
>   bool sev_enabled(void);
>   int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
> -int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
> -int sev_inject_launch_secret(const char *hdr, const char *secret,
> -                             uint64_t gpa, Error **errp);
> -
> -int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size);
> -void sev_es_set_reset_vector(CPUState *cpu);
>   
>   #endif
> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
> index afa19a0a161..0798ab3519a 100644
> --- a/target/i386/sev_i386.h
> +++ b/target/i386/sev_i386.h
> @@ -33,4 +33,11 @@ extern SevCapability *sev_get_capabilities(Error **errp);
>   extern SevAttestationReport *
>   sev_get_attestation_report(const char *mnonce, Error **errp);
>   
> +int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
> +int sev_inject_launch_secret(const char *hdr, const char *secret,
> +                             uint64_t gpa, Error **errp);
> +
> +int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size);
> +void sev_es_set_reset_vector(CPUState *cpu);
> +
>   #endif
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index 68d6b1f783e..0b202138b66 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -37,7 +37,7 @@
>   #include "hw/qdev-properties.h"
>   #include "hw/block/flash.h"
>   #include "sysemu/kvm.h"
> -#include "sysemu/sev.h"
> +#include "sev_i386.h"
>   
>   #define FLASH_SECTOR_SIZE 4096
>   
> 

[PATCH v3 13/22] target/i386/sev: Remove stubs by using code elision

[Philippe Mathieu-Daudé]

Only declare sev_enabled() and sev_es_enabled() when CONFIG_SEV is
set, to allow the compiler to elide unused code. Remove unnecessary
stubs.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/sysemu/sev.h    | 14 +++++++++++++-
 target/i386/sev_i386.h  |  3 ---
 target/i386/cpu.c       | 16 +++++++++-------
 target/i386/sev-stub.c  | 36 ------------------------------------
 target/i386/meson.build |  2 +-
 5 files changed, 23 insertions(+), 48 deletions(-)
 delete mode 100644 target/i386/sev-stub.c

diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index a329ed75c1c..f5c625bb3b3 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -14,9 +14,21 @@
 #ifndef QEMU_SEV_H
 #define QEMU_SEV_H
 
-#include "sysemu/kvm.h"
+#ifndef CONFIG_USER_ONLY
+#include CONFIG_DEVICES /* CONFIG_SEV */
+#endif
 
+#ifdef CONFIG_SEV
 bool sev_enabled(void);
+bool sev_es_enabled(void);
+#else
+#define sev_enabled() 0
+#define sev_es_enabled() 0
+#endif
+
+uint32_t sev_get_cbit_position(void);
+uint32_t sev_get_reduced_phys_bits(void);
+
 int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
 
 #endif
diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index 0798ab3519a..2d9a1a0112e 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -24,10 +24,7 @@
 #define SEV_POLICY_DOMAIN       0x10
 #define SEV_POLICY_SEV          0x20
 
-extern bool sev_es_enabled(void);
 extern SevInfo *sev_get_info(void);
-extern uint32_t sev_get_cbit_position(void);
-extern uint32_t sev_get_reduced_phys_bits(void);
 extern char *sev_get_launch_measurement(void);
 extern SevCapability *sev_get_capabilities(Error **errp);
 extern SevAttestationReport *
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index e169a01713d..27992bdc9f8 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -25,8 +25,8 @@
 #include "tcg/helper-tcg.h"
 #include "sysemu/reset.h"
 #include "sysemu/hvf.h"
+#include "sysemu/sev.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"
@@ -38,6 +38,7 @@
 #include "exec/address-spaces.h"
 #include "hw/boards.h"
 #include "hw/i386/sgx-epc.h"
+#include "sev_i386.h"
 #endif
 
 #include "disas/capstone.h"
@@ -5764,12 +5765,13 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         *edx = 0;
         break;
     case 0x8000001F:
-        *eax = sev_enabled() ? 0x2 : 0;
-        *eax |= sev_es_enabled() ? 0x8 : 0;
-        *ebx = sev_get_cbit_position();
-        *ebx |= sev_get_reduced_phys_bits() << 6;
-        *ecx = 0;
-        *edx = 0;
+        *eax = *ebx = *ecx = *edx = 0;
+        if (sev_enabled()) {
+            *eax = 0x2;
+            *eax |= sev_es_enabled() ? 0x8 : 0;
+            *ebx = sev_get_cbit_position();
+            *ebx |= sev_get_reduced_phys_bits() << 6;
+        }
         break;
     default:
         /* reserved values: zero */
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
deleted file mode 100644
index 8eae5d2fa8d..00000000000
--- a/target/i386/sev-stub.c
+++ /dev/null
@@ -1,36 +0,0 @@
-/*
- * QEMU SEV 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/error.h"
-#include "sev_i386.h"
-
-bool sev_enabled(void)
-{
-    return false;
-}
-
-uint32_t sev_get_cbit_position(void)
-{
-    return 0;
-}
-
-uint32_t sev_get_reduced_phys_bits(void)
-{
-    return 0;
-}
-
-bool sev_es_enabled(void)
-{
-    return false;
-}
diff --git a/target/i386/meson.build b/target/i386/meson.build
index a4f45c3ec1d..ae38dc95635 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'), if_false: files('sev-stub.c'))
+i386_ss.add(when: 'CONFIG_SEV', if_true: files('host-cpu.c'))
 
 # x86 cpu type
 i386_ss.add(when: 'CONFIG_KVM', if_true: files('host-cpu.c'))
-- 
2.31.1

Re: [PATCH v3 13/22] target/i386/sev: Remove stubs by using code elision

[Paolo Bonzini]

On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
> Only declare sev_enabled() and sev_es_enabled() when CONFIG_SEV is
> set, to allow the compiler to elide unused code. Remove unnecessary
> stubs.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   include/sysemu/sev.h    | 14 +++++++++++++-
>   target/i386/sev_i386.h  |  3 ---
>   target/i386/cpu.c       | 16 +++++++++-------
>   target/i386/sev-stub.c  | 36 ------------------------------------
>   target/i386/meson.build |  2 +-
>   5 files changed, 23 insertions(+), 48 deletions(-)
>   delete mode 100644 target/i386/sev-stub.c
> 
> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
> index a329ed75c1c..f5c625bb3b3 100644
> --- a/include/sysemu/sev.h
> +++ b/include/sysemu/sev.h
> @@ -14,9 +14,21 @@
>   #ifndef QEMU_SEV_H
>   #define QEMU_SEV_H
>   
> -#include "sysemu/kvm.h"
> +#ifndef CONFIG_USER_ONLY
> +#include CONFIG_DEVICES /* CONFIG_SEV */
> +#endif
>   
> +#ifdef CONFIG_SEV
>   bool sev_enabled(void);
> +bool sev_es_enabled(void);
> +#else
> +#define sev_enabled() 0
> +#define sev_es_enabled() 0
> +#endif

This means that sev.h can only be included from target-specific files.

An alternative could be:

#ifdef NEED_CPU_H
# include CONFIG_DEVICES
#endif

#if defined NEED_CPU_H && !defined CONFIG_SEV
# define sev_enabled() 0
# define sev_es_enabled() 0
#else
bool sev_enabled(void);
bool sev_es_enabled(void);
#endif

... but in fact sysemu/sev.h _is_ only used from x86-specific files. So 
should it be moved to include/hw/i386, and even merged with 
target/i386/sev_i386.h?  Do we need two files?

Thanks,

Paolo

> +uint32_t sev_get_cbit_position(void);
> +uint32_t sev_get_reduced_phys_bits(void);
> +
>   int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
>   
>   #endif
> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
> index 0798ab3519a..2d9a1a0112e 100644
> --- a/target/i386/sev_i386.h
> +++ b/target/i386/sev_i386.h
> @@ -24,10 +24,7 @@
>   #define SEV_POLICY_DOMAIN       0x10
>   #define SEV_POLICY_SEV          0x20
>   
> -extern bool sev_es_enabled(void);
>   extern SevInfo *sev_get_info(void);
> -extern uint32_t sev_get_cbit_position(void);
> -extern uint32_t sev_get_reduced_phys_bits(void);
>   extern char *sev_get_launch_measurement(void);
>   extern SevCapability *sev_get_capabilities(Error **errp);
>   extern SevAttestationReport *
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index e169a01713d..27992bdc9f8 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -25,8 +25,8 @@
>   #include "tcg/helper-tcg.h"
>   #include "sysemu/reset.h"
>   #include "sysemu/hvf.h"
> +#include "sysemu/sev.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"
> @@ -38,6 +38,7 @@
>   #include "exec/address-spaces.h"
>   #include "hw/boards.h"
>   #include "hw/i386/sgx-epc.h"
> +#include "sev_i386.h"
>   #endif
>   
>   #include "disas/capstone.h"
> @@ -5764,12 +5765,13 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>           *edx = 0;
>           break;
>       case 0x8000001F:
> -        *eax = sev_enabled() ? 0x2 : 0;
> -        *eax |= sev_es_enabled() ? 0x8 : 0;
> -        *ebx = sev_get_cbit_position();
> -        *ebx |= sev_get_reduced_phys_bits() << 6;
> -        *ecx = 0;
> -        *edx = 0;
> +        *eax = *ebx = *ecx = *edx = 0;
> +        if (sev_enabled()) {
> +            *eax = 0x2;
> +            *eax |= sev_es_enabled() ? 0x8 : 0;
> +            *ebx = sev_get_cbit_position();
> +            *ebx |= sev_get_reduced_phys_bits() << 6;
> +        }
>           break;
>       default:
>           /* reserved values: zero */
> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
> deleted file mode 100644
> index 8eae5d2fa8d..00000000000
> --- a/target/i386/sev-stub.c
> +++ /dev/null
> @@ -1,36 +0,0 @@
> -/*
> - * QEMU SEV 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/error.h"
> -#include "sev_i386.h"
> -
> -bool sev_enabled(void)
> -{
> -    return false;
> -}
> -
> -uint32_t sev_get_cbit_position(void)
> -{
> -    return 0;
> -}
> -
> -uint32_t sev_get_reduced_phys_bits(void)
> -{
> -    return 0;
> -}
> -
> -bool sev_es_enabled(void)
> -{
> -    return false;
> -}
> diff --git a/target/i386/meson.build b/target/i386/meson.build
> index a4f45c3ec1d..ae38dc95635 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'), if_false: files('sev-stub.c'))
> +i386_ss.add(when: 'CONFIG_SEV', if_true: files('host-cpu.c'))
>   
>   # x86 cpu type
>   i386_ss.add(when: 'CONFIG_KVM', if_true: files('host-cpu.c'))
> 

Re: [PATCH v3 13/22] target/i386/sev: Remove stubs by using code elision

[Philippe Mathieu-Daudé]

On 10/4/21 10:19, Paolo Bonzini wrote:
> On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
>> Only declare sev_enabled() and sev_es_enabled() when CONFIG_SEV is
>> set, to allow the compiler to elide unused code. Remove unnecessary
>> stubs.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   include/sysemu/sev.h    | 14 +++++++++++++-
>>   target/i386/sev_i386.h  |  3 ---
>>   target/i386/cpu.c       | 16 +++++++++-------
>>   target/i386/sev-stub.c  | 36 ------------------------------------
>>   target/i386/meson.build |  2 +-
>>   5 files changed, 23 insertions(+), 48 deletions(-)
>>   delete mode 100644 target/i386/sev-stub.c
>>
>> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
>> index a329ed75c1c..f5c625bb3b3 100644
>> --- a/include/sysemu/sev.h
>> +++ b/include/sysemu/sev.h
>> @@ -14,9 +14,21 @@
>>   #ifndef QEMU_SEV_H
>>   #define QEMU_SEV_H
>>   -#include "sysemu/kvm.h"
>> +#ifndef CONFIG_USER_ONLY
>> +#include CONFIG_DEVICES /* CONFIG_SEV */
>> +#endif
>>   +#ifdef CONFIG_SEV
>>   bool sev_enabled(void);
>> +bool sev_es_enabled(void);
>> +#else
>> +#define sev_enabled() 0
>> +#define sev_es_enabled() 0
>> +#endif
> 
> This means that sev.h can only be included from target-specific files.
> 
> An alternative could be:
> 
> #ifdef NEED_CPU_H
> # include CONFIG_DEVICES

<command-line>: fatal error: x86_64-linux-user-config-devices.h: No such
file or directory

> #endif
> 
> #if defined NEED_CPU_H && !defined CONFIG_SEV
> # define sev_enabled() 0
> # define sev_es_enabled() 0
> #else
> bool sev_enabled(void);
> bool sev_es_enabled(void);
> #endif
> 
> ... but in fact sysemu/sev.h _is_ only used from x86-specific files. So
> should it be moved to include/hw/i386, and even merged with
> target/i386/sev_i386.h?  Do we need two files?

No clue, I don't think we need. Brijesh?

> 
> Thanks,
> 
> Paolo
> 
>> +uint32_t sev_get_cbit_position(void);
>> +uint32_t sev_get_reduced_phys_bits(void);
>> +
>>   int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
>>     #endif
>> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
>> index 0798ab3519a..2d9a1a0112e 100644
>> --- a/target/i386/sev_i386.h
>> +++ b/target/i386/sev_i386.h
>> @@ -24,10 +24,7 @@
>>   #define SEV_POLICY_DOMAIN       0x10
>>   #define SEV_POLICY_SEV          0x20
>>   -extern bool sev_es_enabled(void);
>>   extern SevInfo *sev_get_info(void);
>> -extern uint32_t sev_get_cbit_position(void);
>> -extern uint32_t sev_get_reduced_phys_bits(void);
>>   extern char *sev_get_launch_measurement(void);
>>   extern SevCapability *sev_get_capabilities(Error **errp);
>>   extern SevAttestationReport *
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index e169a01713d..27992bdc9f8 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -25,8 +25,8 @@
>>   #include "tcg/helper-tcg.h"
>>   #include "sysemu/reset.h"
>>   #include "sysemu/hvf.h"
>> +#include "sysemu/sev.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"
>> @@ -38,6 +38,7 @@
>>   #include "exec/address-spaces.h"
>>   #include "hw/boards.h"
>>   #include "hw/i386/sgx-epc.h"
>> +#include "sev_i386.h"
>>   #endif
>>     #include "disas/capstone.h"
>> @@ -5764,12 +5765,13 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t
>> index, uint32_t count,
>>           *edx = 0;
>>           break;
>>       case 0x8000001F:
>> -        *eax = sev_enabled() ? 0x2 : 0;
>> -        *eax |= sev_es_enabled() ? 0x8 : 0;
>> -        *ebx = sev_get_cbit_position();
>> -        *ebx |= sev_get_reduced_phys_bits() << 6;
>> -        *ecx = 0;
>> -        *edx = 0;
>> +        *eax = *ebx = *ecx = *edx = 0;
>> +        if (sev_enabled()) {
>> +            *eax = 0x2;
>> +            *eax |= sev_es_enabled() ? 0x8 : 0;
>> +            *ebx = sev_get_cbit_position();
>> +            *ebx |= sev_get_reduced_phys_bits() << 6;
>> +        }
>>           break;

Re: [PATCH v3 13/22] target/i386/sev: Remove stubs by using code elision

[Brijesh Singh]

On 10/6/21 11:55 AM, Philippe Mathieu-Daudé wrote:
> On 10/4/21 10:19, Paolo Bonzini wrote:
>> On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
>>> Only declare sev_enabled() and sev_es_enabled() when CONFIG_SEV is
>>> set, to allow the compiler to elide unused code. Remove unnecessary
>>> stubs.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>   include/sysemu/sev.h    | 14 +++++++++++++-
>>>   target/i386/sev_i386.h  |  3 ---
>>>   target/i386/cpu.c       | 16 +++++++++-------
>>>   target/i386/sev-stub.c  | 36 ------------------------------------
>>>   target/i386/meson.build |  2 +-
>>>   5 files changed, 23 insertions(+), 48 deletions(-)
>>>   delete mode 100644 target/i386/sev-stub.c
>>>
>>> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
>>> index a329ed75c1c..f5c625bb3b3 100644
>>> --- a/include/sysemu/sev.h
>>> +++ b/include/sysemu/sev.h
>>> @@ -14,9 +14,21 @@
>>>   #ifndef QEMU_SEV_H
>>>   #define QEMU_SEV_H
>>>   -#include "sysemu/kvm.h"
>>> +#ifndef CONFIG_USER_ONLY
>>> +#include CONFIG_DEVICES /* CONFIG_SEV */
>>> +#endif
>>>   +#ifdef CONFIG_SEV
>>>   bool sev_enabled(void);
>>> +bool sev_es_enabled(void);
>>> +#else
>>> +#define sev_enabled() 0
>>> +#define sev_es_enabled() 0
>>> +#endif
>> This means that sev.h can only be included from target-specific files.
>>
>> An alternative could be:
>>
>> #ifdef NEED_CPU_H
>> # include CONFIG_DEVICES
> <command-line>: fatal error: x86_64-linux-user-config-devices.h: No such
> file or directory
>
>> #endif
>>
>> #if defined NEED_CPU_H && !defined CONFIG_SEV
>> # define sev_enabled() 0
>> # define sev_es_enabled() 0
>> #else
>> bool sev_enabled(void);
>> bool sev_es_enabled(void);
>> #endif
>>
>> ... but in fact sysemu/sev.h _is_ only used from x86-specific files. So
>> should it be moved to include/hw/i386, and even merged with
>> target/i386/sev_i386.h?  Do we need two files?
> No clue, I don't think we need. Brijesh?


Sorry for the late reply, we do not need two files and it can be easily
merged.

thanks

[PATCH v3 14/22] target/i386/sev: Move qmp_query_sev_attestation_report() to sev.c

[Philippe Mathieu-Daudé]

Move qmp_query_sev_attestation_report() from monitor.c to sev.c
and make sev_get_attestation_report() static. We don't need the
stub anymore, remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/i386/sev_i386.h        |  2 --
 target/i386/monitor.c         |  6 ------
 target/i386/sev-sysemu-stub.c |  7 ++++---
 target/i386/sev.c             | 12 ++++++++++--
 4 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index 2d9a1a0112e..5f367f78eb7 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -27,8 +27,6 @@
 extern SevInfo *sev_get_info(void);
 extern char *sev_get_launch_measurement(void);
 extern SevCapability *sev_get_capabilities(Error **errp);
-extern SevAttestationReport *
-sev_get_attestation_report(const char *mnonce, Error **errp);
 
 int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
 int sev_inject_launch_secret(const char *hdr, const char *secret,
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index a9f85acd473..c05d70252a2 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -764,12 +764,6 @@ void qmp_sev_inject_launch_secret(const char *packet_hdr,
     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);
-}
-
 SGXInfo *qmp_query_sgx(Error **errp)
 {
     return sgx_get_info(errp);
diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
index d556b4f091f..813b9a6a03b 100644
--- a/target/i386/sev-sysemu-stub.c
+++ b/target/i386/sev-sysemu-stub.c
@@ -13,6 +13,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/qapi-commands-misc-target.h"
+#include "qapi/qmp/qerror.h"
 #include "qapi/error.h"
 #include "sev_i386.h"
 
@@ -52,9 +53,9 @@ 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;
 }
diff --git a/target/i386/sev.c b/target/i386/sev.c
index aefbef4bb63..91a217bbb85 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -31,6 +31,8 @@
 #include "migration/blocker.h"
 #include "qom/object.h"
 #include "monitor/monitor.h"
+#include "qapi/qapi-commands-misc-target.h"
+#include "qapi/qmp/qerror.h"
 #include "exec/confidential-guest-support.h"
 #include "hw/i386/pc.h"
 
@@ -487,8 +489,8 @@ out:
     return cap;
 }
 
-SevAttestationReport *
-sev_get_attestation_report(const char *mnonce, Error **errp)
+static SevAttestationReport *sev_get_attestation_report(const char *mnonce,
+                                                        Error **errp)
 {
     struct kvm_sev_attestation_report input = {};
     SevAttestationReport *report = NULL;
@@ -549,6 +551,12 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
     return report;
 }
 
+SevAttestationReport *qmp_query_sev_attestation_report(const char *mnonce,
+                                                       Error **errp)
+{
+    return sev_get_attestation_report(mnonce, errp);
+}
+
 static int
 sev_read_file_base64(const char *filename, guchar **data, gsize *len)
 {
-- 
2.31.1

Re: [PATCH v3 14/22] target/i386/sev: Move qmp_query_sev_attestation_report() to sev.c

[Paolo Bonzini]

On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
> Move qmp_query_sev_attestation_report() from monitor.c to sev.c
> and make sev_get_attestation_report() static. We don't need the
> stub anymore, remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

This was done on purpose, but I have no objection to changing it this 
way.  We might in fact remove the indirection for SGX as well, and/or 
even move the implementation of the monitor commands from target/i386 to 
hw/i386 (the monitor is sysemu-specific).

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks,

Paolo

>   target/i386/sev_i386.h        |  2 --
>   target/i386/monitor.c         |  6 ------
>   target/i386/sev-sysemu-stub.c |  7 ++++---
>   target/i386/sev.c             | 12 ++++++++++--
>   4 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
> index 2d9a1a0112e..5f367f78eb7 100644
> --- a/target/i386/sev_i386.h
> +++ b/target/i386/sev_i386.h
> @@ -27,8 +27,6 @@
>   extern SevInfo *sev_get_info(void);
>   extern char *sev_get_launch_measurement(void);
>   extern SevCapability *sev_get_capabilities(Error **errp);
> -extern SevAttestationReport *
> -sev_get_attestation_report(const char *mnonce, Error **errp);
>   
>   int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
>   int sev_inject_launch_secret(const char *hdr, const char *secret,
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index a9f85acd473..c05d70252a2 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -764,12 +764,6 @@ void qmp_sev_inject_launch_secret(const char *packet_hdr,
>       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);
> -}
> -
>   SGXInfo *qmp_query_sgx(Error **errp)
>   {
>       return sgx_get_info(errp);
> diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
> index d556b4f091f..813b9a6a03b 100644
> --- a/target/i386/sev-sysemu-stub.c
> +++ b/target/i386/sev-sysemu-stub.c
> @@ -13,6 +13,7 @@
>   
>   #include "qemu/osdep.h"
>   #include "qapi/qapi-commands-misc-target.h"
> +#include "qapi/qmp/qerror.h"
>   #include "qapi/error.h"
>   #include "sev_i386.h"
>   
> @@ -52,9 +53,9 @@ 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;
>   }
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index aefbef4bb63..91a217bbb85 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -31,6 +31,8 @@
>   #include "migration/blocker.h"
>   #include "qom/object.h"
>   #include "monitor/monitor.h"
> +#include "qapi/qapi-commands-misc-target.h"
> +#include "qapi/qmp/qerror.h"
>   #include "exec/confidential-guest-support.h"
>   #include "hw/i386/pc.h"
>   
> @@ -487,8 +489,8 @@ out:
>       return cap;
>   }
>   
> -SevAttestationReport *
> -sev_get_attestation_report(const char *mnonce, Error **errp)
> +static SevAttestationReport *sev_get_attestation_report(const char *mnonce,
> +                                                        Error **errp)
>   {
>       struct kvm_sev_attestation_report input = {};
>       SevAttestationReport *report = NULL;
> @@ -549,6 +551,12 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
>       return report;
>   }
>   
> +SevAttestationReport *qmp_query_sev_attestation_report(const char *mnonce,
> +                                                       Error **errp)
> +{
> +    return sev_get_attestation_report(mnonce, errp);
> +}
> +
>   static int
>   sev_read_file_base64(const char *filename, guchar **data, gsize *len)
>   {
> 

Re: [PATCH v3 14/22] target/i386/sev: Move qmp_query_sev_attestation_report() to sev.c

[Philippe Mathieu-Daudé]

On 10/4/21 10:23, Paolo Bonzini wrote:
> On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
>> Move qmp_query_sev_attestation_report() from monitor.c to sev.c
>> and make sev_get_attestation_report() static. We don't need the
>> stub anymore, remove it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> This was done on purpose, but I have no objection to changing it this
> way.  We might in fact remove the indirection for SGX as well, and/or
> even move the implementation of the monitor commands from target/i386 to
> hw/i386 (the monitor is sysemu-specific).

OK about SGX, but in another series, this one is already painful enough.

Not sure about moving monitor to hw/, some commands expose hw info (like
info pic) but some others expose architectural features (like info sev).

> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Thanks,
> 
> Paolo

Re: [PATCH v3 14/22] target/i386/sev: Move qmp_query_sev_attestation_report() to sev.c

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> Move qmp_query_sev_attestation_report() from monitor.c to sev.c
> and make sev_get_attestation_report() static. We don't need the
> stub anymore, remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  target/i386/sev_i386.h        |  2 --
>  target/i386/monitor.c         |  6 ------
>  target/i386/sev-sysemu-stub.c |  7 ++++---
>  target/i386/sev.c             | 12 ++++++++++--
>  4 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
> index 2d9a1a0112e..5f367f78eb7 100644
> --- a/target/i386/sev_i386.h
> +++ b/target/i386/sev_i386.h
> @@ -27,8 +27,6 @@
>  extern SevInfo *sev_get_info(void);
>  extern char *sev_get_launch_measurement(void);
>  extern SevCapability *sev_get_capabilities(Error **errp);
> -extern SevAttestationReport *
> -sev_get_attestation_report(const char *mnonce, Error **errp);
>  
>  int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
>  int sev_inject_launch_secret(const char *hdr, const char *secret,
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index a9f85acd473..c05d70252a2 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -764,12 +764,6 @@ void qmp_sev_inject_launch_secret(const char *packet_hdr,
>      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);
> -}
> -
>  SGXInfo *qmp_query_sgx(Error **errp)
>  {
>      return sgx_get_info(errp);
> diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
> index d556b4f091f..813b9a6a03b 100644
> --- a/target/i386/sev-sysemu-stub.c
> +++ b/target/i386/sev-sysemu-stub.c
> @@ -13,6 +13,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "qapi/qapi-commands-misc-target.h"
> +#include "qapi/qmp/qerror.h"
>  #include "qapi/error.h"
>  #include "sev_i386.h"
>  
> @@ -52,9 +53,9 @@ 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);

I did like that message making it clear the reason it was unsupported
was this build, rather than lack of host support or not enabling it.

Dave

>      return NULL;
>  }
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index aefbef4bb63..91a217bbb85 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -31,6 +31,8 @@
>  #include "migration/blocker.h"
>  #include "qom/object.h"
>  #include "monitor/monitor.h"
> +#include "qapi/qapi-commands-misc-target.h"
> +#include "qapi/qmp/qerror.h"
>  #include "exec/confidential-guest-support.h"
>  #include "hw/i386/pc.h"
>  
> @@ -487,8 +489,8 @@ out:
>      return cap;
>  }
>  
> -SevAttestationReport *
> -sev_get_attestation_report(const char *mnonce, Error **errp)
> +static SevAttestationReport *sev_get_attestation_report(const char *mnonce,
> +                                                        Error **errp)
>  {
>      struct kvm_sev_attestation_report input = {};
>      SevAttestationReport *report = NULL;
> @@ -549,6 +551,12 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
>      return report;
>  }
>  
> +SevAttestationReport *qmp_query_sev_attestation_report(const char *mnonce,
> +                                                       Error **errp)
> +{
> +    return sev_get_attestation_report(mnonce, errp);
> +}
> +
>  static int
>  sev_read_file_base64(const char *filename, guchar **data, gsize *len)
>  {
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 14/22] target/i386/sev: Move qmp_query_sev_attestation_report() to sev.c

[Philippe Mathieu-Daudé]

On 10/4/21 11:57, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
>> Move qmp_query_sev_attestation_report() from monitor.c to sev.c
>> and make sev_get_attestation_report() static. We don't need the
>> stub anymore, remove it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  target/i386/sev_i386.h        |  2 --
>>  target/i386/monitor.c         |  6 ------
>>  target/i386/sev-sysemu-stub.c |  7 ++++---
>>  target/i386/sev.c             | 12 ++++++++++--
>>  4 files changed, 14 insertions(+), 13 deletions(-)

>> -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);
> 
> I did like that message making it clear the reason it was unsupported
> was this build, rather than lack of host support or not enabling it.

Yep, no reason to change it, besides, QERR_UNSUPPORTED is deprecated
since 2015! (commit 4629ed1e989):

/*
 * These macros will go away, please don't use in new code, and do not
 * add new ones!
 */

I suppose this is a rebase mistake, thanks for catching it!

Phil.

[PATCH v3 15/22] target/i386/sev: Move qmp_sev_inject_launch_secret() to sev.c

[Philippe Mathieu-Daudé]

Move qmp_sev_inject_launch_secret() from monitor.c to sev.c
and make sev_inject_launch_secret() static. We don't need the
stub anymore, remove it.

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

diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index c05d70252a2..188203da6f2 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -733,37 +733,6 @@ 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);
-}
-
 SGXInfo *qmp_query_sgx(Error **errp)
 {
     return sgx_get_info(errp);
diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
index 813b9a6a03b..66b69540aa5 100644
--- a/target/i386/sev-sysemu-stub.c
+++ b/target/i386/sev-sysemu-stub.c
@@ -33,10 +33,10 @@ SevCapability *sev_get_capabilities(Error **errp)
     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)
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 91a217bbb85..2198d550be2 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -949,6 +949,37 @@ int sev_inject_launch_secret(const char *packet_hdr, const char *secret,
     return 0;
 }
 
+#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);
+}
+
 static int
 sev_es_parse_reset_block(SevInfoBlock *info, uint32_t *addr)
 {
-- 
2.31.1

Re: [PATCH v3 15/22] target/i386/sev: Move qmp_sev_inject_launch_secret() to sev.c

[Paolo Bonzini]

On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
> Move qmp_sev_inject_launch_secret() from monitor.c to sev.c
> and make sev_inject_launch_secret() static. We don't need the
> stub anymore, remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   target/i386/monitor.c         | 31 -------------------------------
>   target/i386/sev-sysemu-stub.c |  6 +++---
>   target/i386/sev.c             | 31 +++++++++++++++++++++++++++++++
>   3 files changed, 34 insertions(+), 34 deletions(-)
> 
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index c05d70252a2..188203da6f2 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -733,37 +733,6 @@ 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);
> -}
> -
>   SGXInfo *qmp_query_sgx(Error **errp)
>   {
>       return sgx_get_info(errp);
> diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
> index 813b9a6a03b..66b69540aa5 100644
> --- a/target/i386/sev-sysemu-stub.c
> +++ b/target/i386/sev-sysemu-stub.c
> @@ -33,10 +33,10 @@ SevCapability *sev_get_capabilities(Error **errp)
>       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)
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 91a217bbb85..2198d550be2 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -949,6 +949,37 @@ int sev_inject_launch_secret(const char *packet_hdr, const char *secret,
>       return 0;
>   }
>   
> +#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);
> +}
> +
>   static int
>   sev_es_parse_reset_block(SevInfoBlock *info, uint32_t *addr)
>   {
> 

Ok, this indirectly addresses my comment on patch 5.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[PATCH v3 16/22] target/i386/sev: Move qmp_query_sev_capabilities() to sev.c

[Philippe Mathieu-Daudé]

Move qmp_query_sev_capabilities() from monitor.c to sev.c
and make sev_get_capabilities() static. We don't need the
stub anymore, remove it.

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

diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index 5f367f78eb7..8d9388d8c5c 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -26,7 +26,6 @@
 
 extern SevInfo *sev_get_info(void);
 extern char *sev_get_launch_measurement(void);
-extern SevCapability *sev_get_capabilities(Error **errp);
 
 int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
 int sev_inject_launch_secret(const char *hdr, const char *secret,
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 188203da6f2..da36522fa15 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -728,11 +728,6 @@ SevLaunchMeasureInfo *qmp_query_sev_launch_measure(Error **errp)
     return info;
 }
 
-SevCapability *qmp_query_sev_capabilities(Error **errp)
-{
-    return sev_get_capabilities(errp);
-}
-
 SGXInfo *qmp_query_sgx(Error **errp)
 {
     return sgx_get_info(errp);
diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
index 66b69540aa5..cc486a1afbe 100644
--- a/target/i386/sev-sysemu-stub.c
+++ b/target/i386/sev-sysemu-stub.c
@@ -27,9 +27,9 @@ char *sev_get_launch_measurement(void)
     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;
 }
 
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 2198d550be2..fce007d6749 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -438,8 +438,7 @@ e_free:
     return 1;
 }
 
-SevCapability *
-sev_get_capabilities(Error **errp)
+static SevCapability *sev_get_capabilities(Error **errp)
 {
     SevCapability *cap = NULL;
     guchar *pdh_data = NULL;
@@ -489,6 +488,11 @@ out:
     return cap;
 }
 
+SevCapability *qmp_query_sev_capabilities(Error **errp)
+{
+    return sev_get_capabilities(errp);
+}
+
 static SevAttestationReport *sev_get_attestation_report(const char *mnonce,
                                                         Error **errp)
 {
-- 
2.31.1

Re: [PATCH v3 16/22] target/i386/sev: Move qmp_query_sev_capabilities() to sev.c

[Paolo Bonzini]

On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
> Move qmp_query_sev_capabilities() from monitor.c to sev.c
> and make sev_get_capabilities() static. We don't need the
> stub anymore, remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   target/i386/sev_i386.h        | 1 -
>   target/i386/monitor.c         | 5 -----
>   target/i386/sev-sysemu-stub.c | 4 ++--
>   target/i386/sev.c             | 8 ++++++--
>   4 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
> index 5f367f78eb7..8d9388d8c5c 100644
> --- a/target/i386/sev_i386.h
> +++ b/target/i386/sev_i386.h
> @@ -26,7 +26,6 @@
>   
>   extern SevInfo *sev_get_info(void);
>   extern char *sev_get_launch_measurement(void);
> -extern SevCapability *sev_get_capabilities(Error **errp);
>   
>   int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
>   int sev_inject_launch_secret(const char *hdr, const char *secret,
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index 188203da6f2..da36522fa15 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -728,11 +728,6 @@ SevLaunchMeasureInfo *qmp_query_sev_launch_measure(Error **errp)
>       return info;
>   }
>   
> -SevCapability *qmp_query_sev_capabilities(Error **errp)
> -{
> -    return sev_get_capabilities(errp);
> -}
> -
>   SGXInfo *qmp_query_sgx(Error **errp)
>   {
>       return sgx_get_info(errp);
> diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
> index 66b69540aa5..cc486a1afbe 100644
> --- a/target/i386/sev-sysemu-stub.c
> +++ b/target/i386/sev-sysemu-stub.c
> @@ -27,9 +27,9 @@ char *sev_get_launch_measurement(void)
>       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;
>   }
>   
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 2198d550be2..fce007d6749 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -438,8 +438,7 @@ e_free:
>       return 1;
>   }
>   
> -SevCapability *
> -sev_get_capabilities(Error **errp)
> +static SevCapability *sev_get_capabilities(Error **errp)
>   {
>       SevCapability *cap = NULL;
>       guchar *pdh_data = NULL;
> @@ -489,6 +488,11 @@ out:
>       return cap;
>   }
>   
> +SevCapability *qmp_query_sev_capabilities(Error **errp)
> +{
> +    return sev_get_capabilities(errp);
> +}
> +
>   static SevAttestationReport *sev_get_attestation_report(const char *mnonce,
>                                                           Error **errp)
>   {
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[PATCH v3 17/22] target/i386/sev: Move qmp_query_sev_launch_measure() to sev.c

[Philippe Mathieu-Daudé]

Move qmp_query_sev_launch_measure() from monitor.c to sev.c
and make sev_get_launch_measurement() static. We don't need the
stub anymore, remove it.

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

diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index 8d9388d8c5c..1699376ad87 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -25,7 +25,6 @@
 #define SEV_POLICY_SEV          0x20
 
 extern SevInfo *sev_get_info(void);
-extern char *sev_get_launch_measurement(void);
 
 int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
 int sev_inject_launch_secret(const char *hdr, const char *secret,
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index da36522fa15..0b38e970c73 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -711,23 +711,6 @@ void hmp_info_sev(Monitor *mon, const QDict *qdict)
     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;
-}
-
 SGXInfo *qmp_query_sgx(Error **errp)
 {
     return sgx_get_info(errp);
diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
index cc486a1afbe..355391c16c4 100644
--- a/target/i386/sev-sysemu-stub.c
+++ b/target/i386/sev-sysemu-stub.c
@@ -22,8 +22,9 @@ SevInfo *sev_get_info(void)
     return NULL;
 }
 
-char *sev_get_launch_measurement(void)
+SevLaunchMeasureInfo *qmp_query_sev_launch_measure(Error **errp)
 {
+    error_setg(errp, QERR_UNSUPPORTED);
     return NULL;
 }
 
diff --git a/target/i386/sev.c b/target/i386/sev.c
index fce007d6749..8e9cce62196 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -718,8 +718,7 @@ free_measurement:
     g_free(measurement);
 }
 
-char *
-sev_get_launch_measurement(void)
+static char *sev_get_launch_measurement(void)
 {
     if (sev_guest &&
         sev_guest->state >= SEV_STATE_LAUNCH_SECRET) {
@@ -729,6 +728,23 @@ sev_get_launch_measurement(void)
     return NULL;
 }
 
+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;
+}
+
 static Notifier sev_machine_done_notify = {
     .notify = sev_launch_get_measure,
 };
-- 
2.31.1

Re: [PATCH v3 17/22] target/i386/sev: Move qmp_query_sev_launch_measure() to sev.c

[Paolo Bonzini]

On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
> Move qmp_query_sev_launch_measure() from monitor.c to sev.c
> and make sev_get_launch_measurement() static. We don't need the
> stub anymore, remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   target/i386/sev_i386.h        |  1 -
>   target/i386/monitor.c         | 17 -----------------
>   target/i386/sev-sysemu-stub.c |  3 ++-
>   target/i386/sev.c             | 20 ++++++++++++++++++--
>   4 files changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
> index 8d9388d8c5c..1699376ad87 100644
> --- a/target/i386/sev_i386.h
> +++ b/target/i386/sev_i386.h
> @@ -25,7 +25,6 @@
>   #define SEV_POLICY_SEV          0x20
>   
>   extern SevInfo *sev_get_info(void);
> -extern char *sev_get_launch_measurement(void);
>   
>   int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
>   int sev_inject_launch_secret(const char *hdr, const char *secret,
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index da36522fa15..0b38e970c73 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -711,23 +711,6 @@ void hmp_info_sev(Monitor *mon, const QDict *qdict)
>       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;
> -}
> -
>   SGXInfo *qmp_query_sgx(Error **errp)
>   {
>       return sgx_get_info(errp);
> diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
> index cc486a1afbe..355391c16c4 100644
> --- a/target/i386/sev-sysemu-stub.c
> +++ b/target/i386/sev-sysemu-stub.c
> @@ -22,8 +22,9 @@ SevInfo *sev_get_info(void)
>       return NULL;
>   }
>   
> -char *sev_get_launch_measurement(void)
> +SevLaunchMeasureInfo *qmp_query_sev_launch_measure(Error **errp)
>   {
> +    error_setg(errp, QERR_UNSUPPORTED);
>       return NULL;
>   }
>   
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index fce007d6749..8e9cce62196 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -718,8 +718,7 @@ free_measurement:
>       g_free(measurement);
>   }
>   
> -char *
> -sev_get_launch_measurement(void)
> +static char *sev_get_launch_measurement(void)
>   {
>       if (sev_guest &&
>           sev_guest->state >= SEV_STATE_LAUNCH_SECRET) {
> @@ -729,6 +728,23 @@ sev_get_launch_measurement(void)
>       return NULL;
>   }
>   
> +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;
> +}
> +
>   static Notifier sev_machine_done_notify = {
>       .notify = sev_launch_get_measure,
>   };
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[PATCH v3 18/22] target/i386/sev: Move qmp_query_sev() & hmp_info_sev() to sev.c

[Philippe Mathieu-Daudé]

Move qmp_query_sev() & hmp_info_sev()() from monitor.c to sev.c
and make sev_get_info() static. We don't need the stub anymore,
remove it. Add a stub for hmp_info_sev().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/i386/sev_i386.h        |  3 ---
 target/i386/monitor.c         | 38 +---------------------------------
 target/i386/sev-sysemu-stub.c | 10 ++++++++-
 target/i386/sev.c             | 39 +++++++++++++++++++++++++++++++++--
 4 files changed, 47 insertions(+), 43 deletions(-)

diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index 1699376ad87..15a959d6174 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -15,7 +15,6 @@
 #define QEMU_SEV_I386_H
 
 #include "sysemu/sev.h"
-#include "qapi/qapi-types-misc-target.h"
 
 #define SEV_POLICY_NODBG        0x1
 #define SEV_POLICY_NOKS         0x2
@@ -24,8 +23,6 @@
 #define SEV_POLICY_DOMAIN       0x10
 #define SEV_POLICY_SEV          0x20
 
-extern SevInfo *sev_get_info(void);
-
 int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
 int sev_inject_launch_secret(const char *hdr, const char *secret,
                              uint64_t gpa, Error **errp);
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 0b38e970c73..890870b252d 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -28,11 +28,9 @@
 #include "monitor/hmp-target.h"
 #include "monitor/hmp.h"
 #include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qerror.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"
@@ -677,40 +675,6 @@ void hmp_info_io_apic(Monitor *mon, const QDict *qdict)
                    "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);
-}
-
 SGXInfo *qmp_query_sgx(Error **errp)
 {
     return sgx_get_info(errp);
diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
index 355391c16c4..1836b32e4fc 100644
--- a/target/i386/sev-sysemu-stub.c
+++ b/target/i386/sev-sysemu-stub.c
@@ -12,13 +12,16 @@
  */
 
 #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;
 }
 
@@ -60,3 +63,8 @@ SevAttestationReport *qmp_query_sev_attestation_report(const char *mnonce,
     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 8e9cce62196..7caaa117ff7 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -27,10 +27,12 @@
 #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"
@@ -375,8 +377,7 @@ sev_get_reduced_phys_bits(void)
     return sev_guest ? sev_guest->reduced_phys_bits : 0;
 }
 
-SevInfo *
-sev_get_info(void)
+static SevInfo *sev_get_info(void)
 {
     SevInfo *info;
 
@@ -395,6 +396,40 @@ sev_get_info(void)
     return info;
 }
 
+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);
+}
+
 static int
 sev_get_pdh_info(int fd, guchar **pdh, size_t *pdh_len, guchar **cert_chain,
                  size_t *cert_chain_len, Error **errp)
-- 
2.31.1

Re: [PATCH v3 18/22] target/i386/sev: Move qmp_query_sev() & hmp_info_sev() to sev.c

[Paolo Bonzini]

On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
> Move qmp_query_sev() & hmp_info_sev()() from monitor.c to sev.c
> and make sev_get_info() static. We don't need the stub anymore,
> remove it. Add a stub for hmp_info_sev().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   target/i386/sev_i386.h        |  3 ---
>   target/i386/monitor.c         | 38 +---------------------------------
>   target/i386/sev-sysemu-stub.c | 10 ++++++++-
>   target/i386/sev.c             | 39 +++++++++++++++++++++++++++++++++--
>   4 files changed, 47 insertions(+), 43 deletions(-)
> 
> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
> index 1699376ad87..15a959d6174 100644
> --- a/target/i386/sev_i386.h
> +++ b/target/i386/sev_i386.h
> @@ -15,7 +15,6 @@
>   #define QEMU_SEV_I386_H
>   
>   #include "sysemu/sev.h"
> -#include "qapi/qapi-types-misc-target.h"
>   
>   #define SEV_POLICY_NODBG        0x1
>   #define SEV_POLICY_NOKS         0x2
> @@ -24,8 +23,6 @@
>   #define SEV_POLICY_DOMAIN       0x10
>   #define SEV_POLICY_SEV          0x20
>   
> -extern SevInfo *sev_get_info(void);
> -
>   int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
>   int sev_inject_launch_secret(const char *hdr, const char *secret,
>                                uint64_t gpa, Error **errp);
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index 0b38e970c73..890870b252d 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -28,11 +28,9 @@
>   #include "monitor/hmp-target.h"
>   #include "monitor/hmp.h"
>   #include "qapi/qmp/qdict.h"
> -#include "qapi/qmp/qerror.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"
> @@ -677,40 +675,6 @@ void hmp_info_io_apic(Monitor *mon, const QDict *qdict)
>                      "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);
> -}
> -
>   SGXInfo *qmp_query_sgx(Error **errp)
>   {
>       return sgx_get_info(errp);
> diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
> index 355391c16c4..1836b32e4fc 100644
> --- a/target/i386/sev-sysemu-stub.c
> +++ b/target/i386/sev-sysemu-stub.c
> @@ -12,13 +12,16 @@
>    */
>   
>   #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;
>   }
>   
> @@ -60,3 +63,8 @@ SevAttestationReport *qmp_query_sev_attestation_report(const char *mnonce,
>       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 8e9cce62196..7caaa117ff7 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -27,10 +27,12 @@
>   #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"
> @@ -375,8 +377,7 @@ sev_get_reduced_phys_bits(void)
>       return sev_guest ? sev_guest->reduced_phys_bits : 0;
>   }
>   
> -SevInfo *
> -sev_get_info(void)
> +static SevInfo *sev_get_info(void)
>   {
>       SevInfo *info;
>   
> @@ -395,6 +396,40 @@ sev_get_info(void)
>       return info;
>   }
>   
> +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);
> +}
> +
>   static int
>   sev_get_pdh_info(int fd, guchar **pdh, size_t *pdh_len, guchar **cert_chain,
>                    size_t *cert_chain_len, Error **errp)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[PATCH v3 19/22] 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 dc53add7eef..96956d0fc41 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -49,6 +49,7 @@ 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);
 void hmp_info_sgx(Monitor *mon, const QDict *qdict);
 
 #endif /* MONITOR_HMP_TARGET_H */
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 3baa1058e2c..6bc27639e01 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 1836b32e4fc..b2a4033a030 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 7caaa117ff7..c6d8fc52eb2 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

Re: [PATCH v3 19/22] monitor: Restrict 'info sev' to x86 targets

[Paolo Bonzini]

On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
> 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 dc53add7eef..96956d0fc41 100644
> --- a/include/monitor/hmp-target.h
> +++ b/include/monitor/hmp-target.h
> @@ -49,6 +49,7 @@ 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);
>   void hmp_info_sgx(Monitor *mon, const QDict *qdict);
>   
>   #endif /* MONITOR_HMP_TARGET_H */
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index 3baa1058e2c..6bc27639e01 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 1836b32e4fc..b2a4033a030 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 7caaa117ff7..c6d8fc52eb2 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"
> 


This is only a cleanup, isn't it?  The #ifdef is already in 
hmp-commands-info.hx.

Anyway,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

but please adjust the commit message in v4.

Paolo

Re: [PATCH v3 19/22] monitor: Restrict 'info sev' to x86 targets

[Philippe Mathieu-Daudé]

On 10/4/21 10:26, Paolo Bonzini wrote:
> On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
>> 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 dc53add7eef..96956d0fc41 100644
>> --- a/include/monitor/hmp-target.h
>> +++ b/include/monitor/hmp-target.h
>> @@ -49,6 +49,7 @@ 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);
>>   void hmp_info_sgx(Monitor *mon, const QDict *qdict);
>>     #endif /* MONITOR_HMP_TARGET_H */
>> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
>> index 3baa1058e2c..6bc27639e01 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 1836b32e4fc..b2a4033a030 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 7caaa117ff7..c6d8fc52eb2 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"
>>
> 
> 
> This is only a cleanup, isn't it?  The #ifdef is already in
> hmp-commands-info.hx.

IIUC the prototype is exposed to all targets, while with
this patch, only to the files including "monitor/hmp-target.h".

You are right the command is only added for TARGET_I386 in
hmp-commands-info.hx.

> 
> Anyway,
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> but please adjust the commit message in v4.

OK, thanks.

[PATCH v3 20/22] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot

[Philippe Mathieu-Daudé]

From: Dov Murik <dovmurik@linux.ibm.com>

Add the sev_add_kernel_loader_hashes function to calculate the hashes of
the kernel/initrd/cmdline and fill a designated OVMF encrypted hash
table area.  For this to work, OVMF must support an encrypted area to
place the data which is advertised via a special GUID in the OVMF reset
table.

The hashes of each of the files is calculated (or the string in the case
of the cmdline with trailing '\0' included).  Each entry in the hashes
table is GUID identified and since they're passed through the
sev_encrypt_flash interface, the hashes will be accumulated by the AMD
PSP measurement (SEV_LAUNCH_MEASURE).

Co-developed-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20210930054915.13252-2-dovmurik@linux.ibm.com>
[PMD: Rebased on top of 0021c4765a6]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/i386/sev_i386.h |  12 ++++
 target/i386/sev.c      | 138 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 150 insertions(+)

diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index 15a959d6174..17031cddd37 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -23,9 +23,21 @@
 #define SEV_POLICY_DOMAIN       0x10
 #define SEV_POLICY_SEV          0x20
 
+typedef struct SevKernelLoaderContext {
+    char *setup_data;
+    size_t setup_size;
+    char *kernel_data;
+    size_t kernel_size;
+    char *initrd_data;
+    size_t initrd_size;
+    char *cmdline_data;
+    size_t cmdline_size;
+} SevKernelLoaderContext;
+
 int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
 int sev_inject_launch_secret(const char *hdr, const char *secret,
                              uint64_t gpa, Error **errp);
+bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp);
 
 int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size);
 void sev_es_set_reset_vector(CPUState *cpu);
diff --git a/target/i386/sev.c b/target/i386/sev.c
index c6d8fc52eb2..91fdf0d4503 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -23,6 +23,7 @@
 #include "qemu/base64.h"
 #include "qemu/module.h"
 #include "qemu/uuid.h"
+#include "crypto/hash.h"
 #include "sysemu/kvm.h"
 #include "sev_i386.h"
 #include "sysemu/sysemu.h"
@@ -86,6 +87,32 @@ typedef struct __attribute__((__packed__)) SevInfoBlock {
     uint32_t reset_addr;
 } SevInfoBlock;
 
+#define SEV_HASH_TABLE_RV_GUID  "7255371f-3a3b-4b04-927b-1da6efa8d454"
+typedef struct QEMU_PACKED SevHashTableDescriptor {
+    /* SEV hash table area guest address */
+    uint32_t base;
+    /* SEV hash table area size (in bytes) */
+    uint32_t size;
+} SevHashTableDescriptor;
+
+/* hard code sha256 digest size */
+#define HASH_SIZE 32
+
+typedef struct QEMU_PACKED SevHashTableEntry {
+    QemuUUID guid;
+    uint16_t len;
+    uint8_t hash[HASH_SIZE];
+} SevHashTableEntry;
+
+typedef struct QEMU_PACKED SevHashTable {
+    QemuUUID guid;
+    uint16_t len;
+    SevHashTableEntry cmdline;
+    SevHashTableEntry initrd;
+    SevHashTableEntry kernel;
+    uint8_t padding[];
+} SevHashTable;
+
 static SevGuestState *sev_guest;
 static Error *sev_mig_blocker;
 
@@ -1151,6 +1178,117 @@ int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
     return 0;
 }
 
+static const QemuUUID sev_hash_table_header_guid = {
+    .data = UUID_LE(0x9438d606, 0x4f22, 0x4cc9, 0xb4, 0x79, 0xa7, 0x93,
+                    0xd4, 0x11, 0xfd, 0x21)
+};
+
+static const QemuUUID sev_kernel_entry_guid = {
+    .data = UUID_LE(0x4de79437, 0xabd2, 0x427f, 0xb8, 0x35, 0xd5, 0xb1,
+                    0x72, 0xd2, 0x04, 0x5b)
+};
+static const QemuUUID sev_initrd_entry_guid = {
+    .data = UUID_LE(0x44baf731, 0x3a2f, 0x4bd7, 0x9a, 0xf1, 0x41, 0xe2,
+                    0x91, 0x69, 0x78, 0x1d)
+};
+static const QemuUUID sev_cmdline_entry_guid = {
+    .data = UUID_LE(0x97d02dd8, 0xbd20, 0x4c94, 0xaa, 0x78, 0xe7, 0x71,
+                    0x4d, 0x36, 0xab, 0x2a)
+};
+
+/*
+ * Add the hashes of the linux kernel/initrd/cmdline to an encrypted guest page
+ * which is included in SEV's initial memory measurement.
+ */
+bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
+{
+    uint8_t *data;
+    SevHashTableDescriptor *area;
+    SevHashTable *ht;
+    uint8_t cmdline_hash[HASH_SIZE];
+    uint8_t initrd_hash[HASH_SIZE];
+    uint8_t kernel_hash[HASH_SIZE];
+    uint8_t *hashp;
+    size_t hash_len = HASH_SIZE;
+    int aligned_len;
+
+    if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
+        error_setg(errp,
+                   "SEV: kernel specified but OVMF has no hash table guid");
+        return false;
+    }
+    area = (SevHashTableDescriptor *)data;
+
+    /*
+     * Calculate hash of kernel command-line with the terminating null byte. If
+     * the user doesn't supply a command-line via -append, the 1-byte "\0" will
+     * be used.
+     */
+    hashp = cmdline_hash;
+    if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, ctx->cmdline_data,
+                           ctx->cmdline_size, &hashp, &hash_len, errp) < 0) {
+        return false;
+    }
+    assert(hash_len == HASH_SIZE);
+
+    /*
+     * Calculate hash of initrd. If the user doesn't supply an initrd via
+     * -initrd, an empty buffer will be used (ctx->initrd_size == 0).
+     */
+    hashp = initrd_hash;
+    if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, ctx->initrd_data,
+                           ctx->initrd_size, &hashp, &hash_len, errp) < 0) {
+        return false;
+    }
+    assert(hash_len == HASH_SIZE);
+
+    /* Calculate hash of the kernel */
+    hashp = kernel_hash;
+    struct iovec iov[2] = {
+        { .iov_base = ctx->setup_data, .iov_len = ctx->setup_size },
+        { .iov_base = ctx->kernel_data, .iov_len = ctx->kernel_size }
+    };
+    if (qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_SHA256, iov, ARRAY_SIZE(iov),
+                            &hashp, &hash_len, errp) < 0) {
+        return false;
+    }
+    assert(hash_len == HASH_SIZE);
+
+    /*
+     * Populate the hashes table in the guest's memory at the OVMF-designated
+     * area for the SEV hashes table
+     */
+    ht = qemu_map_ram_ptr(NULL, area->base);
+
+    ht->guid = sev_hash_table_header_guid;
+    ht->len = sizeof(*ht);
+
+    ht->cmdline.guid = sev_cmdline_entry_guid;
+    ht->cmdline.len = sizeof(ht->cmdline);
+    memcpy(ht->cmdline.hash, cmdline_hash, sizeof(ht->cmdline.hash));
+
+    ht->initrd.guid = sev_initrd_entry_guid;
+    ht->initrd.len = sizeof(ht->initrd);
+    memcpy(ht->initrd.hash, initrd_hash, sizeof(ht->initrd.hash));
+
+    ht->kernel.guid = sev_kernel_entry_guid;
+    ht->kernel.len = sizeof(ht->kernel);
+    memcpy(ht->kernel.hash, kernel_hash, sizeof(ht->kernel.hash));
+
+    /* When calling sev_encrypt_flash, the length has to be 16 byte aligned */
+    aligned_len = ROUND_UP(ht->len, 16);
+    if (aligned_len != ht->len) {
+        /* zero the excess data so the measurement can be reliably calculated */
+        memset(ht->padding, 0, aligned_len - ht->len);
+    }
+
+    if (sev_encrypt_flash((uint8_t *)ht, aligned_len, errp) < 0) {
+        return false;
+    }
+
+    return true;
+}
+
 static void
 sev_register_types(void)
 {
-- 
2.31.1

[PATCH v3 21/22] x86/sev: generate SEV kernel loader hashes in x86_load_linux

[Philippe Mathieu-Daudé]

From: Dov Murik <dovmurik@linux.ibm.com>

If SEV is enabled and a kernel is passed via -kernel, pass the hashes of
kernel/initrd/cmdline in an encrypted guest page to OVMF for SEV
measured boot.

Co-developed-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20210930054915.13252-3-dovmurik@linux.ibm.com>
[PMD: Rebased on top of 0021c4765a6]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/x86.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 41ef9a84a9f..0c7c054e3a0 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -47,6 +47,7 @@
 #include "hw/i386/fw_cfg.h"
 #include "hw/intc/i8259.h"
 #include "hw/rtc/mc146818rtc.h"
+#include "target/i386/sev_i386.h"
 
 #include "hw/acpi/cpu_hotplug.h"
 #include "hw/irq.h"
@@ -780,6 +781,7 @@ void x86_load_linux(X86MachineState *x86ms,
     const char *initrd_filename = machine->initrd_filename;
     const char *dtb_filename = machine->dtb;
     const char *kernel_cmdline = machine->kernel_cmdline;
+    SevKernelLoaderContext sev_load_ctx = {};
 
     /* Align to 16 bytes as a paranoia measure */
     cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
@@ -926,6 +928,8 @@ void x86_load_linux(X86MachineState *x86ms,
     fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_ADDR, cmdline_addr);
     fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, strlen(kernel_cmdline) + 1);
     fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline);
+    sev_load_ctx.cmdline_data = (char *)kernel_cmdline;
+    sev_load_ctx.cmdline_size = strlen(kernel_cmdline) + 1;
 
     if (protocol >= 0x202) {
         stl_p(header + 0x228, cmdline_addr);
@@ -1007,6 +1011,8 @@ void x86_load_linux(X86MachineState *x86ms,
         fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_ADDR, initrd_addr);
         fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size);
         fw_cfg_add_bytes(fw_cfg, FW_CFG_INITRD_DATA, initrd_data, initrd_size);
+        sev_load_ctx.initrd_data = initrd_data;
+        sev_load_ctx.initrd_size = initrd_size;
 
         stl_p(header + 0x218, initrd_addr);
         stl_p(header + 0x21c, initrd_size);
@@ -1065,15 +1071,32 @@ void x86_load_linux(X86MachineState *x86ms,
         load_image_size(dtb_filename, setup_data->data, dtb_size);
     }
 
-    memcpy(setup, header, MIN(sizeof(header), setup_size));
+    /*
+     * If we're starting an encrypted VM, it will be OVMF based, which uses the
+     * efi stub for booting and doesn't require any values to be placed in the
+     * kernel header.  We therefore don't update the header so the hash of the
+     * kernel on the other side of the fw_cfg interface matches the hash of the
+     * file the user passed in.
+     */
+    if (!sev_enabled()) {
+        memcpy(setup, header, MIN(sizeof(header), setup_size));
+    }
 
     fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, prot_addr);
     fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_SIZE, kernel_size);
     fw_cfg_add_bytes(fw_cfg, FW_CFG_KERNEL_DATA, kernel, kernel_size);
+    sev_load_ctx.kernel_data = (char *)kernel;
+    sev_load_ctx.kernel_size = kernel_size;
 
     fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_ADDR, real_addr);
     fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size);
     fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA, setup, setup_size);
+    sev_load_ctx.setup_data = (char *)setup;
+    sev_load_ctx.setup_size = setup_size;
+
+    if (sev_enabled()) {
+        sev_add_kernel_loader_hashes(&sev_load_ctx, &error_fatal);
+    }
 
     option_rom[nb_option_roms].bootindex = 0;
     option_rom[nb_option_roms].name = "linuxboot.bin";
-- 
2.31.1

[PATCH v3 22/22] MAINTAINERS: Cover AMD SEV files

[Philippe Mathieu-Daudé]

Add an entry to list SEV-related files.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 50435b8d2f5..733a5201e76 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3038,6 +3038,13 @@ F: hw/core/clock-vmstate.c
 F: hw/core/qdev-clock.c
 F: docs/devel/clocks.rst
 
+AMD Secure Encrypted Virtualization (SEV)
+S: Orphan
+F: docs/amd-memory-encryption.txt
+F: target/i386/sev*
+F: target/i386/kvm/sev-stub.c
+F: include/sysemu/sev.h
+
 Usermode Emulation
 ------------------
 Overall usermode emulation
-- 
2.31.1

Re: [PATCH v3 22/22] MAINTAINERS: Cover AMD SEV files

[Paolo Bonzini]

On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
> Add an entry to list SEV-related files.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   MAINTAINERS | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 50435b8d2f5..733a5201e76 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3038,6 +3038,13 @@ F: hw/core/clock-vmstate.c
>   F: hw/core/qdev-clock.c
>   F: docs/devel/clocks.rst
>   
> +AMD Secure Encrypted Virtualization (SEV)
> +S: Orphan
> +F: docs/amd-memory-encryption.txt
> +F: target/i386/sev*
> +F: target/i386/kvm/sev-stub.c
> +F: include/sysemu/sev.h

I don't think it qualifies as orphan; it's covered by x86 maintainers.

Paolo

Re: [PATCH v3 22/22] MAINTAINERS: Cover AMD SEV files

[Philippe Mathieu-Daudé]

On 10/4/21 10:27, Paolo Bonzini wrote:
> On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
>> Add an entry to list SEV-related files.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   MAINTAINERS | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 50435b8d2f5..733a5201e76 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -3038,6 +3038,13 @@ F: hw/core/clock-vmstate.c
>>   F: hw/core/qdev-clock.c
>>   F: docs/devel/clocks.rst
>>   +AMD Secure Encrypted Virtualization (SEV)
>> +S: Orphan
>> +F: docs/amd-memory-encryption.txt
>> +F: target/i386/sev*
>> +F: target/i386/kvm/sev-stub.c
>> +F: include/sysemu/sev.h
> 
> I don't think it qualifies as orphan; it's covered by x86 maintainers.

$ ./scripts/get_maintainer.pl -f docs/amd-memory-encryption.txt
get_maintainer.pl: No maintainers found, printing recent contributors.
get_maintainer.pl: Do not blindly cc: them on patches!  Use common sense.
Connor Kuehl <ckuehl@redhat.com> (commit_signer:2/3=67%)
Eduardo Habkost <ehabkost@redhat.com> (commit_signer:2/3=67%)
Tom Lendacky <thomas.lendacky@amd.com> (commit_signer:2/3=67%)
Laszlo Ersek <lersek@redhat.com> (commit_signer:2/3=67%)
Greg Kurz <groug@kaod.org> (commit_signer:1/3=33%)
qemu-devel@nongnu.org (open list:All patches CC here)

$ ./scripts/get_maintainer.pl -f target/i386/sev.c
get_maintainer.pl: No maintainers found, printing recent contributors.
get_maintainer.pl: Do not blindly cc: them on patches!  Use common sense.
"Philippe Mathieu-Daudé" <philmd@redhat.com> (commit_signer:15/29=52%)
"Dr. David Alan Gilbert" <dgilbert@redhat.com> (commit_signer:7/29=24%)
Paolo Bonzini <pbonzini@redhat.com> (commit_signer:7/29=24%)
David Gibson <david@gibson.dropbear.id.au> (commit_signer:7/29=24%)
Eduardo Habkost <ehabkost@redhat.com> (commit_signer:5/29=17%)
qemu-devel@nongnu.org (open list:All patches CC here)

$ ./scripts/get_maintainer.pl -f target/i386/sev.h
get_maintainer.pl: No maintainers found, printing recent contributors.
get_maintainer.pl: Do not blindly cc: them on patches!  Use common sense.
"Philippe Mathieu-Daudé" <philmd@redhat.com> (commit_signer:8/12=67%)
Paolo Bonzini <pbonzini@redhat.com> (commit_signer:4/12=33%)
James Bottomley <jejb@linux.ibm.com> (commit_signer:3/12=25%)
"Dr. David Alan Gilbert" <dgilbert@redhat.com> (commit_signer:3/12=25%)
Connor Kuehl <ckuehl@redhat.com> (commit_signer:3/12=25%)
qemu-devel@nongnu.org (open list:All patches CC here)

I will update the patch to add these files to the "X86 KVM CPUs"
section.

Thanks,

Phil.

Re: [PATCH v3 00/22] target/i386/sev: Housekeeping SEV + measured Linux SEV guest

[Paolo Bonzini]

On 02/10/21 14:52, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> While testing James & Dov patch:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg810571.html
> 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.
> 
> Since v2:
> - Rebased on top of SGX
> - Addressed review comments from Markus / David
> - Included/rebased 'Measured Linux SEV guest' from Dov [1]
> - Added orphean MAINTAINERS section

I have queued Dov's patches already, but apart from that the changes 
from v3 to v4 should be minimal.

Thanks for this work!

Paolo

> [1] https://lore.kernel.org/qemu-devel/20210825073538.959525-1-dovmurik@linux.ibm.com/
> 
> Supersedes: <20210616204328.2611406-1-philmd@redhat.com>
> 
> Dov Murik (2):
>    sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux
>      boot
>    x86/sev: generate SEV kernel loader hashes in x86_load_linux
> 
> Dr. David Alan Gilbert (1):
>    target/i386/sev: sev_get_attestation_report use g_autofree
> 
> Philippe Mathieu-Daudé (19):
>    qapi/misc-target: Wrap long 'SEV Attestation Report' long lines
>    qapi/misc-target: Group SEV QAPI definitions
>    target/i386/kvm: Introduce i386_softmmu_kvm Meson source set
>    target/i386/kvm: Restrict SEV stubs to x86 architecture
>    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/sev: Declare system-specific functions in 'sev_i386.h'
>    target/i386/sev: Remove stubs by using code elision
>    target/i386/sev: Move qmp_query_sev_attestation_report() to sev.c
>    target/i386/sev: Move qmp_sev_inject_launch_secret() to sev.c
>    target/i386/sev: Move qmp_query_sev_capabilities() to sev.c
>    target/i386/sev: Move qmp_query_sev_launch_measure() to sev.c
>    target/i386/sev: Move qmp_query_sev() & hmp_info_sev() to sev.c
>    monitor: Restrict 'info sev' to x86 targets
>    MAINTAINERS: Cover AMD SEV files
> 
>   qapi/misc-target.json                 |  77 ++++----
>   include/monitor/hmp-target.h          |   1 +
>   include/monitor/hmp.h                 |   1 -
>   include/sysemu/sev.h                  |  20 +-
>   target/i386/sev_i386.h                |  32 +--
>   hw/i386/pc_sysfw.c                    |   2 +-
>   hw/i386/x86.c                         |  25 ++-
>   target/i386/cpu.c                     |  17 +-
>   {accel => target/i386}/kvm/sev-stub.c |   0
>   target/i386/monitor.c                 |  92 +--------
>   target/i386/sev-stub.c                |  83 --------
>   target/i386/sev-sysemu-stub.c         |  70 +++++++
>   target/i386/sev.c                     | 268 +++++++++++++++++++++++---
>   MAINTAINERS                           |   7 +
>   accel/kvm/meson.build                 |   1 -
>   target/i386/kvm/meson.build           |   8 +-
>   target/i386/meson.build               |   4 +-
>   17 files changed, 438 insertions(+), 270 deletions(-)
>   rename {accel => target/i386}/kvm/sev-stub.c (100%)
>   delete mode 100644 target/i386/sev-stub.c
>   create mode 100644 target/i386/sev-sysemu-stub.c
>