* [PATCH 1/9] meson: Add optional dependency on IGVM library
2024-02-27 14:50 [PATCH 0/9] Introduce support for IGVM files Roy Hopkins
@ 2024-02-27 14:50 ` Roy Hopkins
2024-02-27 14:50 ` [PATCH 2/9] backends/confidential-guest-support: Add IGVM file parameter Roy Hopkins
` (9 subsequent siblings)
10 siblings, 0 replies; 35+ messages in thread
From: Roy Hopkins @ 2024-02-27 14:50 UTC (permalink / raw)
To: qemu-devel
Cc: Roy Hopkins, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
The IGVM library allows Isolated Guest Virtual Machine files to be
parsed and processed. IGVM files are used to configure guest memory
layout, initial processor state and other configuration pertaining to
secure virtual machines.
This adds the --enable-igvm configure option, enabled by default, which
attempts to locate and link against the IGVM library via pkgconfig and
sets CONFIG_IGVM if found.
The library is added to the system_ss target in backends/meson.build
where the IGVM parsing will be performed by the ConfidentialGuestSupport
object.
Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
backends/meson.build | 3 +++
meson.build | 8 ++++++++
meson_options.txt | 2 ++
scripts/meson-buildoptions.sh | 3 +++
4 files changed, 16 insertions(+)
diff --git a/backends/meson.build b/backends/meson.build
index 8b2b111497..d550ac19f7 100644
--- a/backends/meson.build
+++ b/backends/meson.build
@@ -30,5 +30,8 @@ if have_vhost_user_crypto
endif
system_ss.add(when: gio, if_true: files('dbus-vmstate.c'))
system_ss.add(when: 'CONFIG_SGX', if_true: files('hostmem-epc.c'))
+if igvm.found()
+ system_ss.add(igvm)
+endif
subdir('tpm')
diff --git a/meson.build b/meson.build
index 0ef1654e86..d4949693b0 100644
--- a/meson.build
+++ b/meson.build
@@ -1230,6 +1230,12 @@ if host_os == 'linux' and (have_system or have_tools)
method: 'pkg-config',
required: get_option('libudev'))
endif
+igvm = not_found
+if not get_option('igvm').auto() or have_system
+ igvm = dependency('igvm',
+ method: 'pkg-config',
+ required: get_option('igvm'))
+endif
mpathlibs = [libudev]
mpathpersist = not_found
@@ -2314,6 +2320,7 @@ config_host_data.set('CONFIG_CFI', get_option('cfi'))
config_host_data.set('CONFIG_SELINUX', selinux.found())
config_host_data.set('CONFIG_XEN_BACKEND', xen.found())
config_host_data.set('CONFIG_LIBDW', libdw.found())
+config_host_data.set('CONFIG_IGVM', igvm.found())
if xen.found()
# protect from xen.version() having less than three components
xen_version = xen.version().split('.') + ['0', '0']
@@ -4433,6 +4440,7 @@ summary_info += {'seccomp support': seccomp}
summary_info += {'GlusterFS support': glusterfs}
summary_info += {'hv-balloon support': hv_balloon}
summary_info += {'TPM support': have_tpm}
+summary_info += {'IGVM support': igvm}
summary_info += {'libssh support': libssh}
summary_info += {'lzo support': lzo}
summary_info += {'snappy support': snappy}
diff --git a/meson_options.txt b/meson_options.txt
index 0a99a059ec..4eaba64f4b 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -109,6 +109,8 @@ option('dbus_display', type: 'feature', value: 'auto',
description: '-display dbus support')
option('tpm', type : 'feature', value : 'auto',
description: 'TPM support')
+option('igvm', type: 'feature', value: 'auto',
+ description: 'Independent Guest Virtual Machine (IGVM) file support')
# Do not enable it by default even for Mingw32, because it doesn't
# work on Wine.
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 680fa3f581..38a8183625 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -126,6 +126,7 @@ meson_options_help() {
printf "%s\n" ' hv-balloon hv-balloon driver (requires Glib 2.68+ GTree API)'
printf "%s\n" ' hvf HVF acceleration support'
printf "%s\n" ' iconv Font glyph conversion support'
+ printf "%s\n" ' igvm IGVM file support'
printf "%s\n" ' jack JACK sound support'
printf "%s\n" ' keyring Linux keyring support'
printf "%s\n" ' kvm KVM acceleration support'
@@ -342,6 +343,8 @@ _meson_option_parse() {
--iasl=*) quote_sh "-Diasl=$2" ;;
--enable-iconv) printf "%s" -Diconv=enabled ;;
--disable-iconv) printf "%s" -Diconv=disabled ;;
+ --enable-igvm) printf "%s" -Digvm=enabled ;;
+ --disable-igvm) printf "%s" -Digvm=disabled ;;
--includedir=*) quote_sh "-Dincludedir=$2" ;;
--enable-install-blobs) printf "%s" -Dinstall_blobs=true ;;
--disable-install-blobs) printf "%s" -Dinstall_blobs=false ;;
--
2.43.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH 2/9] backends/confidential-guest-support: Add IGVM file parameter
2024-02-27 14:50 [PATCH 0/9] Introduce support for IGVM files Roy Hopkins
2024-02-27 14:50 ` [PATCH 1/9] meson: Add optional dependency on IGVM library Roy Hopkins
@ 2024-02-27 14:50 ` Roy Hopkins
2024-03-19 15:10 ` Stefano Garzarella
2024-02-27 14:50 ` [PATCH 3/9] backends/confidential-guest-support: Add functions to support IGVM Roy Hopkins
` (8 subsequent siblings)
10 siblings, 1 reply; 35+ messages in thread
From: Roy Hopkins @ 2024-02-27 14:50 UTC (permalink / raw)
To: qemu-devel
Cc: Roy Hopkins, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
In order to add support for parsing IGVM files for secure virtual
machines, a the path to an IGVM file needs to be specified as
part of the guest configuration. It makes sense to add this to
the ConfidentialGuestSupport object as this is common to all secure
virtual machines that potentially could support IGVM based
configuration.
This patch allows the filename to be configured via the QEMU
object model in preparation for subsequent patches that will read and
parse the IGVM file.
Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
backends/confidential-guest-support.c | 21 +++++++++++++++++++++
include/exec/confidential-guest-support.h | 9 +++++++++
qapi/qom.json | 13 +++++++++++++
qemu-options.hx | 8 +++++++-
4 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/backends/confidential-guest-support.c b/backends/confidential-guest-support.c
index 052fde8db0..da436fb736 100644
--- a/backends/confidential-guest-support.c
+++ b/backends/confidential-guest-support.c
@@ -20,8 +20,29 @@ OBJECT_DEFINE_ABSTRACT_TYPE(ConfidentialGuestSupport,
CONFIDENTIAL_GUEST_SUPPORT,
OBJECT)
+#if defined(CONFIG_IGVM)
+static char *get_igvm(Object *obj, Error **errp)
+{
+ ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
+ return g_strdup(cgs->igvm_filename);
+}
+
+static void set_igvm(Object *obj, const char *value, Error **errp)
+{
+ ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
+ g_free(cgs->igvm_filename);
+ cgs->igvm_filename = g_strdup(value);
+}
+#endif
+
static void confidential_guest_support_class_init(ObjectClass *oc, void *data)
{
+#if defined(CONFIG_IGVM)
+ object_class_property_add_str(oc, "igvm-file",
+ get_igvm, set_igvm);
+ object_class_property_set_description(oc, "igvm-file",
+ "Set the IGVM filename to use");
+#endif
}
static void confidential_guest_support_init(Object *obj)
diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
index ba2dd4b5df..b08ad8de4d 100644
--- a/include/exec/confidential-guest-support.h
+++ b/include/exec/confidential-guest-support.h
@@ -51,6 +51,15 @@ struct ConfidentialGuestSupport {
* so 'ready' is not set, we'll abort.
*/
bool ready;
+
+#if defined(CONFIG_IGVM)
+ /*
+ * igvm_filename: Optional filename that specifies a file that contains
+ * the configuration of the guest in Isolated Guest
+ * Virtual Machine (IGVM) format.
+ */
+ char *igvm_filename;
+#endif
};
typedef struct ConfidentialGuestSupportClass {
diff --git a/qapi/qom.json b/qapi/qom.json
index 2a6e49365a..570bdd7d55 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -859,6 +859,18 @@
'base': 'RngProperties',
'data': { '*filename': 'str' } }
+##
+# @ConfidentialGuestProperties:
+#
+# Properties common to objects that are derivatives of confidential-guest-support.
+#
+# @igvm-file: IGVM file to use to configure guest (default: none)
+#
+# Since: 8.2
+##
+{ 'struct': 'ConfidentialGuestProperties',
+ 'data': { '*igvm-file': 'str' } }
+
##
# @SevGuestProperties:
#
@@ -886,6 +898,7 @@
# Since: 2.12
##
{ 'struct': 'SevGuestProperties',
+ 'base': 'ConfidentialGuestProperties',
'data': { '*sev-device': 'str',
'*dh-cert-file': 'str',
'*session-file': 'str',
diff --git a/qemu-options.hx b/qemu-options.hx
index 9be1e5817c..49d9226e35 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -5640,7 +5640,7 @@ SRST
-object secret,id=sec0,keyid=secmaster0,format=base64,\\
data=$SECRET,iv=$(<iv.b64)
- ``-object sev-guest,id=id,cbitpos=cbitpos,reduced-phys-bits=val,[sev-device=string,policy=policy,handle=handle,dh-cert-file=file,session-file=file,kernel-hashes=on|off]``
+ ``-object sev-guest,id=id,cbitpos=cbitpos,reduced-phys-bits=val,[sev-device=string,policy=policy,handle=handle,dh-cert-file=file,session-file=file,kernel-hashes=on|off,igvm-file=file]``
Create a Secure Encrypted Virtualization (SEV) guest object,
which can be used to provide the guest memory encryption support
on AMD processors.
@@ -5684,6 +5684,12 @@ SRST
cmdline to a designated guest firmware page for measured Linux
boot with -kernel. The default is off. (Since 6.2)
+ The ``igvm-file`` is an optional parameter that, when specified,
+ allows an Independent Guest Virtual Machine (IGVM) file to be
+ specified that configures the secure virtual machine and can
+ include, for example, an SVSM module, system firmware, initial
+ boot state, etc.
+
e.g to launch a SEV guest
.. parsed-literal::
--
2.43.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 2/9] backends/confidential-guest-support: Add IGVM file parameter
2024-02-27 14:50 ` [PATCH 2/9] backends/confidential-guest-support: Add IGVM file parameter Roy Hopkins
@ 2024-03-19 15:10 ` Stefano Garzarella
2024-03-20 14:44 ` Roy Hopkins
0 siblings, 1 reply; 35+ messages in thread
From: Stefano Garzarella @ 2024-03-19 15:10 UTC (permalink / raw)
To: Roy Hopkins
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
On Tue, Feb 27, 2024 at 02:50:08PM +0000, Roy Hopkins wrote:
>In order to add support for parsing IGVM files for secure virtual
>machines, a the path to an IGVM file needs to be specified as
>part of the guest configuration. It makes sense to add this to
>the ConfidentialGuestSupport object as this is common to all secure
>virtual machines that potentially could support IGVM based
>configuration.
>
>This patch allows the filename to be configured via the QEMU
>object model in preparation for subsequent patches that will read and
>parse the IGVM file.
>
>Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
>---
> backends/confidential-guest-support.c | 21 +++++++++++++++++++++
> include/exec/confidential-guest-support.h | 9 +++++++++
> qapi/qom.json | 13 +++++++++++++
> qemu-options.hx | 8 +++++++-
> 4 files changed, 50 insertions(+), 1 deletion(-)
>
>diff --git a/backends/confidential-guest-support.c b/backends/confidential-guest-support.c
>index 052fde8db0..da436fb736 100644
>--- a/backends/confidential-guest-support.c
>+++ b/backends/confidential-guest-support.c
>@@ -20,8 +20,29 @@ OBJECT_DEFINE_ABSTRACT_TYPE(ConfidentialGuestSupport,
> CONFIDENTIAL_GUEST_SUPPORT,
> OBJECT)
>
>+#if defined(CONFIG_IGVM)
>+static char *get_igvm(Object *obj, Error **errp)
>+{
>+ ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
>+ return g_strdup(cgs->igvm_filename);
>+}
>+
>+static void set_igvm(Object *obj, const char *value, Error **errp)
>+{
>+ ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
>+ g_free(cgs->igvm_filename);
>+ cgs->igvm_filename = g_strdup(value);
>+}
>+#endif
>+
> static void confidential_guest_support_class_init(ObjectClass *oc, void *data)
> {
>+#if defined(CONFIG_IGVM)
>+ object_class_property_add_str(oc, "igvm-file",
>+ get_igvm, set_igvm);
>+ object_class_property_set_description(oc, "igvm-file",
>+ "Set the IGVM filename to use");
>+#endif
> }
>
> static void confidential_guest_support_init(Object *obj)
>diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
>index ba2dd4b5df..b08ad8de4d 100644
>--- a/include/exec/confidential-guest-support.h
>+++ b/include/exec/confidential-guest-support.h
>@@ -51,6 +51,15 @@ struct ConfidentialGuestSupport {
> * so 'ready' is not set, we'll abort.
> */
> bool ready;
>+
>+#if defined(CONFIG_IGVM)
>+ /*
>+ * igvm_filename: Optional filename that specifies a file that contains
>+ * the configuration of the guest in Isolated Guest
>+ * Virtual Machine (IGVM) format.
>+ */
>+ char *igvm_filename;
>+#endif
> };
>
> typedef struct ConfidentialGuestSupportClass {
>diff --git a/qapi/qom.json b/qapi/qom.json
>index 2a6e49365a..570bdd7d55 100644
>--- a/qapi/qom.json
>+++ b/qapi/qom.json
>@@ -859,6 +859,18 @@
> 'base': 'RngProperties',
> 'data': { '*filename': 'str' } }
>
>+##
>+# @ConfidentialGuestProperties:
>+#
>+# Properties common to objects that are derivatives of confidential-guest-support.
>+#
>+# @igvm-file: IGVM file to use to configure guest (default: none)
>+#
>+# Since: 8.2
Should it be 9.0 or maybe 9.1 ?
>+##
>+{ 'struct': 'ConfidentialGuestProperties',
>+ 'data': { '*igvm-file': 'str' } }
>+
> ##
> # @SevGuestProperties:
> #
>@@ -886,6 +898,7 @@
> # Since: 2.12
> ##
> { 'struct': 'SevGuestProperties',
>+ 'base': 'ConfidentialGuestProperties',
> 'data': { '*sev-device': 'str',
> '*dh-cert-file': 'str',
> '*session-file': 'str',
>diff --git a/qemu-options.hx b/qemu-options.hx
>index 9be1e5817c..49d9226e35 100644
>--- a/qemu-options.hx
>+++ b/qemu-options.hx
>@@ -5640,7 +5640,7 @@ SRST
> -object secret,id=sec0,keyid=secmaster0,format=base64,\\
> data=$SECRET,iv=$(<iv.b64)
>
>- ``-object sev-guest,id=id,cbitpos=cbitpos,reduced-phys-bits=val,[sev-device=string,policy=policy,handle=handle,dh-cert-file=file,session-file=file,kernel-hashes=on|off]``
>+ ``-object sev-guest,id=id,cbitpos=cbitpos,reduced-phys-bits=val,[sev-device=string,policy=policy,handle=handle,dh-cert-file=file,session-file=file,kernel-hashes=on|off,igvm-file=file]``
> Create a Secure Encrypted Virtualization (SEV) guest object,
> which can be used to provide the guest memory encryption support
> on AMD processors.
>@@ -5684,6 +5684,12 @@ SRST
> cmdline to a designated guest firmware page for measured Linux
> boot with -kernel. The default is off. (Since 6.2)
>
>+ The ``igvm-file`` is an optional parameter that, when specified,
>+ allows an Independent Guest Virtual Machine (IGVM) file to be
>+ specified that configures the secure virtual machine and can
>+ include, for example, an SVSM module, system firmware, initial
>+ boot state, etc.
>+
> e.g to launch a SEV guest
>
> .. parsed-literal::
>--
>2.43.0
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 2/9] backends/confidential-guest-support: Add IGVM file parameter
2024-03-19 15:10 ` Stefano Garzarella
@ 2024-03-20 14:44 ` Roy Hopkins
2024-03-20 14:55 ` Daniel P. Berrangé
0 siblings, 1 reply; 35+ messages in thread
From: Roy Hopkins @ 2024-03-20 14:44 UTC (permalink / raw)
To: Stefano Garzarella
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
On Tue, 2024-03-19 at 16:10 +0100, Stefano Garzarella wrote:
> On Tue, Feb 27, 2024 at 02:50:08PM +0000, Roy Hopkins wrote:
> > In order to add support for parsing IGVM files for secure virtual
> > machines, a the path to an IGVM file needs to be specified as
> > part of the guest configuration. It makes sense to add this to
> > the ConfidentialGuestSupport object as this is common to all secure
> > virtual machines that potentially could support IGVM based
> > configuration.
> >
> > This patch allows the filename to be configured via the QEMU
> > object model in preparation for subsequent patches that will read and
> > parse the IGVM file.
> >
> > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> > ---
> > backends/confidential-guest-support.c | 21 +++++++++++++++++++++
> > include/exec/confidential-guest-support.h | 9 +++++++++
> > qapi/qom.json | 13 +++++++++++++
> > qemu-options.hx | 8 +++++++-
> > 4 files changed, 50 insertions(+), 1 deletion(-)
> >
> > diff --git a/backends/confidential-guest-support.c b/backends/confidential-
> > guest-support.c
> > index 052fde8db0..da436fb736 100644
> > --- a/backends/confidential-guest-support.c
> > +++ b/backends/confidential-guest-support.c
> > @@ -20,8 +20,29 @@ OBJECT_DEFINE_ABSTRACT_TYPE(ConfidentialGuestSupport,
> > CONFIDENTIAL_GUEST_SUPPORT,
> > OBJECT)
> >
> > +#if defined(CONFIG_IGVM)
> > +static char *get_igvm(Object *obj, Error **errp)
> > +{
> > + ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
> > + return g_strdup(cgs->igvm_filename);
> > +}
> > +
> > +static void set_igvm(Object *obj, const char *value, Error **errp)
> > +{
> > + ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
> > + g_free(cgs->igvm_filename);
> > + cgs->igvm_filename = g_strdup(value);
> > +}
> > +#endif
> > +
> > static void confidential_guest_support_class_init(ObjectClass *oc, void
> > *data)
> > {
> > +#if defined(CONFIG_IGVM)
> > + object_class_property_add_str(oc, "igvm-file",
> > + get_igvm, set_igvm);
> > + object_class_property_set_description(oc, "igvm-file",
> > + "Set the IGVM filename to use");
> > +#endif
> > }
> >
> > static void confidential_guest_support_init(Object *obj)
> > diff --git a/include/exec/confidential-guest-support.h
> > b/include/exec/confidential-guest-support.h
> > index ba2dd4b5df..b08ad8de4d 100644
> > --- a/include/exec/confidential-guest-support.h
> > +++ b/include/exec/confidential-guest-support.h
> > @@ -51,6 +51,15 @@ struct ConfidentialGuestSupport {
> > * so 'ready' is not set, we'll abort.
> > */
> > bool ready;
> > +
> > +#if defined(CONFIG_IGVM)
> > + /*
> > + * igvm_filename: Optional filename that specifies a file that contains
> > + * the configuration of the guest in Isolated Guest
> > + * Virtual Machine (IGVM) format.
> > + */
> > + char *igvm_filename;
> > +#endif
> > };
> >
> > typedef struct ConfidentialGuestSupportClass {
> > diff --git a/qapi/qom.json b/qapi/qom.json
> > index 2a6e49365a..570bdd7d55 100644
> > --- a/qapi/qom.json
> > +++ b/qapi/qom.json
> > @@ -859,6 +859,18 @@
> > 'base': 'RngProperties',
> > 'data': { '*filename': 'str' } }
> >
> > +##
> > +# @ConfidentialGuestProperties:
> > +#
> > +# Properties common to objects that are derivatives of confidential-guest-
> > support.
> > +#
> > +# @igvm-file: IGVM file to use to configure guest (default: none)
> > +#
> > +# Since: 8.2
>
> Should it be 9.0 or maybe 9.1 ?
Good question. Obviously it is hard to predict which version this will
potentially land in. I can update it to 9.1 because it is unlikely to be in any
version prior to this, but what is the normal convention for choosing a version
number here?
>
> > +##
> > +{ 'struct': 'ConfidentialGuestProperties',
> > + 'data': { '*igvm-file': 'str' } }
> > +
> > ##
> > # @SevGuestProperties:
> > #
> > @@ -886,6 +898,7 @@
> > # Since: 2.12
> > ##
> > { 'struct': 'SevGuestProperties',
> > + 'base': 'ConfidentialGuestProperties',
> > 'data': { '*sev-device': 'str',
> > '*dh-cert-file': 'str',
> > '*session-file': 'str',
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 9be1e5817c..49d9226e35 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -5640,7 +5640,7 @@ SRST
> > -object secret,id=sec0,keyid=secmaster0,format=base64,\\
> > data=$SECRET,iv=$(<iv.b64)
> >
> > - ``-object sev-guest,id=id,cbitpos=cbitpos,reduced-phys-bits=val,[sev-
> > device=string,policy=policy,handle=handle,dh-cert-file=file,session-
> > file=file,kernel-hashes=on|off]``
> > + ``-object sev-guest,id=id,cbitpos=cbitpos,reduced-phys-bits=val,[sev-
> > device=string,policy=policy,handle=handle,dh-cert-file=file,session-
> > file=file,kernel-hashes=on|off,igvm-file=file]``
> > Create a Secure Encrypted Virtualization (SEV) guest object,
> > which can be used to provide the guest memory encryption support
> > on AMD processors.
> > @@ -5684,6 +5684,12 @@ SRST
> > cmdline to a designated guest firmware page for measured Linux
> > boot with -kernel. The default is off. (Since 6.2)
> >
> > + The ``igvm-file`` is an optional parameter that, when specified,
> > + allows an Independent Guest Virtual Machine (IGVM) file to be
> > + specified that configures the secure virtual machine and can
> > + include, for example, an SVSM module, system firmware, initial
> > + boot state, etc.
> > +
> > e.g to launch a SEV guest
> >
> > .. parsed-literal::
> > --
> > 2.43.0
> >
> >
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 2/9] backends/confidential-guest-support: Add IGVM file parameter
2024-03-20 14:44 ` Roy Hopkins
@ 2024-03-20 14:55 ` Daniel P. Berrangé
0 siblings, 0 replies; 35+ messages in thread
From: Daniel P. Berrangé @ 2024-03-20 14:55 UTC (permalink / raw)
To: Roy Hopkins
Cc: Stefano Garzarella, qemu-devel, Paolo Bonzini, Marcelo Tosatti,
Michael S . Tsirkin, Cornelia Huck, Marcel Apfelbaum,
Sergio Lopez, Eduardo Habkost, Alistair Francis, Peter Xu,
David Hildenbrand, Igor Mammedov, Tom Lendacky, Michael Roth,
Jörg Roedel
On Wed, Mar 20, 2024 at 02:44:17PM +0000, Roy Hopkins wrote:
> On Tue, 2024-03-19 at 16:10 +0100, Stefano Garzarella wrote:
> > On Tue, Feb 27, 2024 at 02:50:08PM +0000, Roy Hopkins wrote:
> > > In order to add support for parsing IGVM files for secure virtual
> > > machines, a the path to an IGVM file needs to be specified as
> > > part of the guest configuration. It makes sense to add this to
> > > the ConfidentialGuestSupport object as this is common to all secure
> > > virtual machines that potentially could support IGVM based
> > > configuration.
> > >
> > > This patch allows the filename to be configured via the QEMU
> > > object model in preparation for subsequent patches that will read and
> > > parse the IGVM file.
> > >
> > > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> > > ---
> > > backends/confidential-guest-support.c | 21 +++++++++++++++++++++
> > > include/exec/confidential-guest-support.h | 9 +++++++++
> > > qapi/qom.json | 13 +++++++++++++
> > > qemu-options.hx | 8 +++++++-
> > > 4 files changed, 50 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/backends/confidential-guest-support.c b/backends/confidential-
> > > guest-support.c
> > > index 052fde8db0..da436fb736 100644
> > > --- a/backends/confidential-guest-support.c
> > > +++ b/backends/confidential-guest-support.c
> > > @@ -20,8 +20,29 @@ OBJECT_DEFINE_ABSTRACT_TYPE(ConfidentialGuestSupport,
> > > CONFIDENTIAL_GUEST_SUPPORT,
> > > OBJECT)
> > >
> > > +#if defined(CONFIG_IGVM)
> > > +static char *get_igvm(Object *obj, Error **errp)
> > > +{
> > > + ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
> > > + return g_strdup(cgs->igvm_filename);
> > > +}
> > > +
> > > +static void set_igvm(Object *obj, const char *value, Error **errp)
> > > +{
> > > + ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
> > > + g_free(cgs->igvm_filename);
> > > + cgs->igvm_filename = g_strdup(value);
> > > +}
> > > +#endif
> > > +
> > > static void confidential_guest_support_class_init(ObjectClass *oc, void
> > > *data)
> > > {
> > > +#if defined(CONFIG_IGVM)
> > > + object_class_property_add_str(oc, "igvm-file",
> > > + get_igvm, set_igvm);
> > > + object_class_property_set_description(oc, "igvm-file",
> > > + "Set the IGVM filename to use");
> > > +#endif
> > > }
> > >
> > > static void confidential_guest_support_init(Object *obj)
> > > diff --git a/include/exec/confidential-guest-support.h
> > > b/include/exec/confidential-guest-support.h
> > > index ba2dd4b5df..b08ad8de4d 100644
> > > --- a/include/exec/confidential-guest-support.h
> > > +++ b/include/exec/confidential-guest-support.h
> > > @@ -51,6 +51,15 @@ struct ConfidentialGuestSupport {
> > > * so 'ready' is not set, we'll abort.
> > > */
> > > bool ready;
> > > +
> > > +#if defined(CONFIG_IGVM)
> > > + /*
> > > + * igvm_filename: Optional filename that specifies a file that contains
> > > + * the configuration of the guest in Isolated Guest
> > > + * Virtual Machine (IGVM) format.
> > > + */
> > > + char *igvm_filename;
> > > +#endif
> > > };
> > >
> > > typedef struct ConfidentialGuestSupportClass {
> > > diff --git a/qapi/qom.json b/qapi/qom.json
> > > index 2a6e49365a..570bdd7d55 100644
> > > --- a/qapi/qom.json
> > > +++ b/qapi/qom.json
> > > @@ -859,6 +859,18 @@
> > > 'base': 'RngProperties',
> > > 'data': { '*filename': 'str' } }
> > >
> > > +##
> > > +# @ConfidentialGuestProperties:
> > > +#
> > > +# Properties common to objects that are derivatives of confidential-guest-
> > > support.
> > > +#
> > > +# @igvm-file: IGVM file to use to configure guest (default: none)
> > > +#
> > > +# Since: 8.2
> >
> > Should it be 9.0 or maybe 9.1 ?
>
> Good question. Obviously it is hard to predict which version this will
> potentially land in. I can update it to 9.1 because it is unlikely to be in any
> version prior to this, but what is the normal convention for choosing a version
> number here?
Pick the version number that is soonest release accepting merge
of features.
IOW, most of the time you can pick the version number in current
QEMU git master HEAD. During release freeze (ie right now), you
have to pick the next version number, since features aren't accepted
during freeze.
So choosing '9.1' right now is correct.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 3/9] backends/confidential-guest-support: Add functions to support IGVM
2024-02-27 14:50 [PATCH 0/9] Introduce support for IGVM files Roy Hopkins
2024-02-27 14:50 ` [PATCH 1/9] meson: Add optional dependency on IGVM library Roy Hopkins
2024-02-27 14:50 ` [PATCH 2/9] backends/confidential-guest-support: Add IGVM file parameter Roy Hopkins
@ 2024-02-27 14:50 ` Roy Hopkins
2024-03-01 16:37 ` Daniel P. Berrangé
2024-02-27 14:50 ` [PATCH 4/9] backends/igvm: Implement parsing and processing of IGVM files Roy Hopkins
` (7 subsequent siblings)
10 siblings, 1 reply; 35+ messages in thread
From: Roy Hopkins @ 2024-02-27 14:50 UTC (permalink / raw)
To: qemu-devel
Cc: Roy Hopkins, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
In preparation for supporting the processing of IGVM files to configure
guests, this adds a set of functions to ConfidentialGuestSupport
allowing configuration of secure virtual machines that can be
implemented for each supported isolation platform type such as Intel TDX
or AMD SEV-SNP. These functions will be called by IGVM processing code
in subsequent patches.
This commit provides a default implementation of the functions that
either perform no action or generate a warning or error when they are
called. Targets that support ConfidentalGuestSupport should override
these implementations.
Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
backends/confidential-guest-support.c | 26 ++++++++
include/exec/confidential-guest-support.h | 76 +++++++++++++++++++++++
2 files changed, 102 insertions(+)
diff --git a/backends/confidential-guest-support.c b/backends/confidential-guest-support.c
index da436fb736..42628be8d7 100644
--- a/backends/confidential-guest-support.c
+++ b/backends/confidential-guest-support.c
@@ -14,6 +14,7 @@
#include "qemu/osdep.h"
#include "exec/confidential-guest-support.h"
+#include "qemu/error-report.h"
OBJECT_DEFINE_ABSTRACT_TYPE(ConfidentialGuestSupport,
confidential_guest_support,
@@ -45,8 +46,33 @@ static void confidential_guest_support_class_init(ObjectClass *oc, void *data)
#endif
}
+static int check_support(ConfidentialGuestPlatformType platform,
+ uint16_t platform_version, uint8_t highest_vtl,
+ uint64_t shared_gpa_boundary)
+{
+ /* Default: no support. */
+ return 0;
+}
+
+static int set_guest_state(hwaddr gpa, uint8_t *ptr, uint64_t len,
+ ConfidentialGuestPageType memory_type,
+ uint16_t cpu_index)
+{
+ warn_report("Confidential guest memory not supported");
+ return -1;
+}
+
+static int get_mem_map_entry(int index, ConfidentialGuestMemoryMapEntry *entry)
+{
+ return 1;
+}
+
static void confidential_guest_support_init(Object *obj)
{
+ ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
+ cgs->check_support = check_support;
+ cgs->set_guest_state = set_guest_state;
+ cgs->get_mem_map_entry = get_mem_map_entry;
}
static void confidential_guest_support_finalize(Object *obj)
diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
index b08ad8de4d..c43a1a32f1 100644
--- a/include/exec/confidential-guest-support.h
+++ b/include/exec/confidential-guest-support.h
@@ -21,10 +21,46 @@
#ifndef CONFIG_USER_ONLY
#include "qom/object.h"
+#include "exec/hwaddr.h"
+
+#if defined(CONFIG_IGVM)
+#include "igvm/igvm.h"
+#endif
#define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport, CONFIDENTIAL_GUEST_SUPPORT)
+typedef enum ConfidentialGuestPlatformType {
+ CGS_PLATFORM_SEV,
+ CGS_PLATFORM_SEV_ES,
+ CGS_PLATFORM_SEV_SNP,
+ CGS_PLATFORM_TDX,
+} ConfidentialGuestPlatformType;
+
+typedef enum ConfidentialGuestMemoryType {
+ CGS_MEM_RAM,
+ CGS_MEM_RESERVED,
+ CGS_MEM_ACPI,
+ CGS_MEM_NVS,
+ CGS_MEM_UNUSABLE,
+} ConfidentialGuestMemoryType;
+
+typedef struct ConfidentialGuestMemoryMapEntry {
+ uint64_t gpa;
+ uint64_t size;
+ ConfidentialGuestMemoryType type;
+} ConfidentialGuestMemoryMapEntry;
+
+typedef enum ConfidentialGuestPageType {
+ CGS_PAGE_TYPE_NORMAL,
+ CGS_PAGE_TYPE_VMSA,
+ CGS_PAGE_TYPE_ZERO,
+ CGS_PAGE_TYPE_UNMEASURED,
+ CGS_PAGE_TYPE_SECRETS,
+ CGS_PAGE_TYPE_CPUID,
+ CGS_PAGE_TYPE_REQUIRED_MEMORY,
+} ConfidentialGuestPageType;
+
struct ConfidentialGuestSupport {
Object parent;
@@ -60,6 +96,46 @@ struct ConfidentialGuestSupport {
*/
char *igvm_filename;
#endif
+
+ /*
+ * The following virtual methods need to be implemented by systems that
+ * support confidential guests that can be configured with IGVM and are
+ * used during processing of the IGVM file with process_igvm().
+ */
+
+ /*
+ * Check for to see if this confidential guest supports a particular
+ * platform or configuration
+ */
+ int (*check_support)(ConfidentialGuestPlatformType platform,
+ uint16_t platform_version, uint8_t highest_vtl,
+ uint64_t shared_gpa_boundary);
+
+ /*
+ * Configure part of the state of a guest for a particular set of data, page
+ * type and gpa. This can be used for example to pre-populate and measure
+ * guest memory contents, define private ranges or set the initial CPU state
+ * for one or more CPUs.
+ *
+ * If memory_type is CGS_PAGE_TYPE_VMSA then ptr points to the initial CPU
+ * context for a virtual CPU. The format of the data depends on the type of
+ * confidential virtual machine. For example, for SEV-ES ptr will point to a
+ * vmcb_save_area structure that should be copied into guest memory at the
+ * address specified in gpa. The cpu_index parameter contains the index of
+ * the CPU the VMSA applies to.
+ */
+ int (*set_guest_state)(hwaddr gpa, uint8_t *ptr, uint64_t len,
+ ConfidentialGuestPageType memory_type,
+ uint16_t cpu_index);
+
+ /*
+ * Iterate the system memory map, getting the entry with the given index
+ * that can be populated into guest memory.
+ *
+ * Returns 1 if the index is out of range.
+ */
+ int (*get_mem_map_entry)(int index,
+ ConfidentialGuestMemoryMapEntry *entry);
};
typedef struct ConfidentialGuestSupportClass {
--
2.43.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 3/9] backends/confidential-guest-support: Add functions to support IGVM
2024-02-27 14:50 ` [PATCH 3/9] backends/confidential-guest-support: Add functions to support IGVM Roy Hopkins
@ 2024-03-01 16:37 ` Daniel P. Berrangé
2024-03-12 11:43 ` Roy Hopkins
0 siblings, 1 reply; 35+ messages in thread
From: Daniel P. Berrangé @ 2024-03-01 16:37 UTC (permalink / raw)
To: Roy Hopkins
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
On Tue, Feb 27, 2024 at 02:50:09PM +0000, Roy Hopkins wrote:
> In preparation for supporting the processing of IGVM files to configure
> guests, this adds a set of functions to ConfidentialGuestSupport
> allowing configuration of secure virtual machines that can be
> implemented for each supported isolation platform type such as Intel TDX
> or AMD SEV-SNP. These functions will be called by IGVM processing code
> in subsequent patches.
>
> This commit provides a default implementation of the functions that
> either perform no action or generate a warning or error when they are
> called. Targets that support ConfidentalGuestSupport should override
> these implementations.
>
> Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> ---
> backends/confidential-guest-support.c | 26 ++++++++
> include/exec/confidential-guest-support.h | 76 +++++++++++++++++++++++
> 2 files changed, 102 insertions(+)
>
> diff --git a/backends/confidential-guest-support.c b/backends/confidential-guest-support.c
> index da436fb736..42628be8d7 100644
> --- a/backends/confidential-guest-support.c
> +++ b/backends/confidential-guest-support.c
> @@ -14,6 +14,7 @@
> #include "qemu/osdep.h"
>
> #include "exec/confidential-guest-support.h"
> +#include "qemu/error-report.h"
>
> OBJECT_DEFINE_ABSTRACT_TYPE(ConfidentialGuestSupport,
> confidential_guest_support,
> @@ -45,8 +46,33 @@ static void confidential_guest_support_class_init(ObjectClass *oc, void *data)
> #endif
> }
>
> +static int check_support(ConfidentialGuestPlatformType platform,
> + uint16_t platform_version, uint8_t highest_vtl,
> + uint64_t shared_gpa_boundary)
> +{
> + /* Default: no support. */
> + return 0;
> +}
> +
> +static int set_guest_state(hwaddr gpa, uint8_t *ptr, uint64_t len,
> + ConfidentialGuestPageType memory_type,
> + uint16_t cpu_index)
> +{
> + warn_report("Confidential guest memory not supported");
> + return -1;
> +}
> +
> +static int get_mem_map_entry(int index, ConfidentialGuestMemoryMapEntry *entry)
> +{
> + return 1;
> +}
IIUC, all these can reports errors, and as such I think
they should have an 'Error **errp' parameter, so we can
report precise errors in these methods, rather than
less useful generic errors in the caller.
The above 'warn_report' ought to be an error too, since
it is returning an failure code (-1)
> +
> static void confidential_guest_support_init(Object *obj)
> {
> + ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
> + cgs->check_support = check_support;
> + cgs->set_guest_state = set_guest_state;
> + cgs->get_mem_map_entry = get_mem_map_entry;
> }
>
> static void confidential_guest_support_finalize(Object *obj)
> diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
> index b08ad8de4d..c43a1a32f1 100644
> --- a/include/exec/confidential-guest-support.h
> +++ b/include/exec/confidential-guest-support.h
> @@ -21,10 +21,46 @@
> #ifndef CONFIG_USER_ONLY
>
> #include "qom/object.h"
> +#include "exec/hwaddr.h"
> +
> +#if defined(CONFIG_IGVM)
> +#include "igvm/igvm.h"
> +#endif
>
> #define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
> OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport, CONFIDENTIAL_GUEST_SUPPORT)
>
> +typedef enum ConfidentialGuestPlatformType {
> + CGS_PLATFORM_SEV,
> + CGS_PLATFORM_SEV_ES,
> + CGS_PLATFORM_SEV_SNP,
> + CGS_PLATFORM_TDX,
QEMU only has support for SEV & SEV_ES today. We should leave the
others until we actually get an impl of SEV-SNP/TDX in QEMU that
supports those platforms.
> +} ConfidentialGuestPlatformType;
> +
> +typedef enum ConfidentialGuestMemoryType {
> + CGS_MEM_RAM,
> + CGS_MEM_RESERVED,
> + CGS_MEM_ACPI,
> + CGS_MEM_NVS,
> + CGS_MEM_UNUSABLE,
> +} ConfidentialGuestMemoryType;
> +
> +typedef struct ConfidentialGuestMemoryMapEntry {
> + uint64_t gpa;
> + uint64_t size;
> + ConfidentialGuestMemoryType type;
> +} ConfidentialGuestMemoryMapEntry;
> +
> +typedef enum ConfidentialGuestPageType {
> + CGS_PAGE_TYPE_NORMAL,
> + CGS_PAGE_TYPE_VMSA,
> + CGS_PAGE_TYPE_ZERO,
> + CGS_PAGE_TYPE_UNMEASURED,
> + CGS_PAGE_TYPE_SECRETS,
> + CGS_PAGE_TYPE_CPUID,
> + CGS_PAGE_TYPE_REQUIRED_MEMORY,
> +} ConfidentialGuestPageType;
> +
> struct ConfidentialGuestSupport {
> Object parent;
>
> @@ -60,6 +96,46 @@ struct ConfidentialGuestSupport {
> */
> char *igvm_filename;
> #endif
> +
> + /*
> + * The following virtual methods need to be implemented by systems that
> + * support confidential guests that can be configured with IGVM and are
> + * used during processing of the IGVM file with process_igvm().
> + */
> +
> + /*
> + * Check for to see if this confidential guest supports a particular
> + * platform or configuration
> + */
> + int (*check_support)(ConfidentialGuestPlatformType platform,
> + uint16_t platform_version, uint8_t highest_vtl,
> + uint64_t shared_gpa_boundary);
> +
> + /*
> + * Configure part of the state of a guest for a particular set of data, page
> + * type and gpa. This can be used for example to pre-populate and measure
> + * guest memory contents, define private ranges or set the initial CPU state
> + * for one or more CPUs.
> + *
> + * If memory_type is CGS_PAGE_TYPE_VMSA then ptr points to the initial CPU
> + * context for a virtual CPU. The format of the data depends on the type of
> + * confidential virtual machine. For example, for SEV-ES ptr will point to a
> + * vmcb_save_area structure that should be copied into guest memory at the
> + * address specified in gpa. The cpu_index parameter contains the index of
> + * the CPU the VMSA applies to.
> + */
> + int (*set_guest_state)(hwaddr gpa, uint8_t *ptr, uint64_t len,
> + ConfidentialGuestPageType memory_type,
> + uint16_t cpu_index);
> +
> + /*
> + * Iterate the system memory map, getting the entry with the given index
> + * that can be populated into guest memory.
> + *
> + * Returns 1 if the index is out of range.
> + */
> + int (*get_mem_map_entry)(int index,
> + ConfidentialGuestMemoryMapEntry *entry);
> };
>
> typedef struct ConfidentialGuestSupportClass {
> --
> 2.43.0
>
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 3/9] backends/confidential-guest-support: Add functions to support IGVM
2024-03-01 16:37 ` Daniel P. Berrangé
@ 2024-03-12 11:43 ` Roy Hopkins
0 siblings, 0 replies; 35+ messages in thread
From: Roy Hopkins @ 2024-03-12 11:43 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
On Fri, 2024-03-01 at 16:37 +0000, Daniel P. Berrangé wrote:
> On Tue, Feb 27, 2024 at 02:50:09PM +0000, Roy Hopkins wrote:
> > In preparation for supporting the processing of IGVM files to configure
> > guests, this adds a set of functions to ConfidentialGuestSupport
> > allowing configuration of secure virtual machines that can be
> > implemented for each supported isolation platform type such as Intel TDX
> > or AMD SEV-SNP. These functions will be called by IGVM processing code
> > in subsequent patches.
> >
> > This commit provides a default implementation of the functions that
> > either perform no action or generate a warning or error when they are
> > called. Targets that support ConfidentalGuestSupport should override
> > these implementations.
> >
> > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> > ---
> > backends/confidential-guest-support.c | 26 ++++++++
> > include/exec/confidential-guest-support.h | 76 +++++++++++++++++++++++
> > 2 files changed, 102 insertions(+)
> >
> > diff --git a/backends/confidential-guest-support.c b/backends/confidential-
> > guest-support.c
> > index da436fb736..42628be8d7 100644
> > --- a/backends/confidential-guest-support.c
> > +++ b/backends/confidential-guest-support.c
> > @@ -14,6 +14,7 @@
> > #include "qemu/osdep.h"
> >
> > #include "exec/confidential-guest-support.h"
> > +#include "qemu/error-report.h"
> >
> > OBJECT_DEFINE_ABSTRACT_TYPE(ConfidentialGuestSupport,
> > confidential_guest_support,
> > @@ -45,8 +46,33 @@ static void
> > confidential_guest_support_class_init(ObjectClass *oc, void *data)
> > #endif
> > }
> >
> > +static int check_support(ConfidentialGuestPlatformType platform,
> > + uint16_t platform_version, uint8_t highest_vtl,
> > + uint64_t shared_gpa_boundary)
> > +{
> > + /* Default: no support. */
> > + return 0;
> > +}
> > +
> > +static int set_guest_state(hwaddr gpa, uint8_t *ptr, uint64_t len,
> > + ConfidentialGuestPageType memory_type,
> > + uint16_t cpu_index)
> > +{
> > + warn_report("Confidential guest memory not supported");
> > + return -1;
> > +}
> > +
> > +static int get_mem_map_entry(int index, ConfidentialGuestMemoryMapEntry
> > *entry)
> > +{
> > + return 1;
> > +}
>
> IIUC, all these can reports errors, and as such I think
> they should have an 'Error **errp' parameter, so we can
> report precise errors in these methods, rather than
> less useful generic errors in the caller.
>
> The above 'warn_report' ought to be an error too, since
> it is returning an failure code (-1)
Yes, that makes sense. I've made a comprehensive rework of the error handling in
the patch series which addresses this and your suggestions in the other patches
regarding error handling. I'll submit these as a V2.
>
> > +
> > static void confidential_guest_support_init(Object *obj)
> > {
> > + ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
> > + cgs->check_support = check_support;
> > + cgs->set_guest_state = set_guest_state;
> > + cgs->get_mem_map_entry = get_mem_map_entry;
> > }
> >
> > static void confidential_guest_support_finalize(Object *obj)
> > diff --git a/include/exec/confidential-guest-support.h
> > b/include/exec/confidential-guest-support.h
> > index b08ad8de4d..c43a1a32f1 100644
> > --- a/include/exec/confidential-guest-support.h
> > +++ b/include/exec/confidential-guest-support.h
> > @@ -21,10 +21,46 @@
> > #ifndef CONFIG_USER_ONLY
> >
> > #include "qom/object.h"
> > +#include "exec/hwaddr.h"
> > +
> > +#if defined(CONFIG_IGVM)
> > +#include "igvm/igvm.h"
> > +#endif
> >
> > #define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
> > OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport,
> > CONFIDENTIAL_GUEST_SUPPORT)
> >
> > +typedef enum ConfidentialGuestPlatformType {
> > + CGS_PLATFORM_SEV,
> > + CGS_PLATFORM_SEV_ES,
> > + CGS_PLATFORM_SEV_SNP,
> > + CGS_PLATFORM_TDX,
>
> QEMU only has support for SEV & SEV_ES today. We should leave the
> others until we actually get an impl of SEV-SNP/TDX in QEMU that
> supports those platforms.
Ok. I'll remove the currently unsupported platforms.
>
> > +} ConfidentialGuestPlatformType;
> > +
> > +typedef enum ConfidentialGuestMemoryType {
> > + CGS_MEM_RAM,
> > + CGS_MEM_RESERVED,
> > + CGS_MEM_ACPI,
> > + CGS_MEM_NVS,
> > + CGS_MEM_UNUSABLE,
> > +} ConfidentialGuestMemoryType;
> > +
> > +typedef struct ConfidentialGuestMemoryMapEntry {
> > + uint64_t gpa;
> > + uint64_t size;
> > + ConfidentialGuestMemoryType type;
> > +} ConfidentialGuestMemoryMapEntry;
> > +
> > +typedef enum ConfidentialGuestPageType {
> > + CGS_PAGE_TYPE_NORMAL,
> > + CGS_PAGE_TYPE_VMSA,
> > + CGS_PAGE_TYPE_ZERO,
> > + CGS_PAGE_TYPE_UNMEASURED,
> > + CGS_PAGE_TYPE_SECRETS,
> > + CGS_PAGE_TYPE_CPUID,
> > + CGS_PAGE_TYPE_REQUIRED_MEMORY,
> > +} ConfidentialGuestPageType;
> > +
> > struct ConfidentialGuestSupport {
> > Object parent;
> >
> > @@ -60,6 +96,46 @@ struct ConfidentialGuestSupport {
> > */
> > char *igvm_filename;
> > #endif
> > +
> > + /*
> > + * The following virtual methods need to be implemented by systems that
> > + * support confidential guests that can be configured with IGVM and are
> > + * used during processing of the IGVM file with process_igvm().
> > + */
> > +
> > + /*
> > + * Check for to see if this confidential guest supports a particular
> > + * platform or configuration
> > + */
> > + int (*check_support)(ConfidentialGuestPlatformType platform,
> > + uint16_t platform_version, uint8_t highest_vtl,
> > + uint64_t shared_gpa_boundary);
> > +
> > + /*
> > + * Configure part of the state of a guest for a particular set of data,
> > page
> > + * type and gpa. This can be used for example to pre-populate and
> > measure
> > + * guest memory contents, define private ranges or set the initial CPU
> > state
> > + * for one or more CPUs.
> > + *
> > + * If memory_type is CGS_PAGE_TYPE_VMSA then ptr points to the initial
> > CPU
> > + * context for a virtual CPU. The format of the data depends on the
> > type of
> > + * confidential virtual machine. For example, for SEV-ES ptr will point
> > to a
> > + * vmcb_save_area structure that should be copied into guest memory at
> > the
> > + * address specified in gpa. The cpu_index parameter contains the index
> > of
> > + * the CPU the VMSA applies to.
> > + */
> > + int (*set_guest_state)(hwaddr gpa, uint8_t *ptr, uint64_t len,
> > + ConfidentialGuestPageType memory_type,
> > + uint16_t cpu_index);
> > +
> > + /*
> > + * Iterate the system memory map, getting the entry with the given
> > index
> > + * that can be populated into guest memory.
> > + *
> > + * Returns 1 if the index is out of range.
> > + */
> > + int (*get_mem_map_entry)(int index,
> > + ConfidentialGuestMemoryMapEntry *entry);
> > };
> >
> > typedef struct ConfidentialGuestSupportClass {
> > --
> > 2.43.0
> >
> >
>
> With regards,
> Daniel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 4/9] backends/igvm: Implement parsing and processing of IGVM files
2024-02-27 14:50 [PATCH 0/9] Introduce support for IGVM files Roy Hopkins
` (2 preceding siblings ...)
2024-02-27 14:50 ` [PATCH 3/9] backends/confidential-guest-support: Add functions to support IGVM Roy Hopkins
@ 2024-02-27 14:50 ` Roy Hopkins
2024-03-01 16:51 ` Daniel P. Berrangé
2024-02-27 14:50 ` [PATCH 5/9] i386/pc: Process IGVM file during PC initialization if present Roy Hopkins
` (6 subsequent siblings)
10 siblings, 1 reply; 35+ messages in thread
From: Roy Hopkins @ 2024-02-27 14:50 UTC (permalink / raw)
To: qemu-devel
Cc: Roy Hopkins, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
This commit adds an implementation of an IGVM loader which parses the
file specified as a pararameter to ConfidentialGuestSupport and provides
a function that uses the interface in the same object to configure and
populate guest memory based on the contents of the file.
The IGVM file is parsed when a filename is provided but the code to
process the IGVM file is not yet hooked into target systems. This will
follow in a later commit.
Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
backends/confidential-guest-support.c | 4 +
backends/igvm.c | 718 ++++++++++++++++++++++
backends/meson.build | 1 +
include/exec/confidential-guest-support.h | 5 +
include/exec/igvm.h | 35 ++
5 files changed, 763 insertions(+)
create mode 100644 backends/igvm.c
create mode 100644 include/exec/igvm.h
diff --git a/backends/confidential-guest-support.c b/backends/confidential-guest-support.c
index 42628be8d7..cb7651a8d0 100644
--- a/backends/confidential-guest-support.c
+++ b/backends/confidential-guest-support.c
@@ -15,6 +15,7 @@
#include "exec/confidential-guest-support.h"
#include "qemu/error-report.h"
+#include "exec/igvm.h"
OBJECT_DEFINE_ABSTRACT_TYPE(ConfidentialGuestSupport,
confidential_guest_support,
@@ -33,6 +34,9 @@ static void set_igvm(Object *obj, const char *value, Error **errp)
ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
g_free(cgs->igvm_filename);
cgs->igvm_filename = g_strdup(value);
+#if defined(CONFIG_IGVM)
+ igvm_file_init(cgs);
+#endif
}
#endif
diff --git a/backends/igvm.c b/backends/igvm.c
new file mode 100644
index 0000000000..a6261d796f
--- /dev/null
+++ b/backends/igvm.c
@@ -0,0 +1,718 @@
+/*
+ * QEMU IGVM configuration backend for Confidential Guests
+ *
+ * Copyright (C) 2023-2024 SUSE
+ *
+ * Authors:
+ * Roy Hopkins <roy.hopkins@suse.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"
+
+#if defined(CONFIG_IGVM)
+
+#include "exec/confidential-guest-support.h"
+#include "qemu/queue.h"
+#include "qemu/typedefs.h"
+
+#include "exec/igvm.h"
+#include "qemu/error-report.h"
+#include "hw/boards.h"
+#include "qapi/error.h"
+#include "exec/address-spaces.h"
+
+#include <igvm/igvm.h>
+#include <igvm/igvm_defs.h>
+#include <linux/kvm.h>
+
+typedef struct IgvmParameterData {
+ QTAILQ_ENTRY(IgvmParameterData) next;
+ uint8_t *data;
+ uint32_t size;
+ uint32_t index;
+} IgvmParameterData;
+
+static QTAILQ_HEAD(, IgvmParameterData) parameter_data;
+
+static void directive_page_data(ConfidentialGuestSupport *cgs, int i,
+ uint32_t compatibility_mask);
+static void directive_vp_context(ConfidentialGuestSupport *cgs, int i,
+ uint32_t compatibility_mask);
+static void directive_parameter_area(ConfidentialGuestSupport *cgs, int i,
+ uint32_t compatibility_mask);
+static void directive_parameter_insert(ConfidentialGuestSupport *cgs, int i,
+ uint32_t compatibility_mask);
+static void directive_memory_map(ConfidentialGuestSupport *cgs, int i,
+ uint32_t compatibility_mask);
+static void directive_vp_count(ConfidentialGuestSupport *cgs, int i,
+ uint32_t compatibility_mask);
+static void directive_environment_info(ConfidentialGuestSupport *cgs, int i,
+ uint32_t compatibility_mask);
+static void directive_required_memory(ConfidentialGuestSupport *cgs, int i,
+ uint32_t compatibility_mask);
+
+struct IGVMDirectiveHandler {
+ uint32_t type;
+ void (*handler)(ConfidentialGuestSupport *cgs, int i,
+ uint32_t compatibility_mask);
+};
+
+static struct IGVMDirectiveHandler directive_handlers[] = {
+ { IGVM_VHT_PAGE_DATA, directive_page_data },
+ { IGVM_VHT_VP_CONTEXT, directive_vp_context },
+ { IGVM_VHT_PARAMETER_AREA, directive_parameter_area },
+ { IGVM_VHT_PARAMETER_INSERT, directive_parameter_insert },
+ { IGVM_VHT_MEMORY_MAP, directive_memory_map },
+ { IGVM_VHT_VP_COUNT_PARAMETER, directive_vp_count },
+ { IGVM_VHT_ENVIRONMENT_INFO_PARAMETER, directive_environment_info },
+ { IGVM_VHT_REQUIRED_MEMORY, directive_required_memory },
+};
+
+static void directive(uint32_t type, ConfidentialGuestSupport *cgs, int i,
+ uint32_t compatibility_mask)
+{
+ size_t handler;
+ for (handler = 0; handler < (sizeof(directive_handlers) /
+ sizeof(struct IGVMDirectiveHandler));
+ ++handler) {
+ if (directive_handlers[handler].type == type) {
+ directive_handlers[handler].handler(cgs, i, compatibility_mask);
+ return;
+ }
+ }
+ warn_report("IGVM: Unknown directive encountered when processing file: %X",
+ type);
+}
+
+static void igvm_handle_error(int32_t result, const char *msg)
+{
+ if (result < 0) {
+ error_report("Processing of IGVM file failed: %s (code: %d)", msg,
+ (int)result);
+ exit(EXIT_FAILURE);
+ }
+}
+
+static void *igvm_prepare_memory(uint64_t addr, uint64_t size,
+ int region_identifier)
+{
+ MemoryRegion *igvm_pages = NULL;
+ Int128 gpa_region_size;
+ MemoryRegionSection mrs =
+ memory_region_find(get_system_memory(), addr, size);
+ if (mrs.mr) {
+ if (!memory_region_is_ram(mrs.mr)) {
+ memory_region_unref(mrs.mr);
+ error_report(
+ "Processing of IGVM file failed: Could not prepare memory "
+ "at address 0x%lX due to existing non-RAM region",
+ addr);
+ exit(EXIT_FAILURE);
+ }
+
+ gpa_region_size = int128_make64(size);
+ if (int128_lt(mrs.size, gpa_region_size)) {
+ memory_region_unref(mrs.mr);
+ error_report(
+ "Processing of IGVM file failed: Could not prepare memory "
+ "at address 0x%lX: region size exceeded",
+ addr);
+ exit(EXIT_FAILURE);
+ }
+ return qemu_map_ram_ptr(mrs.mr->ram_block, mrs.offset_within_region);
+ } else {
+ /*
+ * The region_identifier is the is the index of the IGVM directive that
+ * contains the page with the lowest GPA in the region. This will
+ * generate a unique region name.
+ */
+ char region_name[22];
+ snprintf(region_name, sizeof(region_name), "igvm.%X",
+ region_identifier);
+ igvm_pages = g_malloc(sizeof(*igvm_pages));
+ memory_region_init_ram(igvm_pages, NULL, region_name, size,
+ &error_fatal);
+ memory_region_add_subregion(get_system_memory(), addr, igvm_pages);
+ return memory_region_get_ram_ptr(igvm_pages);
+ }
+}
+
+static int igvm_type_to_cgs_type(IgvmPageDataType memory_type, bool unmeasured,
+ bool zero)
+{
+ switch (memory_type) {
+ case NORMAL: {
+ if (unmeasured) {
+ return CGS_PAGE_TYPE_UNMEASURED;
+ } else {
+ return zero ? CGS_PAGE_TYPE_ZERO :
+ CGS_PAGE_TYPE_NORMAL;
+ }
+ }
+ case SECRETS:
+ return CGS_PAGE_TYPE_SECRETS;
+ case CPUID_DATA:
+ return CGS_PAGE_TYPE_CPUID;
+ case CPUID_XF:
+ return CGS_PAGE_TYPE_CPUID;
+ default:
+ return CGS_PAGE_TYPE_UNMEASURED;
+ }
+}
+
+static bool page_attrs_equal(const IGVM_VHS_PAGE_DATA *page_1,
+ const IGVM_VHS_PAGE_DATA *page_2)
+{
+ return ((*(const uint32_t *)&page_1->flags ==
+ *(const uint32_t *)&page_2->flags) &&
+ (page_1->data_type == page_2->data_type) &&
+ (page_1->compatibility_mask == page_2->compatibility_mask));
+}
+
+static void igvm_process_mem_region(ConfidentialGuestSupport *cgs,
+ IgvmHandle igvm, int start_index,
+ uint64_t gpa_start, int page_count,
+ const IgvmPageDataFlags *flags,
+ const IgvmPageDataType page_type)
+{
+ uint8_t *region;
+ IgvmHandle data_handle;
+ const void *data;
+ uint32_t data_size;
+ int i;
+ bool zero = true;
+ const uint64_t page_size = flags->is_2mb_page ? 0x200000 : 0x1000;
+ int result;
+
+ region = igvm_prepare_memory(gpa_start, page_count * page_size,
+ start_index);
+
+ for (i = 0; i < page_count; ++i) {
+ data_handle = igvm_get_header_data(igvm, HEADER_SECTION_DIRECTIVE,
+ i + start_index);
+ if (data_handle == IGVMAPI_NO_DATA) {
+ /* No data indicates a zero page */
+ memset(®ion[i * page_size], 0, page_size);
+ } else if (data_handle < 0) {
+ igvm_handle_error(data_handle, "Invalid file");
+ } else {
+ zero = false;
+ data = igvm_get_buffer(igvm, data_handle);
+ data_size = igvm_get_buffer_size(igvm, data_handle);
+ if (data_size < page_size) {
+ memset(®ion[i * page_size], 0, page_size);
+ } else if (data_size > page_size) {
+ igvm_handle_error(data_handle, "Invalid page data in file");
+ }
+ memcpy(®ion[i * page_size], data, data_size);
+ igvm_free_buffer(igvm, data_handle);
+ }
+ }
+
+ result = cgs->set_guest_state(
+ gpa_start, region, page_size * page_count,
+ igvm_type_to_cgs_type(page_type, flags->unmeasured, zero), 0);
+ if (result != 0) {
+ error_report("IGVM set guest state failed with code %d", result);
+ exit(EXIT_FAILURE);
+ }
+}
+
+static void process_mem_page(ConfidentialGuestSupport *cgs, int i,
+ const IGVM_VHS_PAGE_DATA *page_data)
+{
+ static IGVM_VHS_PAGE_DATA prev_page_data;
+ static uint64_t region_start;
+ static int last_i;
+ static int page_count;
+
+ if (page_data) {
+ if (page_count == 0) {
+ region_start = page_data->gpa;
+ } else {
+ if (!page_attrs_equal(page_data, &prev_page_data) ||
+ ((prev_page_data.gpa +
+ (prev_page_data.flags.is_2mb_page ? 0x200000 : 0x1000)) !=
+ page_data->gpa) ||
+ (last_i != (i - 1))) {
+ /* End of current region */
+ igvm_process_mem_region(cgs, cgs->igvm, i - page_count,
+ region_start, page_count,
+ &prev_page_data.flags,
+ prev_page_data.data_type);
+ page_count = 0;
+ region_start = page_data->gpa;
+ }
+ }
+ memcpy(&prev_page_data, page_data, sizeof(prev_page_data));
+ last_i = i;
+ ++page_count;
+ } else {
+ if (page_count > 0) {
+ igvm_process_mem_region(cgs, cgs->igvm, i - page_count,
+ region_start, page_count,
+ &prev_page_data.flags,
+ prev_page_data.data_type);
+ page_count = 0;
+ }
+ }
+}
+
+static void directive_page_data(ConfidentialGuestSupport *cgs, int i,
+ uint32_t compatibility_mask)
+{
+ IgvmHandle header_handle;
+ const IGVM_VHS_PAGE_DATA *page_data;
+
+ header_handle = igvm_get_header(cgs->igvm, HEADER_SECTION_DIRECTIVE, i);
+ igvm_handle_error(header_handle,
+ "Failed to read directive header from file");
+ page_data =
+ (IGVM_VHS_PAGE_DATA *)(igvm_get_buffer(cgs->igvm, header_handle) +
+ sizeof(IGVM_VHS_VARIABLE_HEADER));
+
+ if (page_data->compatibility_mask == compatibility_mask) {
+ process_mem_page(cgs, i, page_data);
+ }
+ igvm_free_buffer(cgs->igvm, header_handle);
+}
+
+static void directive_vp_context(ConfidentialGuestSupport *cgs, int i,
+ uint32_t compatibility_mask)
+{
+ IgvmHandle header_handle;
+ IGVM_VHS_VP_CONTEXT *vp_context;
+ IgvmHandle data_handle;
+ uint8_t *data;
+ int result;
+
+ header_handle = igvm_get_header(cgs->igvm, HEADER_SECTION_DIRECTIVE, i);
+ igvm_handle_error(header_handle,
+ "Failed to read directive header from file");
+ vp_context =
+ (IGVM_VHS_VP_CONTEXT *)(igvm_get_buffer(cgs->igvm, header_handle) +
+ sizeof(IGVM_VHS_VARIABLE_HEADER));
+
+ if (vp_context->compatibility_mask == compatibility_mask) {
+ data_handle =
+ igvm_get_header_data(cgs->igvm, HEADER_SECTION_DIRECTIVE, i);
+ igvm_handle_error(data_handle,
+ "Failed to read directive data from file");
+
+ data = (uint8_t *)igvm_get_buffer(cgs->igvm, data_handle);
+ result = cgs->set_guest_state(vp_context->gpa, data,
+ igvm_get_buffer_size(cgs->igvm, data_handle),
+ CGS_PAGE_TYPE_VMSA,
+ vp_context->vp_index);
+ igvm_free_buffer(cgs->igvm, data_handle);
+ if (result != 0) {
+ error_report(
+ "IGVM: Failed to set CPU context: error_code=%d",
+ result);
+ exit(EXIT_FAILURE);
+ }
+
+ }
+
+ igvm_free_buffer(cgs->igvm, header_handle);
+}
+
+static void directive_parameter_area(ConfidentialGuestSupport *cgs, int i,
+ uint32_t compatibility_mask)
+{
+ IgvmHandle header_handle;
+ IGVM_VHS_PARAMETER_AREA *param_area;
+ IgvmParameterData *param_entry;
+
+ header_handle = igvm_get_header(cgs->igvm, HEADER_SECTION_DIRECTIVE, i);
+ igvm_handle_error(header_handle,
+ "Failed to read directive header from file");
+ param_area =
+ (IGVM_VHS_PARAMETER_AREA *)(igvm_get_buffer(cgs->igvm, header_handle) +
+ sizeof(IGVM_VHS_VARIABLE_HEADER));
+
+ param_entry = g_new0(IgvmParameterData, 1);
+ param_entry->size = param_area->number_of_bytes;
+ param_entry->index = param_area->parameter_area_index;
+ param_entry->data = g_malloc0(param_entry->size);
+
+ QTAILQ_INSERT_TAIL(¶meter_data, param_entry, next);
+
+ igvm_free_buffer(cgs->igvm, header_handle);
+}
+
+static void directive_parameter_insert(ConfidentialGuestSupport *cgs, int i,
+ uint32_t compatibility_mask)
+{
+ IgvmHandle header_handle;
+ IGVM_VHS_PARAMETER_INSERT *param;
+ IgvmParameterData *param_entry;
+ int result;
+ void *region;
+
+ header_handle = igvm_get_header(cgs->igvm, HEADER_SECTION_DIRECTIVE, i);
+ igvm_handle_error(header_handle,
+ "Failed to read directive header from file");
+ param = (IGVM_VHS_PARAMETER_INSERT *)(igvm_get_buffer(cgs->igvm,
+ header_handle) +
+ sizeof(IGVM_VHS_VARIABLE_HEADER));
+
+ QTAILQ_FOREACH(param_entry, ¶meter_data, next)
+ {
+ if (param_entry->index == param->parameter_area_index) {
+ region = igvm_prepare_memory(param->gpa, param_entry->size, i);
+ if (!region) {
+ error_report(
+ "IGVM: Failed to allocate guest memory region for parameters");
+ exit(EXIT_FAILURE);
+ }
+ memcpy(region, param_entry->data, param_entry->size);
+ g_free(param_entry->data);
+ param_entry->data = NULL;
+
+ result = cgs->set_guest_state(param->gpa, region,
+ param_entry->size,
+ CGS_PAGE_TYPE_UNMEASURED, 0);
+ if (result != 0) {
+ error_report(
+ "IGVM: Failed to set guest state: error_code=%d",
+ result);
+ exit(EXIT_FAILURE);
+ }
+ break;
+ }
+ }
+}
+
+static int cmp_mm_entry(const void *a, const void *b)
+{
+ const IGVM_VHS_MEMORY_MAP_ENTRY *entry_a =
+ (const IGVM_VHS_MEMORY_MAP_ENTRY *)a;
+ const IGVM_VHS_MEMORY_MAP_ENTRY *entry_b =
+ (const IGVM_VHS_MEMORY_MAP_ENTRY *)b;
+ if (entry_a->starting_gpa_page_number < entry_b->starting_gpa_page_number) {
+ return -1;
+ } else if (entry_a->starting_gpa_page_number >
+ entry_b->starting_gpa_page_number) {
+ return 1;
+ } else {
+ return 0;
+ }
+}
+
+static void directive_memory_map(ConfidentialGuestSupport *cgs, int i,
+ uint32_t compatibility_mask)
+{
+ IgvmHandle header_handle;
+ IGVM_VHS_PARAMETER *param;
+ IgvmParameterData *param_entry;
+ int max_entry_count;
+ int entry = 0;
+ IGVM_VHS_MEMORY_MAP_ENTRY *mm_entry;
+ ConfidentialGuestMemoryMapEntry cgmm_entry;
+
+ header_handle = igvm_get_header(cgs->igvm, HEADER_SECTION_DIRECTIVE, i);
+ igvm_handle_error(header_handle,
+ "Failed to read directive header from file");
+ param = (IGVM_VHS_PARAMETER *)(igvm_get_buffer(cgs->igvm, header_handle) +
+ sizeof(IGVM_VHS_VARIABLE_HEADER));
+
+ /* Find the parameter area that should hold the memory map */
+ QTAILQ_FOREACH(param_entry, ¶meter_data, next)
+ {
+ if (param_entry->index == param->parameter_area_index) {
+ max_entry_count = param_entry->size /
+ sizeof(IGVM_VHS_MEMORY_MAP_ENTRY);
+ mm_entry = (IGVM_VHS_MEMORY_MAP_ENTRY *)param_entry->data;
+
+ while (cgs->get_mem_map_entry(entry, &cgmm_entry) == 0) {
+ if (entry > max_entry_count) {
+ error_report(
+ "IGVM: guest memory map size exceeds parameter area defined in IGVM file");
+ exit(EXIT_FAILURE);
+ }
+ mm_entry[entry].starting_gpa_page_number = cgmm_entry.gpa >> 12;
+ mm_entry[entry].number_of_pages = cgmm_entry.size >> 12;
+
+ switch (cgmm_entry.type) {
+ case CGS_MEM_RAM:
+ mm_entry[entry].entry_type = MEMORY;
+ break;
+ case CGS_MEM_RESERVED:
+ mm_entry[entry].entry_type = PLATFORM_RESERVED;
+ break;
+ case CGS_MEM_ACPI:
+ mm_entry[entry].entry_type = PLATFORM_RESERVED;
+ break;
+ case CGS_MEM_NVS:
+ mm_entry[entry].entry_type = PERSISTENT;
+ break;
+ case CGS_MEM_UNUSABLE:
+ mm_entry[entry].entry_type = PLATFORM_RESERVED;
+ break;
+ }
+ ++entry;
+ }
+ /* The entries need to be sorted */
+ qsort(mm_entry, entry, sizeof(IGVM_VHS_MEMORY_MAP_ENTRY),
+ cmp_mm_entry);
+
+ break;
+ }
+ }
+
+ igvm_free_buffer(cgs->igvm, header_handle);
+}
+
+static void directive_vp_count(ConfidentialGuestSupport *cgs, int i,
+ uint32_t compatibility_mask)
+{
+ IgvmHandle header_handle;
+ IGVM_VHS_PARAMETER *param;
+ IgvmParameterData *param_entry;
+ uint32_t *vp_count;
+ CPUState *cpu;
+
+ header_handle = igvm_get_header(cgs->igvm, HEADER_SECTION_DIRECTIVE, i);
+ igvm_handle_error(header_handle,
+ "Failed to read directive header from file");
+ param = (IGVM_VHS_PARAMETER *)(igvm_get_buffer(cgs->igvm, header_handle) +
+ sizeof(IGVM_VHS_VARIABLE_HEADER));
+
+ QTAILQ_FOREACH(param_entry, ¶meter_data, next)
+ {
+ if (param_entry->index == param->parameter_area_index) {
+ vp_count = (uint32_t *)(param_entry->data + param->byte_offset);
+ *vp_count = 0;
+ CPU_FOREACH(cpu)
+ {
+ (*vp_count)++;
+ }
+ break;
+ }
+ }
+
+ igvm_free_buffer(cgs->igvm, header_handle);
+}
+
+static void directive_environment_info(ConfidentialGuestSupport *cgs, int i,
+ uint32_t compatibility_mask)
+{
+ IgvmHandle header_handle;
+ IGVM_VHS_PARAMETER *param;
+ IgvmParameterData *param_entry;
+ IgvmEnvironmentInfo *environmental_state;
+
+ header_handle = igvm_get_header(cgs->igvm, HEADER_SECTION_DIRECTIVE, i);
+ igvm_handle_error(header_handle,
+ "Failed to read directive header from file");
+ param = (IGVM_VHS_PARAMETER *)(igvm_get_buffer(cgs->igvm, header_handle) +
+ sizeof(IGVM_VHS_VARIABLE_HEADER));
+
+ QTAILQ_FOREACH(param_entry, ¶meter_data, next)
+ {
+ if (param_entry->index == param->parameter_area_index) {
+ environmental_state =
+ (IgvmEnvironmentInfo *)(param_entry->data + param->byte_offset);
+ environmental_state->memory_is_shared = 1;
+ break;
+ }
+ }
+
+ igvm_free_buffer(cgs->igvm, header_handle);
+}
+
+static void directive_required_memory(ConfidentialGuestSupport *cgs, int i,
+ uint32_t compatibility_mask)
+{
+ IgvmHandle header_handle;
+ const IGVM_VHS_REQUIRED_MEMORY *mem;
+ uint8_t *region;
+ int result;
+
+ header_handle = igvm_get_header(cgs->igvm, HEADER_SECTION_DIRECTIVE, i);
+ igvm_handle_error(header_handle,
+ "Failed to read directive header from file");
+ mem =
+ (IGVM_VHS_REQUIRED_MEMORY *)(igvm_get_buffer(cgs->igvm, header_handle) +
+ sizeof(IGVM_VHS_VARIABLE_HEADER));
+
+ if (mem->compatibility_mask == compatibility_mask) {
+ region = igvm_prepare_memory(mem->gpa, mem->number_of_bytes, i);
+ result = cgs->set_guest_state(mem->gpa, region,
+ mem->number_of_bytes,
+ CGS_PAGE_TYPE_REQUIRED_MEMORY, 0);
+ if (result != 0) {
+ error_report("IGVM: Failed to set guest state: error_code=%d",
+ result);
+ exit(EXIT_FAILURE);
+ }
+ }
+ igvm_free_buffer(cgs->igvm, header_handle);
+}
+
+static uint32_t supported_platform_compat_mask(ConfidentialGuestSupport *cgs)
+{
+ int32_t result;
+ int i;
+ IgvmHandle header_handle;
+ IGVM_VHS_SUPPORTED_PLATFORM *platform;
+ uint32_t compatibility_mask = 0;
+
+ result = igvm_header_count(cgs->igvm, HEADER_SECTION_PLATFORM);
+ igvm_handle_error(result, "Failed to read platform header count");
+
+ for (i = 0; i < (int)result; ++i) {
+ IgvmVariableHeaderType typ =
+ igvm_get_header_type(cgs->igvm, HEADER_SECTION_PLATFORM, i);
+ if (typ == IGVM_VHT_SUPPORTED_PLATFORM) {
+ header_handle =
+ igvm_get_header(cgs->igvm, HEADER_SECTION_PLATFORM, i);
+ igvm_handle_error(header_handle,
+ "Failed to read platform header from file");
+ platform =
+ (IGVM_VHS_SUPPORTED_PLATFORM *)(igvm_get_buffer(cgs->igvm,
+ header_handle) +
+ sizeof(
+ IGVM_VHS_VARIABLE_HEADER));
+ /* Currently only support SEV-SNP. */
+ if (platform->platform_type == SEV_SNP) {
+ /*
+ * IGVM does not define a platform types of SEV or SEV_ES.
+ * Translate SEV_SNP into CGS_PLATFORM_SEV_ES and
+ * CGS_PLATFORM_SEV and let the cgs function implementations
+ * check whether each IGVM directive results in an operation
+ * that is supported by the particular derivative of SEV.
+ */
+ if (cgs->check_support(
+ CGS_PLATFORM_SEV_SNP, platform->platform_version,
+ platform->highest_vtl, platform->shared_gpa_boundary) ||
+ cgs->check_support(
+ CGS_PLATFORM_SEV_ES, platform->platform_version,
+ platform->highest_vtl, platform->shared_gpa_boundary) ||
+ cgs->check_support(
+ CGS_PLATFORM_SEV, platform->platform_version,
+ platform->highest_vtl, platform->shared_gpa_boundary)) {
+ compatibility_mask = platform->compatibility_mask;
+ break;
+ }
+ } else if (platform->platform_type == TDX) {
+ if (cgs->check_support(
+ CGS_PLATFORM_TDX, platform->platform_version,
+ platform->highest_vtl, platform->shared_gpa_boundary)) {
+ compatibility_mask = platform->compatibility_mask;
+ break;
+ }
+ }
+ igvm_free_buffer(cgs->igvm, header_handle);
+ }
+ }
+ return compatibility_mask;
+}
+
+void igvm_file_init(ConfidentialGuestSupport *cgs)
+{
+ FILE *igvm_file = NULL;
+ uint8_t *igvm_buf = NULL;
+
+ if (cgs->igvm_filename) {
+ IgvmHandle igvm;
+ unsigned long igvm_length;
+
+ igvm_file = fopen(cgs->igvm_filename, "rb");
+ if (!igvm_file) {
+ error_report("IGVM file not found '%s'", cgs->igvm_filename);
+ goto error_out;
+ }
+
+ fseek(igvm_file, 0, SEEK_END);
+ igvm_length = ftell(igvm_file);
+ fseek(igvm_file, 0, SEEK_SET);
+
+ igvm_buf = g_new(uint8_t, igvm_length);
+ if (!igvm_buf) {
+ error_report(
+ "Could not allocate buffer to read file IGVM file '%s'",
+ cgs->igvm_filename);
+ goto error_out;
+ }
+ if (fread(igvm_buf, 1, igvm_length, igvm_file) != igvm_length) {
+ error_report("Unable to load IGVM file '%s'", cgs->igvm_filename);
+ goto error_out;
+ }
+
+ igvm = igvm_new_from_binary(igvm_buf, igvm_length);
+ if (igvm < 0) {
+ error_report("Parsing IGVM file '%s' failed with error_code %d",
+ cgs->igvm_filename, igvm);
+ goto error_out;
+ }
+ fclose(igvm_file);
+ g_free(igvm_buf);
+
+ cgs->igvm = igvm;
+ }
+ return;
+
+error_out:
+ free(igvm_buf);
+ if (igvm_file) {
+ fclose(igvm_file);
+ }
+ exit(EXIT_FAILURE);
+}
+
+void igvm_process(ConfidentialGuestSupport *cgs)
+{
+ int32_t result;
+ int i;
+ uint32_t compatibility_mask;
+ IgvmParameterData *parameter;
+
+ /*
+ * If this is not a Confidential guest or no IGVM has been provided then
+ * this is a no-op.
+ */
+ if (!cgs || !cgs->igvm) {
+ return;
+ }
+
+ QTAILQ_INIT(¶meter_data);
+
+ /*
+ * Check that the IGVM file provides configuration for the current
+ * platform
+ */
+ compatibility_mask = supported_platform_compat_mask(cgs);
+ if (compatibility_mask == 0) {
+ error_report(
+ "IGVM file does not describe a compatible supported platform");
+ exit(EXIT_FAILURE);
+ }
+
+ result = igvm_header_count(cgs->igvm, HEADER_SECTION_DIRECTIVE);
+ igvm_handle_error(result, "Failed to read directive header count");
+ for (i = 0; i < (int)result; ++i) {
+ IgvmVariableHeaderType type =
+ igvm_get_header_type(cgs->igvm, HEADER_SECTION_DIRECTIVE, i);
+ directive(type, cgs, i, compatibility_mask);
+ }
+
+ /*
+ * Contiguous pages of data with compatible flags are grouped together in
+ * order to reduce the number of memory regions we create. Make sure the
+ * last group is processed with this call.
+ */
+ process_mem_page(cgs, i, NULL);
+
+ QTAILQ_FOREACH(parameter, ¶meter_data, next)
+ {
+ g_free(parameter->data);
+ parameter->data = NULL;
+ }
+}
+
+#endif
diff --git a/backends/meson.build b/backends/meson.build
index d550ac19f7..d092850a07 100644
--- a/backends/meson.build
+++ b/backends/meson.build
@@ -32,6 +32,7 @@ system_ss.add(when: gio, if_true: files('dbus-vmstate.c'))
system_ss.add(when: 'CONFIG_SGX', if_true: files('hostmem-epc.c'))
if igvm.found()
system_ss.add(igvm)
+ system_ss.add(files('igvm.c'))
endif
subdir('tpm')
diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
index c43a1a32f1..1a017a8fda 100644
--- a/include/exec/confidential-guest-support.h
+++ b/include/exec/confidential-guest-support.h
@@ -27,6 +27,10 @@
#include "igvm/igvm.h"
#endif
+#if defined(CONFIG_IGVM)
+#include "igvm/igvm.h"
+#endif
+
#define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport, CONFIDENTIAL_GUEST_SUPPORT)
@@ -95,6 +99,7 @@ struct ConfidentialGuestSupport {
* Virtual Machine (IGVM) format.
*/
char *igvm_filename;
+ IgvmHandle igvm;
#endif
/*
diff --git a/include/exec/igvm.h b/include/exec/igvm.h
new file mode 100644
index 0000000000..6f40a3239c
--- /dev/null
+++ b/include/exec/igvm.h
@@ -0,0 +1,35 @@
+/*
+ * QEMU IGVM configuration backend for Confidential Guests
+ *
+ * Copyright (C) 2023-2024 SUSE
+ *
+ * Authors:
+ * Roy Hopkins <roy.hopkins@suse.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.
+ */
+
+#ifndef EXEC_IGVM_H
+#define EXEC_IGVM_H
+
+#include "exec/confidential-guest-support.h"
+
+#if defined(CONFIG_IGVM)
+
+void igvm_file_init(ConfidentialGuestSupport *cgs);
+void igvm_process(ConfidentialGuestSupport *cgs);
+
+#else
+
+static inline void igvm_file_init(ConfidentialGuestSupport *cgs)
+{
+}
+
+static inline void igvm_process(ConfidentialGuestSupport *cgs)
+{
+}
+
+#endif
+
+#endif
--
2.43.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 4/9] backends/igvm: Implement parsing and processing of IGVM files
2024-02-27 14:50 ` [PATCH 4/9] backends/igvm: Implement parsing and processing of IGVM files Roy Hopkins
@ 2024-03-01 16:51 ` Daniel P. Berrangé
2024-03-12 11:58 ` Roy Hopkins
0 siblings, 1 reply; 35+ messages in thread
From: Daniel P. Berrangé @ 2024-03-01 16:51 UTC (permalink / raw)
To: Roy Hopkins
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
On Tue, Feb 27, 2024 at 02:50:10PM +0000, Roy Hopkins wrote:
> This commit adds an implementation of an IGVM loader which parses the
> file specified as a pararameter to ConfidentialGuestSupport and provides
> a function that uses the interface in the same object to configure and
> populate guest memory based on the contents of the file.
>
> The IGVM file is parsed when a filename is provided but the code to
> process the IGVM file is not yet hooked into target systems. This will
> follow in a later commit.
>
> Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> ---
> backends/confidential-guest-support.c | 4 +
> backends/igvm.c | 718 ++++++++++++++++++++++
> backends/meson.build | 1 +
> include/exec/confidential-guest-support.h | 5 +
> include/exec/igvm.h | 35 ++
> 5 files changed, 763 insertions(+)
> create mode 100644 backends/igvm.c
> create mode 100644 include/exec/igvm.h
>
> diff --git a/backends/confidential-guest-support.c b/backends/confidential-guest-support.c
> index 42628be8d7..cb7651a8d0 100644
> --- a/backends/confidential-guest-support.c
> +++ b/backends/confidential-guest-support.c
> @@ -15,6 +15,7 @@
>
> #include "exec/confidential-guest-support.h"
> #include "qemu/error-report.h"
> +#include "exec/igvm.h"
>
> OBJECT_DEFINE_ABSTRACT_TYPE(ConfidentialGuestSupport,
> confidential_guest_support,
> @@ -33,6 +34,9 @@ static void set_igvm(Object *obj, const char *value, Error **errp)
> ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
> g_free(cgs->igvm_filename);
> cgs->igvm_filename = g_strdup(value);
> +#if defined(CONFIG_IGVM)
> + igvm_file_init(cgs);
> +#endif
> }
> #endif
>
> diff --git a/backends/igvm.c b/backends/igvm.c
> new file mode 100644
> index 0000000000..a6261d796f
> --- /dev/null
> +++ b/backends/igvm.c
> @@ -0,0 +1,718 @@
> +/*
> + * QEMU IGVM configuration backend for Confidential Guests
> + *
> + * Copyright (C) 2023-2024 SUSE
> + *
> + * Authors:
> + * Roy Hopkins <roy.hopkins@suse.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"
> +
> +#if defined(CONFIG_IGVM)
> +
> +#include "exec/confidential-guest-support.h"
> +#include "qemu/queue.h"
> +#include "qemu/typedefs.h"
> +
> +#include "exec/igvm.h"
> +#include "qemu/error-report.h"
> +#include "hw/boards.h"
> +#include "qapi/error.h"
> +#include "exec/address-spaces.h"
> +
> +#include <igvm/igvm.h>
> +#include <igvm/igvm_defs.h>
> +#include <linux/kvm.h>
> +
> +typedef struct IgvmParameterData {
> + QTAILQ_ENTRY(IgvmParameterData) next;
> + uint8_t *data;
> + uint32_t size;
> + uint32_t index;
> +} IgvmParameterData;
> +
> +static QTAILQ_HEAD(, IgvmParameterData) parameter_data;
> +
> +static void directive_page_data(ConfidentialGuestSupport *cgs, int i,
> + uint32_t compatibility_mask);
> +static void directive_vp_context(ConfidentialGuestSupport *cgs, int i,
> + uint32_t compatibility_mask);
> +static void directive_parameter_area(ConfidentialGuestSupport *cgs, int i,
> + uint32_t compatibility_mask);
> +static void directive_parameter_insert(ConfidentialGuestSupport *cgs, int i,
> + uint32_t compatibility_mask);
> +static void directive_memory_map(ConfidentialGuestSupport *cgs, int i,
> + uint32_t compatibility_mask);
> +static void directive_vp_count(ConfidentialGuestSupport *cgs, int i,
> + uint32_t compatibility_mask);
> +static void directive_environment_info(ConfidentialGuestSupport *cgs, int i,
> + uint32_t compatibility_mask);
> +static void directive_required_memory(ConfidentialGuestSupport *cgs, int i,
> + uint32_t compatibility_mask);
> +
> +struct IGVMDirectiveHandler {
> + uint32_t type;
> + void (*handler)(ConfidentialGuestSupport *cgs, int i,
> + uint32_t compatibility_mask);
> +};
> +
> +static struct IGVMDirectiveHandler directive_handlers[] = {
> + { IGVM_VHT_PAGE_DATA, directive_page_data },
> + { IGVM_VHT_VP_CONTEXT, directive_vp_context },
> + { IGVM_VHT_PARAMETER_AREA, directive_parameter_area },
> + { IGVM_VHT_PARAMETER_INSERT, directive_parameter_insert },
> + { IGVM_VHT_MEMORY_MAP, directive_memory_map },
> + { IGVM_VHT_VP_COUNT_PARAMETER, directive_vp_count },
> + { IGVM_VHT_ENVIRONMENT_INFO_PARAMETER, directive_environment_info },
> + { IGVM_VHT_REQUIRED_MEMORY, directive_required_memory },
> +};
> +
> +static void directive(uint32_t type, ConfidentialGuestSupport *cgs, int i,
> + uint32_t compatibility_mask)
> +{
> + size_t handler;
> + for (handler = 0; handler < (sizeof(directive_handlers) /
> + sizeof(struct IGVMDirectiveHandler));
> + ++handler) {
> + if (directive_handlers[handler].type == type) {
> + directive_handlers[handler].handler(cgs, i, compatibility_mask);
> + return;
> + }
> + }
> + warn_report("IGVM: Unknown directive encountered when processing file: %X",
> + type);
> +}
If a directive in a IGVM file has functional effects on the
guest behaviour it does not feel right to just ignore it and
carry on launching the guest in a likely incorrect state.
IOW, I think this should be a error, not a warning, and
this method ought to have an 'Error **errp' parameter.
> +
> +static void igvm_handle_error(int32_t result, const char *msg)
> +{
> + if (result < 0) {
> + error_report("Processing of IGVM file failed: %s (code: %d)", msg,
> + (int)result);
> + exit(EXIT_FAILURE);
> + }
> +}
IMHO all the code below should have "Error **errp" parameters
to report and return to the caller. The top level CGS code
that calls into IVGM can use 'error_report_err'. As such
I think this method shouldn't exist, and code shoud directly
call error_setg.
> +
> +static void *igvm_prepare_memory(uint64_t addr, uint64_t size,
> + int region_identifier)
> +{
> + MemoryRegion *igvm_pages = NULL;
> + Int128 gpa_region_size;
> + MemoryRegionSection mrs =
> + memory_region_find(get_system_memory(), addr, size);
> + if (mrs.mr) {
> + if (!memory_region_is_ram(mrs.mr)) {
> + memory_region_unref(mrs.mr);
> + error_report(
> + "Processing of IGVM file failed: Could not prepare memory "
> + "at address 0x%lX due to existing non-RAM region",
> + addr);
> + exit(EXIT_FAILURE);
> + }
> +
> + gpa_region_size = int128_make64(size);
> + if (int128_lt(mrs.size, gpa_region_size)) {
> + memory_region_unref(mrs.mr);
> + error_report(
> + "Processing of IGVM file failed: Could not prepare memory "
> + "at address 0x%lX: region size exceeded",
> + addr);
> + exit(EXIT_FAILURE);
> + }
> + return qemu_map_ram_ptr(mrs.mr->ram_block, mrs.offset_within_region);
> + } else {
> + /*
> + * The region_identifier is the is the index of the IGVM directive that
> + * contains the page with the lowest GPA in the region. This will
> + * generate a unique region name.
> + */
> + char region_name[22];
> + snprintf(region_name, sizeof(region_name), "igvm.%X",
> + region_identifier);
IMO it is preferrable to use
g_autofree char *region_name = g_strdup_printf("igvm.%X", region_identifier);
> + igvm_pages = g_malloc(sizeof(*igvm_pages));
> + memory_region_init_ram(igvm_pages, NULL, region_name, size,
> + &error_fatal);
> + memory_region_add_subregion(get_system_memory(), addr, igvm_pages);
> + return memory_region_get_ram_ptr(igvm_pages);
> + }
> +}
> +
> +static int igvm_type_to_cgs_type(IgvmPageDataType memory_type, bool unmeasured,
> + bool zero)
> +{
> + switch (memory_type) {
> + case NORMAL: {
> + if (unmeasured) {
> + return CGS_PAGE_TYPE_UNMEASURED;
> + } else {
> + return zero ? CGS_PAGE_TYPE_ZERO :
> + CGS_PAGE_TYPE_NORMAL;
> + }
> + }
> + case SECRETS:
> + return CGS_PAGE_TYPE_SECRETS;
> + case CPUID_DATA:
> + return CGS_PAGE_TYPE_CPUID;
> + case CPUID_XF:
> + return CGS_PAGE_TYPE_CPUID;
> + default:
> + return CGS_PAGE_TYPE_UNMEASURED;
> + }
> +}
> +
> +static bool page_attrs_equal(const IGVM_VHS_PAGE_DATA *page_1,
> + const IGVM_VHS_PAGE_DATA *page_2)
> +{
> + return ((*(const uint32_t *)&page_1->flags ==
> + *(const uint32_t *)&page_2->flags) &&
> + (page_1->data_type == page_2->data_type) &&
> + (page_1->compatibility_mask == page_2->compatibility_mask));
> +}
> +void igvm_file_init(ConfidentialGuestSupport *cgs)
> +{
> + FILE *igvm_file = NULL;
> + uint8_t *igvm_buf = NULL;
> +
> + if (cgs->igvm_filename) {
Invert this condition and return immediately, so we
don't have the entire method body uneccessarily indented.
> + IgvmHandle igvm;
> + unsigned long igvm_length;
> +
> + igvm_file = fopen(cgs->igvm_filename, "rb");
> + if (!igvm_file) {
> + error_report("IGVM file not found '%s'", cgs->igvm_filename);
> + goto error_out;
> + }
> +
> + fseek(igvm_file, 0, SEEK_END);
> + igvm_length = ftell(igvm_file);
> + fseek(igvm_file, 0, SEEK_SET);
> +
> + igvm_buf = g_new(uint8_t, igvm_length);
> + if (!igvm_buf) {
> + error_report(
> + "Could not allocate buffer to read file IGVM file '%s'",
> + cgs->igvm_filename);
> + goto error_out;
> + }
> + if (fread(igvm_buf, 1, igvm_length, igvm_file) != igvm_length) {
> + error_report("Unable to load IGVM file '%s'", cgs->igvm_filename);
> + goto error_out;
> + }
> +
> + igvm = igvm_new_from_binary(igvm_buf, igvm_length);
> + if (igvm < 0) {
> + error_report("Parsing IGVM file '%s' failed with error_code %d",
> + cgs->igvm_filename, igvm);
> + goto error_out;
> + }
> + fclose(igvm_file);
> + g_free(igvm_buf);
> +
> + cgs->igvm = igvm;
> + }
> + return;
> +
> +error_out:
> + free(igvm_buf);
> + if (igvm_file) {
> + fclose(igvm_file);
> + }
> + exit(EXIT_FAILURE);
> +}
This can be massively simplified to:
g_autofree uint8_t *buf = NULL;
unsigned long len;
g_autoptr(GError) gerr = NULL;
if (!cgs->igvm_filename) {
return 0;
}
if (!g_file_get_contents(cgs->igvm_filename, (gchar**)&buf, &len, &gerr)) {
error_setg(errp, "Unable to load %s: %s", cgs->igvm_filename, gerr->message);
return -1;
}
if ((cgs->igvm = igvm_new_from_binary(buf, len)) < 0) {
error_setg(errp, "Unable to parse IGVM %s: %s", cgs->igvm_filename, cgs->igvm);
return -1;
}
return 0;
> +
> +void igvm_process(ConfidentialGuestSupport *cgs)
> +{
> + int32_t result;
> + int i;
> + uint32_t compatibility_mask;
> + IgvmParameterData *parameter;
> +
> + /*
> + * If this is not a Confidential guest or no IGVM has been provided then
> + * this is a no-op.
> + */
> + if (!cgs || !cgs->igvm) {
> + return;
> + }
The caller should not be invoking this method if cgs is NULL.
> +
> + QTAILQ_INIT(¶meter_data);
> +
> + /*
> + * Check that the IGVM file provides configuration for the current
> + * platform
> + */
> + compatibility_mask = supported_platform_compat_mask(cgs);
> + if (compatibility_mask == 0) {
> + error_report(
> + "IGVM file does not describe a compatible supported platform");
> + exit(EXIT_FAILURE);
> + }
> +
> + result = igvm_header_count(cgs->igvm, HEADER_SECTION_DIRECTIVE);
> + igvm_handle_error(result, "Failed to read directive header count");
> + for (i = 0; i < (int)result; ++i) {
> + IgvmVariableHeaderType type =
> + igvm_get_header_type(cgs->igvm, HEADER_SECTION_DIRECTIVE, i);
> + directive(type, cgs, i, compatibility_mask);
> + }
> +
> + /*
> + * Contiguous pages of data with compatible flags are grouped together in
> + * order to reduce the number of memory regions we create. Make sure the
> + * last group is processed with this call.
> + */
> + process_mem_page(cgs, i, NULL);
> +
> + QTAILQ_FOREACH(parameter, ¶meter_data, next)
> + {
> + g_free(parameter->data);
> + parameter->data = NULL;
> + }
> +}
> +
> +#endif
> diff --git a/backends/meson.build b/backends/meson.build
> index d550ac19f7..d092850a07 100644
> --- a/backends/meson.build
> +++ b/backends/meson.build
> @@ -32,6 +32,7 @@ system_ss.add(when: gio, if_true: files('dbus-vmstate.c'))
> system_ss.add(when: 'CONFIG_SGX', if_true: files('hostmem-epc.c'))
> if igvm.found()
> system_ss.add(igvm)
> + system_ss.add(files('igvm.c'))
> endif
>
> subdir('tpm')
> diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
> index c43a1a32f1..1a017a8fda 100644
> --- a/include/exec/confidential-guest-support.h
> +++ b/include/exec/confidential-guest-support.h
> @@ -27,6 +27,10 @@
> #include "igvm/igvm.h"
> #endif
>
> +#if defined(CONFIG_IGVM)
> +#include "igvm/igvm.h"
> +#endif
> +
> #define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
> OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport, CONFIDENTIAL_GUEST_SUPPORT)
>
> @@ -95,6 +99,7 @@ struct ConfidentialGuestSupport {
> * Virtual Machine (IGVM) format.
> */
> char *igvm_filename;
> + IgvmHandle igvm;
> #endif
>
> /*
> diff --git a/include/exec/igvm.h b/include/exec/igvm.h
> new file mode 100644
> index 0000000000..6f40a3239c
> --- /dev/null
> +++ b/include/exec/igvm.h
> @@ -0,0 +1,35 @@
> +/*
> + * QEMU IGVM configuration backend for Confidential Guests
> + *
> + * Copyright (C) 2023-2024 SUSE
> + *
> + * Authors:
> + * Roy Hopkins <roy.hopkins@suse.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.
> + */
> +
> +#ifndef EXEC_IGVM_H
> +#define EXEC_IGVM_H
> +
> +#include "exec/confidential-guest-support.h"
> +
> +#if defined(CONFIG_IGVM)
> +
> +void igvm_file_init(ConfidentialGuestSupport *cgs);
> +void igvm_process(ConfidentialGuestSupport *cgs);
Both of these should gain an "Error *errp" parameter and 'int'
return type and leave the error_report + exit to the caller.
> +
> +#else
> +
> +static inline void igvm_file_init(ConfidentialGuestSupport *cgs)
> +{
> +}
> +
> +static inline void igvm_process(ConfidentialGuestSupport *cgs)
> +{
> +}
> +
> +#endif
> +
> +#endif
> --
> 2.43.0
>
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 4/9] backends/igvm: Implement parsing and processing of IGVM files
2024-03-01 16:51 ` Daniel P. Berrangé
@ 2024-03-12 11:58 ` Roy Hopkins
0 siblings, 0 replies; 35+ messages in thread
From: Roy Hopkins @ 2024-03-12 11:58 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
On Fri, 2024-03-01 at 16:51 +0000, Daniel P. Berrangé wrote:
> On Tue, Feb 27, 2024 at 02:50:10PM +0000, Roy Hopkins wrote:
> > This commit adds an implementation of an IGVM loader which parses the
> > file specified as a pararameter to ConfidentialGuestSupport and provides
> > a function that uses the interface in the same object to configure and
> > populate guest memory based on the contents of the file.
> >
> > The IGVM file is parsed when a filename is provided but the code to
> > process the IGVM file is not yet hooked into target systems. This will
> > follow in a later commit.
> >
> > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> > ---
> > backends/confidential-guest-support.c | 4 +
> > backends/igvm.c | 718 ++++++++++++++++++++++
> > backends/meson.build | 1 +
> > include/exec/confidential-guest-support.h | 5 +
> > include/exec/igvm.h | 35 ++
> > 5 files changed, 763 insertions(+)
> > create mode 100644 backends/igvm.c
> > create mode 100644 include/exec/igvm.h
> >
> > diff --git a/backends/confidential-guest-support.c b/backends/confidential-
> > guest-support.c
> > index 42628be8d7..cb7651a8d0 100644
> > --- a/backends/confidential-guest-support.c
> > +++ b/backends/confidential-guest-support.c
> > @@ -15,6 +15,7 @@
> >
> > #include "exec/confidential-guest-support.h"
> > #include "qemu/error-report.h"
> > +#include "exec/igvm.h"
> >
> > OBJECT_DEFINE_ABSTRACT_TYPE(ConfidentialGuestSupport,
> > confidential_guest_support,
> > @@ -33,6 +34,9 @@ static void set_igvm(Object *obj, const char *value, Error
> > **errp)
> > ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
> > g_free(cgs->igvm_filename);
> > cgs->igvm_filename = g_strdup(value);
> > +#if defined(CONFIG_IGVM)
> > + igvm_file_init(cgs);
> > +#endif
> > }
> > #endif
> >
> > diff --git a/backends/igvm.c b/backends/igvm.c
> > new file mode 100644
> > index 0000000000..a6261d796f
> > --- /dev/null
> > +++ b/backends/igvm.c
> > @@ -0,0 +1,718 @@
> > +/*
> > + * QEMU IGVM configuration backend for Confidential Guests
> > + *
> > + * Copyright (C) 2023-2024 SUSE
> > + *
> > + * Authors:
> > + * Roy Hopkins <roy.hopkins@suse.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"
> > +
> > +#if defined(CONFIG_IGVM)
> > +
> > +#include "exec/confidential-guest-support.h"
> > +#include "qemu/queue.h"
> > +#include "qemu/typedefs.h"
> > +
> > +#include "exec/igvm.h"
> > +#include "qemu/error-report.h"
> > +#include "hw/boards.h"
> > +#include "qapi/error.h"
> > +#include "exec/address-spaces.h"
> > +
> > +#include <igvm/igvm.h>
> > +#include <igvm/igvm_defs.h>
> > +#include <linux/kvm.h>
> > +
> > +typedef struct IgvmParameterData {
> > + QTAILQ_ENTRY(IgvmParameterData) next;
> > + uint8_t *data;
> > + uint32_t size;
> > + uint32_t index;
> > +} IgvmParameterData;
> > +
> > +static QTAILQ_HEAD(, IgvmParameterData) parameter_data;
> > +
> > +static void directive_page_data(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask);
> > +static void directive_vp_context(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask);
> > +static void directive_parameter_area(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask);
> > +static void directive_parameter_insert(ConfidentialGuestSupport *cgs, int
> > i,
> > + uint32_t compatibility_mask);
> > +static void directive_memory_map(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask);
> > +static void directive_vp_count(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask);
> > +static void directive_environment_info(ConfidentialGuestSupport *cgs, int
> > i,
> > + uint32_t compatibility_mask);
> > +static void directive_required_memory(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask);
> > +
> > +struct IGVMDirectiveHandler {
> > + uint32_t type;
> > + void (*handler)(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask);
> > +};
> > +
> > +static struct IGVMDirectiveHandler directive_handlers[] = {
> > + { IGVM_VHT_PAGE_DATA, directive_page_data },
> > + { IGVM_VHT_VP_CONTEXT, directive_vp_context },
> > + { IGVM_VHT_PARAMETER_AREA, directive_parameter_area },
> > + { IGVM_VHT_PARAMETER_INSERT, directive_parameter_insert },
> > + { IGVM_VHT_MEMORY_MAP, directive_memory_map },
> > + { IGVM_VHT_VP_COUNT_PARAMETER, directive_vp_count },
> > + { IGVM_VHT_ENVIRONMENT_INFO_PARAMETER, directive_environment_info },
> > + { IGVM_VHT_REQUIRED_MEMORY, directive_required_memory },
> > +};
> > +
> > +static void directive(uint32_t type, ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask)
> > +{
> > + size_t handler;
> > + for (handler = 0; handler < (sizeof(directive_handlers) /
> > + sizeof(struct IGVMDirectiveHandler));
> > + ++handler) {
> > + if (directive_handlers[handler].type == type) {
> > + directive_handlers[handler].handler(cgs, i,
> > compatibility_mask);
> > + return;
> > + }
> > + }
> > + warn_report("IGVM: Unknown directive encountered when processing file:
> > %X",
> > + type);
> > +}
>
> If a directive in a IGVM file has functional effects on the
> guest behaviour it does not feel right to just ignore it and
> carry on launching the guest in a likely incorrect state.
>
> IOW, I think this should be a error, not a warning, and
> this method ought to have an 'Error **errp' parameter.
Agreed. I'll update the code to generate errors on any failure or unknown
directives/configuration from the IGVM file.
>
> > +
> > +static void igvm_handle_error(int32_t result, const char *msg)
> > +{
> > + if (result < 0) {
> > + error_report("Processing of IGVM file failed: %s (code: %d)", msg,
> > + (int)result);
> > + exit(EXIT_FAILURE);
> > + }
> > +}
>
> IMHO all the code below should have "Error **errp" parameters
> to report and return to the caller. The top level CGS code
> that calls into IVGM can use 'error_report_err'. As such
> I think this method shouldn't exist, and code shoud directly
> call error_setg.
Right, V2 will include a rework of the error handling using a combination of
return codes and "Error **errp" as recommended in "error.h". This method has
been removed as part of that rework.
>
> > +
> > +static void *igvm_prepare_memory(uint64_t addr, uint64_t size,
> > + int region_identifier)
> > +{
> > + MemoryRegion *igvm_pages = NULL;
> > + Int128 gpa_region_size;
> > + MemoryRegionSection mrs =
> > + memory_region_find(get_system_memory(), addr, size);
> > + if (mrs.mr) {
> > + if (!memory_region_is_ram(mrs.mr)) {
> > + memory_region_unref(mrs.mr);
> > + error_report(
> > + "Processing of IGVM file failed: Could not prepare memory "
> > + "at address 0x%lX due to existing non-RAM region",
> > + addr);
> > + exit(EXIT_FAILURE);
> > + }
> > +
> > + gpa_region_size = int128_make64(size);
> > + if (int128_lt(mrs.size, gpa_region_size)) {
> > + memory_region_unref(mrs.mr);
> > + error_report(
> > + "Processing of IGVM file failed: Could not prepare memory "
> > + "at address 0x%lX: region size exceeded",
> > + addr);
> > + exit(EXIT_FAILURE);
> > + }
> > + return qemu_map_ram_ptr(mrs.mr->ram_block,
> > mrs.offset_within_region);
> > + } else {
> > + /*
> > + * The region_identifier is the is the index of the IGVM directive
> > that
> > + * contains the page with the lowest GPA in the region. This will
> > + * generate a unique region name.
> > + */
> > + char region_name[22];
> > + snprintf(region_name, sizeof(region_name), "igvm.%X",
> > + region_identifier);
>
> IMO it is preferrable to use
>
> g_autofree char *region_name = g_strdup_printf("igvm.%X",
> region_identifier);
Ok, I'll make this change.
>
> > + igvm_pages = g_malloc(sizeof(*igvm_pages));
> > + memory_region_init_ram(igvm_pages, NULL, region_name, size,
> > + &error_fatal);
> > + memory_region_add_subregion(get_system_memory(), addr, igvm_pages);
> > + return memory_region_get_ram_ptr(igvm_pages);
> > + }
> > +}
> > +
> > +static int igvm_type_to_cgs_type(IgvmPageDataType memory_type, bool
> > unmeasured,
> > + bool zero)
> > +{
> > + switch (memory_type) {
> > + case NORMAL: {
> > + if (unmeasured) {
> > + return CGS_PAGE_TYPE_UNMEASURED;
> > + } else {
> > + return zero ? CGS_PAGE_TYPE_ZERO :
> > + CGS_PAGE_TYPE_NORMAL;
> > + }
> > + }
> > + case SECRETS:
> > + return CGS_PAGE_TYPE_SECRETS;
> > + case CPUID_DATA:
> > + return CGS_PAGE_TYPE_CPUID;
> > + case CPUID_XF:
> > + return CGS_PAGE_TYPE_CPUID;
> > + default:
> > + return CGS_PAGE_TYPE_UNMEASURED;
> > + }
> > +}
> > +
> > +static bool page_attrs_equal(const IGVM_VHS_PAGE_DATA *page_1,
> > + const IGVM_VHS_PAGE_DATA *page_2)
> > +{
> > + return ((*(const uint32_t *)&page_1->flags ==
> > + *(const uint32_t *)&page_2->flags) &&
> > + (page_1->data_type == page_2->data_type) &&
> > + (page_1->compatibility_mask == page_2->compatibility_mask));
> > +}
>
>
>
>
> > +void igvm_file_init(ConfidentialGuestSupport *cgs)
> > +{
> > + FILE *igvm_file = NULL;
> > + uint8_t *igvm_buf = NULL;
> > +
> > + if (cgs->igvm_filename) {
>
> Invert this condition and return immediately, so we
> don't have the entire method body uneccessarily indented.
Ok.
>
> > + IgvmHandle igvm;
> > + unsigned long igvm_length;
> > +
> > + igvm_file = fopen(cgs->igvm_filename, "rb");
> > + if (!igvm_file) {
> > + error_report("IGVM file not found '%s'", cgs->igvm_filename);
> > + goto error_out;
> > + }
> > +
> > + fseek(igvm_file, 0, SEEK_END);
> > + igvm_length = ftell(igvm_file);
> > + fseek(igvm_file, 0, SEEK_SET);
> > +
> > + igvm_buf = g_new(uint8_t, igvm_length);
> > + if (!igvm_buf) {
> > + error_report(
> > + "Could not allocate buffer to read file IGVM file '%s'",
> > + cgs->igvm_filename);
> > + goto error_out;
> > + }
> > + if (fread(igvm_buf, 1, igvm_length, igvm_file) != igvm_length) {
> > + error_report("Unable to load IGVM file '%s'", cgs-
> > >igvm_filename);
> > + goto error_out;
> > + }
> > +
> > + igvm = igvm_new_from_binary(igvm_buf, igvm_length);
> > + if (igvm < 0) {
> > + error_report("Parsing IGVM file '%s' failed with error_code
> > %d",
> > + cgs->igvm_filename, igvm);
> > + goto error_out;
> > + }
> > + fclose(igvm_file);
> > + g_free(igvm_buf);
> > +
> > + cgs->igvm = igvm;
> > + }
> > + return;
> > +
> > +error_out:
> > + free(igvm_buf);
> > + if (igvm_file) {
> > + fclose(igvm_file);
> > + }
> > + exit(EXIT_FAILURE);
> > +}
>
> This can be massively simplified to:
>
> g_autofree uint8_t *buf = NULL;
> unsigned long len;
> g_autoptr(GError) gerr = NULL;
>
> if (!cgs->igvm_filename) {
> return 0;
> }
>
> if (!g_file_get_contents(cgs->igvm_filename, (gchar**)&buf, &len, &gerr))
> {
> error_setg(errp, "Unable to load %s: %s", cgs->igvm_filename, gerr-
> >message);
> return -1;
> }
>
> if ((cgs->igvm = igvm_new_from_binary(buf, len)) < 0) {
> error_setg(errp, "Unable to parse IGVM %s: %s", cgs->igvm_filename,
> cgs->igvm);
> return -1;
> }
>
> return 0;
>
Much nicer! Thank you.
> > +
> > +void igvm_process(ConfidentialGuestSupport *cgs)
> > +{
> > + int32_t result;
> > + int i;
> > + uint32_t compatibility_mask;
> > + IgvmParameterData *parameter;
> > +
> > + /*
> > + * If this is not a Confidential guest or no IGVM has been provided
> > then
> > + * this is a no-op.
> > + */
> > + if (!cgs || !cgs->igvm) {
> > + return;
> > + }
>
> The caller should not be invoking this method if cgs is NULL.
Ok.
>
> > +
> > + QTAILQ_INIT(¶meter_data);
> > +
> > + /*
> > + * Check that the IGVM file provides configuration for the current
> > + * platform
> > + */
> > + compatibility_mask = supported_platform_compat_mask(cgs);
> > + if (compatibility_mask == 0) {
> > + error_report(
> > + "IGVM file does not describe a compatible supported platform");
> > + exit(EXIT_FAILURE);
> > + }
> > +
> > + result = igvm_header_count(cgs->igvm, HEADER_SECTION_DIRECTIVE);
> > + igvm_handle_error(result, "Failed to read directive header count");
> > + for (i = 0; i < (int)result; ++i) {
> > + IgvmVariableHeaderType type =
> > + igvm_get_header_type(cgs->igvm, HEADER_SECTION_DIRECTIVE, i);
> > + directive(type, cgs, i, compatibility_mask);
> > + }
> > +
> > + /*
> > + * Contiguous pages of data with compatible flags are grouped together
> > in
> > + * order to reduce the number of memory regions we create. Make sure
> > the
> > + * last group is processed with this call.
> > + */
> > + process_mem_page(cgs, i, NULL);
> > +
> > + QTAILQ_FOREACH(parameter, ¶meter_data, next)
> > + {
> > + g_free(parameter->data);
> > + parameter->data = NULL;
> > + }
> > +}
> > +
> > +#endif
> > diff --git a/backends/meson.build b/backends/meson.build
> > index d550ac19f7..d092850a07 100644
> > --- a/backends/meson.build
> > +++ b/backends/meson.build
> > @@ -32,6 +32,7 @@ system_ss.add(when: gio, if_true: files('dbus-vmstate.c'))
> > system_ss.add(when: 'CONFIG_SGX', if_true: files('hostmem-epc.c'))
> > if igvm.found()
> > system_ss.add(igvm)
> > + system_ss.add(files('igvm.c'))
> > endif
> >
> > subdir('tpm')
> > diff --git a/include/exec/confidential-guest-support.h
> > b/include/exec/confidential-guest-support.h
> > index c43a1a32f1..1a017a8fda 100644
> > --- a/include/exec/confidential-guest-support.h
> > +++ b/include/exec/confidential-guest-support.h
> > @@ -27,6 +27,10 @@
> > #include "igvm/igvm.h"
> > #endif
> >
> > +#if defined(CONFIG_IGVM)
> > +#include "igvm/igvm.h"
> > +#endif
> > +
> > #define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
> > OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport,
> > CONFIDENTIAL_GUEST_SUPPORT)
> >
> > @@ -95,6 +99,7 @@ struct ConfidentialGuestSupport {
> > * Virtual Machine (IGVM) format.
> > */
> > char *igvm_filename;
> > + IgvmHandle igvm;
> > #endif
> >
> > /*
> > diff --git a/include/exec/igvm.h b/include/exec/igvm.h
> > new file mode 100644
> > index 0000000000..6f40a3239c
> > --- /dev/null
> > +++ b/include/exec/igvm.h
> > @@ -0,0 +1,35 @@
> > +/*
> > + * QEMU IGVM configuration backend for Confidential Guests
> > + *
> > + * Copyright (C) 2023-2024 SUSE
> > + *
> > + * Authors:
> > + * Roy Hopkins <roy.hopkins@suse.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.
> > + */
> > +
> > +#ifndef EXEC_IGVM_H
> > +#define EXEC_IGVM_H
> > +
> > +#include "exec/confidential-guest-support.h"
> > +
> > +#if defined(CONFIG_IGVM)
> > +
> > +void igvm_file_init(ConfidentialGuestSupport *cgs);
> > +void igvm_process(ConfidentialGuestSupport *cgs);
>
> Both of these should gain an "Error *errp" parameter and 'int'
> return type and leave the error_report + exit to the caller.
>
This will be addressed as per my first comment above.
> > +
> > +#else
> > +
> > +static inline void igvm_file_init(ConfidentialGuestSupport *cgs)
> > +{
> > +}
> > +
> > +static inline void igvm_process(ConfidentialGuestSupport *cgs)
> > +{
> > +}
> > +
> > +#endif
> > +
> > +#endif
> > --
> > 2.43.0
> >
> >
>
> With regards,
> Daniel
Daniel - Many thanks for taking the time to look at this.
Regards,
Roy
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 5/9] i386/pc: Process IGVM file during PC initialization if present
2024-02-27 14:50 [PATCH 0/9] Introduce support for IGVM files Roy Hopkins
` (3 preceding siblings ...)
2024-02-27 14:50 ` [PATCH 4/9] backends/igvm: Implement parsing and processing of IGVM files Roy Hopkins
@ 2024-02-27 14:50 ` Roy Hopkins
2024-02-27 14:50 ` [PATCH 6/9] i386/pc: Skip initialization of system FW when using IGVM Roy Hopkins
` (5 subsequent siblings)
10 siblings, 0 replies; 35+ messages in thread
From: Roy Hopkins @ 2024-02-27 14:50 UTC (permalink / raw)
To: qemu-devel
Cc: Roy Hopkins, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
An IGVM file contains configuration of a guest that supports
confidential computing hardware. As part of the PC system
initialisation, the IGVM needs to be processed to apply this
configuration before the guest is started.
This patch introduces processing of a provided IGVM file at the end of
the current PC initialization steps. If an IGVM file has been provided
then the directives in the file are processed completing the
initalization of the target.
If no IGVM file has been specified by the user then no there is no
intended consequences in these changes.
Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
backends/confidential-guest-support.c | 18 ++++++++++++++++++
hw/i386/pc_piix.c | 4 ++++
hw/i386/pc_q35.c | 4 ++++
include/exec/confidential-guest-support.h | 17 +++++++++++++++++
4 files changed, 43 insertions(+)
diff --git a/backends/confidential-guest-support.c b/backends/confidential-guest-support.c
index cb7651a8d0..881eef6b42 100644
--- a/backends/confidential-guest-support.c
+++ b/backends/confidential-guest-support.c
@@ -82,3 +82,21 @@ static void confidential_guest_support_init(Object *obj)
static void confidential_guest_support_finalize(Object *obj)
{
}
+
+bool cgs_is_igvm(ConfidentialGuestSupport *cgs)
+{
+#if defined(CONFIG_IGVM)
+ return cgs && cgs->igvm;
+#else
+ return false;
+#endif
+}
+
+void cgs_process_igvm(ConfidentialGuestSupport *cgs)
+{
+#if defined(CONFIG_IGVM)
+ if (cgs && cgs_is_igvm(cgs)) {
+ igvm_process(cgs);
+ }
+#endif
+}
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index ec7c07b362..b449944ee8 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -66,6 +66,7 @@
#include "hw/i386/acpi-build.h"
#include "kvm/kvm-cpu.h"
#include "target/i386/cpu.h"
+#include "exec/confidential-guest-support.h"
#define XEN_IOAPIC_NUM_PIRQS 128ULL
@@ -370,6 +371,9 @@ static void pc_init1(MachineState *machine,
x86_nvdimm_acpi_dsmio,
x86ms->fw_cfg, OBJECT(pcms));
}
+
+ /* Apply confidential guest state from IGVM if supplied */
+ cgs_process_igvm(machine->cgs);
}
typedef enum PCSouthBridgeOption {
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 53fb3db26d..ecccf518e2 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -58,6 +58,7 @@
#include "hw/mem/nvdimm.h"
#include "hw/i386/acpi-build.h"
#include "target/i386/cpu.h"
+#include "exec/confidential-guest-support.h"
/* ICH9 AHCI has 6 ports */
#define MAX_SATA_PORTS 6
@@ -324,6 +325,9 @@ static void pc_q35_init(MachineState *machine)
x86_nvdimm_acpi_dsmio,
x86ms->fw_cfg, OBJECT(pcms));
}
+
+ /* Apply confidential guest state from IGVM if supplied */
+ cgs_process_igvm(machine->cgs);
}
#define DEFINE_Q35_MACHINE(suffix, name, compatfn, optionfn) \
diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
index 1a017a8fda..2bc99d84cd 100644
--- a/include/exec/confidential-guest-support.h
+++ b/include/exec/confidential-guest-support.h
@@ -147,6 +147,23 @@ typedef struct ConfidentialGuestSupportClass {
ObjectClass parent;
} ConfidentialGuestSupportClass;
+/*
+ * Check whether the configuration of the confidential guest is provided
+ * using an IGVM file. IGVM configuration can include the system firmware,
+ * initial CPU state and other configuration that should override standard
+ * system initialization. This function should be used by platforms to
+ * determine which devices and configuration to include during system
+ * initialization.
+ */
+bool cgs_is_igvm(ConfidentialGuestSupport *cgs);
+/*
+ * If IGVM is supported and an IGVM file has been specified then the
+ * configuration described in the file is applied to the guest.
+ * Configuration of a confidential guest includes the layout of the
+ * guest memory, including firmware and initial CPU state.
+ */
+void cgs_process_igvm(ConfidentialGuestSupport *cgs);
+
#endif /* !CONFIG_USER_ONLY */
#endif /* QEMU_CONFIDENTIAL_GUEST_SUPPORT_H */
--
2.43.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH 6/9] i386/pc: Skip initialization of system FW when using IGVM
2024-02-27 14:50 [PATCH 0/9] Introduce support for IGVM files Roy Hopkins
` (4 preceding siblings ...)
2024-02-27 14:50 ` [PATCH 5/9] i386/pc: Process IGVM file during PC initialization if present Roy Hopkins
@ 2024-02-27 14:50 ` Roy Hopkins
2024-03-01 16:54 ` Daniel P. Berrangé
2024-03-27 13:28 ` Ani Sinha
2024-02-27 14:50 ` [PATCH 7/9] i386/sev: Refactor setting of reset vector and initial CPU state Roy Hopkins
` (4 subsequent siblings)
10 siblings, 2 replies; 35+ messages in thread
From: Roy Hopkins @ 2024-02-27 14:50 UTC (permalink / raw)
To: qemu-devel
Cc: Roy Hopkins, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
When using an IGVM file the configuration of the system firmware is
defined by IGVM directives contained in the file. Therefore the default
system firmware should not be initialized when an IGVM file has been
provided.
This commit checks to see if an IGVM file has been provided and, if it
has then the standard system firmware initialization is skipped and any
prepared flash devices are cleaned up.
Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
hw/i386/pc.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f8eb684a49..17bb211708 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -63,6 +63,7 @@
#include "e820_memory_layout.h"
#include "trace.h"
#include CONFIG_DEVICES
+#include "exec/confidential-guest-support.h"
#ifdef CONFIG_XEN_EMU
#include "hw/xen/xen-legacy-backend.h"
@@ -1023,8 +1024,15 @@ void pc_memory_init(PCMachineState *pcms,
}
}
- /* Initialize PC system firmware */
- pc_system_firmware_init(pcms, rom_memory);
+ /*
+ * If this is a confidential guest configured using IGVM then the IGVM
+ * configuration will include the system firmware. In this case do not
+ * initialise PC system firmware.
+ */
+ if (!cgs_is_igvm(machine->cgs)) {
+ /* Initialize PC system firmware */
+ pc_system_firmware_init(pcms, rom_memory);
+ }
option_rom_mr = g_malloc(sizeof(*option_rom_mr));
memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
--
2.43.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 6/9] i386/pc: Skip initialization of system FW when using IGVM
2024-02-27 14:50 ` [PATCH 6/9] i386/pc: Skip initialization of system FW when using IGVM Roy Hopkins
@ 2024-03-01 16:54 ` Daniel P. Berrangé
2024-03-12 12:04 ` Roy Hopkins
2024-03-27 13:28 ` Ani Sinha
1 sibling, 1 reply; 35+ messages in thread
From: Daniel P. Berrangé @ 2024-03-01 16:54 UTC (permalink / raw)
To: Roy Hopkins
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
On Tue, Feb 27, 2024 at 02:50:12PM +0000, Roy Hopkins wrote:
> When using an IGVM file the configuration of the system firmware is
> defined by IGVM directives contained in the file. Therefore the default
> system firmware should not be initialized when an IGVM file has been
> provided.
>
> This commit checks to see if an IGVM file has been provided and, if it
> has then the standard system firmware initialization is skipped and any
> prepared flash devices are cleaned up.
>
> Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> ---
> hw/i386/pc.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f8eb684a49..17bb211708 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -63,6 +63,7 @@
> #include "e820_memory_layout.h"
> #include "trace.h"
> #include CONFIG_DEVICES
> +#include "exec/confidential-guest-support.h"
>
> #ifdef CONFIG_XEN_EMU
> #include "hw/xen/xen-legacy-backend.h"
> @@ -1023,8 +1024,15 @@ void pc_memory_init(PCMachineState *pcms,
> }
> }
>
> - /* Initialize PC system firmware */
> - pc_system_firmware_init(pcms, rom_memory);
> + /*
> + * If this is a confidential guest configured using IGVM then the IGVM
> + * configuration will include the system firmware. In this case do not
> + * initialise PC system firmware.
> + */
> + if (!cgs_is_igvm(machine->cgs)) {
> + /* Initialize PC system firmware */
> + pc_system_firmware_init(pcms, rom_memory);
> + }
If the user has given explicit pflash config I think we should be
reporting an error for the invalid configuration, rather than
silently ignoring their mistake.
>
> option_rom_mr = g_malloc(sizeof(*option_rom_mr));
> memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
> --
> 2.43.0
>
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 6/9] i386/pc: Skip initialization of system FW when using IGVM
2024-03-01 16:54 ` Daniel P. Berrangé
@ 2024-03-12 12:04 ` Roy Hopkins
0 siblings, 0 replies; 35+ messages in thread
From: Roy Hopkins @ 2024-03-12 12:04 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
On Fri, 2024-03-01 at 16:54 +0000, Daniel P. Berrangé wrote:
> On Tue, Feb 27, 2024 at 02:50:12PM +0000, Roy Hopkins wrote:
> > When using an IGVM file the configuration of the system firmware is
> > defined by IGVM directives contained in the file. Therefore the default
> > system firmware should not be initialized when an IGVM file has been
> > provided.
> >
> > This commit checks to see if an IGVM file has been provided and, if it
> > has then the standard system firmware initialization is skipped and any
> > prepared flash devices are cleaned up.
> >
> > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> > ---
> > hw/i386/pc.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index f8eb684a49..17bb211708 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -63,6 +63,7 @@
> > #include "e820_memory_layout.h"
> > #include "trace.h"
> > #include CONFIG_DEVICES
> > +#include "exec/confidential-guest-support.h"
> >
> > #ifdef CONFIG_XEN_EMU
> > #include "hw/xen/xen-legacy-backend.h"
> > @@ -1023,8 +1024,15 @@ void pc_memory_init(PCMachineState *pcms,
> > }
> > }
> >
> > - /* Initialize PC system firmware */
> > - pc_system_firmware_init(pcms, rom_memory);
> > + /*
> > + * If this is a confidential guest configured using IGVM then the IGVM
> > + * configuration will include the system firmware. In this case do not
> > + * initialise PC system firmware.
> > + */
> > + if (!cgs_is_igvm(machine->cgs)) {
> > + /* Initialize PC system firmware */
> > + pc_system_firmware_init(pcms, rom_memory);
> > + }
>
> If the user has given explicit pflash config I think we should be
> reporting an error for the invalid configuration, rather than
> silently ignoring their mistake.
>
Ok. In that case, I'll replace this patch and move the check into
pc_system_firmware_init() in "pc_sysfw.c". I can then check for the presence of
an IGVM file after the firmware configuration has completed, generating an error
if pflash has been configured when an IGVM file is provided.
> >
> > option_rom_mr = g_malloc(sizeof(*option_rom_mr));
> > memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
> > --
> > 2.43.0
> >
> >
>
> With regards,
> Daniel
Many thanks,
Roy
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 6/9] i386/pc: Skip initialization of system FW when using IGVM
2024-02-27 14:50 ` [PATCH 6/9] i386/pc: Skip initialization of system FW when using IGVM Roy Hopkins
2024-03-01 16:54 ` Daniel P. Berrangé
@ 2024-03-27 13:28 ` Ani Sinha
2024-03-27 14:13 ` Roy Hopkins
1 sibling, 1 reply; 35+ messages in thread
From: Ani Sinha @ 2024-03-27 13:28 UTC (permalink / raw)
To: Roy Hopkins
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel, Gerd Hoffmann,
Vitaly Kuznetsov
> On 27 Feb 2024, at 20:20, Roy Hopkins <roy.hopkins@suse.com> wrote:
>
> When using an IGVM file the configuration of the system firmware is
> defined by IGVM directives contained in the file. Therefore the default
> system firmware should not be initialized when an IGVM file has been
> provided.
>
> This commit checks to see if an IGVM file has been provided and, if it
> has then the standard system firmware initialization is skipped and any
> prepared flash devices are cleaned up.
>
> Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> ---
> hw/i386/pc.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f8eb684a49..17bb211708 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -63,6 +63,7 @@
> #include "e820_memory_layout.h"
> #include "trace.h"
> #include CONFIG_DEVICES
> +#include "exec/confidential-guest-support.h"
>
> #ifdef CONFIG_XEN_EMU
> #include "hw/xen/xen-legacy-backend.h"
> @@ -1023,8 +1024,15 @@ void pc_memory_init(PCMachineState *pcms,
> }
> }
>
> - /* Initialize PC system firmware */
> - pc_system_firmware_init(pcms, rom_memory);
> + /*
> + * If this is a confidential guest configured using IGVM then the IGVM
> + * configuration will include the system firmware. In this case do not
> + * initialise PC system firmware.
> + */
> + if (!cgs_is_igvm(machine->cgs)) {
> + /* Initialize PC system firmware */
> + pc_system_firmware_init(pcms, rom_memory);
> + }
Ok so this makes QEMU mot load the default fw as provided in the QEMU command line. It does not specify how the packaged fw specified within IGVM would be processed and loaded. Am I understanding this right?
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 6/9] i386/pc: Skip initialization of system FW when using IGVM
2024-03-27 13:28 ` Ani Sinha
@ 2024-03-27 14:13 ` Roy Hopkins
2024-03-28 10:34 ` Ani Sinha
0 siblings, 1 reply; 35+ messages in thread
From: Roy Hopkins @ 2024-03-27 14:13 UTC (permalink / raw)
To: Ani Sinha
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel, Gerd Hoffmann,
Vitaly Kuznetsov
On Wed, 2024-03-27 at 18:58 +0530, Ani Sinha wrote:
>
>
> > On 27 Feb 2024, at 20:20, Roy Hopkins <roy.hopkins@suse.com> wrote:
> >
> > When using an IGVM file the configuration of the system firmware is
> > defined by IGVM directives contained in the file. Therefore the default
> > system firmware should not be initialized when an IGVM file has been
> > provided.
> >
> > This commit checks to see if an IGVM file has been provided and, if it
> > has then the standard system firmware initialization is skipped and any
> > prepared flash devices are cleaned up.
> >
> > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> > ---
> > hw/i386/pc.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index f8eb684a49..17bb211708 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -63,6 +63,7 @@
> > #include "e820_memory_layout.h"
> > #include "trace.h"
> > #include CONFIG_DEVICES
> > +#include "exec/confidential-guest-support.h"
> >
> > #ifdef CONFIG_XEN_EMU
> > #include "hw/xen/xen-legacy-backend.h"
> > @@ -1023,8 +1024,15 @@ void pc_memory_init(PCMachineState *pcms,
> > }
> > }
> >
> > - /* Initialize PC system firmware */
> > - pc_system_firmware_init(pcms, rom_memory);
> > + /*
> > + * If this is a confidential guest configured using IGVM then the IGVM
> > + * configuration will include the system firmware. In this case do not
> > + * initialise PC system firmware.
> > + */
> > + if (!cgs_is_igvm(machine->cgs)) {
> > + /* Initialize PC system firmware */
> > + pc_system_firmware_init(pcms, rom_memory);
> > + }
>
> Ok so this makes QEMU mot load the default fw as provided in the QEMU command
> line. It does not specify how the packaged fw specified within IGVM would be
> processed and loaded. Am I understanding this right?
>
Yes. Although as suggested by Daniel, I've now changed this so if firmware is
specified on the command line in conflict with the IGVM file then an error is
displayed. The IGVM file itself describes how the firmware binary is populated
into guest memory and launched.
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 6/9] i386/pc: Skip initialization of system FW when using IGVM
2024-03-27 14:13 ` Roy Hopkins
@ 2024-03-28 10:34 ` Ani Sinha
2024-03-28 11:03 ` Ani Sinha
0 siblings, 1 reply; 35+ messages in thread
From: Ani Sinha @ 2024-03-28 10:34 UTC (permalink / raw)
To: Roy Hopkins
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel, Gerd Hoffmann,
Vitaly Kuznetsov
> On 27 Mar 2024, at 19:43, Roy Hopkins <roy.hopkins@suse.com> wrote:
>
> On Wed, 2024-03-27 at 18:58 +0530, Ani Sinha wrote:
>>
>>
>>> On 27 Feb 2024, at 20:20, Roy Hopkins <roy.hopkins@suse.com> wrote:
>>>
>>> When using an IGVM file the configuration of the system firmware is
>>> defined by IGVM directives contained in the file. Therefore the default
>>> system firmware should not be initialized when an IGVM file has been
>>> provided.
>>>
>>> This commit checks to see if an IGVM file has been provided and, if it
>>> has then the standard system firmware initialization is skipped and any
>>> prepared flash devices are cleaned up.
>>>
>>> Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
>>> ---
>>> hw/i386/pc.c | 12 ++++++++++--
>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index f8eb684a49..17bb211708 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -63,6 +63,7 @@
>>> #include "e820_memory_layout.h"
>>> #include "trace.h"
>>> #include CONFIG_DEVICES
>>> +#include "exec/confidential-guest-support.h"
>>>
>>> #ifdef CONFIG_XEN_EMU
>>> #include "hw/xen/xen-legacy-backend.h"
>>> @@ -1023,8 +1024,15 @@ void pc_memory_init(PCMachineState *pcms,
>>> }
>>> }
>>>
>>> - /* Initialize PC system firmware */
>>> - pc_system_firmware_init(pcms, rom_memory);
>>> + /*
>>> + * If this is a confidential guest configured using IGVM then the IGVM
>>> + * configuration will include the system firmware. In this case do not
>>> + * initialise PC system firmware.
>>> + */
>>> + if (!cgs_is_igvm(machine->cgs)) {
>>> + /* Initialize PC system firmware */
>>> + pc_system_firmware_init(pcms, rom_memory);
>>> + }
>>
>> Ok so this makes QEMU mot load the default fw as provided in the QEMU command
>> line. It does not specify how the packaged fw specified within IGVM would be
>> processed and loaded. Am I understanding this right?
>>
> Yes. Although as suggested by Daniel, I've now changed this so if firmware is
> specified on the command line in conflict with the IGVM file then an error is
> displayed.
Does IGVM _must_ mandatorily contain a firmware? If not, then before error is displayed, we should check if the file does have a firmware.
> The IGVM file itself describes how the firmware binary is populated
> into guest memory and launched.
Ok so I looked at the doc here: https://docs.rs/igvm_defs/0.1.7/igvm_defs/
I do not see anything related to firmware in the spec.
Secondly, how the firmware is to be loaded is hypervisor specific. So there must be QEMU specific implementation to load the firmware from IGVM. Secondly, should a new directive (and associated definitions) be introduced that instructs hypervisors to load the firmware present in IGVM?
Something is missing here it seems and I am willing to dive into filling the missing parts.
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 6/9] i386/pc: Skip initialization of system FW when using IGVM
2024-03-28 10:34 ` Ani Sinha
@ 2024-03-28 11:03 ` Ani Sinha
0 siblings, 0 replies; 35+ messages in thread
From: Ani Sinha @ 2024-03-28 11:03 UTC (permalink / raw)
To: Roy Hopkins
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel, Gerd Hoffmann,
Vitaly Kuznetsov
> On 28 Mar 2024, at 16:04, Ani Sinha <anisinha@redhat.com> wrote:
>
>
>
>> On 27 Mar 2024, at 19:43, Roy Hopkins <roy.hopkins@suse.com> wrote:
>>
>> On Wed, 2024-03-27 at 18:58 +0530, Ani Sinha wrote:
>>>
>>>
>>>> On 27 Feb 2024, at 20:20, Roy Hopkins <roy.hopkins@suse.com> wrote:
>>>>
>>>> When using an IGVM file the configuration of the system firmware is
>>>> defined by IGVM directives contained in the file. Therefore the default
>>>> system firmware should not be initialized when an IGVM file has been
>>>> provided.
>>>>
>>>> This commit checks to see if an IGVM file has been provided and, if it
>>>> has then the standard system firmware initialization is skipped and any
>>>> prepared flash devices are cleaned up.
>>>>
>>>> Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
>>>> ---
>>>> hw/i386/pc.c | 12 ++++++++++--
>>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index f8eb684a49..17bb211708 100644
>>>> --- a/hw/i386/pc.c
>>>> +++ b/hw/i386/pc.c
>>>> @@ -63,6 +63,7 @@
>>>> #include "e820_memory_layout.h"
>>>> #include "trace.h"
>>>> #include CONFIG_DEVICES
>>>> +#include "exec/confidential-guest-support.h"
>>>>
>>>> #ifdef CONFIG_XEN_EMU
>>>> #include "hw/xen/xen-legacy-backend.h"
>>>> @@ -1023,8 +1024,15 @@ void pc_memory_init(PCMachineState *pcms,
>>>> }
>>>> }
>>>>
>>>> - /* Initialize PC system firmware */
>>>> - pc_system_firmware_init(pcms, rom_memory);
>>>> + /*
>>>> + * If this is a confidential guest configured using IGVM then the IGVM
>>>> + * configuration will include the system firmware. In this case do not
>>>> + * initialise PC system firmware.
>>>> + */
>>>> + if (!cgs_is_igvm(machine->cgs)) {
>>>> + /* Initialize PC system firmware */
>>>> + pc_system_firmware_init(pcms, rom_memory);
>>>> + }
>>>
>>> Ok so this makes QEMU mot load the default fw as provided in the QEMU command
>>> line. It does not specify how the packaged fw specified within IGVM would be
>>> processed and loaded. Am I understanding this right?
>>>
>> Yes. Although as suggested by Daniel, I've now changed this so if firmware is
>> specified on the command line in conflict with the IGVM file then an error is
>> displayed.
>
> Does IGVM _must_ mandatorily contain a firmware? If not, then before error is displayed, we should check if the file does have a firmware.
>
>> The IGVM file itself describes how the firmware binary is populated
>> into guest memory and launched.
>
> Ok so I looked at the doc here: https://docs.rs/igvm_defs/0.1.7/igvm_defs/
> I do not see anything related to firmware in the spec.
> Secondly, how the firmware is to be loaded is hypervisor specific. So there must be QEMU specific implementation to load the firmware from IGVM. Secondly, should a new directive (and associated definitions) be introduced that instructs hypervisors to load the firmware present in IGVM?
> Something is missing here it seems and I am willing to dive into filling the missing parts.
I guess what I was missing was https://github.com/roy-hopkins/buildigvm/blob/main/src/ovmf_firmware.rs .
Seems something like this is supposed to load ovmf image into memory.
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 7/9] i386/sev: Refactor setting of reset vector and initial CPU state
2024-02-27 14:50 [PATCH 0/9] Introduce support for IGVM files Roy Hopkins
` (5 preceding siblings ...)
2024-02-27 14:50 ` [PATCH 6/9] i386/pc: Skip initialization of system FW when using IGVM Roy Hopkins
@ 2024-02-27 14:50 ` Roy Hopkins
2024-03-01 17:01 ` Daniel P. Berrangé
2024-02-27 14:50 ` [PATCH 8/9] i386/sev: Implement ConfidentialGuestSupport functions for SEV Roy Hopkins
` (3 subsequent siblings)
10 siblings, 1 reply; 35+ messages in thread
From: Roy Hopkins @ 2024-02-27 14:50 UTC (permalink / raw)
To: qemu-devel
Cc: Roy Hopkins, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
When an SEV guest is started, the reset vector and state are
extracted from metadata that is contained in the firmware volume.
In preparation for using IGVM to setup the initial CPU state,
the code has been refactored to populate vmcb_save_area for each
CPU which is then applied during guest startup and CPU reset.
Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
target/i386/sev.c | 288 +++++++++++++++++++++++++++++++++++++++++-----
target/i386/sev.h | 110 ++++++++++++++++++
2 files changed, 369 insertions(+), 29 deletions(-)
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 173de91afe..d6902432fd 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -74,9 +74,7 @@ struct SevGuestState {
SevState state;
gchar *measurement;
- uint32_t reset_cs;
- uint32_t reset_ip;
- bool reset_data_valid;
+ QTAILQ_HEAD(, SevLaunchVmsa) launch_vmsa;
};
#define DEFAULT_GUEST_POLICY 0x1 /* disable debug */
@@ -99,6 +97,12 @@ typedef struct QEMU_PACKED SevHashTableDescriptor {
/* hard code sha256 digest size */
#define HASH_SIZE 32
+/* Convert between SEV-ES VMSA and SegmentCache flags/attributes */
+#define FLAGS_VMSA_TO_SEGCACHE(flags) \
+ ((((flags) & 0xff00) << 12) | (((flags) & 0xff) << 8))
+#define FLAGS_SEGCACHE_TO_VMSA(flags) \
+ ((((flags) & 0xff00) >> 8) | (((flags) & 0xf00000) >> 12))
+
typedef struct QEMU_PACKED SevHashTableEntry {
QemuUUID guid;
uint16_t len;
@@ -125,6 +129,15 @@ typedef struct QEMU_PACKED PaddedSevHashTable {
QEMU_BUILD_BUG_ON(sizeof(PaddedSevHashTable) % 16 != 0);
static SevGuestState *sev_guest;
+
+typedef struct SevLaunchVmsa {
+ QTAILQ_ENTRY(SevLaunchVmsa) next;
+
+ uint16_t cpu_index;
+ uint64_t gpa;
+ struct sev_es_save_area vmsa;
+} SevLaunchVmsa;
+
static Error *sev_mig_blocker;
static const char *const sev_fw_errlist[] = {
@@ -291,6 +304,149 @@ sev_guest_finalize(Object *obj)
{
}
+static void sev_apply_cpu_context(CPUState *cpu)
+{
+ SevGuestState *sev_guest = SEV_GUEST(MACHINE(qdev_get_machine())->cgs);
+ X86CPU *x86;
+ CPUX86State *env;
+ struct SevLaunchVmsa *launch_vmsa;
+
+ /* See if an initial VMSA has been provided for this CPU */
+ QTAILQ_FOREACH(launch_vmsa, &sev_guest->launch_vmsa, next)
+ {
+ if (cpu->cpu_index == launch_vmsa->cpu_index) {
+ x86 = X86_CPU(cpu);
+ env = &x86->env;
+
+ /*
+ * Ideally we would provide the VMSA directly to kvm which would
+ * ensure that the resulting initial VMSA measurement which is
+ * calculated during KVM_SEV_LAUNCH_UPDATE_VMSA is calculated from
+ * exactly what we provide here. Currently this is not possible so
+ * we need to copy the parts of the VMSA structure that we currently
+ * support into the CPU state.
+ */
+ cpu_load_efer(env, launch_vmsa->vmsa.efer);
+ cpu_x86_update_cr4(env, launch_vmsa->vmsa.cr4);
+ cpu_x86_update_cr0(env, launch_vmsa->vmsa.cr0);
+ cpu_x86_update_cr3(env, launch_vmsa->vmsa.cr3);
+
+ cpu_x86_load_seg_cache(
+ env, R_CS, launch_vmsa->vmsa.cs.selector,
+ launch_vmsa->vmsa.cs.base, launch_vmsa->vmsa.cs.limit,
+ FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.cs.attrib));
+ cpu_x86_load_seg_cache(
+ env, R_DS, launch_vmsa->vmsa.ds.selector,
+ launch_vmsa->vmsa.ds.base, launch_vmsa->vmsa.ds.limit,
+ FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.ds.attrib));
+ cpu_x86_load_seg_cache(
+ env, R_ES, launch_vmsa->vmsa.es.selector,
+ launch_vmsa->vmsa.es.base, launch_vmsa->vmsa.es.limit,
+ FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.es.attrib));
+ cpu_x86_load_seg_cache(
+ env, R_FS, launch_vmsa->vmsa.fs.selector,
+ launch_vmsa->vmsa.fs.base, launch_vmsa->vmsa.fs.limit,
+ FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.fs.attrib));
+ cpu_x86_load_seg_cache(
+ env, R_GS, launch_vmsa->vmsa.gs.selector,
+ launch_vmsa->vmsa.gs.base, launch_vmsa->vmsa.gs.limit,
+ FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.gs.attrib));
+ cpu_x86_load_seg_cache(
+ env, R_SS, launch_vmsa->vmsa.ss.selector,
+ launch_vmsa->vmsa.ss.base, launch_vmsa->vmsa.ss.limit,
+ FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.ss.attrib));
+
+ env->gdt.base = launch_vmsa->vmsa.gdtr.base;
+ env->gdt.limit = launch_vmsa->vmsa.gdtr.limit;
+ env->idt.base = launch_vmsa->vmsa.idtr.base;
+ env->idt.limit = launch_vmsa->vmsa.idtr.limit;
+
+ env->regs[R_EAX] = launch_vmsa->vmsa.rax;
+ env->regs[R_ECX] = launch_vmsa->vmsa.rcx;
+ env->regs[R_EDX] = launch_vmsa->vmsa.rdx;
+ env->regs[R_EBX] = launch_vmsa->vmsa.rbx;
+ env->regs[R_ESP] = launch_vmsa->vmsa.rsp;
+ env->regs[R_EBP] = launch_vmsa->vmsa.rbp;
+ env->regs[R_ESI] = launch_vmsa->vmsa.rsi;
+ env->regs[R_EDI] = launch_vmsa->vmsa.rdi;
+#ifdef TARGET_X86_64
+ env->regs[R_R8] = launch_vmsa->vmsa.r8;
+ env->regs[R_R9] = launch_vmsa->vmsa.r9;
+ env->regs[R_R10] = launch_vmsa->vmsa.r10;
+ env->regs[R_R11] = launch_vmsa->vmsa.r11;
+ env->regs[R_R12] = launch_vmsa->vmsa.r12;
+ env->regs[R_R13] = launch_vmsa->vmsa.r13;
+ env->regs[R_R14] = launch_vmsa->vmsa.r14;
+ env->regs[R_R15] = launch_vmsa->vmsa.r15;
+#endif
+ env->eip = launch_vmsa->vmsa.rip;
+ break;
+ }
+ }
+}
+
+static int sev_set_cpu_context(uint16_t cpu_index, const void *ctx,
+ uint32_t ctx_len, hwaddr gpa)
+{
+ SevGuestState *sev_guest = SEV_GUEST(MACHINE(qdev_get_machine())->cgs);
+ SevLaunchVmsa *launch_vmsa;
+ CPUState *cpu;
+ bool exists = false;
+
+ /*
+ * Setting the CPU context is only supported for SEV-ES. The context buffer
+ * will contain a sev_es_save_area from the Linux kernel which is defined by
+ * "Table B-4. VMSA Layout, State Save Area for SEV-ES" in the AMD64 APM,
+ * Volume 2.
+ */
+
+ if (!sev_es_enabled()) {
+ error_report("SEV: unable to set CPU context: Not supported");
+ return 1;
+ }
+
+ if (ctx_len < sizeof(struct sev_es_save_area)) {
+ error_report("SEV: unable to set CPU context: "
+ "Invalid context provided");
+ return 1;
+ }
+
+ cpu = qemu_get_cpu(cpu_index);
+ if (!cpu) {
+ error_report("SEV: unable to set CPU context for out of bounds "
+ "CPU index %d", cpu_index);
+ return 1;
+ }
+
+ /*
+ * If the context of this VP has already been set then replace it with the
+ * new context.
+ */
+ QTAILQ_FOREACH(launch_vmsa, &sev_guest->launch_vmsa, next)
+ {
+ if (cpu_index == launch_vmsa->cpu_index) {
+ launch_vmsa->gpa = gpa;
+ memcpy(&launch_vmsa->vmsa, ctx, sizeof(launch_vmsa->vmsa));
+ exists = true;
+ break;
+ }
+ }
+
+ if (!exists) {
+ /* New VP context */
+ launch_vmsa = g_new0(SevLaunchVmsa, 1);
+ memcpy(&launch_vmsa->vmsa, ctx, sizeof(launch_vmsa->vmsa));
+ launch_vmsa->cpu_index = cpu_index;
+ launch_vmsa->gpa = gpa;
+ QTAILQ_INSERT_TAIL(&sev_guest->launch_vmsa, launch_vmsa, next);
+ }
+
+ /* Synchronise the VMSA with the current CPU state */
+ sev_apply_cpu_context(cpu);
+
+ return 0;
+}
+
static char *
sev_guest_get_session_file(Object *obj, Error **errp)
{
@@ -394,6 +550,7 @@ sev_guest_instance_init(Object *obj)
object_property_add_uint32_ptr(obj, "reduced-phys-bits",
&sev->reduced_phys_bits,
OBJ_PROP_FLAG_READWRITE);
+ QTAILQ_INIT(&sev->launch_vmsa);
}
/* sev guest info */
@@ -784,6 +941,16 @@ static int
sev_launch_update_vmsa(SevGuestState *sev)
{
int ret, fw_error;
+ CPUState *cpu;
+
+ /*
+ * The initial CPU state is measured as part of KVM_SEV_LAUNCH_UPDATE_VMSA.
+ * Synchronise the CPU state to any provided launch VMSA structures.
+ */
+ CPU_FOREACH(cpu) {
+ sev_apply_cpu_context(cpu);
+ }
+
ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL, &fw_error);
if (ret) {
@@ -1196,34 +1363,99 @@ sev_es_find_reset_vector(void *flash_ptr, uint64_t flash_size,
return sev_es_parse_reset_block(info, addr);
}
-void sev_es_set_reset_vector(CPUState *cpu)
-{
- X86CPU *x86;
- CPUX86State *env;
- /* Only update if we have valid reset information */
- if (!sev_guest || !sev_guest->reset_data_valid) {
- return;
- }
+static void seg_to_vmsa(const SegmentCache *cpu_seg, struct vmcb_seg *vmsa_seg)
+{
+ vmsa_seg->selector = cpu_seg->selector;
+ vmsa_seg->base = cpu_seg->base;
+ vmsa_seg->limit = cpu_seg->limit;
+ vmsa_seg->attrib = FLAGS_SEGCACHE_TO_VMSA(cpu_seg->flags);
+}
- /* Do not update the BSP reset state */
- if (cpu->cpu_index == 0) {
- return;
- }
+static void initialize_vmsa(const CPUState *cpu, struct sev_es_save_area *vmsa)
+{
+ const X86CPU *x86 = X86_CPU(cpu);
+ const CPUX86State *env = &x86->env;
- x86 = X86_CPU(cpu);
- env = &x86->env;
+ /*
+ * Initialize the SEV-ES save area from the current state of
+ * the CPU. The entire state does not need to be copied, only the state
+ * that is copied back to the CPUState in sev_apply_cpu_context.
+ */
+ memset(vmsa, 0, sizeof(struct sev_es_save_area));
+ vmsa->efer = env->efer;
+ vmsa->cr0 = env->cr[0];
+ vmsa->cr3 = env->cr[3];
+ vmsa->cr4 = env->cr[4];
+
+ seg_to_vmsa(&env->segs[R_CS], &vmsa->cs);
+ seg_to_vmsa(&env->segs[R_DS], &vmsa->ds);
+ seg_to_vmsa(&env->segs[R_ES], &vmsa->es);
+ seg_to_vmsa(&env->segs[R_FS], &vmsa->fs);
+ seg_to_vmsa(&env->segs[R_GS], &vmsa->gs);
+ seg_to_vmsa(&env->segs[R_SS], &vmsa->ss);
+
+ seg_to_vmsa(&env->gdt, &vmsa->gdtr);
+ seg_to_vmsa(&env->idt, &vmsa->idtr);
+
+ vmsa->rax = env->regs[R_EAX];
+ vmsa->rcx = env->regs[R_ECX];
+ vmsa->rdx = env->regs[R_EDX];
+ vmsa->rbx = env->regs[R_EBX];
+ vmsa->rsp = env->regs[R_ESP];
+ vmsa->rbp = env->regs[R_EBP];
+ vmsa->rsi = env->regs[R_ESI];
+ vmsa->rdi = env->regs[R_EDI];
+
+#ifdef TARGET_X86_64
+ vmsa->r8 = env->regs[R_R8];
+ vmsa->r9 = env->regs[R_R9];
+ vmsa->r10 = env->regs[R_R10];
+ vmsa->r11 = env->regs[R_R11];
+ vmsa->r12 = env->regs[R_R12];
+ vmsa->r13 = env->regs[R_R13];
+ vmsa->r14 = env->regs[R_R14];
+ vmsa->r15 = env->regs[R_R15];
+#endif
+
+ vmsa->rip = env->eip;
+}
- cpu_x86_load_seg_cache(env, R_CS, 0xf000, sev_guest->reset_cs, 0xffff,
- DESC_P_MASK | DESC_S_MASK | DESC_CS_MASK |
- DESC_R_MASK | DESC_A_MASK);
+static void sev_es_set_ap_context(uint32_t reset_addr)
+{
+ CPUState *cpu;
+ struct sev_es_save_area vmsa;
+ SegmentCache cs;
+
+ cs.selector = 0xf000;
+ cs.base = reset_addr & 0xffff0000;
+ cs.limit = 0xffff;
+ cs.flags = DESC_P_MASK | DESC_S_MASK | DESC_CS_MASK | DESC_R_MASK |
+ DESC_A_MASK;
+
+ CPU_FOREACH(cpu) {
+ if (cpu->cpu_index == 0) {
+ /* Do not update the BSP reset state */
+ continue;
+ }
+ initialize_vmsa(cpu, &vmsa);
+ seg_to_vmsa(&cs, &vmsa.cs);
+ vmsa.rip = reset_addr & 0x0000ffff;
+ sev_set_cpu_context(cpu->cpu_index, &vmsa,
+ sizeof(struct sev_es_save_area), 0);
+ sev_apply_cpu_context(cpu);
+ }
+}
- env->eip = sev_guest->reset_ip;
+void sev_es_set_reset_vector(CPUState *cpu)
+{
+ if (sev_enabled()) {
+ sev_apply_cpu_context(cpu);
+ }
}
int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
{
- CPUState *cpu;
uint32_t addr;
int ret;
@@ -1238,14 +1470,12 @@ int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
return ret;
}
+ /*
+ * The reset vector is saved into a CPU context for each AP but not for
+ * the BSP. This is applied during guest startup or when the CPU is reset.
+ */
if (addr) {
- sev_guest->reset_cs = addr & 0xffff0000;
- sev_guest->reset_ip = addr & 0x0000ffff;
- sev_guest->reset_data_valid = true;
-
- CPU_FOREACH(cpu) {
- sev_es_set_reset_vector(cpu);
- }
+ sev_es_set_ap_context(addr);
}
return 0;
diff --git a/target/i386/sev.h b/target/i386/sev.h
index e7499c95b1..1fd896d896 100644
--- a/target/i386/sev.h
+++ b/target/i386/sev.h
@@ -38,6 +38,116 @@ typedef struct SevKernelLoaderContext {
size_t cmdline_size;
} SevKernelLoaderContext;
+/* Save area definition for SEV-ES and SEV-SNP guests */
+struct QEMU_PACKED sev_es_save_area {
+ struct vmcb_seg es;
+ struct vmcb_seg cs;
+ struct vmcb_seg ss;
+ struct vmcb_seg ds;
+ struct vmcb_seg fs;
+ struct vmcb_seg gs;
+ struct vmcb_seg gdtr;
+ struct vmcb_seg ldtr;
+ struct vmcb_seg idtr;
+ struct vmcb_seg tr;
+ uint64_t vmpl0_ssp;
+ uint64_t vmpl1_ssp;
+ uint64_t vmpl2_ssp;
+ uint64_t vmpl3_ssp;
+ uint64_t u_cet;
+ uint8_t reserved_0xc8[2];
+ uint8_t vmpl;
+ uint8_t cpl;
+ uint8_t reserved_0xcc[4];
+ uint64_t efer;
+ uint8_t reserved_0xd8[104];
+ uint64_t xss;
+ uint64_t cr4;
+ uint64_t cr3;
+ uint64_t cr0;
+ uint64_t dr7;
+ uint64_t dr6;
+ uint64_t rflags;
+ uint64_t rip;
+ uint64_t dr0;
+ uint64_t dr1;
+ uint64_t dr2;
+ uint64_t dr3;
+ uint64_t dr0_addr_mask;
+ uint64_t dr1_addr_mask;
+ uint64_t dr2_addr_mask;
+ uint64_t dr3_addr_mask;
+ uint8_t reserved_0x1c0[24];
+ uint64_t rsp;
+ uint64_t s_cet;
+ uint64_t ssp;
+ uint64_t isst_addr;
+ uint64_t rax;
+ uint64_t star;
+ uint64_t lstar;
+ uint64_t cstar;
+ uint64_t sfmask;
+ uint64_t kernel_gs_base;
+ uint64_t sysenter_cs;
+ uint64_t sysenter_esp;
+ uint64_t sysenter_eip;
+ uint64_t cr2;
+ uint8_t reserved_0x248[32];
+ uint64_t g_pat;
+ uint64_t dbgctl;
+ uint64_t br_from;
+ uint64_t br_to;
+ uint64_t last_excp_from;
+ uint64_t last_excp_to;
+ uint8_t reserved_0x298[80];
+ uint32_t pkru;
+ uint32_t tsc_aux;
+ uint8_t reserved_0x2f0[24];
+ uint64_t rcx;
+ uint64_t rdx;
+ uint64_t rbx;
+ uint64_t reserved_0x320; /* rsp already available at 0x01d8 */
+ uint64_t rbp;
+ uint64_t rsi;
+ uint64_t rdi;
+ uint64_t r8;
+ uint64_t r9;
+ uint64_t r10;
+ uint64_t r11;
+ uint64_t r12;
+ uint64_t r13;
+ uint64_t r14;
+ uint64_t r15;
+ uint8_t reserved_0x380[16];
+ uint64_t guest_exit_info_1;
+ uint64_t guest_exit_info_2;
+ uint64_t guest_exit_int_info;
+ uint64_t guest_nrip;
+ uint64_t sev_features;
+ uint64_t vintr_ctrl;
+ uint64_t guest_exit_code;
+ uint64_t virtual_tom;
+ uint64_t tlb_id;
+ uint64_t pcpu_id;
+ uint64_t event_inj;
+ uint64_t xcr0;
+ uint8_t reserved_0x3f0[16];
+
+ /* Floating point area */
+ uint64_t x87_dp;
+ uint32_t mxcsr;
+ uint16_t x87_ftw;
+ uint16_t x87_fsw;
+ uint16_t x87_fcw;
+ uint16_t x87_fop;
+ uint16_t x87_ds;
+ uint16_t x87_cs;
+ uint64_t x87_rip;
+ uint8_t fpreg_x87[80];
+ uint8_t fpreg_xmm[256];
+ uint8_t fpreg_ymm[256];
+};
+
#ifdef CONFIG_SEV
bool sev_enabled(void);
bool sev_es_enabled(void);
--
2.43.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 7/9] i386/sev: Refactor setting of reset vector and initial CPU state
2024-02-27 14:50 ` [PATCH 7/9] i386/sev: Refactor setting of reset vector and initial CPU state Roy Hopkins
@ 2024-03-01 17:01 ` Daniel P. Berrangé
2024-03-12 15:45 ` Roy Hopkins
0 siblings, 1 reply; 35+ messages in thread
From: Daniel P. Berrangé @ 2024-03-01 17:01 UTC (permalink / raw)
To: Roy Hopkins
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
On Tue, Feb 27, 2024 at 02:50:13PM +0000, Roy Hopkins wrote:
> When an SEV guest is started, the reset vector and state are
> extracted from metadata that is contained in the firmware volume.
>
> In preparation for using IGVM to setup the initial CPU state,
> the code has been refactored to populate vmcb_save_area for each
> CPU which is then applied during guest startup and CPU reset.
>
> Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> ---
> target/i386/sev.c | 288 +++++++++++++++++++++++++++++++++++++++++-----
> target/i386/sev.h | 110 ++++++++++++++++++
> 2 files changed, 369 insertions(+), 29 deletions(-)
>
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 173de91afe..d6902432fd 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -74,9 +74,7 @@ struct SevGuestState {
> SevState state;
> gchar *measurement;
>
> - uint32_t reset_cs;
> - uint32_t reset_ip;
> - bool reset_data_valid;
> + QTAILQ_HEAD(, SevLaunchVmsa) launch_vmsa;
> };
>
> #define DEFAULT_GUEST_POLICY 0x1 /* disable debug */
> @@ -99,6 +97,12 @@ typedef struct QEMU_PACKED SevHashTableDescriptor {
> /* hard code sha256 digest size */
> #define HASH_SIZE 32
>
> +/* Convert between SEV-ES VMSA and SegmentCache flags/attributes */
> +#define FLAGS_VMSA_TO_SEGCACHE(flags) \
> + ((((flags) & 0xff00) << 12) | (((flags) & 0xff) << 8))
> +#define FLAGS_SEGCACHE_TO_VMSA(flags) \
> + ((((flags) & 0xff00) >> 8) | (((flags) & 0xf00000) >> 12))
> +
> typedef struct QEMU_PACKED SevHashTableEntry {
> QemuUUID guid;
> uint16_t len;
> @@ -125,6 +129,15 @@ typedef struct QEMU_PACKED PaddedSevHashTable {
> QEMU_BUILD_BUG_ON(sizeof(PaddedSevHashTable) % 16 != 0);
>
> static SevGuestState *sev_guest;
> +
> +typedef struct SevLaunchVmsa {
> + QTAILQ_ENTRY(SevLaunchVmsa) next;
> +
> + uint16_t cpu_index;
> + uint64_t gpa;
> + struct sev_es_save_area vmsa;
> +} SevLaunchVmsa;
> +
> static Error *sev_mig_blocker;
>
> static const char *const sev_fw_errlist[] = {
> @@ -291,6 +304,149 @@ sev_guest_finalize(Object *obj)
> {
> }
>
> +static void sev_apply_cpu_context(CPUState *cpu)
> +{
> + SevGuestState *sev_guest = SEV_GUEST(MACHINE(qdev_get_machine())->cgs);
> + X86CPU *x86;
> + CPUX86State *env;
> + struct SevLaunchVmsa *launch_vmsa;
> +
> + /* See if an initial VMSA has been provided for this CPU */
> + QTAILQ_FOREACH(launch_vmsa, &sev_guest->launch_vmsa, next)
> + {
> + if (cpu->cpu_index == launch_vmsa->cpu_index) {
> + x86 = X86_CPU(cpu);
> + env = &x86->env;
> +
> + /*
> + * Ideally we would provide the VMSA directly to kvm which would
> + * ensure that the resulting initial VMSA measurement which is
> + * calculated during KVM_SEV_LAUNCH_UPDATE_VMSA is calculated from
> + * exactly what we provide here. Currently this is not possible so
> + * we need to copy the parts of the VMSA structure that we currently
> + * support into the CPU state.
> + */
This sounds like it is saying that the code is not honouring
everything in the VMSA defiend by the IGVM file ?
If so, that is pretty awkward. The VMSA is effectively an external
ABI between QEMU and the guest owner (or whatever is validating
guest attestation reports for them), and thus predictability and
stability of this over time is critical.
We don't want the attestation process to be dependent/variable on
the particular QEMU/KVM version, because any upgrade to QEMU/KVM
could then alter the effective VMSA that the guest owner sees.
We've already suffered pain in this respect not long ago when the
kernel arbitrarily changed a default setting which altered the
VMSA it exposed, breaking existing apps that validate attestation.
What will it take to provide the full VMSA to KVM, so that we can
guarantee to the guest owner than the VMSA for the guest is going
to perfectly match what their IGVM defined ?
> + cpu_load_efer(env, launch_vmsa->vmsa.efer);
> + cpu_x86_update_cr4(env, launch_vmsa->vmsa.cr4);
> + cpu_x86_update_cr0(env, launch_vmsa->vmsa.cr0);
> + cpu_x86_update_cr3(env, launch_vmsa->vmsa.cr3);
> +
> + cpu_x86_load_seg_cache(
> + env, R_CS, launch_vmsa->vmsa.cs.selector,
> + launch_vmsa->vmsa.cs.base, launch_vmsa->vmsa.cs.limit,
> + FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.cs.attrib));
> + cpu_x86_load_seg_cache(
> + env, R_DS, launch_vmsa->vmsa.ds.selector,
> + launch_vmsa->vmsa.ds.base, launch_vmsa->vmsa.ds.limit,
> + FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.ds.attrib));
> + cpu_x86_load_seg_cache(
> + env, R_ES, launch_vmsa->vmsa.es.selector,
> + launch_vmsa->vmsa.es.base, launch_vmsa->vmsa.es.limit,
> + FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.es.attrib));
> + cpu_x86_load_seg_cache(
> + env, R_FS, launch_vmsa->vmsa.fs.selector,
> + launch_vmsa->vmsa.fs.base, launch_vmsa->vmsa.fs.limit,
> + FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.fs.attrib));
> + cpu_x86_load_seg_cache(
> + env, R_GS, launch_vmsa->vmsa.gs.selector,
> + launch_vmsa->vmsa.gs.base, launch_vmsa->vmsa.gs.limit,
> + FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.gs.attrib));
> + cpu_x86_load_seg_cache(
> + env, R_SS, launch_vmsa->vmsa.ss.selector,
> + launch_vmsa->vmsa.ss.base, launch_vmsa->vmsa.ss.limit,
> + FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.ss.attrib));
> +
> + env->gdt.base = launch_vmsa->vmsa.gdtr.base;
> + env->gdt.limit = launch_vmsa->vmsa.gdtr.limit;
> + env->idt.base = launch_vmsa->vmsa.idtr.base;
> + env->idt.limit = launch_vmsa->vmsa.idtr.limit;
> +
> + env->regs[R_EAX] = launch_vmsa->vmsa.rax;
> + env->regs[R_ECX] = launch_vmsa->vmsa.rcx;
> + env->regs[R_EDX] = launch_vmsa->vmsa.rdx;
> + env->regs[R_EBX] = launch_vmsa->vmsa.rbx;
> + env->regs[R_ESP] = launch_vmsa->vmsa.rsp;
> + env->regs[R_EBP] = launch_vmsa->vmsa.rbp;
> + env->regs[R_ESI] = launch_vmsa->vmsa.rsi;
> + env->regs[R_EDI] = launch_vmsa->vmsa.rdi;
> +#ifdef TARGET_X86_64
> + env->regs[R_R8] = launch_vmsa->vmsa.r8;
> + env->regs[R_R9] = launch_vmsa->vmsa.r9;
> + env->regs[R_R10] = launch_vmsa->vmsa.r10;
> + env->regs[R_R11] = launch_vmsa->vmsa.r11;
> + env->regs[R_R12] = launch_vmsa->vmsa.r12;
> + env->regs[R_R13] = launch_vmsa->vmsa.r13;
> + env->regs[R_R14] = launch_vmsa->vmsa.r14;
> + env->regs[R_R15] = launch_vmsa->vmsa.r15;
> +#endif
> + env->eip = launch_vmsa->vmsa.rip;
> + break;
> + }
> + }
> +}
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 7/9] i386/sev: Refactor setting of reset vector and initial CPU state
2024-03-01 17:01 ` Daniel P. Berrangé
@ 2024-03-12 15:45 ` Roy Hopkins
2024-03-12 16:12 ` Daniel P. Berrangé
0 siblings, 1 reply; 35+ messages in thread
From: Roy Hopkins @ 2024-03-12 15:45 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
On Fri, 2024-03-01 at 17:01 +0000, Daniel P. Berrangé wrote:
> On Tue, Feb 27, 2024 at 02:50:13PM +0000, Roy Hopkins wrote:
> > When an SEV guest is started, the reset vector and state are
> > extracted from metadata that is contained in the firmware volume.
> >
> > In preparation for using IGVM to setup the initial CPU state,
> > the code has been refactored to populate vmcb_save_area for each
> > CPU which is then applied during guest startup and CPU reset.
> >
> > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> > ---
> > target/i386/sev.c | 288 +++++++++++++++++++++++++++++++++++++++++-----
> > target/i386/sev.h | 110 ++++++++++++++++++
> > 2 files changed, 369 insertions(+), 29 deletions(-)
> >
> > diff --git a/target/i386/sev.c b/target/i386/sev.c
> > index 173de91afe..d6902432fd 100644
> > --- a/target/i386/sev.c
> > +++ b/target/i386/sev.c
> > @@ -74,9 +74,7 @@ struct SevGuestState {
> > SevState state;
> > gchar *measurement;
> >
> > - uint32_t reset_cs;
> > - uint32_t reset_ip;
> > - bool reset_data_valid;
> > + QTAILQ_HEAD(, SevLaunchVmsa) launch_vmsa;
> > };
> >
> > #define DEFAULT_GUEST_POLICY 0x1 /* disable debug */
> > @@ -99,6 +97,12 @@ typedef struct QEMU_PACKED SevHashTableDescriptor {
> > /* hard code sha256 digest size */
> > #define HASH_SIZE 32
> >
> > +/* Convert between SEV-ES VMSA and SegmentCache flags/attributes */
> > +#define FLAGS_VMSA_TO_SEGCACHE(flags) \
> > + ((((flags) & 0xff00) << 12) | (((flags) & 0xff) << 8))
> > +#define FLAGS_SEGCACHE_TO_VMSA(flags) \
> > + ((((flags) & 0xff00) >> 8) | (((flags) & 0xf00000) >> 12))
> > +
> > typedef struct QEMU_PACKED SevHashTableEntry {
> > QemuUUID guid;
> > uint16_t len;
> > @@ -125,6 +129,15 @@ typedef struct QEMU_PACKED PaddedSevHashTable {
> > QEMU_BUILD_BUG_ON(sizeof(PaddedSevHashTable) % 16 != 0);
> >
> > static SevGuestState *sev_guest;
> > +
> > +typedef struct SevLaunchVmsa {
> > + QTAILQ_ENTRY(SevLaunchVmsa) next;
> > +
> > + uint16_t cpu_index;
> > + uint64_t gpa;
> > + struct sev_es_save_area vmsa;
> > +} SevLaunchVmsa;
> > +
> > static Error *sev_mig_blocker;
> >
> > static const char *const sev_fw_errlist[] = {
> > @@ -291,6 +304,149 @@ sev_guest_finalize(Object *obj)
> > {
> > }
> >
> > +static void sev_apply_cpu_context(CPUState *cpu)
> > +{
> > + SevGuestState *sev_guest = SEV_GUEST(MACHINE(qdev_get_machine())->cgs);
> > + X86CPU *x86;
> > + CPUX86State *env;
> > + struct SevLaunchVmsa *launch_vmsa;
> > +
> > + /* See if an initial VMSA has been provided for this CPU */
> > + QTAILQ_FOREACH(launch_vmsa, &sev_guest->launch_vmsa, next)
> > + {
> > + if (cpu->cpu_index == launch_vmsa->cpu_index) {
> > + x86 = X86_CPU(cpu);
> > + env = &x86->env;
> > +
> > + /*
> > + * Ideally we would provide the VMSA directly to kvm which
> > would
> > + * ensure that the resulting initial VMSA measurement which is
> > + * calculated during KVM_SEV_LAUNCH_UPDATE_VMSA is calculated
> > from
> > + * exactly what we provide here. Currently this is not possible
> > so
> > + * we need to copy the parts of the VMSA structure that we
> > currently
> > + * support into the CPU state.
> > + */
>
> This sounds like it is saying that the code is not honouring
> everything in the VMSA defiend by the IGVM file ?
>
> If so, that is pretty awkward. The VMSA is effectively an external
> ABI between QEMU and the guest owner (or whatever is validating
> guest attestation reports for them), and thus predictability and
> stability of this over time is critical.
>
> We don't want the attestation process to be dependent/variable on
> the particular QEMU/KVM version, because any upgrade to QEMU/KVM
> could then alter the effective VMSA that the guest owner sees.
>
> We've already suffered pain in this respect not long ago when the
> kernel arbitrarily changed a default setting which altered the
> VMSA it exposed, breaking existing apps that validate attestation.
>
> What will it take to provide the full VMSA to KVM, so that we can
> guarantee to the guest owner than the VMSA for the guest is going
> to perfectly match what their IGVM defined ?
>
Yes, the fact that we have to copy the individual fields from the VMSA to
"CPUX86State" is less than ideal - a problem made worse by the fact that the
kernel does not allow direct control over some of the fields from userspace,
"sev_features" being a good example here where "SVM_SEV_FEAT_DEBUG_SWAP" is
unconditionally added by the kernel.
The kernel VMSA is at least predictable. So, although we cannot yet allow full
flexibility in providing a complete VMSA from QEMU and guarantee it will be
honoured, we could check to see if any settings conflict with those imposed by
the kernel and exit with an error if this is the case. I chose not to implement
for this first series but could easily add a patch to support this. The problem
here is that it ties the version of QEMU to VMSA handling functionality in the
kernel. Any change to the VMSA handling in the kernel would potentially
invalidate the checks in QEMU. The one upside here is that this will easily be
detectable by the attestation measurement not matching the expected measurement
of the IGVM file. But it will be difficult for the user to determine what the
discrepancy is.
The ideal solution is to add or modify a KVM ioctl to allow the VMSA to be set
directly, overriding the state in "CPUX86State". The current
KVM_SEV_LAUNCH_UPDATE_VMSA ioctl triggers the synchronisation of the VMSA but
does not allow it to be specified directly. This could be modified for what we
need. The SEV-SNP kernel patches add KVM_SEV_SNP_LAUNCH_UPDATE which allows a
page type of VMSA to be updated, although the current patch series does not
support using this to set the initial state of the VMSA:
https://lore.kernel.org/lkml/20231230172351.574091-19-michael.roth@amd.com/ I
have experimented with this myself and have successfully modified the SEV-SNP
kernel patches to support directly setting the VMSA from QEMU.
On the other hand, I have also verified that I can indeed measure an IGVM file
loaded using the VMSA synchronisation method currently employed and get a
matching measurement from the SEV attestation report.
What would you suggest is the best way forward for this?
> > + cpu_load_efer(env, launch_vmsa->vmsa.efer);
> > + cpu_x86_update_cr4(env, launch_vmsa->vmsa.cr4);
> > + cpu_x86_update_cr0(env, launch_vmsa->vmsa.cr0);
> > + cpu_x86_update_cr3(env, launch_vmsa->vmsa.cr3);
> > +
> > + cpu_x86_load_seg_cache(
> > + env, R_CS, launch_vmsa->vmsa.cs.selector,
> > + launch_vmsa->vmsa.cs.base, launch_vmsa->vmsa.cs.limit,
> > + FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.cs.attrib));
> > + cpu_x86_load_seg_cache(
> > + env, R_DS, launch_vmsa->vmsa.ds.selector,
> > + launch_vmsa->vmsa.ds.base, launch_vmsa->vmsa.ds.limit,
> > + FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.ds.attrib));
> > + cpu_x86_load_seg_cache(
> > + env, R_ES, launch_vmsa->vmsa.es.selector,
> > + launch_vmsa->vmsa.es.base, launch_vmsa->vmsa.es.limit,
> > + FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.es.attrib));
> > + cpu_x86_load_seg_cache(
> > + env, R_FS, launch_vmsa->vmsa.fs.selector,
> > + launch_vmsa->vmsa.fs.base, launch_vmsa->vmsa.fs.limit,
> > + FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.fs.attrib));
> > + cpu_x86_load_seg_cache(
> > + env, R_GS, launch_vmsa->vmsa.gs.selector,
> > + launch_vmsa->vmsa.gs.base, launch_vmsa->vmsa.gs.limit,
> > + FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.gs.attrib));
> > + cpu_x86_load_seg_cache(
> > + env, R_SS, launch_vmsa->vmsa.ss.selector,
> > + launch_vmsa->vmsa.ss.base, launch_vmsa->vmsa.ss.limit,
> > + FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.ss.attrib));
> > +
> > + env->gdt.base = launch_vmsa->vmsa.gdtr.base;
> > + env->gdt.limit = launch_vmsa->vmsa.gdtr.limit;
> > + env->idt.base = launch_vmsa->vmsa.idtr.base;
> > + env->idt.limit = launch_vmsa->vmsa.idtr.limit;
> > +
> > + env->regs[R_EAX] = launch_vmsa->vmsa.rax;
> > + env->regs[R_ECX] = launch_vmsa->vmsa.rcx;
> > + env->regs[R_EDX] = launch_vmsa->vmsa.rdx;
> > + env->regs[R_EBX] = launch_vmsa->vmsa.rbx;
> > + env->regs[R_ESP] = launch_vmsa->vmsa.rsp;
> > + env->regs[R_EBP] = launch_vmsa->vmsa.rbp;
> > + env->regs[R_ESI] = launch_vmsa->vmsa.rsi;
> > + env->regs[R_EDI] = launch_vmsa->vmsa.rdi;
> > +#ifdef TARGET_X86_64
> > + env->regs[R_R8] = launch_vmsa->vmsa.r8;
> > + env->regs[R_R9] = launch_vmsa->vmsa.r9;
> > + env->regs[R_R10] = launch_vmsa->vmsa.r10;
> > + env->regs[R_R11] = launch_vmsa->vmsa.r11;
> > + env->regs[R_R12] = launch_vmsa->vmsa.r12;
> > + env->regs[R_R13] = launch_vmsa->vmsa.r13;
> > + env->regs[R_R14] = launch_vmsa->vmsa.r14;
> > + env->regs[R_R15] = launch_vmsa->vmsa.r15;
> > +#endif
> > + env->eip = launch_vmsa->vmsa.rip;
> > + break;
> > + }
> > + }
> > +}
>
>
> With regards,
> Daniel
Regards,
Roy
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 7/9] i386/sev: Refactor setting of reset vector and initial CPU state
2024-03-12 15:45 ` Roy Hopkins
@ 2024-03-12 16:12 ` Daniel P. Berrangé
2024-03-18 11:49 ` Roy Hopkins
0 siblings, 1 reply; 35+ messages in thread
From: Daniel P. Berrangé @ 2024-03-12 16:12 UTC (permalink / raw)
To: Roy Hopkins, Paolo Bonzini
Cc: qemu-devel, Marcelo Tosatti, Michael S . Tsirkin, Cornelia Huck,
Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost, Alistair Francis,
Peter Xu, David Hildenbrand, Igor Mammedov, Tom Lendacky,
Michael Roth, Jörg Roedel
On Tue, Mar 12, 2024 at 03:45:20PM +0000, Roy Hopkins wrote:
> On Fri, 2024-03-01 at 17:01 +0000, Daniel P. Berrangé wrote:
> > On Tue, Feb 27, 2024 at 02:50:13PM +0000, Roy Hopkins wrote:
> > > + /*
> > > + * Ideally we would provide the VMSA directly to kvm which
> > > would
> > > + * ensure that the resulting initial VMSA measurement which is
> > > + * calculated during KVM_SEV_LAUNCH_UPDATE_VMSA is calculated
> > > from
> > > + * exactly what we provide here. Currently this is not possible
> > > so
> > > + * we need to copy the parts of the VMSA structure that we
> > > currently
> > > + * support into the CPU state.
> > > + */
> >
> > This sounds like it is saying that the code is not honouring
> > everything in the VMSA defiend by the IGVM file ?
> >
> > If so, that is pretty awkward. The VMSA is effectively an external
> > ABI between QEMU and the guest owner (or whatever is validating
> > guest attestation reports for them), and thus predictability and
> > stability of this over time is critical.
> >
> > We don't want the attestation process to be dependent/variable on
> > the particular QEMU/KVM version, because any upgrade to QEMU/KVM
> > could then alter the effective VMSA that the guest owner sees.
> >
> > We've already suffered pain in this respect not long ago when the
> > kernel arbitrarily changed a default setting which altered the
> > VMSA it exposed, breaking existing apps that validate attestation.
> >
> > What will it take to provide the full VMSA to KVM, so that we can
> > guarantee to the guest owner than the VMSA for the guest is going
> > to perfectly match what their IGVM defined ?
> >
>
> Yes, the fact that we have to copy the individual fields from the VMSA to
> "CPUX86State" is less than ideal - a problem made worse by the fact that the
> kernel does not allow direct control over some of the fields from userspace,
> "sev_features" being a good example here where "SVM_SEV_FEAT_DEBUG_SWAP" is
> unconditionally added by the kernel.
Ah yes, the SVM_SEV_FEAT_DEBUG_SWAP feature is the one I couldn't remember
the name of in my quoted text above, that break our apps when the kernel
suddenly set it by default (thankfully now reverted in Linux with
5abf6dceb066f2b02b225fd561440c98a8062681).
> The kernel VMSA is at least predictable. So, although we cannot yet allow full
> flexibility in providing a complete VMSA from QEMU and guarantee it will be
> honoured, we could check to see if any settings conflict with those imposed by
> the kernel and exit with an error if this is the case. I chose not to implement
> for this first series but could easily add a patch to support this. The problem
> here is that it ties the version of QEMU to VMSA handling functionality in the
> kernel. Any change to the VMSA handling in the kernel would potentially
> invalidate the checks in QEMU. The one upside here is that this will easily be
> detectable by the attestation measurement not matching the expected measurement
> of the IGVM file. But it will be difficult for the user to determine what the
> discrepancy is.
Yes, the difficulty in diagnosis is the big thing I'm worried about from
a distro supportability POV. The DEBUG_SWAP issue caused us a bunch of
pain and that's before CVMs are even widely used.
I agree that hardcoding checks in QEMU is pretty unpleasant, and probably
not something that I'd want us to do. I'd want QEMU to be able to live
query the kernel's default initial VMSA, if it were to be reporting
differences vs the IGVM provided VMSA. I dn't think there's a way to
do that nicely though - i only know of ftrace probes to dump it informally.
I guess if we know & document what subset of the VMSA QEMU /can/ directly
control, that at least narrows down where to look if something does change
or go wrong.
> The ideal solution is to add or modify a KVM ioctl to allow the VMSA to be set
> directly, overriding the state in "CPUX86State". The current
> KVM_SEV_LAUNCH_UPDATE_VMSA ioctl triggers the synchronisation of the VMSA but
> does not allow it to be specified directly. This could be modified for what we
> need. The SEV-SNP kernel patches add KVM_SEV_SNP_LAUNCH_UPDATE which allows a
> page type of VMSA to be updated, although the current patch series does not
> support using this to set the initial state of the VMSA:
> https://lore.kernel.org/lkml/20231230172351.574091-19-michael.roth@amd.com/ I
> have experimented with this myself and have successfully modified the SEV-SNP
> kernel patches to support directly setting the VMSA from QEMU.
>
> On the other hand, I have also verified that I can indeed measure an IGVM file
> loaded using the VMSA synchronisation method currently employed and get a
> matching measurement from the SEV attestation report.
>
> What would you suggest is the best way forward for this?
I'll delegate to Paolo for an opinion on the possiblity of new (or
updated) ioctls to provide the full VMSA data.
If we can't directly set the full VMSA, then next best option is a
more formal way query to VMSA. That way libvirt could report on
what the default initial kernel VMSA state is, which could be useful
debug info for any bug reports.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 7/9] i386/sev: Refactor setting of reset vector and initial CPU state
2024-03-12 16:12 ` Daniel P. Berrangé
@ 2024-03-18 11:49 ` Roy Hopkins
0 siblings, 0 replies; 35+ messages in thread
From: Roy Hopkins @ 2024-03-18 11:49 UTC (permalink / raw)
To: Daniel P. Berrangé, Paolo Bonzini
Cc: qemu-devel, Marcelo Tosatti, Michael S . Tsirkin, Cornelia Huck,
Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost, Alistair Francis,
Peter Xu, David Hildenbrand, Igor Mammedov, Tom Lendacky,
Michael Roth, Jörg Roedel
On Tue, 2024-03-12 at 16:12 +0000, Daniel P. Berrangé wrote:
> On Tue, Mar 12, 2024 at 03:45:20PM +0000, Roy Hopkins wrote:
> > On Fri, 2024-03-01 at 17:01 +0000, Daniel P. Berrangé wrote:
> > > On Tue, Feb 27, 2024 at 02:50:13PM +0000, Roy Hopkins wrote:
> > > > + /*
> > > > + * Ideally we would provide the VMSA directly to kvm which
> > > > would
> > > > + * ensure that the resulting initial VMSA measurement which
> > > > is
> > > > + * calculated during KVM_SEV_LAUNCH_UPDATE_VMSA is
> > > > calculated
> > > > from
> > > > + * exactly what we provide here. Currently this is not
> > > > possible
> > > > so
> > > > + * we need to copy the parts of the VMSA structure that we
> > > > currently
> > > > + * support into the CPU state.
> > > > + */
> > >
> > > This sounds like it is saying that the code is not honouring
> > > everything in the VMSA defiend by the IGVM file ?
> > >
> > > If so, that is pretty awkward. The VMSA is effectively an external
> > > ABI between QEMU and the guest owner (or whatever is validating
> > > guest attestation reports for them), and thus predictability and
> > > stability of this over time is critical.
> > >
> > > We don't want the attestation process to be dependent/variable on
> > > the particular QEMU/KVM version, because any upgrade to QEMU/KVM
> > > could then alter the effective VMSA that the guest owner sees.
> > >
> > > We've already suffered pain in this respect not long ago when the
> > > kernel arbitrarily changed a default setting which altered the
> > > VMSA it exposed, breaking existing apps that validate attestation.
> > >
> > > What will it take to provide the full VMSA to KVM, so that we can
> > > guarantee to the guest owner than the VMSA for the guest is going
> > > to perfectly match what their IGVM defined ?
> > >
> >
> > Yes, the fact that we have to copy the individual fields from the VMSA to
> > "CPUX86State" is less than ideal - a problem made worse by the fact that the
> > kernel does not allow direct control over some of the fields from userspace,
> > "sev_features" being a good example here where "SVM_SEV_FEAT_DEBUG_SWAP" is
> > unconditionally added by the kernel.
>
> Ah yes, the SVM_SEV_FEAT_DEBUG_SWAP feature is the one I couldn't remember
> the name of in my quoted text above, that break our apps when the kernel
> suddenly set it by default (thankfully now reverted in Linux with
> 5abf6dceb066f2b02b225fd561440c98a8062681).
>
> > The kernel VMSA is at least predictable. So, although we cannot yet allow
> > full
> > flexibility in providing a complete VMSA from QEMU and guarantee it will be
> > honoured, we could check to see if any settings conflict with those imposed
> > by
> > the kernel and exit with an error if this is the case. I chose not to
> > implement
> > for this first series but could easily add a patch to support this. The
> > problem
> > here is that it ties the version of QEMU to VMSA handling functionality in
> > the
> > kernel. Any change to the VMSA handling in the kernel would potentially
> > invalidate the checks in QEMU. The one upside here is that this will easily
> > be
> > detectable by the attestation measurement not matching the expected
> > measurement
> > of the IGVM file. But it will be difficult for the user to determine what
> > the
> > discrepancy is.
>
> Yes, the difficulty in diagnosis is the big thing I'm worried about from
> a distro supportability POV. The DEBUG_SWAP issue caused us a bunch of
> pain and that's before CVMs are even widely used.
>
> I agree that hardcoding checks in QEMU is pretty unpleasant, and probably
> not something that I'd want us to do. I'd want QEMU to be able to live
> query the kernel's default initial VMSA, if it were to be reporting
> differences vs the IGVM provided VMSA. I dn't think there's a way to
> do that nicely though - i only know of ftrace probes to dump it informally.
>
> I guess if we know & document what subset of the VMSA QEMU /can/ directly
> control, that at least narrows down where to look if something does change
> or go wrong.
>
Yes, it makes sense to document the subset that can be reliably set by QEMU,
along with any modifications made byt the kernel. Perhaps I should go one step
further and check that the VMSA does not contain any entries beyond what is
copied in "sev_apply_cpu_context()"? If any field other than those explicitly
copied by this function contain a non-zero value then an error is generated. As
you suggest this will limit the scope of any measurement differences to the
documented subset.
> > The ideal solution is to add or modify a KVM ioctl to allow the VMSA to be
> > set
> > directly, overriding the state in "CPUX86State". The current
> > KVM_SEV_LAUNCH_UPDATE_VMSA ioctl triggers the synchronisation of the VMSA
> > but
> > does not allow it to be specified directly. This could be modified for what
> > we
> > need. The SEV-SNP kernel patches add KVM_SEV_SNP_LAUNCH_UPDATE which allows
> > a
> > page type of VMSA to be updated, although the current patch series does not
> > support using this to set the initial state of the VMSA:
> > https://lore.kernel.org/lkml/20231230172351.574091-19-michael.roth@amd.com/
> > I
> > have experimented with this myself and have successfully modified the SEV-
> > SNP
> > kernel patches to support directly setting the VMSA from QEMU.
> >
> > On the other hand, I have also verified that I can indeed measure an IGVM
> > file
> > loaded using the VMSA synchronisation method currently employed and get a
> > matching measurement from the SEV attestation report.
> >
> > What would you suggest is the best way forward for this?
>
> I'll delegate to Paolo for an opinion on the possiblity of new (or
> updated) ioctls to provide the full VMSA data.
>
> If we can't directly set the full VMSA, then next best option is a
> more formal way query to VMSA. That way libvirt could report on
> what the default initial kernel VMSA state is, which could be useful
> debug info for any bug reports.
Setting the full VMSA definitely seems like the right option here. Querying the
VMSA that was actually measured would obviously give us the ability to diagnose
problems with the measurement but does not allow full compatibility with the
IGVM specification. This will potentially restrict the types of guests that can
be packaged using IGVM.
Another thing to bear in mind is that with the incoming host kernel support for
SEV-SNP, there are more constraints on how the VMSA is measured and populated.
In particular, the current patches for SEV-SNP automatically sync and measure
the VMSA as the final stage of guest measurement, requiring the IGVM file to
provide the VMSA as the final directive for the measurement to match. Also, the
kernel hardcodes the VMSA GPA, again requiring the IGVM file to match. If we
have the ability to provide the VMSA directly (including the GPA of the VMSA)
then these restrictions are removed.
I'd suggest that for SEV and SEV-ES, the current method of syncing certain
fields (and updating the QEMU documentation to describe this) is sufficient for
now. And perhaps this is ok for SEV-SNP too, but we should pursue the ability to
provide the full VMSA at least in the SEV-SNP case.
>
> With regards,
> Daniel
Kind regards,
Roy
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 8/9] i386/sev: Implement ConfidentialGuestSupport functions for SEV
2024-02-27 14:50 [PATCH 0/9] Introduce support for IGVM files Roy Hopkins
` (6 preceding siblings ...)
2024-02-27 14:50 ` [PATCH 7/9] i386/sev: Refactor setting of reset vector and initial CPU state Roy Hopkins
@ 2024-02-27 14:50 ` Roy Hopkins
2024-02-27 14:50 ` [PATCH 9/9] docs/system: Add documentation on support for IGVM Roy Hopkins
` (2 subsequent siblings)
10 siblings, 0 replies; 35+ messages in thread
From: Roy Hopkins @ 2024-02-27 14:50 UTC (permalink / raw)
To: qemu-devel
Cc: Roy Hopkins, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
The ConfidentialGuestSupport object defines a number of virtual
functions that are called during processing of IGVM directives to query
or configure initial guest state. In order to support processing of IGVM
files, these functions need to be implemented by relevant isolation
hardware support code such as SEV.
This commit implements the required functions for SEV-ES and adds
support for processing IGVM files for configuring the guest.
Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
target/i386/sev.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 77 insertions(+)
diff --git a/target/i386/sev.c b/target/i386/sev.c
index d6902432fd..b85ccff9db 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -37,6 +37,7 @@
#include "qapi/qapi-commands-misc-target.h"
#include "exec/confidential-guest-support.h"
#include "hw/i386/pc.h"
+#include "hw/i386/e820_memory_layout.h"
#include "exec/address-spaces.h"
#define TYPE_SEV_GUEST "sev-guest"
@@ -170,6 +171,9 @@ static const char *const sev_fw_errlist[] = {
#define SEV_FW_MAX_ERROR ARRAY_SIZE(sev_fw_errlist)
+static int sev_launch_update_data(SevGuestState *sev_guest, uint8_t *addr,
+ uint64_t len);
+
static int
sev_ioctl(int fd, int cmd, void *data, int *error)
{
@@ -304,6 +308,14 @@ sev_guest_finalize(Object *obj)
{
}
+static int cgs_check_support(ConfidentialGuestPlatformType platform,
+ uint16_t platform_version, uint8_t highest_vtl,
+ uint64_t shared_gpa_boundary)
+{
+ return (((platform == CGS_PLATFORM_SEV_ES) && sev_es_enabled()) ||
+ ((platform == CGS_PLATFORM_SEV) && sev_enabled())) ? 1 : 0;
+}
+
static void sev_apply_cpu_context(CPUState *cpu)
{
SevGuestState *sev_guest = SEV_GUEST(MACHINE(qdev_get_machine())->cgs);
@@ -447,6 +459,65 @@ static int sev_set_cpu_context(uint16_t cpu_index, const void *ctx,
return 0;
}
+static int cgs_set_guest_state(hwaddr gpa, uint8_t *ptr, uint64_t len,
+ ConfidentialGuestPageType memory_type,
+ uint16_t cpu_index)
+{
+ SevGuestState *sev = SEV_GUEST(MACHINE(qdev_get_machine())->cgs);
+ int ret = 1;
+
+ if (!sev_enabled()) {
+ error_report("%s: attempt to configure guest memory, but SEV "
+ "is not enabled",
+ __func__);
+ } else if (memory_type == CGS_PAGE_TYPE_VMSA) {
+ if (!sev_es_enabled()) {
+ error_report("%s: attempt to configure initial VMSA, but SEV-ES "
+ "is not supported",
+ __func__);
+ } else {
+ ret = sev_set_cpu_context(cpu_index, ptr, len, gpa);
+ }
+ } else if ((memory_type != CGS_PAGE_TYPE_REQUIRED_MEMORY) &&
+ (memory_type != CGS_PAGE_TYPE_UNMEASURED)) {
+ ret = sev_launch_update_data(sev, ptr, len);
+ } else {
+ error_report(
+ "%s: attempted to configure guest memory to use memory_type %d, "
+ "but this type is not supported by SEV-ES",
+ __func__, (int)memory_type);
+ }
+ return ret;
+}
+
+static int cgs_get_mem_map_entry(int index,
+ ConfidentialGuestMemoryMapEntry *entry)
+{
+ if ((index < 0) || (index >= e820_get_num_entries())) {
+ return 1;
+ }
+ entry->gpa = e820_table[index].address;
+ entry->size = e820_table[index].length;
+ switch (e820_table[index].type) {
+ case E820_RAM:
+ entry->type = CGS_MEM_RAM;
+ break;
+ case E820_RESERVED:
+ entry->type = CGS_MEM_RESERVED;
+ break;
+ case E820_ACPI:
+ entry->type = CGS_MEM_ACPI;
+ break;
+ case E820_NVS:
+ entry->type = CGS_MEM_NVS;
+ break;
+ case E820_UNUSABLE:
+ entry->type = CGS_MEM_UNUSABLE;
+ break;
+ }
+ return 0;
+}
+
static char *
sev_guest_get_session_file(Object *obj, Error **errp)
{
@@ -538,6 +609,7 @@ static void
sev_guest_instance_init(Object *obj)
{
SevGuestState *sev = SEV_GUEST(obj);
+ ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE);
sev->policy = DEFAULT_GUEST_POLICY;
@@ -550,6 +622,11 @@ sev_guest_instance_init(Object *obj)
object_property_add_uint32_ptr(obj, "reduced-phys-bits",
&sev->reduced_phys_bits,
OBJ_PROP_FLAG_READWRITE);
+
+ cgs->check_support = cgs_check_support;
+ cgs->set_guest_state = cgs_set_guest_state;
+ cgs->get_mem_map_entry = cgs_get_mem_map_entry;
+
QTAILQ_INIT(&sev->launch_vmsa);
}
--
2.43.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH 9/9] docs/system: Add documentation on support for IGVM
2024-02-27 14:50 [PATCH 0/9] Introduce support for IGVM files Roy Hopkins
` (7 preceding siblings ...)
2024-02-27 14:50 ` [PATCH 8/9] i386/sev: Implement ConfidentialGuestSupport functions for SEV Roy Hopkins
@ 2024-02-27 14:50 ` Roy Hopkins
2024-03-01 17:10 ` Daniel P. Berrangé
2024-03-19 15:07 ` [PATCH 0/9] Introduce support for IGVM files Stefano Garzarella
2024-03-20 15:35 ` Ani Sinha
10 siblings, 1 reply; 35+ messages in thread
From: Roy Hopkins @ 2024-02-27 14:50 UTC (permalink / raw)
To: qemu-devel
Cc: Roy Hopkins, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
IGVM support has been implemented for Confidential Guests that support
AMD SEV and AMD SEV-ES. Add some documentation that gives some
background on the IGVM format and how to use it to configure a
confidential guest.
Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
docs/system/igvm.rst | 58 +++++++++++++++++++++++++++++++++++++++++++
docs/system/index.rst | 1 +
2 files changed, 59 insertions(+)
create mode 100644 docs/system/igvm.rst
diff --git a/docs/system/igvm.rst b/docs/system/igvm.rst
new file mode 100644
index 0000000000..bb0c43f0ee
--- /dev/null
+++ b/docs/system/igvm.rst
@@ -0,0 +1,58 @@
+Independent Guest Virtual Machine (IGVM) support
+================================================
+
+IGVM files are designed to encaspulate all the information required to launch a
+virtual machine on any given virtualization stack in a deterministic way. This
+allows the cryptographic measurement of initial guest state for Confidential
+Guests to be calculated when the IGVM file is built, allowing a relying party to
+verify the initial state of a guest via a remote attestation.
+
+QEMU supports IGVM files through the Confidential Guest Support object. An igvm
+filename can optionally be passed to the object which will subsequently be
+parsed and used to configure the guest state prior to launching the guest.
+
+Further Information on IGVM
+---------------------------
+
+Information about the IGVM format, including links to the format specification
+and documentation for the Rust and C libraries can be found at the project
+repository:
+
+https://github.com/microsoft/igvm
+
+
+Supported Confidential Guests
+-----------------------------
+
+Currently, IGVM files can be provided for Confidential Guests on host systems
+that support AMD SEV and SEV-ES.
+
+IGVM files contain a set of directives. Not every directive is supported by
+every Confidential Guest type. For example, setting the initial CPU state is not
+supported on AMD SEV due to the platform not supporting encrypted save state
+regions. However, this is supported on SEV-ES.
+
+When an IGVM file contains directives that are not supported for the active
+platform, an error is displayed and the guest launch is aborted.
+
+Firmware Images with IGVM
+-------------------------
+
+When an IGVM filename is specified for a Confidential Guest Support object it
+overrides the default handling of system firmware: the firmware image, such as
+an OVMF binary should be contained as a payload of the IGVM file and not
+provided as a flash drive. The default QEMU firmware is not automatically mapped
+into guest memory.
+
+Running a Confidential Guest configured using IGVM
+--------------------------------------------------
+
+To run a confidential guest configured with IGVM you need to add the
+``igvm-file`` parameter to the "confidential guest support" object:
+
+Example (for AMD SEV)::
+
+ qemu-system-x86_64 \
+ <other parameters> \
+ -machine ...,confidential-guest-support=sev0 \
+ -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1,igvm-file=/path/to/guest.igvm
diff --git a/docs/system/index.rst b/docs/system/index.rst
index c21065e519..6235dfab87 100644
--- a/docs/system/index.rst
+++ b/docs/system/index.rst
@@ -38,4 +38,5 @@ or Hypervisor.Framework.
security
multi-process
confidential-guest-support
+ igvm
vm-templating
--
2.43.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 9/9] docs/system: Add documentation on support for IGVM
2024-02-27 14:50 ` [PATCH 9/9] docs/system: Add documentation on support for IGVM Roy Hopkins
@ 2024-03-01 17:10 ` Daniel P. Berrangé
2024-03-18 15:59 ` Roy Hopkins
0 siblings, 1 reply; 35+ messages in thread
From: Daniel P. Berrangé @ 2024-03-01 17:10 UTC (permalink / raw)
To: Roy Hopkins
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
On Tue, Feb 27, 2024 at 02:50:15PM +0000, Roy Hopkins wrote:
> IGVM support has been implemented for Confidential Guests that support
> AMD SEV and AMD SEV-ES. Add some documentation that gives some
> background on the IGVM format and how to use it to configure a
> confidential guest.
>
> Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> ---
> docs/system/igvm.rst | 58 +++++++++++++++++++++++++++++++++++++++++++
> docs/system/index.rst | 1 +
> 2 files changed, 59 insertions(+)
> create mode 100644 docs/system/igvm.rst
> +Firmware Images with IGVM
> +-------------------------
> +
> +When an IGVM filename is specified for a Confidential Guest Support object it
> +overrides the default handling of system firmware: the firmware image, such as
> +an OVMF binary should be contained as a payload of the IGVM file and not
> +provided as a flash drive. The default QEMU firmware is not automatically mapped
> +into guest memory.
IIUC, in future the IGVM file could contain both the OVMF and SVSM
binaries ?
I'm also wondering if there can be dependancies between the IGVM
file and the broader QEMU configuration ? eg if SVSM gains suupport
for data persistence, potentially we might need some pflash device
exposed as storage for SVSM to use. Would such a dependancy be
something expressed in the IGVM file, or would it be knowledge that
is out of band ?
Finally, if we think of the IGVM file as simply yet another firmware
file format, then it raises of question of integration into the
QEMU firmware descriptors.
Right now when defining a guest in libvirt if you can say 'type=bios'
or 'type=uefi', and libvirt consults the firmware descriptors to find
the binary to use.
If the OS distro provides IGVM files instead of traditional raw OVMF
binaries for SEV/TDX/etc, then from libvirt's POV I think having this
expressed in the firmware descriptors is highly desirable.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 9/9] docs/system: Add documentation on support for IGVM
2024-03-01 17:10 ` Daniel P. Berrangé
@ 2024-03-18 15:59 ` Roy Hopkins
2024-03-18 16:21 ` Daniel P. Berrangé
0 siblings, 1 reply; 35+ messages in thread
From: Roy Hopkins @ 2024-03-18 15:59 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
On Fri, 2024-03-01 at 17:10 +0000, Daniel P. Berrangé wrote:
> On Tue, Feb 27, 2024 at 02:50:15PM +0000, Roy Hopkins wrote:
> > IGVM support has been implemented for Confidential Guests that support
> > AMD SEV and AMD SEV-ES. Add some documentation that gives some
> > background on the IGVM format and how to use it to configure a
> > confidential guest.
> >
> > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> > ---
> > docs/system/igvm.rst | 58 +++++++++++++++++++++++++++++++++++++++++++
> > docs/system/index.rst | 1 +
> > 2 files changed, 59 insertions(+)
> > create mode 100644 docs/system/igvm.rst
>
>
> > +Firmware Images with IGVM
> > +-------------------------
> > +
> > +When an IGVM filename is specified for a Confidential Guest Support object
> > it
> > +overrides the default handling of system firmware: the firmware image, such
> > as
> > +an OVMF binary should be contained as a payload of the IGVM file and not
> > +provided as a flash drive. The default QEMU firmware is not automatically
> > mapped
> > +into guest memory.
>
> IIUC, in future the IGVM file could contain both the OVMF and SVSM
> binaries ?
>
> I'm also wondering if there can be dependancies between the IGVM
> file and the broader QEMU configuration ? eg if SVSM gains suupport
> for data persistence, potentially we might need some pflash device
> exposed as storage for SVSM to use. Would such a dependancy be
> something expressed in the IGVM file, or would it be knowledge that
> is out of band ?
>
Yes, the IGVM file can indeed contain both OVMF and SVSM binaries. In fact, that
is exactly what we are doing with the COCONUT-SVSM project. See [1] for the IGVM
builder we use to package OVMF, bootloader components and the SVSM ELF binary.
Data persistence is something that is definitely going to be needed in the SVSM.
At present, this cannot be configured using any of the directives in the IGVM
specification but instead requires QEMU to be configured correctly to support
the application embedded within the IGVM file itself. You could however populate
metadata pages using IGVM that describe the storage that is _expected_ to be
present, and validate that within the firmware itself.
The real value from IGVM comes from the ability to describe the initial memory
and initial CPU state which all forms part of the launch measurement and initial
boot procedure, allowing the expected launch measurement to be calculated from a
single IGVM file for multiple virtualisation stacks or configurations. Thus,
most of the directives in the IGVM file directly have an effect on the launch
measurement. I'm not sure configuring a storage device or other hardware
configuration fits well with this.
> Finally, if we think of the IGVM file as simply yet another firmware
> file format, then it raises of question of integration into the
> QEMU firmware descriptors.
>
> Right now when defining a guest in libvirt if you can say 'type=bios'
> or 'type=uefi', and libvirt consults the firmware descriptors to find
> the binary to use.
>
> If the OS distro provides IGVM files instead of traditional raw OVMF
> binaries for SEV/TDX/etc, then from libvirt's POV I think having this
> expressed in the firmware descriptors is highly desirable.
>
Whether IGVM is just another firmware file format or not, it certainly is used
mutually exclusively with other firmware files. Integration with firmware
descriptors does seem to make sense.
One further question if this is the case, would we want to switch from
specifying an "igvm-file" as a parameter on the "ConfidentialGuestSupport"
object to providing the file using the "-bios" parameter, or maybe even a
dedicated "-igvm" parameter?
>
> With regards,
> Daniel
[1] COCONUT IGVM builder:
https://github.com/coconut-svsm/svsm/tree/main/igvmbuilder
Regards,
Roy
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 9/9] docs/system: Add documentation on support for IGVM
2024-03-18 15:59 ` Roy Hopkins
@ 2024-03-18 16:21 ` Daniel P. Berrangé
2024-03-20 15:45 ` Roy Hopkins
0 siblings, 1 reply; 35+ messages in thread
From: Daniel P. Berrangé @ 2024-03-18 16:21 UTC (permalink / raw)
To: Roy Hopkins
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
On Mon, Mar 18, 2024 at 03:59:31PM +0000, Roy Hopkins wrote:
> On Fri, 2024-03-01 at 17:10 +0000, Daniel P. Berrangé wrote:
> > On Tue, Feb 27, 2024 at 02:50:15PM +0000, Roy Hopkins wrote:
> > > IGVM support has been implemented for Confidential Guests that support
> > > AMD SEV and AMD SEV-ES. Add some documentation that gives some
> > > background on the IGVM format and how to use it to configure a
> > > confidential guest.
> > >
> > > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> > > ---
> > > docs/system/igvm.rst | 58 +++++++++++++++++++++++++++++++++++++++++++
> > > docs/system/index.rst | 1 +
> > > 2 files changed, 59 insertions(+)
> > > create mode 100644 docs/system/igvm.rst
> >
> >
> > > +Firmware Images with IGVM
> > > +-------------------------
> > > +
> > > +When an IGVM filename is specified for a Confidential Guest Support object
> > > it
> > > +overrides the default handling of system firmware: the firmware image, such
> > > as
> > > +an OVMF binary should be contained as a payload of the IGVM file and not
> > > +provided as a flash drive. The default QEMU firmware is not automatically
> > > mapped
> > > +into guest memory.
> >
> > IIUC, in future the IGVM file could contain both the OVMF and SVSM
> > binaries ?
> >
> > I'm also wondering if there can be dependancies between the IGVM
> > file and the broader QEMU configuration ? eg if SVSM gains suupport
> > for data persistence, potentially we might need some pflash device
> > exposed as storage for SVSM to use. Would such a dependancy be
> > something expressed in the IGVM file, or would it be knowledge that
> > is out of band ?
> >
> Yes, the IGVM file can indeed contain both OVMF and SVSM binaries. In fact, that
> is exactly what we are doing with the COCONUT-SVSM project. See [1] for the IGVM
> builder we use to package OVMF, bootloader components and the SVSM ELF binary.
>
> Data persistence is something that is definitely going to be needed in the SVSM.
> At present, this cannot be configured using any of the directives in the IGVM
> specification but instead requires QEMU to be configured correctly to support
> the application embedded within the IGVM file itself. You could however populate
> metadata pages using IGVM that describe the storage that is _expected_ to be
> present, and validate that within the firmware itself.
>
> The real value from IGVM comes from the ability to describe the initial memory
> and initial CPU state which all forms part of the launch measurement and initial
> boot procedure, allowing the expected launch measurement to be calculated from a
> single IGVM file for multiple virtualisation stacks or configurations. Thus,
> most of the directives in the IGVM file directly have an effect on the launch
> measurement. I'm not sure configuring a storage device or other hardware
> configuration fits well with this.
Yeah, I can understand if IGVM scope should be limited to just memory
and CPU setup.
If we use the firmeware descriptor files, we could define capabilities
in that to express a need for a particular type of persistent storage
to back the vTPM. So having this info in IGVM files isn't critical.
> > Finally, if we think of the IGVM file as simply yet another firmware
> > file format, then it raises of question of integration into the
> > QEMU firmware descriptors.
> >
> > Right now when defining a guest in libvirt if you can say 'type=bios'
> > or 'type=uefi', and libvirt consults the firmware descriptors to find
> > the binary to use.
> >
> > If the OS distro provides IGVM files instead of traditional raw OVMF
> > binaries for SEV/TDX/etc, then from libvirt's POV I think having this
> > expressed in the firmware descriptors is highly desirable.
> >
>
> Whether IGVM is just another firmware file format or not, it certainly is used
> mutually exclusively with other firmware files. Integration with firmware
> descriptors does seem to make sense.
>
> One further question if this is the case, would we want to switch from
> specifying an "igvm-file" as a parameter on the "ConfidentialGuestSupport"
> object to providing the file using the "-bios" parameter, or maybe even a
> dedicated "-igvm" parameter?
If the IGVM format is flexible enough that it could be used for any VM
type, even non-confidential VMs, then having its config be separate from
ConfidentialGuestSUpport would make sense. If it is fundamentally tied
to CVMs, then just a property is fine I guess.
Probably best to stay away from -bios, to avoid overloading new semantics
onto a long standing argument.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 9/9] docs/system: Add documentation on support for IGVM
2024-03-18 16:21 ` Daniel P. Berrangé
@ 2024-03-20 15:45 ` Roy Hopkins
2024-03-20 15:52 ` Daniel P. Berrangé
0 siblings, 1 reply; 35+ messages in thread
From: Roy Hopkins @ 2024-03-20 15:45 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
On Mon, 2024-03-18 at 16:21 +0000, Daniel P. Berrangé wrote:
> On Mon, Mar 18, 2024 at 03:59:31PM +0000, Roy Hopkins wrote:
> > On Fri, 2024-03-01 at 17:10 +0000, Daniel P. Berrangé wrote:
> > > On Tue, Feb 27, 2024 at 02:50:15PM +0000, Roy Hopkins wrote:
> > > > IGVM support has been implemented for Confidential Guests that support
> > > > AMD SEV and AMD SEV-ES. Add some documentation that gives some
> > > > background on the IGVM format and how to use it to configure a
> > > > confidential guest.
> > > >
> > > > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> > > > ---
> > > > docs/system/igvm.rst | 58 +++++++++++++++++++++++++++++++++++++++++++
> > > > docs/system/index.rst | 1 +
> > > > 2 files changed, 59 insertions(+)
> > > > create mode 100644 docs/system/igvm.rst
> > >
> > >
> > > > +Firmware Images with IGVM
> > > > +-------------------------
> > > > +
> > > > +When an IGVM filename is specified for a Confidential Guest Support
> > > > object
> > > > it
> > > > +overrides the default handling of system firmware: the firmware image,
> > > > such
> > > > as
> > > > +an OVMF binary should be contained as a payload of the IGVM file and
> > > > not
> > > > +provided as a flash drive. The default QEMU firmware is not
> > > > automatically
> > > > mapped
> > > > +into guest memory.
> > >
> > > IIUC, in future the IGVM file could contain both the OVMF and SVSM
> > > binaries ?
> > >
> > > I'm also wondering if there can be dependancies between the IGVM
> > > file and the broader QEMU configuration ? eg if SVSM gains suupport
> > > for data persistence, potentially we might need some pflash device
> > > exposed as storage for SVSM to use. Would such a dependancy be
> > > something expressed in the IGVM file, or would it be knowledge that
> > > is out of band ?
> > >
> > Yes, the IGVM file can indeed contain both OVMF and SVSM binaries. In fact,
> > that
> > is exactly what we are doing with the COCONUT-SVSM project. See [1] for the
> > IGVM
> > builder we use to package OVMF, bootloader components and the SVSM ELF
> > binary.
> >
> > Data persistence is something that is definitely going to be needed in the
> > SVSM.
> > At present, this cannot be configured using any of the directives in the
> > IGVM
> > specification but instead requires QEMU to be configured correctly to
> > support
> > the application embedded within the IGVM file itself. You could however
> > populate
> > metadata pages using IGVM that describe the storage that is _expected_ to be
> > present, and validate that within the firmware itself.
> >
> > The real value from IGVM comes from the ability to describe the initial
> > memory
> > and initial CPU state which all forms part of the launch measurement and
> > initial
> > boot procedure, allowing the expected launch measurement to be calculated
> > from a
> > single IGVM file for multiple virtualisation stacks or configurations. Thus,
> > most of the directives in the IGVM file directly have an effect on the
> > launch
> > measurement. I'm not sure configuring a storage device or other hardware
> > configuration fits well with this.
>
> Yeah, I can understand if IGVM scope should be limited to just memory
> and CPU setup.
>
> If we use the firmeware descriptor files, we could define capabilities
> in that to express a need for a particular type of persistent storage
> to back the vTPM. So having this info in IGVM files isn't critical.
I'll need to look into firmware descriptor files as I'm unfamiliar with how they
work. Would I need to make any additions to this patch series to support this in
QEMU? Or is this all handled by libvirt?
>
> > > Finally, if we think of the IGVM file as simply yet another firmware
> > > file format, then it raises of question of integration into the
> > > QEMU firmware descriptors.
> > >
> > > Right now when defining a guest in libvirt if you can say 'type=bios'
> > > or 'type=uefi', and libvirt consults the firmware descriptors to find
> > > the binary to use.
> > >
> > > If the OS distro provides IGVM files instead of traditional raw OVMF
> > > binaries for SEV/TDX/etc, then from libvirt's POV I think having this
> > > expressed in the firmware descriptors is highly desirable.
> > >
> >
> > Whether IGVM is just another firmware file format or not, it certainly is
> > used
> > mutually exclusively with other firmware files. Integration with firmware
> > descriptors does seem to make sense.
> >
> > One further question if this is the case, would we want to switch from
> > specifying an "igvm-file" as a parameter on the "ConfidentialGuestSupport"
> > object to providing the file using the "-bios" parameter, or maybe even a
> > dedicated "-igvm" parameter?
>
> If the IGVM format is flexible enough that it could be used for any VM
> type, even non-confidential VMs, then having its config be separate from
> ConfidentialGuestSUpport would make sense. If it is fundamentally tied
> to CVMs, then just a property is fine I guess.
>
> Probably best to stay away from -bios, to avoid overloading new semantics
> onto a long standing argument.
>
> With regards,
> Daniel
Currently, the IGVM specification only contains support for confidential
platforms. It could theoretically be used for non-confidential platforms but
that would require changes to the IGVM specification itself to support this. I
don't think it makes sense to extend this to non-confidential VMs until the
specification supports this, so I'll leave it as a property of
ConfidentialGuestSupport.
Regards,
Roy
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 9/9] docs/system: Add documentation on support for IGVM
2024-03-20 15:45 ` Roy Hopkins
@ 2024-03-20 15:52 ` Daniel P. Berrangé
0 siblings, 0 replies; 35+ messages in thread
From: Daniel P. Berrangé @ 2024-03-20 15:52 UTC (permalink / raw)
To: Roy Hopkins
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
On Wed, Mar 20, 2024 at 03:45:24PM +0000, Roy Hopkins wrote:
> On Mon, 2024-03-18 at 16:21 +0000, Daniel P. Berrangé wrote:
> > On Mon, Mar 18, 2024 at 03:59:31PM +0000, Roy Hopkins wrote:
> > > On Fri, 2024-03-01 at 17:10 +0000, Daniel P. Berrangé wrote:
> > > > On Tue, Feb 27, 2024 at 02:50:15PM +0000, Roy Hopkins wrote:
> > > > > IGVM support has been implemented for Confidential Guests that support
> > > > > AMD SEV and AMD SEV-ES. Add some documentation that gives some
> > > > > background on the IGVM format and how to use it to configure a
> > > > > confidential guest.
> > > > >
> > > > > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> > > > > ---
> > > > > docs/system/igvm.rst | 58 +++++++++++++++++++++++++++++++++++++++++++
> > > > > docs/system/index.rst | 1 +
> > > > > 2 files changed, 59 insertions(+)
> > > > > create mode 100644 docs/system/igvm.rst
> > > >
> > > >
> > > > > +Firmware Images with IGVM
> > > > > +-------------------------
> > > > > +
> > > > > +When an IGVM filename is specified for a Confidential Guest Support
> > > > > object
> > > > > it
> > > > > +overrides the default handling of system firmware: the firmware image,
> > > > > such
> > > > > as
> > > > > +an OVMF binary should be contained as a payload of the IGVM file and
> > > > > not
> > > > > +provided as a flash drive. The default QEMU firmware is not
> > > > > automatically
> > > > > mapped
> > > > > +into guest memory.
> > > >
> > > > IIUC, in future the IGVM file could contain both the OVMF and SVSM
> > > > binaries ?
> > > >
> > > > I'm also wondering if there can be dependancies between the IGVM
> > > > file and the broader QEMU configuration ? eg if SVSM gains suupport
> > > > for data persistence, potentially we might need some pflash device
> > > > exposed as storage for SVSM to use. Would such a dependancy be
> > > > something expressed in the IGVM file, or would it be knowledge that
> > > > is out of band ?
> > > >
> > > Yes, the IGVM file can indeed contain both OVMF and SVSM binaries. In fact,
> > > that
> > > is exactly what we are doing with the COCONUT-SVSM project. See [1] for the
> > > IGVM
> > > builder we use to package OVMF, bootloader components and the SVSM ELF
> > > binary.
> > >
> > > Data persistence is something that is definitely going to be needed in the
> > > SVSM.
> > > At present, this cannot be configured using any of the directives in the
> > > IGVM
> > > specification but instead requires QEMU to be configured correctly to
> > > support
> > > the application embedded within the IGVM file itself. You could however
> > > populate
> > > metadata pages using IGVM that describe the storage that is _expected_ to be
> > > present, and validate that within the firmware itself.
> > >
> > > The real value from IGVM comes from the ability to describe the initial
> > > memory
> > > and initial CPU state which all forms part of the launch measurement and
> > > initial
> > > boot procedure, allowing the expected launch measurement to be calculated
> > > from a
> > > single IGVM file for multiple virtualisation stacks or configurations. Thus,
> > > most of the directives in the IGVM file directly have an effect on the
> > > launch
> > > measurement. I'm not sure configuring a storage device or other hardware
> > > configuration fits well with this.
> >
> > Yeah, I can understand if IGVM scope should be limited to just memory
> > and CPU setup.
> >
> > If we use the firmeware descriptor files, we could define capabilities
> > in that to express a need for a particular type of persistent storage
> > to back the vTPM. So having this info in IGVM files isn't critical.
>
> I'll need to look into firmware descriptor files as I'm unfamiliar with how they
> work. Would I need to make any additions to this patch series to support this in
> QEMU? Or is this all handled by libvirt?
Probably little more than change docs/interop/firmware.json
to add 'igvm' as a FirmwareDevice option to indicate this is
a new way of exposing it to the guest.
At some point we might also want to expand FirmwareFeature
eg to report a "vtpm" feature.
> > > Whether IGVM is just another firmware file format or not, it certainly is
> > > used
> > > mutually exclusively with other firmware files. Integration with firmware
> > > descriptors does seem to make sense.
> > >
> > > One further question if this is the case, would we want to switch from
> > > specifying an "igvm-file" as a parameter on the "ConfidentialGuestSupport"
> > > object to providing the file using the "-bios" parameter, or maybe even a
> > > dedicated "-igvm" parameter?
> >
> > If the IGVM format is flexible enough that it could be used for any VM
> > type, even non-confidential VMs, then having its config be separate from
> > ConfidentialGuestSUpport would make sense. If it is fundamentally tied
> > to CVMs, then just a property is fine I guess.
> >
> > Probably best to stay away from -bios, to avoid overloading new semantics
> > onto a long standing argument.
>
> Currently, the IGVM specification only contains support for confidential
> platforms. It could theoretically be used for non-confidential platforms but
> that would require changes to the IGVM specification itself to support this. I
> don't think it makes sense to extend this to non-confidential VMs until the
> specification supports this, so I'll leave it as a property of
> ConfidentialGuestSupport.
Sounds good.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/9] Introduce support for IGVM files
2024-02-27 14:50 [PATCH 0/9] Introduce support for IGVM files Roy Hopkins
` (8 preceding siblings ...)
2024-02-27 14:50 ` [PATCH 9/9] docs/system: Add documentation on support for IGVM Roy Hopkins
@ 2024-03-19 15:07 ` Stefano Garzarella
2024-03-20 14:40 ` Roy Hopkins
2024-03-20 15:35 ` Ani Sinha
10 siblings, 1 reply; 35+ messages in thread
From: Stefano Garzarella @ 2024-03-19 15:07 UTC (permalink / raw)
To: Roy Hopkins
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
Hi Roy,
thanks for this series!
On Tue, Feb 27, 2024 at 02:50:06PM +0000, Roy Hopkins wrote:
>Hi everyone,
>
>This initial patch series submission adds the capability to configure
>confidential guests using files that conform to the Independent Guest Virtual
>Machine (IGVM) file format. The series is based on the master branch commit
>1b330da. Alternatively, the series is available here:
>https://github.com/roy-hopkins/qemu/tree/igvm_master_v1
>
>I look forward to welcoming your comments!
I saw that the series has been a posted for a while and maybe you're
going to send v2, so I'll bring back some little things that I saw, but
I didn't do yet a deep review:
- We use "Isolated Guest Virtual Machine" or "Independent Guest Virtual
Machine", are they interchangeable for IGVM?
- `./scripts/checkpatch.pl --codespell` reported some warnings:
5/9 Checking commit 81f60e5cdd01 (i386/pc: Process IGVM file during PC initialization if present)
WARNING: 'initalization' may be misspelled - perhaps 'initialization'?
#15:
initalization of the target.
^^^^^^^^^^^^^
9/9 Checking commit 66745c0bb940 (docs/system: Add documentation on support for IGVM)
WARNING: 'encaspulate' may be misspelled - perhaps 'encapsulate'?
#27: FILE: docs/system/igvm.rst:4:
+IGVM files are designed to encaspulate all the information required to launch a
^^^^^^^^^^^
Thanks,
Stefano
>
>Why do we need Independent Guest Virtual Machine (IGVM) files?
>==============================================================
>
>IGVM files describe, using a set of directives, the memory layout and initial
>configuration of a guest that supports isolation technologies such as AMD
>SEV-SNP and Intel TDX. By encapsulating all of this information in a single
>configuration file and applying the directives in the order they are specified
>when the guest is initialized, it becomes straightforward to pre-calculate the
>cryptographic measurement of the guest initial state, thus aiding in remote
>attestation processes.
>
>IGVM files can also be used to configure non-standard guest memory layouts,
>payloads or startup configurations. A good example of this is to use IGVM to
>deploy and configure an SVSM module in the guest which supports running at
>multiple VMPLs. The SVSM can be configured to start directly into 32-bit or
>64-bit code. This patch series was developed with this purpose in mind to
>support the COCONUT-SVSM project:
>https://github.com/coconut-svsm/svsm
>
>More information and background on the IGVM file format can be found on the
>project page at:
>https://github.com/microsoft/igvm
>
>What this patch series introduces
>=================================
>
>This series adds a build-time configuration option (--enable-igvm) to add
>support for launching a guest using an IGVM file. It extends the current
>ConfidentialGuestSupport object to allow an IGVM filename to be specified.
>
>The directives in the IGVM file are parsed and the confidential guest is
>configured through new virtual methods added to the ConfidentialGuestSupport
>object. These virtual functions have been implemented for AMD SEV and AMD
>SEV-ES.
>
>Many of the IGVM directives require capabilities that are not supported in SEV
>and SEV-ES, so support for IGVM directives will need to be considered when
>support for SEV-SNP, TDX or other technologies is introduced to QEMU. Any
>directive that is not currently supported results in an error report.
>
>Dependencies
>============
>
>In order to enable IGVM support, you will need the IGVM library installed.
>Instructions on building and installing it can be found here:
>https://github.com/microsoft/igvm/tree/main/igvm_c
>
>As mentioned above, this series was developed as part of the effort for
>COCONUT-SVSM. COCONUT-SVSM requires support for AMD SEV-SNP which is not
>available in current QEMU. Therefore this series has also been applied on top of
>the AMD SEV-SNP branch (https://github.com/AMDESE/qemu/tree/snp-v3-wip). You can
>find that version of the series here:
>https://github.com/roy-hopkins/qemu/commits/snp-v3-wip-igvm_v2/
>
>Generating IGVM files
>=====================
>
>To try this out you will need to generate an IGVM file that is compatible with
>the SEV platform you are testing on. I've created a tool that can create a
>simple IGVM file that packages an OVMF binary for AMD SEV or AMD SEV-ES. The
>tool is available here:
>https://github.com/roy-hopkins/buildigvm
>
>I have tested this on an AMD EPYC Genoa system configured to support SEV. Both
>SEV and SEV-ES have been tested using IGVM files generated using the buildigvm
>tool. The SEV-SNP alternative patch set has also been tested using COCONUT-SVSM.
>
>Roy Hopkins (9):
> meson: Add optional dependency on IGVM library
> backends/confidential-guest-support: Add IGVM file parameter
> backends/confidential-guest-support: Add functions to support IGVM
> backends/igvm: Implement parsing and processing of IGVM files
> i386/pc: Process IGVM file during PC initialization if present
> i386/pc: Skip initialization of system FW when using IGVM
> i386/sev: Refactor setting of reset vector and initial CPU state
> i386/sev: Implement ConfidentialGuestSupport functions for SEV
> docs/system: Add documentation on support for IGVM
>
> backends/confidential-guest-support.c | 69 +++
> backends/igvm.c | 718 ++++++++++++++++++++++
> backends/meson.build | 4 +
> docs/system/igvm.rst | 58 ++
> docs/system/index.rst | 1 +
> hw/i386/pc.c | 12 +-
> hw/i386/pc_piix.c | 4 +
> hw/i386/pc_q35.c | 4 +
> include/exec/confidential-guest-support.h | 107 ++++
> include/exec/igvm.h | 35 ++
> meson.build | 8 +
> meson_options.txt | 2 +
> qapi/qom.json | 13 +
> qemu-options.hx | 8 +-
> scripts/meson-buildoptions.sh | 3 +
> target/i386/sev.c | 365 ++++++++++-
> target/i386/sev.h | 110 ++++
> 17 files changed, 1489 insertions(+), 32 deletions(-)
> create mode 100644 backends/igvm.c
> create mode 100644 docs/system/igvm.rst
> create mode 100644 include/exec/igvm.h
>
>--
>2.43.0
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 0/9] Introduce support for IGVM files
2024-03-19 15:07 ` [PATCH 0/9] Introduce support for IGVM files Stefano Garzarella
@ 2024-03-20 14:40 ` Roy Hopkins
0 siblings, 0 replies; 35+ messages in thread
From: Roy Hopkins @ 2024-03-20 14:40 UTC (permalink / raw)
To: Stefano Garzarella
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael S . Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
On Tue, 2024-03-19 at 16:07 +0100, Stefano Garzarella wrote:
> Hi Roy,
> thanks for this series!
>
> On Tue, Feb 27, 2024 at 02:50:06PM +0000, Roy Hopkins wrote:
> > Hi everyone,
> >
> > This initial patch series submission adds the capability to configure
> > confidential guests using files that conform to the Independent Guest
> > Virtual
> > Machine (IGVM) file format. The series is based on the master branch commit
> > 1b330da. Alternatively, the series is available here:
> > https://github.com/roy-hopkins/qemu/tree/igvm_master_v1
> >
> > I look forward to welcoming your comments!
>
> I saw that the series has been a posted for a while and maybe you're
> going to send v2, so I'll bring back some little things that I saw, but
> I didn't do yet a deep review:
>
> - We use "Isolated Guest Virtual Machine" or "Independent Guest Virtual
> Machine", are they interchangeable for IGVM?
>
> - `./scripts/checkpatch.pl --codespell` reported some warnings:
>
> 5/9 Checking commit 81f60e5cdd01 (i386/pc: Process IGVM file during PC
> initialization if present)
> WARNING: 'initalization' may be misspelled - perhaps 'initialization'?
> #15:
> initalization of the target.
> ^^^^^^^^^^^^^
>
> 9/9 Checking commit 66745c0bb940 (docs/system: Add documentation on support
> for IGVM)
>
> WARNING: 'encaspulate' may be misspelled - perhaps 'encapsulate'?
> #27: FILE: docs/system/igvm.rst:4:
> +IGVM files are designed to encaspulate all the information required to launch
> a
> ^^^^^^^^^^^
>
> Thanks,
> Stefano
>
Thanks for the initial review Stefano. I'll be posting a v2 shortly and will
address your comments.
Regards,
Roy
[snip]
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/9] Introduce support for IGVM files
2024-02-27 14:50 [PATCH 0/9] Introduce support for IGVM files Roy Hopkins
` (9 preceding siblings ...)
2024-03-19 15:07 ` [PATCH 0/9] Introduce support for IGVM files Stefano Garzarella
@ 2024-03-20 15:35 ` Ani Sinha
10 siblings, 0 replies; 35+ messages in thread
From: Ani Sinha @ 2024-03-20 15:35 UTC (permalink / raw)
To: Roy Hopkins
Cc: qemu-devel, Paolo Bonzini, Marcelo Tosatti, Michael Tsirkin,
Cornelia Huck, Marcel Apfelbaum, Sergio Lopez, Eduardo Habkost,
Alistair Francis, Peter Xu, David Hildenbrand, Igor Mammedov,
Tom Lendacky, Michael Roth, Jörg Roedel
> On 27 Feb 2024, at 20:20, Roy Hopkins <roy.hopkins@suse.com> wrote:
>
> Hi everyone,
>
> This initial patch series submission adds the capability to configure
> confidential guests using files that conform to the Independent Guest Virtual
> Machine (IGVM) file format. The series is based on the master branch commit
> 1b330da. Alternatively, the series is available here:
> https://github.com/roy-hopkins/qemu/tree/igvm_master_v1
>
> I look forward to welcoming your comments!
>
> Why do we need Independent Guest Virtual Machine (IGVM) files?
> ==============================================================
>
> IGVM files describe, using a set of directives, the memory layout and initial
> configuration of a guest that supports isolation technologies such as AMD
> SEV-SNP and Intel TDX. By encapsulating all of this information in a single
> configuration file and applying the directives in the order they are specified
> when the guest is initialized, it becomes straightforward to pre-calculate the
> cryptographic measurement of the guest initial state, thus aiding in remote
> attestation processes.
>
> IGVM files can also be used to configure non-standard guest memory layouts,
> payloads or startup configurations. A good example of this is to use IGVM to
> deploy and configure an SVSM module in the guest which supports running at
> multiple VMPLs. The SVSM can be configured to start directly into 32-bit or
> 64-bit code. This patch series was developed with this purpose in mind to
> support the COCONUT-SVSM project:
> https://github.com/coconut-svsm/svsm
>
> More information and background on the IGVM file format can be found on the
> project page at:
> https://github.com/microsoft/igvm
>
> What this patch series introduces
> =================================
>
> This series adds a build-time configuration option (--enable-igvm) to add
> support for launching a guest using an IGVM file. It extends the current
> ConfidentialGuestSupport object to allow an IGVM filename to be specified.
>
> The directives in the IGVM file are parsed and the confidential guest is
> configured through new virtual methods added to the ConfidentialGuestSupport
> object. These virtual functions have been implemented for AMD SEV and AMD
> SEV-ES.
>
> Many of the IGVM directives require capabilities that are not supported in SEV
> and SEV-ES, so support for IGVM directives will need to be considered when
> support for SEV-SNP, TDX or other technologies is introduced to QEMU. Any
> directive that is not currently supported results in an error report.
>
> Dependencies
> ============
>
> In order to enable IGVM support, you will need the IGVM library installed.
> Instructions on building and installing it can be found here:
> https://github.com/microsoft/igvm/tree/main/igvm_c
>
> As mentioned above, this series was developed as part of the effort for
> COCONUT-SVSM. COCONUT-SVSM requires support for AMD SEV-SNP which is not
> available in current QEMU. Therefore this series has also been applied on top of
> the AMD SEV-SNP branch (https://github.com/AMDESE/qemu/tree/snp-v3-wip). You can
> find that version of the series here:
> https://github.com/roy-hopkins/qemu/commits/snp-v3-wip-igvm_v2/
>
> Generating IGVM files
> =====================
>
> To try this out you will need to generate an IGVM file that is compatible with
> the SEV platform you are testing on. I've created a tool that can create a
> simple IGVM file that packages an OVMF binary for AMD SEV or AMD SEV-ES. The
> tool is available here:
> https://github.com/roy-hopkins/buildigvm
>
> I have tested this on an AMD EPYC Genoa system configured to support SEV. Both
> SEV and SEV-ES have been tested using IGVM files generated using the buildigvm
> tool. The SEV-SNP alternative patch set has also been tested using COCONUT-SVSM.
Could you please also CC me in this patchset please? Thanks.
>
> Roy Hopkins (9):
> meson: Add optional dependency on IGVM library
> backends/confidential-guest-support: Add IGVM file parameter
> backends/confidential-guest-support: Add functions to support IGVM
> backends/igvm: Implement parsing and processing of IGVM files
> i386/pc: Process IGVM file during PC initialization if present
> i386/pc: Skip initialization of system FW when using IGVM
> i386/sev: Refactor setting of reset vector and initial CPU state
> i386/sev: Implement ConfidentialGuestSupport functions for SEV
> docs/system: Add documentation on support for IGVM
>
> backends/confidential-guest-support.c | 69 +++
> backends/igvm.c | 718 ++++++++++++++++++++++
> backends/meson.build | 4 +
> docs/system/igvm.rst | 58 ++
> docs/system/index.rst | 1 +
> hw/i386/pc.c | 12 +-
> hw/i386/pc_piix.c | 4 +
> hw/i386/pc_q35.c | 4 +
> include/exec/confidential-guest-support.h | 107 ++++
> include/exec/igvm.h | 35 ++
> meson.build | 8 +
> meson_options.txt | 2 +
> qapi/qom.json | 13 +
> qemu-options.hx | 8 +-
> scripts/meson-buildoptions.sh | 3 +
> target/i386/sev.c | 365 ++++++++++-
> target/i386/sev.h | 110 ++++
> 17 files changed, 1489 insertions(+), 32 deletions(-)
> create mode 100644 backends/igvm.c
> create mode 100644 docs/system/igvm.rst
> create mode 100644 include/exec/igvm.h
>
> --
> 2.43.0
>
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread