| Message ID | 20240102232136.38778-1-Ashish.Kalra@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sev: Add support for allowing zero SEV ASIDs. | expand |
On Tue, Jan 02, 2024, Ashish Kalra wrote: > @@ -2172,8 +2176,10 @@ void sev_vm_destroy(struct kvm *kvm) > > void __init sev_set_cpu_caps(void) > { > - if (!sev_enabled) > + if (!sev_guests_enabled) { Ugh, what a mess. The module param will show sev_enabled=false, but the caps and CPUID will show SEV=true. And this is doubly silly because "sev_enabled" is never actually checked, e.g. if misc cgroup support is disabled, KVM_SEV_INIT will try to reclaim ASIDs and eventually fail with -EBUSY, which is super confusing to users. The other weirdness is that KVM can cause sev_enabled=false && sev_es_enabled=true, but if *userspace* sets sev_enabled=false then sev_es_enabled is also forced off. In other words, the least awful option seems to be to keep sev_enabled true :-( > kvm_cpu_cap_clear(X86_FEATURE_SEV); > + return; This is blatantly wrong, as it can result in KVM advertising SEV-ES if SEV is disabled by the user. > + } > if (!sev_es_enabled) > kvm_cpu_cap_clear(X86_FEATURE_SEV_ES); > } > @@ -2229,9 +2235,11 @@ 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)); > - sev_supported = true; > + 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? */ > if (!sev_es_enabled) > @@ -2262,7 +2270,8 @@ void __init sev_hardware_setup(void) > if (boot_cpu_has(X86_FEATURE_SEV)) > pr_info("SEV %s (ASIDs %u - %u)\n", > sev_supported ? "enabled" : "disabled", > - min_sev_asid, max_sev_asid); > + sev_supported ? min_sev_asid : 0, > + sev_supported ? max_sev_asid : 0); I honestly think we should print the "garbage" values. The whole point of printing the min/max SEV ASIDs was to help users understand why SEV is disabled, i.e. printing zeroes is counterproductive. > if (boot_cpu_has(X86_FEATURE_SEV_ES)) > pr_info("SEV-ES %s (ASIDs %u - %u)\n", > sev_es_supported ? "enabled" : "disabled", It's all a bit gross, but I think we want something like this (I'm definitely open to suggestions though): diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index d0c580607f00..bfac6d17462a 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, the + * 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) { @@ -2232,8 +2238,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? */ @@ -2264,8 +2272,9 @@ 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", - min_sev_asid, max_sev_asid); + sev_supported ? (min_sev_asid <= max_sev_asid ? "enabled" : "unusable") : "disabled", + sev_supported ? min_sev_asid : 0, + sev_supported ? max_sev_asid : 0); if (boot_cpu_has(X86_FEATURE_SEV_ES)) pr_info("SEV-ES %s (ASIDs %u - %u)\n", sev_es_supported ? "enabled" : "disabled",
Hello Sean, On 1/2/2024 6:30 PM, Sean Christopherson wrote: > On Tue, Jan 02, 2024, Ashish Kalra wrote: >> @@ -2172,8 +2176,10 @@ void sev_vm_destroy(struct kvm *kvm) >> >> void __init sev_set_cpu_caps(void) >> { >> - if (!sev_enabled) >> + if (!sev_guests_enabled) { > Ugh, what a mess. The module param will show sev_enabled=false, but the caps > and CPUID will show SEV=true. > > And this is doubly silly because "sev_enabled" is never actually checked, e.g. > if misc cgroup support is disabled, KVM_SEV_INIT will try to reclaim ASIDs and > eventually fail with -EBUSY, which is super confusing to users. But this is what we expect that KVM_SEV_INIT will fail. In this case, sev_asid_new() will not actually try to reclaim any ASIDs as sev_misc_cg_try_charge() will fail before any ASID bitmap walking/reclamation and return an error which will eventually return -EBUSY to the user. > > The other weirdness is that KVM can cause sev_enabled=false && sev_es_enabled=true, > but if *userspace* sets sev_enabled=false then sev_es_enabled is also forced off. But that is already the behavior without this patch applied. > > In other words, the least awful option seems to be to keep sev_enabled true :-( > >> kvm_cpu_cap_clear(X86_FEATURE_SEV); >> + return; > This is blatantly wrong, as it can result in KVM advertising SEV-ES if SEV is > disabled by the user. No, this ensures that we don't advertise any SEV capability if neither SEV/SEV-ES or in future SNP is enabled. > >> + } >> if (!sev_es_enabled) >> kvm_cpu_cap_clear(X86_FEATURE_SEV_ES); >> } >> @@ -2229,9 +2235,11 @@ 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)); >> - sev_supported = true; >> + 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? */ >> if (!sev_es_enabled) >> @@ -2262,7 +2270,8 @@ void __init sev_hardware_setup(void) >> if (boot_cpu_has(X86_FEATURE_SEV)) >> pr_info("SEV %s (ASIDs %u - %u)\n", >> sev_supported ? "enabled" : "disabled", >> - min_sev_asid, max_sev_asid); >> + sev_supported ? min_sev_asid : 0, >> + sev_supported ? max_sev_asid : 0); > I honestly think we should print the "garbage" values. The whole point of > printing the min/max SEV ASIDs was to help users understand why SEV is disabled, > i.e. printing zeroes is counterproductive. > >> if (boot_cpu_has(X86_FEATURE_SEV_ES)) >> pr_info("SEV-ES %s (ASIDs %u - %u)\n", >> sev_es_supported ? "enabled" : "disabled", > It's all a bit gross, but I think we want something like this (I'm definitely > open to suggestions though): > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index d0c580607f00..bfac6d17462a 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, the > + * 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; > This will still return -EBUSY to user. This check here or the failure return from sev_misc_cg_try_charge() are quite similar in that sense. My point is that the same is achieved quite cleanly with sev_misc_cg_try_charge() too. > 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) { > @@ -2232,8 +2238,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? */ > @@ -2264,8 +2272,9 @@ 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", > - min_sev_asid, max_sev_asid); > + sev_supported ? (min_sev_asid <= max_sev_asid ? "enabled" : "unusable") : "disabled", > + sev_supported ? min_sev_asid : 0, > + sev_supported ? max_sev_asid : 0); We are not showing min and max ASIDs for SEV as {0,0} with this patch as sev_supported is true ? Thanks, Ashish > if (boot_cpu_has(X86_FEATURE_SEV_ES)) > pr_info("SEV-ES %s (ASIDs %u - %u)\n", > sev_es_supported ? "enabled" : "disabled",
On Wed, Jan 03, 2024, Ashish Kalra wrote: > Hello Sean, > > On 1/2/2024 6:30 PM, Sean Christopherson wrote: > > On Tue, Jan 02, 2024, Ashish Kalra wrote: > > > @@ -2172,8 +2176,10 @@ void sev_vm_destroy(struct kvm *kvm) > > > void __init sev_set_cpu_caps(void) > > > { > > > - if (!sev_enabled) > > > + if (!sev_guests_enabled) { > > Ugh, what a mess. The module param will show sev_enabled=false, but the caps > > and CPUID will show SEV=true. > > > > And this is doubly silly because "sev_enabled" is never actually checked, e.g. > > if misc cgroup support is disabled, KVM_SEV_INIT will try to reclaim ASIDs and > > eventually fail with -EBUSY, which is super confusing to users. > > But this is what we expect that KVM_SEV_INIT will fail. In this case, > sev_asid_new() will not actually try to reclaim any ASIDs as sev_misc_cg_try_charge() > will fail before any ASID bitmap walking/reclamation and return an error which > will eventually return -EBUSY to the user. Please read what I wrote. "if misc cgroup support is disabled", i.e. if CONFIG_CGROUP_MISC=n, then sev_misc_cg_try_charge() is a nop. > > The other weirdness is that KVM can cause sev_enabled=false && sev_es_enabled=true, > > but if *userspace* sets sev_enabled=false then sev_es_enabled is also forced off. > But that is already the behavior without this patch applied. > > > > In other words, the least awful option seems to be to keep sev_enabled true :-( > > > > > kvm_cpu_cap_clear(X86_FEATURE_SEV); > > > + return; > > This is blatantly wrong, as it can result in KVM advertising SEV-ES if SEV is > > disabled by the user. > No, this ensures that we don't advertise any SEV capability if neither > SEV/SEV-ES or in future SNP is enabled. No, it does not. There is an early return statement here that prevents KVM from invoking kvm_cpu_cap_clear() for X86_FEATURE_SEV_ES. Do I think userspace will actually be tripped up by seeing SEV_ES without SEV? No. Is it unnecessarily confusing? Yes. > > > + } > > > if (!sev_es_enabled) > > > kvm_cpu_cap_clear(X86_FEATURE_SEV_ES); > > > } > > > @@ -2229,9 +2235,11 @@ 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)); > > > - sev_supported = true; > > > + 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? */ > > > if (!sev_es_enabled) > > > @@ -2262,7 +2270,8 @@ void __init sev_hardware_setup(void) > > > if (boot_cpu_has(X86_FEATURE_SEV)) > > > pr_info("SEV %s (ASIDs %u - %u)\n", > > > sev_supported ? "enabled" : "disabled", > > > - min_sev_asid, max_sev_asid); > > > + sev_supported ? min_sev_asid : 0, > > > + sev_supported ? max_sev_asid : 0); > > I honestly think we should print the "garbage" values. The whole point of > > printing the min/max SEV ASIDs was to help users understand why SEV is disabled, > > i.e. printing zeroes is counterproductive. > > > > > if (boot_cpu_has(X86_FEATURE_SEV_ES)) > > > pr_info("SEV-ES %s (ASIDs %u - %u)\n", > > > sev_es_supported ? "enabled" : "disabled", > > It's all a bit gross, but I think we want something like this (I'm definitely > > open to suggestions though): > > > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > > index d0c580607f00..bfac6d17462a 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, the > > + * 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; > > This will still return -EBUSY to user. Huh? The above is obviously -ENOTTY, and I don't see anything in the call stack that will convert it to -EBUSY. > This check here or the failure return from sev_misc_cg_try_charge() are quite > similar in that sense. > > My point is that the same is achieved quite cleanly with > sev_misc_cg_try_charge() too. "Without additional effort" is not synonymous with "cleanly". Relying on an accounting restriction that is completely orthogonal to basic functionality is not "clean". > > 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) { > > @@ -2232,8 +2238,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? */ > > @@ -2264,8 +2272,9 @@ 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", > > - min_sev_asid, max_sev_asid); > > + sev_supported ? (min_sev_asid <= max_sev_asid ? "enabled" : "unusable") : "disabled", > > + sev_supported ? min_sev_asid : 0, > > + sev_supported ? max_sev_asid : 0); > > We are not showing min and max ASIDs for SEV as {0,0} with this patch as > sev_supported is true ? Yes, and that is deliberate. See this from above: : I honestly think we should print the "garbage" values. The whole point of : printing the min/max SEV ASIDs was to help users understand why SEV is disabled, : i.e. printing zeroes is counterproductive.
On 1/3/2024 3:10 PM, Sean Christopherson wrote: > On Wed, Jan 03, 2024, Ashish Kalra wrote: >> Hello Sean, >> >> On 1/2/2024 6:30 PM, Sean Christopherson wrote: >>> On Tue, Jan 02, 2024, Ashish Kalra wrote: >>>> @@ -2172,8 +2176,10 @@ void sev_vm_destroy(struct kvm *kvm) >>>> void __init sev_set_cpu_caps(void) >>>> { >>>> - if (!sev_enabled) >>>> + if (!sev_guests_enabled) { >>> Ugh, what a mess. The module param will show sev_enabled=false, but the caps >>> and CPUID will show SEV=true. >>> >>> And this is doubly silly because "sev_enabled" is never actually checked, e.g. >>> if misc cgroup support is disabled, KVM_SEV_INIT will try to reclaim ASIDs and >>> eventually fail with -EBUSY, which is super confusing to users. >> But this is what we expect that KVM_SEV_INIT will fail. In this case, >> sev_asid_new() will not actually try to reclaim any ASIDs as sev_misc_cg_try_charge() >> will fail before any ASID bitmap walking/reclamation and return an error which >> will eventually return -EBUSY to the user. > Please read what I wrote. "if misc cgroup support is disabled", i.e. if > CONFIG_CGROUP_MISC=n, then sev_misc_cg_try_charge() is a nop. > >>> The other weirdness is that KVM can cause sev_enabled=false && sev_es_enabled=true, >>> but if *userspace* sets sev_enabled=false then sev_es_enabled is also forced off. >> But that is already the behavior without this patch applied. >>> In other words, the least awful option seems to be to keep sev_enabled true :-( >>> >>>> kvm_cpu_cap_clear(X86_FEATURE_SEV); >>>> + return; >>> This is blatantly wrong, as it can result in KVM advertising SEV-ES if SEV is >>> disabled by the user. >> No, this ensures that we don't advertise any SEV capability if neither >> SEV/SEV-ES or in future SNP is enabled. > No, it does not. There is an early return statement here that prevents KVM from > invoking kvm_cpu_cap_clear() for X86_FEATURE_SEV_ES. Do I think userspace will > actually be tripped up by seeing SEV_ES without SEV? No. Is it unnecessarily > confusing? Yes. > >>>> + } >>>> if (!sev_es_enabled) >>>> kvm_cpu_cap_clear(X86_FEATURE_SEV_ES); >>>> } >>>> @@ -2229,9 +2235,11 @@ 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)); >>>> - sev_supported = true; >>>> + 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? */ >>>> if (!sev_es_enabled) >>>> @@ -2262,7 +2270,8 @@ void __init sev_hardware_setup(void) >>>> if (boot_cpu_has(X86_FEATURE_SEV)) >>>> pr_info("SEV %s (ASIDs %u - %u)\n", >>>> sev_supported ? "enabled" : "disabled", >>>> - min_sev_asid, max_sev_asid); >>>> + sev_supported ? min_sev_asid : 0, >>>> + sev_supported ? max_sev_asid : 0); >>> I honestly think we should print the "garbage" values. The whole point of >>> printing the min/max SEV ASIDs was to help users understand why SEV is disabled, >>> i.e. printing zeroes is counterproductive. >>> >>>> if (boot_cpu_has(X86_FEATURE_SEV_ES)) >>>> pr_info("SEV-ES %s (ASIDs %u - %u)\n", >>>> sev_es_supported ? "enabled" : "disabled", >>> It's all a bit gross, but I think we want something like this (I'm definitely >>> open to suggestions though): >>> >>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c >>> index d0c580607f00..bfac6d17462a 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, the >>> + * 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; >> This will still return -EBUSY to user. > Huh? The above is obviously -ENOTTY, and I don't see anything in the call stack > that will convert it to -EBUSY. Actually, sev_asid_new() returning failure to sev_guest_init() will cause it to return -EBUSY to user. Thanks, Ashish >> This check here or the failure return from sev_misc_cg_try_charge() are quite >> similar in that sense. >> >> My point is that the same is achieved quite cleanly with >> sev_misc_cg_try_charge() too. > "Without additional effort" is not synonymous with "cleanly". Relying on an > accounting restriction that is completely orthogonal to basic functionality is > not "clean". > >>> 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) { >>> @@ -2232,8 +2238,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? */ >>> @@ -2264,8 +2272,9 @@ 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", >>> - min_sev_asid, max_sev_asid); >>> + sev_supported ? (min_sev_asid <= max_sev_asid ? "enabled" : "unusable") : "disabled", >>> + sev_supported ? min_sev_asid : 0, >>> + sev_supported ? max_sev_asid : 0); >> We are not showing min and max ASIDs for SEV as {0,0} with this patch as >> sev_supported is true ? > Yes, and that is deliberate. See this from above: > > : I honestly think we should print the "garbage" values. The whole point of > : printing the min/max SEV ASIDs was to help users understand why SEV is disabled, > : i.e. printing zeroes is counterproductive.
On Wed, Jan 03, 2024, Ashish Kalra wrote: > On 1/3/2024 3:10 PM, Sean Christopherson wrote: > > > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > > > > index d0c580607f00..bfac6d17462a 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, the > > > > + * 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; > > > This will still return -EBUSY to user. > > Huh? The above is obviously -ENOTTY, and I don't see anything in the call stack > > that will convert it to -EBUSY. > > Actually, sev_asid_new() returning failure to sev_guest_init() will cause it > to return -EBUSY to user. Argh, I see it now. That too should be fixed, e.g. diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index d0c580607f00..79eb11083ad5 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -246,21 +246,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)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 4900c078045a..ad41008ca0d9 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -59,10 +59,14 @@ 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); + +/* When true, at least one type of SEV guest is enabled to run */ +static bool sev_guests_enabled; #else #define sev_enabled false #define sev_es_enabled false #define sev_es_debug_swap_enabled false +#define sev_guests_enabled false #endif /* CONFIG_KVM_AMD_SEV */ static u8 sev_enc_bit; @@ -1854,7 +1858,7 @@ int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp) struct kvm_sev_cmd sev_cmd; int r; - if (!sev_enabled) + if (!sev_guests_enabled) return -ENOTTY; if (!argp) @@ -2172,8 +2176,10 @@ void sev_vm_destroy(struct kvm *kvm) void __init sev_set_cpu_caps(void) { - if (!sev_enabled) + if (!sev_guests_enabled) { kvm_cpu_cap_clear(X86_FEATURE_SEV); + return; + } if (!sev_es_enabled) kvm_cpu_cap_clear(X86_FEATURE_SEV_ES); } @@ -2229,9 +2235,11 @@ 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)); - sev_supported = true; + 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? */ if (!sev_es_enabled) @@ -2262,7 +2270,8 @@ void __init sev_hardware_setup(void) if (boot_cpu_has(X86_FEATURE_SEV)) pr_info("SEV %s (ASIDs %u - %u)\n", sev_supported ? "enabled" : "disabled", - min_sev_asid, max_sev_asid); + sev_supported ? min_sev_asid : 0, + sev_supported ? max_sev_asid : 0); if (boot_cpu_has(X86_FEATURE_SEV_ES)) pr_info("SEV-ES %s (ASIDs %u - %u)\n", sev_es_supported ? "enabled" : "disabled", @@ -2270,6 +2279,7 @@ void __init sev_hardware_setup(void) sev_enabled = sev_supported; sev_es_enabled = sev_es_supported; + sev_guests_enabled = sev_enabled || sev_es_enabled; 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; @@ -2278,7 +2288,7 @@ void __init sev_hardware_setup(void) void sev_hardware_unsetup(void) { - if (!sev_enabled) + if (!sev_guests_enabled) return; /* No need to take sev_bitmap_lock, all VMs have been destroyed. */ @@ -2293,7 +2303,7 @@ void sev_hardware_unsetup(void) int sev_cpu_init(struct svm_cpu_data *sd) { - if (!sev_enabled) + if (!sev_guests_enabled) return 0; sd->sev_vmcbs = kcalloc(nr_asids, sizeof(void *), GFP_KERNEL);