Re: [PATCH 2/2] KVM: SVM: add some info prints to SEV init

[Aleksandr Mikhalitsyn]

On Fri, May 19, 2023 at 8:17 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Tue, Apr 04, 2023, Alexander Miqqqqkhalitsyn wrote:
> > Let's add a few pr_info's to sev_hardware_setup to make SEV/SEV-ES
> > enabling a little bit handier for users. Right now it's too hard
> > to guess why SEV/SEV-ES are failing to enable.
>
> Hmm, I'm somewhat torn, but I'm against taking this patch, at least not in its
> current form.  I appreciated that determining why KVM isn't enabling SEV/SEV-ES
> is annoying, but there's very little actionable information provided here that
> isn't also super obvious.  I also don't want to start us down a slippery slope
> of printing out messages every time KVM doesn't enable a feature.
>
> If someone tries to enable SEV and doesn't check that their CPU supports SEV,
> then IMO that's on them.  Ditto for SEV-ES.
>
> The NPT thing is mildly interesting, but practically speaking I don't expect that
> to ever be a hindrace for generic enabling.  Ditto for MMIO caching.
>
> The decode assists check is (a) completely unactionable for the vast, vast majority
> of users and (b) is a WARN_ON_ONCE() condition.
>
> The ASID stuff is by far the most interesting, but that's also quite interesting
> for when SEV and SEV-ES _are_ fully supported.
>
> So if we want to provide the user more info, I'd prefer to do something like the
> below, which I think would be more helpful and would avoid my slippery slope
> concerns.

Dear Sean,

Thanks for looking into this!

I agree with your points, let's go that way and print only ASID stuff
as it can be not obvious to the end-user.

I'm ready to prepare -v2 if you don't mind.

Kind regards,
Alex

>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index c25aeb550cd9..eb4c6e3812d9 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2216,7 +2216,6 @@ void __init sev_hardware_setup(void)
>         if (misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count))
>                 goto out;
>
> -       pr_info("SEV supported: %u ASIDs\n", sev_asid_count);
>         sev_supported = true;
>
>         /* SEV-ES support requested? */
> @@ -2243,11 +2242,16 @@ void __init sev_hardware_setup(void)
>         sev_es_asid_count = min_sev_asid - 1;
>         if (misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count))
>                 goto out;
> -
> -       pr_info("SEV-ES supported: %u ASIDs\n", sev_es_asid_count);
>         sev_es_supported = true;
>
>  out:
> +       if (boot_cpu_has(X86_FEATURE_SEV))
> +               pr_info("SEV %s (ASIDs %u - %u)\n",
> +                       sev_supported ? "enabled" : "disabled", ...);
> +       if (boot_cpu_has(X86_FEATURE_SEV_ES))
> +               pr_info("SEV-ES %s (ASIDs %u - %u)\n",
> +                       sev_es_supported ? "enabled" : "disabled", ...);
> +
>         sev_enabled = sev_supported;
>         sev_es_enabled = sev_es_supported;
>  #endif