IGVM files can contain an initial VMSA that should be applied to each
vcpu as part of the initial guest state. The sev_features flags are
provided as part of the VMSA structure. However, KVM only allows
sev_features to be set during initialization and not as the guest is
being prepared for launch.
This patch queries KVM for the supported set of sev_features flags and
processes the IGVM file during kvm_init to determine any sev_features
flags set in the IGVM file. These are then provided in the call to
KVM_SEV_INIT2 to ensure the guest state matches that specified in the
IGVM file.
This does cause the IGVM file to be processed twice. Firstly to extract
the sev_features then secondly to actually configure the guest. However,
the first pass is largely ignored meaning the overhead is minimal.
Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
target/i386/sev.c | 145 ++++++++++++++++++++++++++++++++++++++++------
1 file changed, 126 insertions(+), 19 deletions(-)
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 688b378c42..4b9d74207b 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -117,6 +117,8 @@ struct SevCommonState {
uint32_t cbitpos;
uint32_t reduced_phys_bits;
bool kernel_hashes;
+ uint64_t sev_features;
+ uint64_t supported_sev_features;
/* runtime state */
uint8_t api_major;
@@ -490,7 +492,40 @@ static void sev_apply_cpu_context(CPUState *cpu)
}
}
-static int check_vmsa_supported(hwaddr gpa, const struct sev_es_save_area *vmsa,
+static int check_sev_features(SevCommonState *sev_common, uint64_t sev_features,
+ Error **errp)
+{
+ /*
+ * Ensure SEV_FEATURES is configured for correct SEV hardware and that
+ * the requested features are supported. If SEV-SNP is enabled then
+ * that feature must be enabled, otherwise it must be cleared.
+ */
+ if (sev_snp_enabled() && !(sev_features & SVM_SEV_FEAT_SNP_ACTIVE)) {
+ error_setg(
+ errp,
+ "%s: SEV_SNP is enabled but is not enabled in VMSA sev_features",
+ __func__);
+ return -1;
+ } else if (!sev_snp_enabled() &&
+ (sev_features & SVM_SEV_FEAT_SNP_ACTIVE)) {
+ error_setg(
+ errp,
+ "%s: SEV_SNP is not enabled but is enabled in VMSA sev_features",
+ __func__);
+ return -1;
+ }
+ if (sev_features & ~sev_common->supported_sev_features) {
+ error_setg(errp,
+ "%s: VMSA contains unsupported sev_features: %lX, "
+ "supported features: %lX",
+ __func__, sev_features, sev_common->supported_sev_features);
+ return -1;
+ }
+ return 0;
+}
+
+static int check_vmsa_supported(SevCommonState *sev_common, hwaddr gpa,
+ const struct sev_es_save_area *vmsa,
Error **errp)
{
struct sev_es_save_area vmsa_check;
@@ -556,24 +591,10 @@ static int check_vmsa_supported(hwaddr gpa, const struct sev_es_save_area *vmsa,
vmsa_check.x87_fcw = 0;
vmsa_check.mxcsr = 0;
- if (sev_snp_enabled()) {
- if (vmsa_check.sev_features != SVM_SEV_FEAT_SNP_ACTIVE) {
- error_setg(errp,
- "%s: sev_features in the VMSA contains an unsupported "
- "value. For SEV-SNP, sev_features must be set to %x.",
- __func__, SVM_SEV_FEAT_SNP_ACTIVE);
- return -1;
- }
- vmsa_check.sev_features = 0;
- } else {
- if (vmsa_check.sev_features != 0) {
- error_setg(errp,
- "%s: sev_features in the VMSA contains an unsupported "
- "value. For SEV-ES and SEV, sev_features must be "
- "set to 0.", __func__);
- return -1;
- }
+ if (check_sev_features(sev_common, vmsa_check.sev_features, errp) < 0) {
+ return -1;
}
+ vmsa_check.sev_features = 0;
if (!buffer_is_zero(&vmsa_check, sizeof(vmsa_check))) {
error_setg(errp,
@@ -1663,6 +1684,25 @@ static int sev_snp_kvm_type(X86ConfidentialGuest *cg)
return KVM_X86_SNP_VM;
}
+static int sev_init_supported_features(SevCommonState *sev_common, Error **errp)
+{
+ /* Query KVM for the supported set of sev_features */
+ struct kvm_device_attr attr = {
+ .group = KVM_X86_GRP_SEV,
+ .attr = KVM_X86_SEV_VMSA_FEATURES,
+ .addr = (unsigned long)&sev_common->supported_sev_features,
+ };
+ if (kvm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr) < 0) {
+ error_setg(errp, "%s: failed to query supported sev_features",
+ __func__);
+ return -1;
+ }
+ if (sev_snp_enabled()) {
+ sev_common->supported_sev_features |= SVM_SEV_FEAT_SNP_ACTIVE;
+ }
+ return 0;
+}
+
static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
{
char *devname;
@@ -1743,6 +1783,10 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
}
}
+ if (sev_init_supported_features(sev_common, errp) < 0) {
+ return -1;
+ }
+
trace_kvm_sev_init();
if (x86_klass->kvm_type(X86_CONFIDENTIAL_GUEST(sev_common)) == KVM_X86_DEFAULT_VM) {
cmd = sev_es_enabled() ? KVM_SEV_ES_INIT : KVM_SEV_INIT;
@@ -1750,6 +1794,38 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
ret = sev_ioctl(sev_common->sev_fd, cmd, NULL, &fw_error);
} else {
struct kvm_sev_init args = { 0 };
+ MachineState *machine = MACHINE(qdev_get_machine());
+
+ /*
+ * If configuration is provided via an IGVM file then the IGVM file
+ * might contain configuration of the initial vcpu context. For SEV
+ * the vcpu context includes the sev_features which should be applied
+ * to the vcpu.
+ *
+ * KVM does not synchronize sev_features from CPU state. Instead it
+ * requires sev_features to be provided as part of this initialization
+ * call which is subsequently automatically applied to the VMSA of
+ * each vcpu.
+ *
+ * The IGVM file is normally processed after initialization. Therefore
+ * we need to pre-process it here to extract sev_features in order to
+ * provide it to KVM_SEV_INIT2. Each cgs_* function that is called by
+ * the IGVM processor detects this pre-process by observing the state
+ * as SEV_STATE_UNINIT.
+ */
+ if (machine->igvm) {
+ if (IGVM_CFG_GET_CLASS(machine->igvm)
+ ->process(machine->igvm, machine->cgs, errp) == -1) {
+ return -1;
+ }
+ /*
+ * KVM maintains a bitmask of allowed sev_features. This does not
+ * include SVM_SEV_FEAT_SNP_ACTIVE which is set accordingly by KVM
+ * itself. Therefore we need to clear this flag.
+ */
+ args.vmsa_features = sev_common->sev_features &
+ ~SVM_SEV_FEAT_SNP_ACTIVE;
+ }
ret = sev_ioctl(sev_common->sev_fd, KVM_SEV_INIT2, &args, &fw_error);
}
@@ -2348,6 +2424,24 @@ static int cgs_set_guest_state(hwaddr gpa, uint8_t *ptr, uint64_t len,
SevCommonState *sev_common = SEV_COMMON(MACHINE(qdev_get_machine())->cgs);
SevCommonStateClass *klass = SEV_COMMON_GET_CLASS(sev_common);
+ if (sev_common->state == SEV_STATE_UNINIT) {
+ /* Pre-processing of IGVM file called from sev_common_kvm_init() */
+ if ((cpu_index == 0) && (memory_type == CGS_PAGE_TYPE_VMSA)) {
+ const struct sev_es_save_area *sa =
+ (const struct sev_es_save_area *)ptr;
+ if (len < sizeof(*sa)) {
+ error_setg(errp, "%s: invalid VMSA length encountered",
+ __func__);
+ return -1;
+ }
+ if (check_sev_features(sev_common, sa->sev_features, errp) < 0) {
+ return -1;
+ }
+ sev_common->sev_features = sa->sev_features;
+ }
+ return 0;
+ }
+
if (!sev_enabled()) {
error_setg(errp, "%s: attempt to configure guest memory, but SEV "
"is not enabled", __func__);
@@ -2367,7 +2461,8 @@ static int cgs_set_guest_state(hwaddr gpa, uint8_t *ptr, uint64_t len,
__func__);
return -1;
}
- if (check_vmsa_supported(gpa, (const struct sev_es_save_area *)ptr,
+ if (check_vmsa_supported(sev_common, gpa,
+ (const struct sev_es_save_area *)ptr,
errp) < 0) {
return -1;
}
@@ -2421,6 +2516,12 @@ static int cgs_get_mem_map_entry(int index,
ConfidentialGuestMemoryMapEntry *entry,
Error **errp)
{
+ SevCommonState *sev_common = SEV_COMMON(MACHINE(qdev_get_machine())->cgs);
+ if (sev_common->state == SEV_STATE_UNINIT) {
+ /* Pre-processing of IGVM file called from sev_common_kvm_init() */
+ return 1;
+ }
+
if ((index < 0) || (index >= e820_get_num_entries())) {
return 1;
}
@@ -2451,6 +2552,12 @@ static int cgs_set_guest_policy(ConfidentialGuestPolicyType policy_type,
uint32_t policy_data1_size, void *policy_data2,
uint32_t policy_data2_size, Error **errp)
{
+ SevCommonState *sev_common = SEV_COMMON(MACHINE(qdev_get_machine())->cgs);
+ if (sev_common->state == SEV_STATE_UNINIT) {
+ /* Pre-processing of IGVM file called from sev_common_kvm_init() */
+ return 0;
+ }
+
if (policy_type != GUEST_POLICY_SEV) {
error_setg(errp, "%s: Invalid guest policy type provided for SEV: %d",
__func__, policy_type);
--
2.43.0On Fri, Jun 21, 2024 at 03:29:18PM +0100, Roy Hopkins wrote:
> IGVM files can contain an initial VMSA that should be applied to each
> vcpu as part of the initial guest state. The sev_features flags are
> provided as part of the VMSA structure. However, KVM only allows
> sev_features to be set during initialization and not as the guest is
> being prepared for launch.
>
> This patch queries KVM for the supported set of sev_features flags and
> processes the IGVM file during kvm_init to determine any sev_features
> flags set in the IGVM file. These are then provided in the call to
> KVM_SEV_INIT2 to ensure the guest state matches that specified in the
> IGVM file.
>
> This does cause the IGVM file to be processed twice. Firstly to extract
> the sev_features then secondly to actually configure the guest. However,
> the first pass is largely ignored meaning the overhead is minimal.
>
> Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> ---
> target/i386/sev.c | 145 ++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 126 insertions(+), 19 deletions(-)
>
> static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> {
> char *devname;
> @@ -1743,6 +1783,10 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> }
> }
>
> + if (sev_init_supported_features(sev_common, errp) < 0) {
> + return -1;
> + }
> +
> trace_kvm_sev_init();
> if (x86_klass->kvm_type(X86_CONFIDENTIAL_GUEST(sev_common)) == KVM_X86_DEFAULT_VM) {
> cmd = sev_es_enabled() ? KVM_SEV_ES_INIT : KVM_SEV_INIT;
> @@ -1750,6 +1794,38 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> ret = sev_ioctl(sev_common->sev_fd, cmd, NULL, &fw_error);
> } else {
> struct kvm_sev_init args = { 0 };
> + MachineState *machine = MACHINE(qdev_get_machine());
> +
> + /*
> + * If configuration is provided via an IGVM file then the IGVM file
> + * might contain configuration of the initial vcpu context. For SEV
> + * the vcpu context includes the sev_features which should be applied
> + * to the vcpu.
> + *
> + * KVM does not synchronize sev_features from CPU state. Instead it
> + * requires sev_features to be provided as part of this initialization
> + * call which is subsequently automatically applied to the VMSA of
> + * each vcpu.
> + *
> + * The IGVM file is normally processed after initialization. Therefore
> + * we need to pre-process it here to extract sev_features in order to
> + * provide it to KVM_SEV_INIT2. Each cgs_* function that is called by
> + * the IGVM processor detects this pre-process by observing the state
> + * as SEV_STATE_UNINIT.
> + */
> + if (machine->igvm) {
> + if (IGVM_CFG_GET_CLASS(machine->igvm)
> + ->process(machine->igvm, machine->cgs, errp) == -1) {
> + return -1;
> + }
> + /*
> + * KVM maintains a bitmask of allowed sev_features. This does not
> + * include SVM_SEV_FEAT_SNP_ACTIVE which is set accordingly by KVM
> + * itself. Therefore we need to clear this flag.
> + */
> + args.vmsa_features = sev_common->sev_features &
> + ~SVM_SEV_FEAT_SNP_ACTIVE;
> + }
>
> ret = sev_ioctl(sev_common->sev_fd, KVM_SEV_INIT2, &args, &fw_error);
> }
What happens if the code path takes us down the KVM_SEV_INIT
route, rather than KVM_SEV_INIT2 ? Should we be reporting an
error indicating that IGVM usage is incompatible with the legacy
KVM_SEV_INIT path ?
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 :|
On Mon, 2024-06-24 at 15:14 +0100, Daniel P. Berrangé wrote:
> On Fri, Jun 21, 2024 at 03:29:18PM +0100, Roy Hopkins wrote:
> > IGVM files can contain an initial VMSA that should be applied to each
> > vcpu as part of the initial guest state. The sev_features flags are
> > provided as part of the VMSA structure. However, KVM only allows
> > sev_features to be set during initialization and not as the guest is
> > being prepared for launch.
> >
> > This patch queries KVM for the supported set of sev_features flags and
> > processes the IGVM file during kvm_init to determine any sev_features
> > flags set in the IGVM file. These are then provided in the call to
> > KVM_SEV_INIT2 to ensure the guest state matches that specified in the
> > IGVM file.
> >
> > This does cause the IGVM file to be processed twice. Firstly to extract
> > the sev_features then secondly to actually configure the guest. However,
> > the first pass is largely ignored meaning the overhead is minimal.
> >
> > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> > ---
> > target/i386/sev.c | 145 ++++++++++++++++++++++++++++++++++++++++------
> > 1 file changed, 126 insertions(+), 19 deletions(-)
> >
>
> > static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> > {
> > char *devname;
> > @@ -1743,6 +1783,10 @@ static int
> > sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> > }
> > }
> >
> > + if (sev_init_supported_features(sev_common, errp) < 0) {
> > + return -1;
> > + }
> > +
> > trace_kvm_sev_init();
> > if (x86_klass->kvm_type(X86_CONFIDENTIAL_GUEST(sev_common)) ==
> > KVM_X86_DEFAULT_VM) {
> > cmd = sev_es_enabled() ? KVM_SEV_ES_INIT : KVM_SEV_INIT;
> > @@ -1750,6 +1794,38 @@ static int
> > sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> > ret = sev_ioctl(sev_common->sev_fd, cmd, NULL, &fw_error);
> > } else {
> > struct kvm_sev_init args = { 0 };
> > + MachineState *machine = MACHINE(qdev_get_machine());
> > +
> > + /*
> > + * If configuration is provided via an IGVM file then the IGVM file
> > + * might contain configuration of the initial vcpu context. For SEV
> > + * the vcpu context includes the sev_features which should be
> > applied
> > + * to the vcpu.
> > + *
> > + * KVM does not synchronize sev_features from CPU state. Instead it
> > + * requires sev_features to be provided as part of this
> > initialization
> > + * call which is subsequently automatically applied to the VMSA of
> > + * each vcpu.
> > + *
> > + * The IGVM file is normally processed after initialization.
> > Therefore
> > + * we need to pre-process it here to extract sev_features in order
> > to
> > + * provide it to KVM_SEV_INIT2. Each cgs_* function that is called
> > by
> > + * the IGVM processor detects this pre-process by observing the
> > state
> > + * as SEV_STATE_UNINIT.
> > + */
> > + if (machine->igvm) {
> > + if (IGVM_CFG_GET_CLASS(machine->igvm)
> > + ->process(machine->igvm, machine->cgs, errp) == -1) {
> > + return -1;
> > + }
> > + /*
> > + * KVM maintains a bitmask of allowed sev_features. This does
> > not
> > + * include SVM_SEV_FEAT_SNP_ACTIVE which is set accordingly by
> > KVM
> > + * itself. Therefore we need to clear this flag.
> > + */
> > + args.vmsa_features = sev_common->sev_features &
> > + ~SVM_SEV_FEAT_SNP_ACTIVE;
> > + }
> >
> > ret = sev_ioctl(sev_common->sev_fd, KVM_SEV_INIT2, &args,
> > &fw_error);
> > }
>
> What happens if the code path takes us down the KVM_SEV_INIT
> route, rather than KVM_SEV_INIT2 ? Should we be reporting an
> error indicating that IGVM usage is incompatible with the legacy
> KVM_SEV_INIT path ?
>
>
> With regards,
> Daniel
The idea is that sev_common->supported_sev_features is initialised to 0 for the
legacy path meaning that the call to check_sev_features() that occurs when the
IGVM file is parsed will flag an error stating 'VMSA contains unsupported
sev_features' if any features are set. This should ensure the IGVM file matches
the settings in the legacy kernel.
However, I've just run this against an older kernel and found an error: the
kvm_ioctl in sev_init_supported_features() is only supported in newer kernels
meaning that the launch is aborted on older kernels. I'll update this patch to
fix this and ensure supported_sev_features is 0 for the KVM_SEV_INIT case.
Regards,
Roy
© 2016 - 2024 Red Hat, Inc.