Compute the set of features to be stored in the VMSA when KVM is
initialized; move it from there into kvm_sev_info when SEV is initialized,
and then into the initial VMSA.
The new variable can then be used to return the set of supported features
to userspace, via the KVM_GET_DEVICE_ATTR ioctl.
Signed-off-by: Paolo Bonzini <[email protected]>
Message-Id: <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
---
.../virt/kvm/x86/amd-memory-encryption.rst | 12 ++++++++++
arch/x86/include/uapi/asm/kvm.h | 1 +
arch/x86/kvm/svm/sev.c | 23 +++++++++++++++++--
arch/x86/kvm/svm/svm.c | 1 +
arch/x86/kvm/svm/svm.h | 1 +
5 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/Documentation/virt/kvm/x86/amd-memory-encryption.rst b/Documentation/virt/kvm/x86/amd-memory-encryption.rst
index 37c5c37f4f6e..5ed11bc16b96 100644
--- a/Documentation/virt/kvm/x86/amd-memory-encryption.rst
+++ b/Documentation/virt/kvm/x86/amd-memory-encryption.rst
@@ -424,6 +424,18 @@ issued by the hypervisor to make the guest ready for execution.
Returns: 0 on success, -negative on error
+Device attribute API
+====================
+
+Attributes of the SEV implementation can be retrieved through the
+``KVM_HAS_DEVICE_ATTR`` and ``KVM_GET_DEVICE_ATTR`` ioctls on the ``/dev/kvm``
+device node.
+
+Currently only one attribute is implemented:
+
+* group 0, attribute ``KVM_X86_SEV_VMSA_FEATURES``: return the set of all
+ bits that are accepted in the ``vmsa_features`` of ``KVM_SEV_INIT2``.
+
Firmware Management
===================
diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index b305daff056e..cccaa5ff6d01 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -459,6 +459,7 @@ struct kvm_sync_regs {
/* attributes for system fd (group 0) */
#define KVM_X86_XCOMP_GUEST_SUPP 0
+#define KVM_X86_SEV_VMSA_FEATURES 1
struct kvm_vmx_nested_state_data {
__u8 vmcs12[KVM_STATE_NESTED_VMX_VMCS_SIZE];
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index f760106c31f8..53e958805ab9 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -59,10 +59,12 @@ module_param_named(sev_es, sev_es_enabled, bool, 0444);
/* enable/disable SEV-ES DebugSwap support */
static bool sev_es_debug_swap_enabled = true;
module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0444);
+static u64 sev_supported_vmsa_features;
#else
#define sev_enabled false
#define sev_es_enabled false
#define sev_es_debug_swap_enabled false
+#define sev_supported_vmsa_features 0
#endif /* CONFIG_KVM_AMD_SEV */
static u8 sev_enc_bit;
@@ -612,8 +614,7 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
save->xss = svm->vcpu.arch.ia32_xss;
save->dr6 = svm->vcpu.arch.dr6;
- if (sev_es_debug_swap_enabled)
- save->sev_features |= SVM_SEV_FEAT_DEBUG_SWAP;
+ save->sev_features = sev_supported_vmsa_features;
pr_debug("Virtual Machine Save Area (VMSA):\n");
print_hex_dump_debug("", DUMP_PREFIX_NONE, 16, 1, save, sizeof(*save), false);
@@ -1849,6 +1850,20 @@ int sev_vm_move_enc_context_from(struct kvm *kvm, unsigned int source_fd)
return ret;
}
+int sev_dev_get_attr(u64 attr, u64 *val)
+{
+ switch (attr) {
+#ifdef CONFIG_KVM_AMD_SEV
+ case KVM_X86_SEV_VMSA_FEATURES:
+ *val = sev_supported_vmsa_features;
+ return 0;
+#endif
+
+ default:
+ return -ENXIO;
+ }
+}
+
int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp)
{
struct kvm_sev_cmd sev_cmd;
@@ -2276,6 +2291,10 @@ void __init sev_hardware_setup(void)
if (!sev_es_enabled || !cpu_feature_enabled(X86_FEATURE_DEBUG_SWAP) ||
!cpu_feature_enabled(X86_FEATURE_NO_NESTED_DATA_BP))
sev_es_debug_swap_enabled = false;
+
+ sev_supported_vmsa_features = 0;
+ if (sev_es_debug_swap_enabled)
+ sev_supported_vmsa_features |= SVM_SEV_FEAT_DEBUG_SWAP;
#endif
}
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index e90b429c84f1..aa1792f402ab 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -5014,6 +5014,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
.enable_smi_window = svm_enable_smi_window,
#endif
+ .dev_get_attr = sev_dev_get_attr,
.mem_enc_ioctl = sev_mem_enc_ioctl,
.mem_enc_register_region = sev_mem_enc_register_region,
.mem_enc_unregister_region = sev_mem_enc_unregister_region,
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 8ef95139cd24..d630026b23b0 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -671,6 +671,7 @@ void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu);
extern unsigned int max_sev_asid;
void sev_vm_destroy(struct kvm *kvm);
+int sev_dev_get_attr(u64 attr, u64 *val);
int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp);
int sev_mem_enc_register_region(struct kvm *kvm,
struct kvm_enc_region *range);
--
2.39.1
On Fri, Feb 23, 2024, Paolo Bonzini wrote:
> Compute the set of features to be stored in the VMSA when KVM is
> initialized; move it from there into kvm_sev_info when SEV is initialized,
> and then into the initial VMSA.
>
> The new variable can then be used to return the set of supported features
> to userspace, via the KVM_GET_DEVICE_ATTR ioctl.
>
> Signed-off-by: Paolo Bonzini <[email protected]>
> Message-Id: <[email protected]>
Maybe in v3 we'll find out whether or not you can triple-stamp a double-stamp :-)
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index f760106c31f8..53e958805ab9 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -59,10 +59,12 @@ module_param_named(sev_es, sev_es_enabled, bool, 0444);
> /* enable/disable SEV-ES DebugSwap support */
> static bool sev_es_debug_swap_enabled = true;
> module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0444);
> +static u64 sev_supported_vmsa_features;
> #else
> #define sev_enabled false
> #define sev_es_enabled false
> #define sev_es_debug_swap_enabled false
> +#define sev_supported_vmsa_features 0
Ok, I've reached my breaking point. Compiling sev.c for CONFIG_KVM_AMD_SEV=n is
getting untenable. Splattering #ifdefs _inside_ SEV specific functions is weird
and confusing.
And unless dead code elimination isn't as effective as I think it is, we don't
even need any stuba since sev_guest() and sev_es_guest() are __always_inline
specifically so that useless code can be elided. Or if we want to avoid use of
IS_ENABLED(), we could add four stubs, which is still well worth it.
Note, I also have a separate series that I will post today (I hope) that gives
__svm_sev_es_vcpu_run() similar treatment (the 32-bit "support" in assembly is
all kinds of stupid).
Attached patches are compile-tested only, though I'll try to take them for a spin
on hardware later today.
On 2/23/24 17:07, Sean Christopherson wrote:
> And unless dead code elimination isn't as effective as I think it is,
> we don't even need any stubs since sev_guest() and sev_es_guest()
> are __always_inline specifically so that useless code can be elided.
> Or if we want to avoid use of IS_ENABLED(), we could add four stubs,
> which is still well worth it.
This particular #ifdef was needed to avoid a compilation failure, but
I'll check your patches and include them.
Paolo
On Fri, Feb 23, 2024, Paolo Bonzini wrote:
> On 2/23/24 17:07, Sean Christopherson wrote:
> > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > > index f760106c31f8..53e958805ab9 100644
> > > --- a/arch/x86/kvm/svm/sev.c
> > > +++ b/arch/x86/kvm/svm/sev.c
> > > @@ -59,10 +59,12 @@ module_param_named(sev_es, sev_es_enabled, bool, 0444);
> > > /* enable/disable SEV-ES DebugSwap support */
> > > static bool sev_es_debug_swap_enabled = true;
> > > module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0444);
> > > +static u64 sev_supported_vmsa_features;
> > > #else
> > > #define sev_enabled false
> > > #define sev_es_enabled false
> > > #define sev_es_debug_swap_enabled false
> > > +#define sev_supported_vmsa_features 0
> >
> > Ok, I've reached my breaking point. Compiling sev.c for CONFIG_KVM_AMD_SEV=n is
> > getting untenable. Splattering #ifdefs _inside_ SEV specific functions is weird
> > and confusing.
>
> Ok, I think in some cases I prefer stubs but I'll weave your 4 patches in
> v3.
No problem, I don't have a strong preference. I initially added stubs instead of
the IS_ENABLED(). The main reason I switched is when I realized that sev_set_cpu_caps()
*cleared* capabilities, and so decided it would be safer to have a separate patch
that effectively stubbed out the global SEV calls.
On 2/23/24 17:07, Sean Christopherson wrote:
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index f760106c31f8..53e958805ab9 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -59,10 +59,12 @@ module_param_named(sev_es, sev_es_enabled, bool, 0444);
>> /* enable/disable SEV-ES DebugSwap support */
>> static bool sev_es_debug_swap_enabled = true;
>> module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0444);
>> +static u64 sev_supported_vmsa_features;
>> #else
>> #define sev_enabled false
>> #define sev_es_enabled false
>> #define sev_es_debug_swap_enabled false
>> +#define sev_supported_vmsa_features 0
>
> Ok, I've reached my breaking point. Compiling sev.c for CONFIG_KVM_AMD_SEV=n is
> getting untenable. Splattering #ifdefs _inside_ SEV specific functions is weird
> and confusing.
Ok, I think in some cases I prefer stubs but I'll weave your 4 patches
in v3.
Paolo
> And unless dead code elimination isn't as effective as I think it is, we don't
> even need any stuba since sev_guest() and sev_es_guest() are __always_inline
> specifically so that useless code can be elided. Or if we want to avoid use of
> IS_ENABLED(), we could add four stubs, which is still well worth it.
>
> Note, I also have a separate series that I will post today (I hope) that gives
> __svm_sev_es_vcpu_run() similar treatment (the 32-bit "support" in assembly is
> all kinds of stupid).
>
> Attached patches are compile-tested only, though I'll try to take them for a spin
> on hardware later today.