| Message ID | 20210709215550.32496-3-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) support | expand |
On 10/07/2021 0:55, Brijesh Singh wrote: > To launch the SEV-SNP guest, a user can specify up to 8 parameters. > Passing all parameters through command line can be difficult. To simplify > the launch parameter passing, introduce a .ini-like config file that can be > used for passing the parameters to the launch flow. > > The contents of the config file will look like this: > > $ cat snp-launch.init > > # SNP launch parameters > [SEV-SNP] > init_flags = 0 > policy = 0x1000 > id_block = "YWFhYWFhYWFhYWFhYWFhCg==" > > > Add 'snp' property that can be used to indicate that SEV guest launch > should enable the SNP support. > > SEV-SNP guest launch examples: > > 1) launch without additional parameters > > $(QEMU_CLI) \ > -object sev-guest,id=sev0,snp=on > > 2) launch with optional parameters > $(QEMU_CLI) \ > -object sev-guest,id=sev0,snp=on,launch-config=<file> > Not directly SNP-related, but in an internal communication Connor told me he wants to allow the SEV configuration (like dh-cert-file etc.) to be set using QMP commands when the machine launches instead (or in addition to) setting them via QEMU command-line parameters. Whatever the configuration solution decided for the SEV parameters should also apply to these new SNP settings (policy, id_block, etc.). -Dov
cc'ing in armbru, since he knows about our command line - have we got a neater way of doing this, or something else that reads config file? Could the existing -readconfig work? Although this is a fairly large chunk of data, I don't think it's any larger than our block device configs on a bad day. Dave * Brijesh Singh (brijesh.singh@amd.com) wrote: > To launch the SEV-SNP guest, a user can specify up to 8 parameters. > Passing all parameters through command line can be difficult. To simplify > the launch parameter passing, introduce a .ini-like config file that can be > used for passing the parameters to the launch flow. > > The contents of the config file will look like this: > > $ cat snp-launch.init > > # SNP launch parameters > [SEV-SNP] > init_flags = 0 > policy = 0x1000 > id_block = "YWFhYWFhYWFhYWFhYWFhCg==" Wouldn't the 'gosvw' and 'hostdata' also be in there? Dave > > Add 'snp' property that can be used to indicate that SEV guest launch > should enable the SNP support. > > SEV-SNP guest launch examples: > > 1) launch without additional parameters > > $(QEMU_CLI) \ > -object sev-guest,id=sev0,snp=on > > 2) launch with optional parameters > $(QEMU_CLI) \ > -object sev-guest,id=sev0,snp=on,launch-config=<file> > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > docs/amd-memory-encryption.txt | 81 +++++++++++- > qapi/qom.json | 6 + > target/i386/sev.c | 227 +++++++++++++++++++++++++++++++++ > 3 files changed, 312 insertions(+), 2 deletions(-) > > diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt > index ffca382b5f..322bf38f68 100644 > --- a/docs/amd-memory-encryption.txt > +++ b/docs/amd-memory-encryption.txt > @@ -22,8 +22,8 @@ support for notifying a guest's operating system when certain types of VMEXITs > are about to occur. This allows the guest to selectively share information with > the hypervisor to satisfy the requested function. > > -Launching > ---------- > +Launching (SEV and SEV-ES) > +-------------------------- > Boot images (such as bios) must be encrypted before a guest can be booted. The > MEMORY_ENCRYPT_OP ioctl provides commands to encrypt the images: LAUNCH_START, > LAUNCH_UPDATE_DATA, LAUNCH_MEASURE and LAUNCH_FINISH. These four commands > @@ -113,6 +113,83 @@ a SEV-ES guest: > - Requires in-kernel irqchip - the burden is placed on the hypervisor to > manage booting APs. > > +Launching (SEV-SNP) > +------------------- > +Boot images (such as bios) must be encrypted before a guest can be booted. The > +MEMORY_ENCRYPT_OP ioctl provides commands to encrypt the images: > +KVM_SNP_INIT, SNP_LAUNCH_START, SNP_LAUNCH_UPDATE, and SNP_LAUNCH_FINISH. These > +four commands together generate a fresh memory encryption key for the VM, > +encrypt the boot images for a successful launch. > + > +KVM_SNP_INIT is called first to initialize the SEV-SNP firmware and SNP > +features in the KVM. The feature flags value can be provided through the > +launch-config file. > + > ++------------+-------+----------+---------------------------------+ > +| key | type | default | meaning | > ++------------+-------+----------+---------------------------------+ > +| init_flags | hex | 0 | SNP feature flags | > ++-----------------------------------------------------------------+ > + > +Note: currently the init_flags must be zero. > + > +SNP_LAUNCH_START is called first to create a cryptographic launch context > +within the firmware. To create this context, guest owner must provide a guest > +policy and other parameters as described in the SEV-SNP firmware > +specification. The launch parameters should be specified in the launch-config > +ini file and should be treated as a binary blob and must be passed as-is to > +the SEV-SNP firmware. > + > +The SNP_LAUNCH_START uses the following parameters from the launch-config > +file. See the SEV-SNP specification for more details. > + > ++--------+-------+----------+----------------------------------------------+ > +| key | type | default | meaning | > ++--------+-------+----------+----------------------------------------------+ > +| policy | hex | 0x30000 | a 64-bit guest policy | > +| imi_en | bool | 0 | 1 when IMI is enabled | > +| ma_end | bool | 0 | 1 when migration agent is used | > +| gosvw | string| 0 | 16-byte base64 encoded string for the guest | > +| | | | OS visible workaround. | > ++--------+-------+----------+----------------------------------------------+ > + > +SNP_LAUNCH_UPDATE encrypts the memory region using the cryptographic context > +created via the SNP_LAUNCH_START command. If required, this command can be called > +multiple times to encrypt different memory regions. The command also calculates > +the measurement of the memory contents as it encrypts. > + > +SNP_LAUNCH_FINISH finalizes the guest launch flow. Optionally, while finalizing > +the launch the firmware can perform checks on the launch digest computing > +through the SNP_LAUNCH_UPDATE. To perform the check the user must supply > +the id block, authentication blob and host data that should be included in the > +attestation report. See the SEV-SNP spec for further details. > + > +The SNP_LAUNCH_FINISH uses the following parameters from the launch-config file. > + > ++------------+-------+----------+----------------------------------------------+ > +| key | type | default | meaning | > ++------------+-------+----------+----------------------------------------------+ > +| id_block | string| none | base64 encoded ID block | > ++------------+-------+----------+----------------------------------------------+ > +| id_auth | string| none | base64 encoded authentication information | > ++------------+-------+----------+----------------------------------------------+ > +| auth_key_en| bool | 0 | auth block contains author key | > ++------------+-------+----------+----------------------------------------------+ > +| host_data | string| none | host provided data | > ++------------+-------+----------+----------------------------------------------+ > + > +To launch a SEV-SNP guest > + > +# ${QEMU} \ > + -machine ...,confidential-guest-support=sev0 \ > + -object sev-guest,id=sev0,cbitpos=51,reduced-phys-bits=1,snp=on > + > +To launch a SEV-SNP guest with launch configuration > + > +# ${QEMU} \ > + -machine ...,confidential-guest-support=sev0 \ > + -object sev-guest,id=sev0,cbitpos=51,reduced-phys-bits=1,snp=on,launch-config=<config> > + > Debugging > ----------- > Since the memory contents of a SEV guest are encrypted, hypervisor access to > diff --git a/qapi/qom.json b/qapi/qom.json > index 652be317b8..bdf89fda27 100644 > --- a/qapi/qom.json > +++ b/qapi/qom.json > @@ -749,6 +749,10 @@ > # @reduced-phys-bits: number of bits in physical addresses that become > # unavailable when SEV is enabled > # > +# @snp: SEV-SNP is enabled (default: 0) > +# > +# @launch-config: launch config file to use > +# > # Since: 2.12 > ## > { 'struct': 'SevGuestProperties', > @@ -758,6 +762,8 @@ > '*policy': 'uint32', > '*handle': 'uint32', > '*cbitpos': 'uint32', > + '*snp': 'bool', > + '*launch-config': 'str', > 'reduced-phys-bits': 'uint32' } } > > ## > diff --git a/target/i386/sev.c b/target/i386/sev.c > index 83df8c09f6..6b238ef969 100644 > --- a/target/i386/sev.c > +++ b/target/i386/sev.c > @@ -37,6 +37,11 @@ > #define TYPE_SEV_GUEST "sev-guest" > OBJECT_DECLARE_SIMPLE_TYPE(SevGuestState, SEV_GUEST) > > +struct snp_launch_config { > + struct kvm_snp_init init; > + struct kvm_sev_snp_launch_start start; > + struct kvm_sev_snp_launch_finish finish; > +}; > > /** > * SevGuestState: > @@ -58,6 +63,8 @@ struct SevGuestState { > char *session_file; > uint32_t cbitpos; > uint32_t reduced_phys_bits; > + char *launch_config_file; > + bool snp; > > /* runtime state */ > uint32_t handle; > @@ -72,10 +79,13 @@ struct SevGuestState { > uint32_t reset_cs; > uint32_t reset_ip; > bool reset_data_valid; > + > + struct snp_launch_config snp_config; > }; > > #define DEFAULT_GUEST_POLICY 0x1 /* disable debug */ > #define DEFAULT_SEV_DEVICE "/dev/sev" > +#define DEFAULT_SEV_SNP_POLICY 0x30000 > > #define SEV_INFO_BLOCK_GUID "00f771de-1a7e-4fcb-890e-68c77e2fb44e" > typedef struct __attribute__((__packed__)) SevInfoBlock { > @@ -298,6 +308,212 @@ sev_guest_set_sev_device(Object *obj, const char *value, Error **errp) > sev->sev_device = g_strdup(value); > } > > +static void > +sev_guest_set_snp(Object *obj, bool value, Error **errp) > +{ > + SevGuestState *sev = SEV_GUEST(obj); > + > + sev->snp = value; > +} > + > +static bool > +sev_guest_get_snp(Object *obj, Error **errp) > +{ > + SevGuestState *sev = SEV_GUEST(obj); > + > + return sev->snp; > +} > + > + > +static char * > +sev_guest_get_launch_config_file(Object *obj, Error **errp) > +{ > + SevGuestState *s = SEV_GUEST(obj); > + > + return g_strdup(s->launch_config_file); > +} > + > +static int > +config_read_uint64(GKeyFile *f, const char *key, uint64_t *value, Error **errp) > +{ > + g_autoptr(GError) error = NULL; > + g_autofree gchar *str = NULL; > + uint64_t res; > + > + str = g_key_file_get_string(f, "SEV-SNP", key, &error); > + if (!str) { > + /* key not found */ > + return 0; > + } > + > + res = g_ascii_strtoull(str, NULL, 16); > + if (res == G_MAXUINT64) { > + error_setg(errp, "Failed to convert %s", str); > + return 1; > + } > + > + *value = res; > + return 0; > +} > + > +static int > +config_read_bool(GKeyFile *f, const char *key, bool *value, Error **errp) > +{ > + g_autoptr(GError) error = NULL; > + gboolean val; > + > + val = g_key_file_get_boolean(f, "SEV-SNP", key, &error); > + if (!val && g_error_matches(error, G_KEY_FILE_ERROR, > + G_KEY_FILE_ERROR_INVALID_VALUE)) { > + error_setg(errp, "%s", error->message); > + return 1; > + } > + > + *value = val; > + return 0; > +} > + > +static int > +config_read_blob(GKeyFile *f, const char *key, uint8_t *blob, uint32_t len, > + Error **errp) > +{ > + g_autoptr(GError) error = NULL; > + g_autofree guchar *data = NULL; > + g_autofree gchar *base64 = NULL; > + gsize size; > + > + base64 = g_key_file_get_string(f, "SEV-SNP", key, &error); > + if (!base64) { > + /* key not found */ > + return 0; > + } > + > + /* lets decode the value string */ > + data = g_base64_decode(base64, &size); > + if (!data) { > + error_setg(errp, "failed to decode '%s'", key); > + return 1; > + } > + > + /* verify the length */ > + if (len != size) { > + error_setg(errp, "invalid length for key '%s' (expected %d got %ld)", > + key, len, size); > + return 1; > + } > + > + memcpy(blob, data, size); > + return 0; > +} > + > +static int > +snp_parse_launch_config(SevGuestState *sev, const char *file, Error **errp) > +{ > + g_autoptr(GError) error = NULL; > + g_autoptr(GKeyFile) key_file = g_key_file_new(); > + struct kvm_sev_snp_launch_start *start = &sev->snp_config.start; > + struct kvm_snp_init *init = &sev->snp_config.init; > + struct kvm_sev_snp_launch_finish *finish = &sev->snp_config.finish; > + uint8_t *id_block = NULL, *id_auth = NULL; > + > + if (!g_key_file_load_from_file(key_file, file, G_KEY_FILE_NONE, &error)) { > + error_setg(errp, "Error loading config file: %s", error->message); > + return 1; > + } > + > + /* Check the group first */ > + if (!g_key_file_has_group(key_file, "SEV-SNP")) { > + error_setg(errp, "Error parsing config file, group SEV-SNP not found"); > + return 1; > + } > + > + /* Get the init_flags used in KVM_SNP_INIT */ > + if (config_read_uint64(key_file, "init_flags", > + (uint64_t *)&init->flags, errp)) { > + goto err; > + } > + > + /* Get the policy used in LAUNCH_START */ > + if (config_read_uint64(key_file, "policy", > + (uint64_t *)&start->policy, errp)) { > + goto err; > + } > + > + /* Get IMI_EN used in LAUNCH_START */ > + if (config_read_bool(key_file, "imi_en", (bool *)&start->imi_en, errp)) { > + goto err; > + } > + > + /* Get MA_EN used in LAUNCH_START */ > + if (config_read_bool(key_file, "imi_en", (bool *)&start->ma_en, errp)) { > + goto err; > + } > + > + /* Get GOSVW used in LAUNCH_START */ > + if (config_read_blob(key_file, "gosvw", (uint8_t *)&start->gosvw, > + sizeof(start->gosvw), errp)) { > + goto err; > + } > + > + /* Get ID block used in LAUNCH_FINISH */ > + if (g_key_file_has_key(key_file, "SEV-SNP", "id_block", &error)) { > + > + id_block = g_malloc(KVM_SEV_SNP_ID_BLOCK_SIZE); > + > + if (config_read_blob(key_file, "id_block", id_block, > + KVM_SEV_SNP_ID_BLOCK_SIZE, errp)) { > + goto err; > + } > + > + finish->id_block_uaddr = (unsigned long)id_block; > + finish->id_block_en = 1; > + } > + > + /* Get authentication block used in LAUNCH_FINISH */ > + if (g_key_file_has_key(key_file, "SEV-SNP", "id_auth", &error)) { > + > + id_auth = g_malloc(KVM_SEV_SNP_ID_AUTH_SIZE); > + > + if (config_read_blob(key_file, "auth_block", id_auth, > + KVM_SEV_SNP_ID_AUTH_SIZE, errp)) { > + goto err; > + } > + > + finish->id_auth_uaddr = (unsigned long)id_auth; > + > + /* Get AUTH_KEY_EN used in LAUNCH_FINISH */ > + if (config_read_bool(key_file, "auth_key_en", > + (bool *)&finish->auth_key_en, errp)) { > + goto err; > + } > + } > + > + /* Get host_data used in LAUNCH_FINISH */ > + if (config_read_blob(key_file, "host_data", (uint8_t *)&finish->host_data, > + sizeof(finish->host_data), errp)) { > + goto err; > + } > + > + return 0; > + > +err: > + g_free(id_block); > + g_free(id_auth); > + return 1; > +} > + > +static void > +sev_guest_set_launch_config_file(Object *obj, const char *value, Error **errp) > +{ > + SevGuestState *s = SEV_GUEST(obj); > + > + if (snp_parse_launch_config(s, value, errp)) { > + return; > + } > + > + s->launch_config_file = g_strdup(value); > +} > + > static void > sev_guest_class_init(ObjectClass *oc, void *data) > { > @@ -316,6 +532,16 @@ sev_guest_class_init(ObjectClass *oc, void *data) > sev_guest_set_session_file); > object_class_property_set_description(oc, "session-file", > "guest owners session parameters (encoded with base64)"); > + object_class_property_add_bool(oc, "snp", > + sev_guest_get_snp, > + sev_guest_set_snp); > + object_class_property_set_description(oc, "snp", > + "enable SEV-SNP support"); > + object_class_property_add_str(oc, "launch-config", > + sev_guest_get_launch_config_file, > + sev_guest_set_launch_config_file); > + object_class_property_set_description(oc, "launch-config", > + "the file provides the SEV-SNP guest launch parameters"); > } > > static void > @@ -325,6 +551,7 @@ sev_guest_instance_init(Object *obj) > > sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE); > sev->policy = DEFAULT_GUEST_POLICY; > + sev->snp_config.start.policy = DEFAULT_SEV_SNP_POLICY; > object_property_add_uint32_ptr(obj, "policy", &sev->policy, > OBJ_PROP_FLAG_READWRITE); > object_property_add_uint32_ptr(obj, "handle", &sev->handle, > -- > 2.17.1 >
On Fri, Jul 09, 2021 at 04:55:46PM -0500, Brijesh Singh wrote: > To launch the SEV-SNP guest, a user can specify up to 8 parameters. > Passing all parameters through command line can be difficult. This sentence applies to pretty much everything in QEMU and the SEV-SNP example is nowhere near an extreme example IMHO. > To simplify > the launch parameter passing, introduce a .ini-like config file that can be > used for passing the parameters to the launch flow. Inventing a new config file format for usage by just one specific niche feature in QEMU is something I'd say we do not want. Our long term goal in QEMU is to move to a world where 100% of QEMU configuration is provided in JSON format, using the QAPI schema to define the accepted input set. > > The contents of the config file will look like this: > > $ cat snp-launch.init > > # SNP launch parameters > [SEV-SNP] > init_flags = 0 > policy = 0x1000 > id_block = "YWFhYWFhYWFhYWFhYWFhCg==" These parameters are really tiny and trivial to provide on the command line, so I'm not finding this config file compelling. > > > Add 'snp' property that can be used to indicate that SEV guest launch > should enable the SNP support. > > SEV-SNP guest launch examples: > > 1) launch without additional parameters > > $(QEMU_CLI) \ > -object sev-guest,id=sev0,snp=on > > 2) launch with optional parameters > $(QEMU_CLI) \ > -object sev-guest,id=sev0,snp=on,launch-config=<file> > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > docs/amd-memory-encryption.txt | 81 +++++++++++- > qapi/qom.json | 6 + > target/i386/sev.c | 227 +++++++++++++++++++++++++++++++++ > 3 files changed, 312 insertions(+), 2 deletions(-) Regards, Daniel
On 7/12/21 9:43 AM, Daniel P. Berrangé wrote: > On Fri, Jul 09, 2021 at 04:55:46PM -0500, Brijesh Singh wrote: >> To launch the SEV-SNP guest, a user can specify up to 8 parameters. >> Passing all parameters through command line can be difficult. > > This sentence applies to pretty much everything in QEMU and the > SEV-SNP example is nowhere near an extreme example IMHO. > >> To simplify >> the launch parameter passing, introduce a .ini-like config file that can be >> used for passing the parameters to the launch flow. > > Inventing a new config file format for usage by just one specific > niche feature in QEMU is something I'd say we do not want. > > Our long term goal in QEMU is to move to a world where 100% of > QEMU configuration is provided in JSON format, using the QAPI > schema to define the accepted input set. > I am open to all suggestions. I was trying to avoid passing all these parameters through the command line because some of them can be huge (up to a page size) >> >> The contents of the config file will look like this: >> >> $ cat snp-launch.init >> >> # SNP launch parameters >> [SEV-SNP] >> init_flags = 0 >> policy = 0x1000 >> id_block = "YWFhYWFhYWFhYWFhYWFhCg==" > > These parameters are really tiny and trivial to provide on the command > line, so I'm not finding this config file compelling. > I have only included 3 small parameters. Other parameters can be up to a page size. The breakdown looks like this: policy: 8 bytes flags: 8 bytes id_block: 96 bytes id_auth: 4096 bytes host_data: 32 bytes gosvw: 16 bytes >> >> >> Add 'snp' property that can be used to indicate that SEV guest launch >> should enable the SNP support. >> >> SEV-SNP guest launch examples: >> >> 1) launch without additional parameters >> >> $(QEMU_CLI) \ >> -object sev-guest,id=sev0,snp=on >> >> 2) launch with optional parameters >> $(QEMU_CLI) \ >> -object sev-guest,id=sev0,snp=on,launch-config=<file> >> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> docs/amd-memory-encryption.txt | 81 +++++++++++- >> qapi/qom.json | 6 + >> target/i386/sev.c | 227 +++++++++++++++++++++++++++++++++ >> 3 files changed, 312 insertions(+), 2 deletions(-) > > Regards, > Daniel >
On 7/12/21 9:34 AM, Dr. David Alan Gilbert wrote: >> >> $ cat snp-launch.init >> >> # SNP launch parameters >> [SEV-SNP] >> init_flags = 0 >> policy = 0x1000 >> id_block = "YWFhYWFhYWFhYWFhYWFhCg==" > > Wouldn't the 'gosvw' and 'hostdata' also be in there? > I did not included all the 8 parameters in the commit messages, mainly because some of them are big. I just picked 3 smaller ones. -Brijesh
* Brijesh Singh (brijesh.singh@amd.com) wrote: > > > On 7/12/21 9:34 AM, Dr. David Alan Gilbert wrote: > > > > > > $ cat snp-launch.init > > > > > > # SNP launch parameters > > > [SEV-SNP] > > > init_flags = 0 > > > policy = 0x1000 > > > id_block = "YWFhYWFhYWFhYWFhYWFhCg==" > > > > Wouldn't the 'gosvw' and 'hostdata' also be in there? > > > > I did not included all the 8 parameters in the commit messages, mainly > because some of them are big. I just picked 3 smaller ones. It would be good to have a full example, even if one of them was something like: stuff = ".....<upto 4096 bytes>...." so we'd just be able to get more of an idea. Dave > -Brijesh >
On Mon, Jul 12, 2021 at 10:56:40AM -0500, Brijesh Singh wrote: > > > On 7/12/21 9:43 AM, Daniel P. Berrangé wrote: > > On Fri, Jul 09, 2021 at 04:55:46PM -0500, Brijesh Singh wrote: > > > To launch the SEV-SNP guest, a user can specify up to 8 parameters. > > > Passing all parameters through command line can be difficult. > > > > This sentence applies to pretty much everything in QEMU and the > > SEV-SNP example is nowhere near an extreme example IMHO. > > > > > To simplify > > > the launch parameter passing, introduce a .ini-like config file that can be > > > used for passing the parameters to the launch flow. > > > > Inventing a new config file format for usage by just one specific > > niche feature in QEMU is something I'd say we do not want. > > > > Our long term goal in QEMU is to move to a world where 100% of > > QEMU configuration is provided in JSON format, using the QAPI > > schema to define the accepted input set. > > > > I am open to all suggestions. I was trying to avoid passing all these > parameters through the command line because some of them can be huge (up to > a page size) > > > > > > > > The contents of the config file will look like this: > > > > > > $ cat snp-launch.init > > > > > > # SNP launch parameters > > > [SEV-SNP] > > > init_flags = 0 > > > policy = 0x1000 > > > id_block = "YWFhYWFhYWFhYWFhYWFhCg==" > > > > These parameters are really tiny and trivial to provide on the command > > line, so I'm not finding this config file compelling. > > > > I have only included 3 small parameters. Other parameters can be up to a > page size. The breakdown looks like this: > > policy: 8 bytes > flags: 8 bytes > id_block: 96 bytes > id_auth: 4096 bytes > host_data: 32 bytes > gosvw: 16 bytes Only the id_auth parameter is really considered large here. When you say "up to a page size", that implies that the size is actually variable. Is that correct, and if so, what is a real world common size going to be ? Is the common size much smaller than this upper limit ? If so I'd just ignore the issue entirely. If not, then, 4k on the command line is certainly ugly, but isn't technically impossible. It would be similarly ugly to have this value stuffed into a libvirt XML configuration for that matter. One option is to supply only that one parameter via an external file, with the file being an opaque blob whose context is the parameter value thus avoiding inventing a custom file format parser. When "id_auth" is described as "authentication data", are there any secrecy requirements around this ? QEMU does have the '-object secret' framework for passing blobs of sensitive data to QEMU and can allow passing via a file: https://qemu-project.gitlab.io/qemu/system/secrets.html Even if this doesn't actually need to be kept private, we could decide to simply (ab)use the 'secret' object anyway as a way to let it be passed in out of band via a file. Libvirt also has a 'secret' concept for passing blobs of sensitive data out of band from the main XML config, which could again be leveraged. It does feel a little dirty to be using 'secrets' in libvirt and QEMU for data that doesn't actually need to be secret, just as a way to avoid huge config files. So we could just ignore the secrets and directly have 'id_auth_file' as a parameter and directly reference a file. Regards, Daniel
Brijesh Singh <brijesh.singh@amd.com> writes: > To launch the SEV-SNP guest, a user can specify up to 8 parameters. > Passing all parameters through command line can be difficult. To simplify > the launch parameter passing, introduce a .ini-like config file that can be > used for passing the parameters to the launch flow. > > The contents of the config file will look like this: > > $ cat snp-launch.init > > # SNP launch parameters > [SEV-SNP] > init_flags = 0 > policy = 0x1000 > id_block = "YWFhYWFhYWFhYWFhYWFhCg==" > > > Add 'snp' property that can be used to indicate that SEV guest launch > should enable the SNP support. > > SEV-SNP guest launch examples: > > 1) launch without additional parameters > > $(QEMU_CLI) \ > -object sev-guest,id=sev0,snp=on > > 2) launch with optional parameters > $(QEMU_CLI) \ > -object sev-guest,id=sev0,snp=on,launch-config=<file> > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> I acknowledge doing complex configuration on the command line can be awkward. But if we added a separate configuration file for every configurable thing where that's the case, we'd have too many already, and we'd constantly grow more. I don't think this is a viable solution. In my opinion, much of what we do on the command line should be done in configuration files instead. Not in several different configuration languages, mind, but using one common language for all our configuration needs. Some of us argue this language already exists: QMP. It can't do everything the command line can do, but that's a matter of putting in the work. However, JSON isn't a good configuration language[1]. To get a decent one, we'd have to to extend JSON[2], or wrap another concrete syntax around QMP's abstract syntax. But this doesn't help you at all *now*. I recommend to do exactly what we've done before for complex configuration: define it in the QAPI schema, so we can use both dotted keys and JSON on the command line, and can have QMP, too. Examples: -blockdev, -display, -compat. Questions? [1] https://www.lucidchart.com/techblog/2018/07/16/why-json-isnt-a-good-configuration-language/ [2] Thanks, but no thanks. Let's make new and interesting mistakes instead of repeating old and tired ones.
On 7/12/21 11:24 AM, Daniel P. Berrangé wrote:>> >> policy: 8 bytes >> flags: 8 bytes >> id_block: 96 bytes >> id_auth: 4096 bytes >> host_data: 32 bytes >> gosvw: 16 bytes > > Only the id_auth parameter is really considered large here. > > When you say "up to a page size", that implies that the size is > actually variable. Is that correct, and if so, what is a real > world common size going to be ? Is the common size much smaller > than this upper limit ? If so I'd just ignore the issue entirely. Looking at the recent spec, it appears that id_auth is fixed to 4K. > > If not, then, 4k on the command line is certainly ugly, but isn't > technically impossible. It would be similarly ugly to have this > value stuffed into a libvirt XML configuration for that matter. > > One option is to supply only that one parameter via an external > file, with the file being an opaque blob whose context is the > parameter value thus avoiding inventing a custom file format > parser. > > When "id_auth" is described as "authentication data", are there > any secrecy requirements around this ? > Yes this sounds much better, we have been using the similar approach for the SEV in which we pass the PDH and session blob through the file. > QEMU does have the '-object secret' framework for passing blobs > of sensitive data to QEMU and can allow passing via a file: > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fqemu-project.gitlab.io%2Fqemu%2Fsystem%2Fsecrets.html&data=04%7C01%7Cbrijesh.singh%40amd.com%7C891fdc1ab0d8483aecb808d945519054%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637617038899405482%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=8AHUC3DeyauxT4Pd2ZkUJkDyu9XHtexybM0BgdRlego%3D&reserved=0 > > Even if this doesn't actually need to be kept private, we > could decide to simply (ab)use the 'secret' object anyway > as a way to let it be passed in out of band via a file. > The content of the field does not need to be protected. It's a public information, so I am not sure the secrets object fits here. thanks
On Fri, Jul 09, 2021 at 04:55:46PM -0500, Brijesh Singh wrote: > To launch the SEV-SNP guest, a user can specify up to 8 parameters. > Passing all parameters through command line can be difficult. To simplify > the launch parameter passing, introduce a .ini-like config file that can be > used for passing the parameters to the launch flow. I agree with Markus' assessment that we are probably going to be better off reusing what we already have for other complex options rather than inventing yet another ini file. Additional things I noted: > +++ b/qapi/qom.json > @@ -749,6 +749,10 @@ > # @reduced-phys-bits: number of bits in physical addresses that become > # unavailable when SEV is enabled > # > +# @snp: SEV-SNP is enabled (default: 0) Here you list 0... > +# > +# @launch-config: launch config file to use Both additions (if we keep the launch-config addition) are missing '(since 6.1)' notations. > +# > # Since: 2.12 > ## > { 'struct': 'SevGuestProperties', > @@ -758,6 +762,8 @@ > '*policy': 'uint32', > '*handle': 'uint32', > '*cbitpos': 'uint32', > + '*snp': 'bool', ...but here you state snp is bool. That means the default is 'false', not '0'. > + '*launch-config': 'str', > 'reduced-phys-bits': 'uint32' } } >
On 7/13/21 8:46 AM, Markus Armbruster wrote: > Brijesh Singh <brijesh.singh@amd.com> writes: > >> To launch the SEV-SNP guest, a user can specify up to 8 parameters. >> Passing all parameters through command line can be difficult. To simplify >> the launch parameter passing, introduce a .ini-like config file that can be >> used for passing the parameters to the launch flow. >> >> The contents of the config file will look like this: >> >> $ cat snp-launch.init >> >> # SNP launch parameters >> [SEV-SNP] >> init_flags = 0 >> policy = 0x1000 >> id_block = "YWFhYWFhYWFhYWFhYWFhCg==" >> >> >> Add 'snp' property that can be used to indicate that SEV guest launch >> should enable the SNP support. >> >> SEV-SNP guest launch examples: >> >> 1) launch without additional parameters >> >> $(QEMU_CLI) \ >> -object sev-guest,id=sev0,snp=on >> >> 2) launch with optional parameters >> $(QEMU_CLI) \ >> -object sev-guest,id=sev0,snp=on,launch-config=<file> >> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > I acknowledge doing complex configuration on the command line can be > awkward. But if we added a separate configuration file for every > configurable thing where that's the case, we'd have too many already, > and we'd constantly grow more. I don't think this is a viable solution. > > In my opinion, much of what we do on the command line should be done in > configuration files instead. Not in several different configuration > languages, mind, but using one common language for all our configuration > needs. > > Some of us argue this language already exists: QMP. It can't do > everything the command line can do, but that's a matter of putting in > the work. However, JSON isn't a good configuration language[1]. To get > a decent one, we'd have to to extend JSON[2], or wrap another concrete > syntax around QMP's abstract syntax. > > But this doesn't help you at all *now*. > > I recommend to do exactly what we've done before for complex > configuration: define it in the QAPI schema, so we can use both dotted > keys and JSON on the command line, and can have QMP, too. Examples: > -blockdev, -display, -compat. > > Questions? I will take a look at the blockdev and try modeling after that. if I run into any questions then I will ask. thanks for the pointer Markus. -Brijesh
On Tue, Jul 13, 2021 at 03:46:19PM +0200, Markus Armbruster wrote: > Brijesh Singh <brijesh.singh@amd.com> writes: > > > To launch the SEV-SNP guest, a user can specify up to 8 parameters. > > Passing all parameters through command line can be difficult. To simplify > > the launch parameter passing, introduce a .ini-like config file that can be > > used for passing the parameters to the launch flow. > > > > The contents of the config file will look like this: > > > > $ cat snp-launch.init > > > > # SNP launch parameters > > [SEV-SNP] > > init_flags = 0 > > policy = 0x1000 > > id_block = "YWFhYWFhYWFhYWFhYWFhCg==" > > > > > > Add 'snp' property that can be used to indicate that SEV guest launch > > should enable the SNP support. > > > > SEV-SNP guest launch examples: > > > > 1) launch without additional parameters > > > > $(QEMU_CLI) \ > > -object sev-guest,id=sev0,snp=on > > > > 2) launch with optional parameters > > $(QEMU_CLI) \ > > -object sev-guest,id=sev0,snp=on,launch-config=<file> > > > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > > I acknowledge doing complex configuration on the command line can be > awkward. But if we added a separate configuration file for every > configurable thing where that's the case, we'd have too many already, > and we'd constantly grow more. I don't think this is a viable solution. > > In my opinion, much of what we do on the command line should be done in > configuration files instead. Not in several different configuration > languages, mind, but using one common language for all our configuration > needs. > > Some of us argue this language already exists: QMP. It can't do > everything the command line can do, but that's a matter of putting in > the work. However, JSON isn't a good configuration language[1]. To get > a decent one, we'd have to to extend JSON[2], or wrap another concrete > syntax around QMP's abstract syntax. > > But this doesn't help you at all *now*. > > I recommend to do exactly what we've done before for complex > configuration: define it in the QAPI schema, so we can use both dotted > keys and JSON on the command line, and can have QMP, too. Examples: > -blockdev, -display, -compat. > > Questions? Hi Markus, Daniel, I'm dealing with similar considerations with some SNP config options relating to CPUID enforcement, so I've started looking into this as well, but am still a little confused on the best way to proceed. I see that -blockdev supports both JSON command-line arguments (via qobject_input_visitor_new) and dotted keys (via qobject_input_vistior_new_keyval). We could introduce a new config group do the same, maybe something specific to ConfidentialGuestSupport objects, e.g.: -confidential-guest-support sev-guest,id=sev0,key_a.subkey_b=... and use the same mechanisms to parse the options, but this seems to either involve adding a layer of option translations between command-line and the underlying object properties, or, if we keep the 1:1 mapping between QAPI-defined keys and object properties, it basically becomes a way to pass a different Visitor implementation into object_property_set(), in this case one created by object_input_visitor_new_keyval() instead of opts_visitor_new(). In either case, genericizing it beyond CGS/SEV would basically be introducing: -object2 sev-guest,id=sev0,key_a.subkey_b=... Which one seems preferable? Or is the answer neither? I've also been looking at whether this could all be handled via -object, and it seems -object already supports JSON command-line arguments, and that switching it from using OptsVisitor to QObjectVisitor for non-JSON case would be enough to have it handle dotted keys, but I'm not sure what the fall-out would be compatibility-wise. All lot of that falls under making sure the QObject/keyval parser is compatible with existing command-lines parsed via OptsVisitor. One example where there still seems to be a difference is lack of support for ranges such as "cpus=1-4" in keyval parser. Daniel had a series that addressed this: https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg08248.html but it doesn't seem to have made it into the tree, which is why I feel like maybe there are complications with this approach I haven't considered? Thanks! -Mike > > > [1] https://www.lucidchart.com/techblog/2018/07/16/why-json-isnt-a-good-configuration-language/ > > [2] Thanks, but no thanks. Let's make new and interesting mistakes > instead of repeating old and tired ones. >
On Tue, Jul 20, 2021 at 02:42:12PM -0500, Michael Roth wrote: > On Tue, Jul 13, 2021 at 03:46:19PM +0200, Markus Armbruster wrote: > > Brijesh Singh <brijesh.singh@amd.com> writes: > > > > > To launch the SEV-SNP guest, a user can specify up to 8 parameters. > > > Passing all parameters through command line can be difficult. To simplify > > > the launch parameter passing, introduce a .ini-like config file that can be > > > used for passing the parameters to the launch flow. > > > > > > The contents of the config file will look like this: > > > > > > $ cat snp-launch.init > > > > > > # SNP launch parameters > > > [SEV-SNP] > > > init_flags = 0 > > > policy = 0x1000 > > > id_block = "YWFhYWFhYWFhYWFhYWFhCg==" > > > > > > > > > Add 'snp' property that can be used to indicate that SEV guest launch > > > should enable the SNP support. > > > > > > SEV-SNP guest launch examples: > > > > > > 1) launch without additional parameters > > > > > > $(QEMU_CLI) \ > > > -object sev-guest,id=sev0,snp=on > > > > > > 2) launch with optional parameters > > > $(QEMU_CLI) \ > > > -object sev-guest,id=sev0,snp=on,launch-config=<file> > > > > > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > > > > I acknowledge doing complex configuration on the command line can be > > awkward. But if we added a separate configuration file for every > > configurable thing where that's the case, we'd have too many already, > > and we'd constantly grow more. I don't think this is a viable solution. > > > > In my opinion, much of what we do on the command line should be done in > > configuration files instead. Not in several different configuration > > languages, mind, but using one common language for all our configuration > > needs. > > > > Some of us argue this language already exists: QMP. It can't do > > everything the command line can do, but that's a matter of putting in > > the work. However, JSON isn't a good configuration language[1]. To get > > a decent one, we'd have to to extend JSON[2], or wrap another concrete > > syntax around QMP's abstract syntax. > > > > But this doesn't help you at all *now*. > > > > I recommend to do exactly what we've done before for complex > > configuration: define it in the QAPI schema, so we can use both dotted > > keys and JSON on the command line, and can have QMP, too. Examples: > > -blockdev, -display, -compat. > > > > Questions? > > Hi Markus, Daniel, > > I'm dealing with similar considerations with some SNP config options > relating to CPUID enforcement, so I've started looking into this as > well, but am still a little confused on the best way to proceed. > > I see that -blockdev supports both JSON command-line arguments (via > qobject_input_visitor_new) and dotted keys > (via qobject_input_vistior_new_keyval). > > We could introduce a new config group do the same, maybe something specific > to ConfidentialGuestSupport objects, e.g.: > > -confidential-guest-support sev-guest,id=sev0,key_a.subkey_b=... We don't wnt to be adding new CLI options anymore. -object with json syntx should ultimately be able to cover everything we'll ever need to do. > > and use the same mechanisms to parse the options, but this seems to > either involve adding a layer of option translations between command-line > and the underlying object properties, or, if we keep the 1:1 mapping > between QAPI-defined keys and object properties, it basically becomes a > way to pass a different Visitor implementation into object_property_set(), > in this case one created by object_input_visitor_new_keyval() instead of > opts_visitor_new(). > > In either case, genericizing it beyond CGS/SEV would basically be > introducing: > > -object2 sev-guest,id=sev0,key_a.subkey_b=... > > Which one seems preferable? Or is the answer neither? Yep, neither is the answer. > > I've also been looking at whether this could all be handled via -object, > and it seems -object already supports JSON command-line arguments, and that > switching it from using OptsVisitor to QObjectVisitor for non-JSON case > would be enough to have it handle dotted keys, but I'm not sure what the > fall-out would be compatibility-wise. > > All lot of that falls under making sure the QObject/keyval parser is > compatible with existing command-lines parsed via OptsVisitor. One example > where there still seems to be a difference is lack of support for ranges > such as "cpus=1-4" in keyval parser. Daniel had a series that addressed > this: > > https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg08248.html > > but it doesn't seem to have made it into the tree, which is why I feel like > maybe there are complications with this approach I haven't considered? The core problem with QemuOpts is that it has hacked various hacks grafted onto it to cope with non-scalar data. My patch was adding yet another hack. It very hard to introduce new hacks without risk of causing incompatibility with other existing hacks, or introducing unexpected edge cases that we don't anticipate. Ultimately I think we've come to the conclusion that QemuOpts is an unfixable dead end that should be left alone. Our future is trending towards being entirely JSON, configured via the QMP monitor not the CLI. As such the json support for -object is a step towards the pure JSON world. IOW, if you have things that work today with QemuOpts that's fine. If, however, you're coming across situations that need non-scalar data and it doesn't work with QemuOpts, then just declare that -object json syntax is mandatory for that scenario. Don't invest time trying to improve QemuOpts for non-scalar data, nor inventing new CLI args. FWIW, specificallly in the case of parameters that take an integer range, like cores=1-4, in JSON we'd end up representing that as a array of integers and having to specify all values explicitly. This is quite verbose, but functionally works. I think it would have been nice if we defined an explicit 'bitmap' scalar data type in QAPI for these kind of things, but at this point in time I think that ship has sailed, and trying to add that now would create an inconsistent representation across different places. Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Tue, Jul 20, 2021 at 02:42:12PM -0500, Michael Roth wrote: >> On Tue, Jul 13, 2021 at 03:46:19PM +0200, Markus Armbruster wrote: [...] >> > I recommend to do exactly what we've done before for complex >> > configuration: define it in the QAPI schema, so we can use both dotted >> > keys and JSON on the command line, and can have QMP, too. Examples: >> > -blockdev, -display, -compat. >> > >> > Questions? >> >> Hi Markus, Daniel, >> >> I'm dealing with similar considerations with some SNP config options >> relating to CPUID enforcement, so I've started looking into this as >> well, but am still a little confused on the best way to proceed. >> >> I see that -blockdev supports both JSON command-line arguments (via >> qobject_input_visitor_new) and dotted keys >> (via qobject_input_vistior_new_keyval). Yes. Convenience function qobject_input_visitor_new_str() provides this. >> We could introduce a new config group do the same, maybe something specific >> to ConfidentialGuestSupport objects, e.g.: >> >> -confidential-guest-support sev-guest,id=sev0,key_a.subkey_b=... > > We don't wnt to be adding new CLI options anymore. -object with json > syntx should ultimately be able to cover everything we'll ever need > to do. Depends. When you want a CLI option to create a single QOM object, then -object is commonly the way to go. >> and use the same mechanisms to parse the options, but this seems to >> either involve adding a layer of option translations between command-line >> and the underlying object properties, or, if we keep the 1:1 mapping >> between QAPI-defined keys and object properties, it basically becomes a >> way to pass a different Visitor implementation into object_property_set(), >> in this case one created by object_input_visitor_new_keyval() instead of >> opts_visitor_new(). qobject_input_visitor_new_str() provides 1:1, i.e. common abstract syntax, and concrete syntax either JSON or dotted keys. Note that the latter is slightly less expressive (can't do empty arrays and objects, may fall apart for type 'any'). If you run into these limitations, use JSON. Machines should always use JSON. qobject_input_visitor_new_str() works by wrapping the "right" visitor around the option argument. Another way to provide a human-friendly interface in addition to a machine-friendly one is to translate from human to the machine interface. HMP works that way: HMP commands wrap around QMP commands. The QMP commands are generated from the QAPI schema. The HMP commands are written by hand, which is maximally flexible, but also laborious. >> In either case, genericizing it beyond CGS/SEV would basically be >> introducing: >> >> -object2 sev-guest,id=sev0,key_a.subkey_b=... That's because existing -object doesn't use keyval_parse() + the keyval QObjct input visitor, it uses QemuOpts and the options visitor, for backward compatibility with all their (barely understood) features and warts. Unfortunate, because even new user-creatable objects can't escape QemuOpts. QemuOpts needs to go. But replacing it is difficult and scary in places. -object is such a place. >> Which one seems preferable? Or is the answer neither? > > Yep, neither is the answer. Welcome to the QemuOpts swamp, here's your waders and a leaky bucket. >> I've also been looking at whether this could all be handled via -object, >> and it seems -object already supports JSON command-line arguments, and that >> switching it from using OptsVisitor to QObjectVisitor for non-JSON case >> would be enough to have it handle dotted keys, but I'm not sure what the >> fall-out would be compatibility-wise. It's complicated, and nobody knows for sure. That's why we didn't dare to take the jump, and instead grafted on JSON syntax without changing the old syntax. Excuse me while I sacrifice another rubber chicken to the backward compatibility idol. >> All lot of that falls under making sure the QObject/keyval parser is >> compatible with existing command-lines parsed via OptsVisitor. One example >> where there still seems to be a difference is lack of support for ranges >> such as "cpus=1-4" in keyval parser. Daniel had a series that addressed >> this: >> >> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg08248.html >> >> but it doesn't seem to have made it into the tree, which is why I feel like >> maybe there are complications with this approach I haven't considered? > > The core problem with QemuOpts is that it has hacked various hacks > grafted onto it to cope with non-scalar data. My patch was adding > yet another hack. It very hard to introduce new hacks without risk > of causing incompatibility with other existing hacks, or introducing > unexpected edge cases that we don't anticipate. Some of the thornier compatibility issues with QemuOpts are due to unforeseen edge cases of, and interactions between features. > Ultimately I think we've come to the conclusion that QemuOpts is an > unfixable dead end that should be left alone. Our future is trending > towards being entirely JSON, configured via the QMP monitor not the > CLI. As such the json support for -object is a step towards the pure > JSON world. QemuOpts served us well for a while, but we've long grown out of its limitations. It needs to go. QemuOpts not providing an adequate CLI language does not imply we can't have an adequate CLI language. The "everything QMP" movement is due to other factors. I have serious reservations about the idea, actually. > IOW, if you have things that work today with QemuOpts that's fine. > > If, however, you're coming across situations that need non-scalar > data and it doesn't work with QemuOpts, then just declare that > -object json syntax is mandatory for that scenario. Don't invest > time trying to improve QemuOpts for non-scalar data, nor inventing > new CLI args. I agree 100% with "don't mess with QemuOpts". That mess is complete. Whether a new CLI option makes sense should be decided case by case. "Must use JSON" feels acceptable for things basically only management applications use. > FWIW, specificallly in the case of parameters that take an integer > range, like cores=1-4, in JSON we'd end up representing that as a > array of integers and having to specify all values explicitly. > This is quite verbose, but functionally works. > > I think it would have been nice if we defined an explicit 'bitmap' > scalar data type in QAPI for these kind of things, but at this point > in time I think that ship has sailed, and trying to add that now > would create an inconsistent representation across different places. External representation (i.e. JSON) should be as consistent as we can make it. Multiple different internal representations can be okay. So far, we represent JSON arrays as linked lists. Optionally representing certain arrays of small integers as bit vectors feels feasible. Whether it's worth the effort is another question.
On Wed, Jul 21, 2021 at 03:08:37PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Tue, Jul 20, 2021 at 02:42:12PM -0500, Michael Roth wrote: > >> On Tue, Jul 13, 2021 at 03:46:19PM +0200, Markus Armbruster wrote: > > [...] > > >> > I recommend to do exactly what we've done before for complex > >> > configuration: define it in the QAPI schema, so we can use both dotted > >> > keys and JSON on the command line, and can have QMP, too. Examples: > >> > -blockdev, -display, -compat. > >> > > >> > Questions? > >> > >> Hi Markus, Daniel, > >> > >> I'm dealing with similar considerations with some SNP config options > >> relating to CPUID enforcement, so I've started looking into this as > >> well, but am still a little confused on the best way to proceed. > >> > >> I see that -blockdev supports both JSON command-line arguments (via > >> qobject_input_visitor_new) and dotted keys > >> (via qobject_input_vistior_new_keyval). > > Yes. Convenience function qobject_input_visitor_new_str() provides > this. > > >> We could introduce a new config group do the same, maybe something specific > >> to ConfidentialGuestSupport objects, e.g.: > >> > >> -confidential-guest-support sev-guest,id=sev0,key_a.subkey_b=... > > > > We don't wnt to be adding new CLI options anymore. -object with json > > syntx should ultimately be able to cover everything we'll ever need > > to do. > > Depends. When you want a CLI option to create a single QOM object, then > -object is commonly the way to go. So if I've read things correctly the fact that this is a question of how to define properties of a single QOM object lends itself to using -object rather than attempting a new -blockdev-like option group, and as such if we want to allow structured parameters we should use pure JSON instead of attempting to layer anything on top of the current keyval parser. > > >> and use the same mechanisms to parse the options, but this seems to > >> either involve adding a layer of option translations between command-line > >> and the underlying object properties, or, if we keep the 1:1 mapping > >> between QAPI-defined keys and object properties, it basically becomes a > >> way to pass a different Visitor implementation into object_property_set(), > >> in this case one created by object_input_visitor_new_keyval() instead of > >> opts_visitor_new(). > > qobject_input_visitor_new_str() provides 1:1, i.e. common abstract > syntax, and concrete syntax either JSON or dotted keys. Note that the > latter is slightly less expressive (can't do empty arrays and objects, > may fall apart for type 'any'). If you run into these limitations, use > JSON. Machines should always use JSON. > > qobject_input_visitor_new_str() works by wrapping the "right" visitor > around the option argument. Another way to provide a human-friendly > interface in addition to a machine-friendly one is to translate from > human to the machine interface. HMP works that way: HMP commands wrap > around QMP commands. The QMP commands are generated from the QAPI > schema. The HMP commands are written by hand, which is maximally > flexible, but also laborious. > > >> In either case, genericizing it beyond CGS/SEV would basically be > >> introducing: > >> > >> -object2 sev-guest,id=sev0,key_a.subkey_b=... > > That's because existing -object doesn't use keyval_parse() + the keyval > QObjct input visitor, it uses QemuOpts and the options visitor, for > backward compatibility with all their (barely understood) features and > warts. > > Unfortunate, because even new user-creatable objects can't escape > QemuOpts. > > QemuOpts needs to go. But replacing it is difficult and scary in > places. -object is such a place. > > >> Which one seems preferable? Or is the answer neither? > > > > Yep, neither is the answer. > > Welcome to the QemuOpts swamp, here's your waders and a leaky bucket. *backs slowly away from swamp* :) So back to the question of whether we need structured parameters. The main driver for this seems to be that the options are currently defined via a config file, which was originally introduced to cope with: a) lots of parameters (8) - not really that significant compared to some other objects/options. b) large page-size parameters - mainly applies to 'id_auth', which could be broken out as individual files/blobs and passed in via normal file path string arguments - already how we handle dh-cert-file and session-file c) separating SNP-specific parameters from the base sev-guest object properties - could possibly be done with a new 'sev-snp-guest' object, which would also help disambiguate stuff like the 32-bit sev/sev-es 'policy' arguments from the 64-bit version in SNP, and can still re-use common properties like 'cbitpos' via some base object - maybe some other benefits, need to look into it more. If they aren't any objections I'll take a stab at this early next week. Will be on PTO until then, but will follow-up soon as I'm back in office. > > Ultimately I think we've come to the conclusion that QemuOpts is an > > unfixable dead end that should be left alone. Our future is trending > > towards being entirely JSON, configured via the QMP monitor not the > > CLI. As such the json support for -object is a step towards the pure > > JSON world. > > QemuOpts served us well for a while, but we've long grown out of its > limitations. It needs to go. > > QemuOpts not providing an adequate CLI language does not imply we can't > have an adequate CLI language. The "everything QMP" movement is due to > other factors. I have serious reservations about the idea, actually. > > > IOW, if you have things that work today with QemuOpts that's fine. > > > > If, however, you're coming across situations that need non-scalar > > data and it doesn't work with QemuOpts, then just declare that > > -object json syntax is mandatory for that scenario. Don't invest > > time trying to improve QemuOpts for non-scalar data, nor inventing > > new CLI args. > > I agree 100% with "don't mess with QemuOpts". That mess is complete. > > Whether a new CLI option makes sense should be decided case by case. > > "Must use JSON" feels acceptable for things basically only management > applications use. Yah, it's great that QAPI/object_add already takes care of the management side of things, but giving up the option of using human-readable/non-JSON command-line args is still a tough pill to swallow, at least as a developer where I know some significant portion of my life will be spent debugging parser errors from bash. *backs up too far, walks into adjacent swamp with no waders* If more cases where structured arguments for Objects really make sense and thus necessitate JSON-only, it would be great if there was some developer tool, e.g. scripts/qemu-cmdline-helper-devs-only that could handle this translation to JSON or QMP and maybe serve as a test-bed for this awesome new cmdline syntax that provides all the expressiveness of QAPI and could abstract away the QemuOpts-specific option formats while still allowing for periodic reworks. Or maybe -readconfig is the better starting point? Or is it too late for that already? -x-readconfig2 ? :) *disappears into swamp*
diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt index ffca382b5f..322bf38f68 100644 --- a/docs/amd-memory-encryption.txt +++ b/docs/amd-memory-encryption.txt @@ -22,8 +22,8 @@ support for notifying a guest's operating system when certain types of VMEXITs are about to occur. This allows the guest to selectively share information with the hypervisor to satisfy the requested function. -Launching ---------- +Launching (SEV and SEV-ES) +-------------------------- Boot images (such as bios) must be encrypted before a guest can be booted. The MEMORY_ENCRYPT_OP ioctl provides commands to encrypt the images: LAUNCH_START, LAUNCH_UPDATE_DATA, LAUNCH_MEASURE and LAUNCH_FINISH. These four commands @@ -113,6 +113,83 @@ a SEV-ES guest: - Requires in-kernel irqchip - the burden is placed on the hypervisor to manage booting APs. +Launching (SEV-SNP) +------------------- +Boot images (such as bios) must be encrypted before a guest can be booted. The +MEMORY_ENCRYPT_OP ioctl provides commands to encrypt the images: +KVM_SNP_INIT, SNP_LAUNCH_START, SNP_LAUNCH_UPDATE, and SNP_LAUNCH_FINISH. These +four commands together generate a fresh memory encryption key for the VM, +encrypt the boot images for a successful launch. + +KVM_SNP_INIT is called first to initialize the SEV-SNP firmware and SNP +features in the KVM. The feature flags value can be provided through the +launch-config file. + ++------------+-------+----------+---------------------------------+ +| key | type | default | meaning | ++------------+-------+----------+---------------------------------+ +| init_flags | hex | 0 | SNP feature flags | ++-----------------------------------------------------------------+ + +Note: currently the init_flags must be zero. + +SNP_LAUNCH_START is called first to create a cryptographic launch context +within the firmware. To create this context, guest owner must provide a guest +policy and other parameters as described in the SEV-SNP firmware +specification. The launch parameters should be specified in the launch-config +ini file and should be treated as a binary blob and must be passed as-is to +the SEV-SNP firmware. + +The SNP_LAUNCH_START uses the following parameters from the launch-config +file. See the SEV-SNP specification for more details. + ++--------+-------+----------+----------------------------------------------+ +| key | type | default | meaning | ++--------+-------+----------+----------------------------------------------+ +| policy | hex | 0x30000 | a 64-bit guest policy | +| imi_en | bool | 0 | 1 when IMI is enabled | +| ma_end | bool | 0 | 1 when migration agent is used | +| gosvw | string| 0 | 16-byte base64 encoded string for the guest | +| | | | OS visible workaround. | ++--------+-------+----------+----------------------------------------------+ + +SNP_LAUNCH_UPDATE encrypts the memory region using the cryptographic context +created via the SNP_LAUNCH_START command. If required, this command can be called +multiple times to encrypt different memory regions. The command also calculates +the measurement of the memory contents as it encrypts. + +SNP_LAUNCH_FINISH finalizes the guest launch flow. Optionally, while finalizing +the launch the firmware can perform checks on the launch digest computing +through the SNP_LAUNCH_UPDATE. To perform the check the user must supply +the id block, authentication blob and host data that should be included in the +attestation report. See the SEV-SNP spec for further details. + +The SNP_LAUNCH_FINISH uses the following parameters from the launch-config file. + ++------------+-------+----------+----------------------------------------------+ +| key | type | default | meaning | ++------------+-------+----------+----------------------------------------------+ +| id_block | string| none | base64 encoded ID block | ++------------+-------+----------+----------------------------------------------+ +| id_auth | string| none | base64 encoded authentication information | ++------------+-------+----------+----------------------------------------------+ +| auth_key_en| bool | 0 | auth block contains author key | ++------------+-------+----------+----------------------------------------------+ +| host_data | string| none | host provided data | ++------------+-------+----------+----------------------------------------------+ + +To launch a SEV-SNP guest + +# ${QEMU} \ + -machine ...,confidential-guest-support=sev0 \ + -object sev-guest,id=sev0,cbitpos=51,reduced-phys-bits=1,snp=on + +To launch a SEV-SNP guest with launch configuration + +# ${QEMU} \ + -machine ...,confidential-guest-support=sev0 \ + -object sev-guest,id=sev0,cbitpos=51,reduced-phys-bits=1,snp=on,launch-config=<config> + Debugging ----------- Since the memory contents of a SEV guest are encrypted, hypervisor access to diff --git a/qapi/qom.json b/qapi/qom.json index 652be317b8..bdf89fda27 100644 --- a/qapi/qom.json +++ b/qapi/qom.json @@ -749,6 +749,10 @@ # @reduced-phys-bits: number of bits in physical addresses that become # unavailable when SEV is enabled # +# @snp: SEV-SNP is enabled (default: 0) +# +# @launch-config: launch config file to use +# # Since: 2.12 ## { 'struct': 'SevGuestProperties', @@ -758,6 +762,8 @@ '*policy': 'uint32', '*handle': 'uint32', '*cbitpos': 'uint32', + '*snp': 'bool', + '*launch-config': 'str', 'reduced-phys-bits': 'uint32' } } ## diff --git a/target/i386/sev.c b/target/i386/sev.c index 83df8c09f6..6b238ef969 100644 --- a/target/i386/sev.c +++ b/target/i386/sev.c @@ -37,6 +37,11 @@ #define TYPE_SEV_GUEST "sev-guest" OBJECT_DECLARE_SIMPLE_TYPE(SevGuestState, SEV_GUEST) +struct snp_launch_config { + struct kvm_snp_init init; + struct kvm_sev_snp_launch_start start; + struct kvm_sev_snp_launch_finish finish; +}; /** * SevGuestState: @@ -58,6 +63,8 @@ struct SevGuestState { char *session_file; uint32_t cbitpos; uint32_t reduced_phys_bits; + char *launch_config_file; + bool snp; /* runtime state */ uint32_t handle; @@ -72,10 +79,13 @@ struct SevGuestState { uint32_t reset_cs; uint32_t reset_ip; bool reset_data_valid; + + struct snp_launch_config snp_config; }; #define DEFAULT_GUEST_POLICY 0x1 /* disable debug */ #define DEFAULT_SEV_DEVICE "/dev/sev" +#define DEFAULT_SEV_SNP_POLICY 0x30000 #define SEV_INFO_BLOCK_GUID "00f771de-1a7e-4fcb-890e-68c77e2fb44e" typedef struct __attribute__((__packed__)) SevInfoBlock { @@ -298,6 +308,212 @@ sev_guest_set_sev_device(Object *obj, const char *value, Error **errp) sev->sev_device = g_strdup(value); } +static void +sev_guest_set_snp(Object *obj, bool value, Error **errp) +{ + SevGuestState *sev = SEV_GUEST(obj); + + sev->snp = value; +} + +static bool +sev_guest_get_snp(Object *obj, Error **errp) +{ + SevGuestState *sev = SEV_GUEST(obj); + + return sev->snp; +} + + +static char * +sev_guest_get_launch_config_file(Object *obj, Error **errp) +{ + SevGuestState *s = SEV_GUEST(obj); + + return g_strdup(s->launch_config_file); +} + +static int +config_read_uint64(GKeyFile *f, const char *key, uint64_t *value, Error **errp) +{ + g_autoptr(GError) error = NULL; + g_autofree gchar *str = NULL; + uint64_t res; + + str = g_key_file_get_string(f, "SEV-SNP", key, &error); + if (!str) { + /* key not found */ + return 0; + } + + res = g_ascii_strtoull(str, NULL, 16); + if (res == G_MAXUINT64) { + error_setg(errp, "Failed to convert %s", str); + return 1; + } + + *value = res; + return 0; +} + +static int +config_read_bool(GKeyFile *f, const char *key, bool *value, Error **errp) +{ + g_autoptr(GError) error = NULL; + gboolean val; + + val = g_key_file_get_boolean(f, "SEV-SNP", key, &error); + if (!val && g_error_matches(error, G_KEY_FILE_ERROR, + G_KEY_FILE_ERROR_INVALID_VALUE)) { + error_setg(errp, "%s", error->message); + return 1; + } + + *value = val; + return 0; +} + +static int +config_read_blob(GKeyFile *f, const char *key, uint8_t *blob, uint32_t len, + Error **errp) +{ + g_autoptr(GError) error = NULL; + g_autofree guchar *data = NULL; + g_autofree gchar *base64 = NULL; + gsize size; + + base64 = g_key_file_get_string(f, "SEV-SNP", key, &error); + if (!base64) { + /* key not found */ + return 0; + } + + /* lets decode the value string */ + data = g_base64_decode(base64, &size); + if (!data) { + error_setg(errp, "failed to decode '%s'", key); + return 1; + } + + /* verify the length */ + if (len != size) { + error_setg(errp, "invalid length for key '%s' (expected %d got %ld)", + key, len, size); + return 1; + } + + memcpy(blob, data, size); + return 0; +} + +static int +snp_parse_launch_config(SevGuestState *sev, const char *file, Error **errp) +{ + g_autoptr(GError) error = NULL; + g_autoptr(GKeyFile) key_file = g_key_file_new(); + struct kvm_sev_snp_launch_start *start = &sev->snp_config.start; + struct kvm_snp_init *init = &sev->snp_config.init; + struct kvm_sev_snp_launch_finish *finish = &sev->snp_config.finish; + uint8_t *id_block = NULL, *id_auth = NULL; + + if (!g_key_file_load_from_file(key_file, file, G_KEY_FILE_NONE, &error)) { + error_setg(errp, "Error loading config file: %s", error->message); + return 1; + } + + /* Check the group first */ + if (!g_key_file_has_group(key_file, "SEV-SNP")) { + error_setg(errp, "Error parsing config file, group SEV-SNP not found"); + return 1; + } + + /* Get the init_flags used in KVM_SNP_INIT */ + if (config_read_uint64(key_file, "init_flags", + (uint64_t *)&init->flags, errp)) { + goto err; + } + + /* Get the policy used in LAUNCH_START */ + if (config_read_uint64(key_file, "policy", + (uint64_t *)&start->policy, errp)) { + goto err; + } + + /* Get IMI_EN used in LAUNCH_START */ + if (config_read_bool(key_file, "imi_en", (bool *)&start->imi_en, errp)) { + goto err; + } + + /* Get MA_EN used in LAUNCH_START */ + if (config_read_bool(key_file, "imi_en", (bool *)&start->ma_en, errp)) { + goto err; + } + + /* Get GOSVW used in LAUNCH_START */ + if (config_read_blob(key_file, "gosvw", (uint8_t *)&start->gosvw, + sizeof(start->gosvw), errp)) { + goto err; + } + + /* Get ID block used in LAUNCH_FINISH */ + if (g_key_file_has_key(key_file, "SEV-SNP", "id_block", &error)) { + + id_block = g_malloc(KVM_SEV_SNP_ID_BLOCK_SIZE); + + if (config_read_blob(key_file, "id_block", id_block, + KVM_SEV_SNP_ID_BLOCK_SIZE, errp)) { + goto err; + } + + finish->id_block_uaddr = (unsigned long)id_block; + finish->id_block_en = 1; + } + + /* Get authentication block used in LAUNCH_FINISH */ + if (g_key_file_has_key(key_file, "SEV-SNP", "id_auth", &error)) { + + id_auth = g_malloc(KVM_SEV_SNP_ID_AUTH_SIZE); + + if (config_read_blob(key_file, "auth_block", id_auth, + KVM_SEV_SNP_ID_AUTH_SIZE, errp)) { + goto err; + } + + finish->id_auth_uaddr = (unsigned long)id_auth; + + /* Get AUTH_KEY_EN used in LAUNCH_FINISH */ + if (config_read_bool(key_file, "auth_key_en", + (bool *)&finish->auth_key_en, errp)) { + goto err; + } + } + + /* Get host_data used in LAUNCH_FINISH */ + if (config_read_blob(key_file, "host_data", (uint8_t *)&finish->host_data, + sizeof(finish->host_data), errp)) { + goto err; + } + + return 0; + +err: + g_free(id_block); + g_free(id_auth); + return 1; +} + +static void +sev_guest_set_launch_config_file(Object *obj, const char *value, Error **errp) +{ + SevGuestState *s = SEV_GUEST(obj); + + if (snp_parse_launch_config(s, value, errp)) { + return; + } + + s->launch_config_file = g_strdup(value); +} + static void sev_guest_class_init(ObjectClass *oc, void *data) { @@ -316,6 +532,16 @@ sev_guest_class_init(ObjectClass *oc, void *data) sev_guest_set_session_file); object_class_property_set_description(oc, "session-file", "guest owners session parameters (encoded with base64)"); + object_class_property_add_bool(oc, "snp", + sev_guest_get_snp, + sev_guest_set_snp); + object_class_property_set_description(oc, "snp", + "enable SEV-SNP support"); + object_class_property_add_str(oc, "launch-config", + sev_guest_get_launch_config_file, + sev_guest_set_launch_config_file); + object_class_property_set_description(oc, "launch-config", + "the file provides the SEV-SNP guest launch parameters"); } static void @@ -325,6 +551,7 @@ sev_guest_instance_init(Object *obj) sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE); sev->policy = DEFAULT_GUEST_POLICY; + sev->snp_config.start.policy = DEFAULT_SEV_SNP_POLICY; object_property_add_uint32_ptr(obj, "policy", &sev->policy, OBJ_PROP_FLAG_READWRITE); object_property_add_uint32_ptr(obj, "handle", &sev->handle,
To launch the SEV-SNP guest, a user can specify up to 8 parameters. Passing all parameters through command line can be difficult. To simplify the launch parameter passing, introduce a .ini-like config file that can be used for passing the parameters to the launch flow. The contents of the config file will look like this: $ cat snp-launch.init # SNP launch parameters [SEV-SNP] init_flags = 0 policy = 0x1000 id_block = "YWFhYWFhYWFhYWFhYWFhCg==" Add 'snp' property that can be used to indicate that SEV guest launch should enable the SNP support. SEV-SNP guest launch examples: 1) launch without additional parameters $(QEMU_CLI) \ -object sev-guest,id=sev0,snp=on 2) launch with optional parameters $(QEMU_CLI) \ -object sev-guest,id=sev0,snp=on,launch-config=<file> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- docs/amd-memory-encryption.txt | 81 +++++++++++- qapi/qom.json | 6 + target/i386/sev.c | 227 +++++++++++++++++++++++++++++++++ 3 files changed, 312 insertions(+), 2 deletions(-)