Re: [PATCH v2] x86/sev: Add support for allowing zero SEV ASIDs.

[Kalra, Ashish]

On 1/3/2024 11:28 PM, Tom Lendacky wrote:

> On 1/3/24 20:46, Ashish Kalra wrote:
> > From: Ashish Kalra <ashish.kalra@xxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> >   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",