| Message ID | 20240104190520.62510-1-Ashish.Kalra@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] x86/sev: Add support for allowing zero SEV ASIDs. | expand |
On 1/4/24 13:05, Ashish Kalra wrote: > From: Ashish Kalra <ashish.kalra@amd.com> > > Some BIOSes allow the end user to set the minimum SEV ASID value > (CPUID 0x8000001F_EDX) to be greater than the maximum number of > encrypted guests, or maximum SEV ASID value (CPUID 0x8000001F_ECX) > in order to dedicate all the SEV ASIDs to SEV-ES or SEV-SNP. > > The SEV support, as coded, does not handle the case where the minimum > SEV ASID value can be greater than the maximum SEV ASID value. > As a result, the following confusing message is issued: > > [ 30.715724] kvm_amd: SEV enabled (ASIDs 1007 - 1006) > > Fix the support to properly handle this case. > > Fixes: 916391a2d1dc ("KVM: SVM: Add support for SEV-ES capability in KVM") > Suggested-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > Cc: stable@vger.kernel.org One minor comment below that could maybe be done when merging vs sending a another version? Otherwise... Acked-by: Tom Lendacky <thomas.lendacky@amd.com> > --- > arch/x86/kvm/svm/sev.c | 40 ++++++++++++++++++++++++---------------- > 1 file changed, 24 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 4900c078045a..2112c94bac76 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -143,8 +143,20 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev) > > static int sev_asid_new(struct kvm_sev_info *sev) > { > - int asid, min_asid, max_asid, ret; > + /* > + * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid. > + * SEV-ES-enabled guest can use from 1 to min_sev_asid - 1. > + * Note: min ASID can end up larger than the max if basic SEV support is > + * effectively disabled by disallowing use of ASIDs for SEV guests. > + */ > + unsigned int min_asid = sev->es_active ? 1 : min_sev_asid; > + unsigned int max_asid = sev->es_active ? min_sev_asid - 1 : max_sev_asid; > + unsigned int asid; > bool retry = true; > + int ret; > + > + if (min_asid > max_asid) > + return -ENOTTY; > > WARN_ON(sev->misc_cg); > sev->misc_cg = get_current_misc_cg(); > @@ -157,12 +169,6 @@ static int sev_asid_new(struct kvm_sev_info *sev) > > mutex_lock(&sev_bitmap_lock); > > - /* > - * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid. > - * SEV-ES-enabled guest can use from 1 to min_sev_asid - 1. > - */ > - min_asid = sev->es_active ? 1 : min_sev_asid; > - max_asid = sev->es_active ? min_sev_asid - 1 : max_sev_asid; > again: > asid = find_next_zero_bit(sev_asid_bitmap, max_asid + 1, min_asid); > if (asid > max_asid) { > @@ -246,21 +252,20 @@ static void sev_unbind_asid(struct kvm *kvm, unsigned int handle) > static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > { > struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > - int asid, ret; > + int ret; > > if (kvm->created_vcpus) > return -EINVAL; > > - ret = -EBUSY; > if (unlikely(sev->active)) > - return ret; > + return -EINVAL; > > sev->active = true; > sev->es_active = argp->id == KVM_SEV_ES_INIT; > - asid = sev_asid_new(sev); > - if (asid < 0) > + ret = sev_asid_new(sev); > + if (ret < 0) > goto e_no_asid; > - sev->asid = asid; > + sev->asid = ret; > > ret = sev_platform_init(&argp->error); > if (ret) > @@ -2229,8 +2234,10 @@ void __init sev_hardware_setup(void) > goto out; > } > > - sev_asid_count = max_sev_asid - min_sev_asid + 1; > - WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count)); > + if (min_sev_asid <= max_sev_asid) { > + sev_asid_count = max_sev_asid - min_sev_asid + 1; > + WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count)); > + } > sev_supported = true; > > /* SEV-ES support requested? */ > @@ -2261,7 +2268,8 @@ void __init sev_hardware_setup(void) > out: > if (boot_cpu_has(X86_FEATURE_SEV)) > pr_info("SEV %s (ASIDs %u - %u)\n", > - sev_supported ? "enabled" : "disabled", > + sev_supported ? (min_sev_asid <= max_sev_asid ? "enabled" : "unusable") > + : "disabled", Just a nit with the alignment, it would look better if the ":" was lined up under the first "?". Thanks, Tom > min_sev_asid, max_sev_asid); > if (boot_cpu_has(X86_FEATURE_SEV_ES)) > pr_info("SEV-ES %s (ASIDs %u - %u)\n",
On Thu, Jan 04, 2024, Ashish Kalra wrote: > From: Ashish Kalra <ashish.kalra@amd.com> > > Some BIOSes allow the end user to set the minimum SEV ASID value > (CPUID 0x8000001F_EDX) to be greater than the maximum number of > encrypted guests, or maximum SEV ASID value (CPUID 0x8000001F_ECX) > in order to dedicate all the SEV ASIDs to SEV-ES or SEV-SNP. > > The SEV support, as coded, does not handle the case where the minimum > SEV ASID value can be greater than the maximum SEV ASID value. > As a result, the following confusing message is issued: > > [ 30.715724] kvm_amd: SEV enabled (ASIDs 1007 - 1006) > > Fix the support to properly handle this case. > > Fixes: 916391a2d1dc ("KVM: SVM: Add support for SEV-ES capability in KVM") > Suggested-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > Cc: stable@vger.kernel.org > --- > arch/x86/kvm/svm/sev.c | 40 ++++++++++++++++++++++++---------------- This should be ~3 patches: 1. Convert ASID variables/params to unsigned integers. 2. Return -EINVAL instead of -EBUSY 3. The actual fix here E.g if #2 breaks userspace (extremely unlikely) then bisection should point at exactly that, not at a commit with a whole pile of unrelated things going on. I'll send a v4, #1 should also be accompanied by a cleanup of sev_asid_new() to not multiplex the ASID with the return code. It can simply set sev->asid directly, which as a bonus makes sev_asid_new() and sev_asid_free() more symmetric.
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 4900c078045a..2112c94bac76 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -143,8 +143,20 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev) static int sev_asid_new(struct kvm_sev_info *sev) { - int asid, min_asid, max_asid, ret; + /* + * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid. + * SEV-ES-enabled guest can use from 1 to min_sev_asid - 1. + * Note: min ASID can end up larger than the max if basic SEV support is + * effectively disabled by disallowing use of ASIDs for SEV guests. + */ + unsigned int min_asid = sev->es_active ? 1 : min_sev_asid; + unsigned int max_asid = sev->es_active ? min_sev_asid - 1 : max_sev_asid; + unsigned int asid; bool retry = true; + int ret; + + if (min_asid > max_asid) + return -ENOTTY; WARN_ON(sev->misc_cg); sev->misc_cg = get_current_misc_cg(); @@ -157,12 +169,6 @@ static int sev_asid_new(struct kvm_sev_info *sev) mutex_lock(&sev_bitmap_lock); - /* - * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid. - * SEV-ES-enabled guest can use from 1 to min_sev_asid - 1. - */ - min_asid = sev->es_active ? 1 : min_sev_asid; - max_asid = sev->es_active ? min_sev_asid - 1 : max_sev_asid; again: asid = find_next_zero_bit(sev_asid_bitmap, max_asid + 1, min_asid); if (asid > max_asid) { @@ -246,21 +252,20 @@ static void sev_unbind_asid(struct kvm *kvm, unsigned int handle) static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) { struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; - int asid, ret; + int ret; if (kvm->created_vcpus) return -EINVAL; - ret = -EBUSY; if (unlikely(sev->active)) - return ret; + return -EINVAL; sev->active = true; sev->es_active = argp->id == KVM_SEV_ES_INIT; - asid = sev_asid_new(sev); - if (asid < 0) + ret = sev_asid_new(sev); + if (ret < 0) goto e_no_asid; - sev->asid = asid; + sev->asid = ret; ret = sev_platform_init(&argp->error); if (ret) @@ -2229,8 +2234,10 @@ void __init sev_hardware_setup(void) goto out; } - sev_asid_count = max_sev_asid - min_sev_asid + 1; - WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count)); + if (min_sev_asid <= max_sev_asid) { + sev_asid_count = max_sev_asid - min_sev_asid + 1; + WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count)); + } sev_supported = true; /* SEV-ES support requested? */ @@ -2261,7 +2268,8 @@ void __init sev_hardware_setup(void) out: if (boot_cpu_has(X86_FEATURE_SEV)) pr_info("SEV %s (ASIDs %u - %u)\n", - sev_supported ? "enabled" : "disabled", + sev_supported ? (min_sev_asid <= max_sev_asid ? "enabled" : "unusable") + : "disabled", min_sev_asid, max_sev_asid); if (boot_cpu_has(X86_FEATURE_SEV_ES)) pr_info("SEV-ES %s (ASIDs %u - %u)\n",