| Message ID | 20240104024656.57821-1-Ashish.Kalra@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] x86/sev: Add support for allowing zero SEV ASIDs. | expand |
On 1/3/24 20:46, 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 | 41 +++++++++++++++++++++++++---------------- > 1 file changed, 25 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 4900c078045a..651d671ff8ae 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -143,8 +143,21 @@ 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; > + Remove this blank line. > 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 +170,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 +253,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 +2235,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) { Shouldn't this be: if (min_sev_asid <= max_sev_asid) ? You only want to do the misc_cg_set_capactity() call if you can have SEV guests. Thanks, Tom > + 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 +2269,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",
On 1/3/2024 11:28 PM, Tom Lendacky wrote: > On 1/3/24 20:46, 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 | 41 +++++++++++++++++++++++++---------------- >> 1 file changed, 25 insertions(+), 16 deletions(-) >> >> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c >> index 4900c078045a..651d671ff8ae 100644 >> --- a/arch/x86/kvm/svm/sev.c >> +++ b/arch/x86/kvm/svm/sev.c >> @@ -143,8 +143,21 @@ 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; >> + > > Remove this blank line. > >> 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 +170,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 +253,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 +2235,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) { > > Shouldn't this be: if (min_sev_asid <= max_sev_asid) ? > > You only want to do the misc_cg_set_capactity() call if you can have > SEV guests. > > Yes, it should be, will fix it and post v3. Thanks, Ashish >> + 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 +2269,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",
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 4900c078045a..651d671ff8ae 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -143,8 +143,21 @@ 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 +170,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 +253,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 +2235,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 +2269,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",