Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <157262960837.2838.17520432516398899751.stgit@naples-babu.amd.com> <157262962352.2838.15656190309312238595.stgit@naples-babu.amd.com>
In-Reply-To: <157262962352.2838.15656190309312238595.stgit@naples-babu.amd.com>
From:   Jim Mattson <jmattson@google.com>
Date:   Fri, 1 Nov 2019 11:29:22 -0700
Message-ID: <CALMp9eQT=a99YhraQZ+awMKOWK=3tg=m9NppZnsvK0Q1PWxbAw@mail.gmail.com>
Subject: Re: [PATCH 2/4] kvm: svm: Enable UMIP feature on AMD
To:     "Moger, Babu" <Babu.Moger@amd.com>
Cc:     "tglx@linutronix.de" <tglx@linutronix.de>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "bp@alien8.de" <bp@alien8.de>, "hpa@zytor.com" <hpa@zytor.com>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "rkrcmar@redhat.com" <rkrcmar@redhat.com>,
        "sean.j.christopherson@intel.com" <sean.j.christopherson@intel.com>,
        "vkuznets@redhat.com" <vkuznets@redhat.com>,
        "wanpengli@tencent.com" <wanpengli@tencent.com>,
        "x86@kernel.org" <x86@kernel.org>,
        "joro@8bytes.org" <joro@8bytes.org>,
        "luto@kernel.org" <luto@kernel.org>,
        "zohar@linux.ibm.com" <zohar@linux.ibm.com>,
        "yamada.masahiro@socionext.com" <yamada.masahiro@socionext.com>,
        "nayna@linux.ibm.com" <nayna@linux.ibm.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Nov 1, 2019 at 10:33 AM Moger, Babu <Babu.Moger@amd.com> wrote:
>
> AMD 2nd generation EPYC processors support UMIP (User-Mode Instruction
> Prevention) feature. The UMIP feature prevents the execution of certain
> instructions if the Current Privilege Level (CPL) is greater than 0.
> If any of these instructions are executed with CPL > 0 and UMIP
> is enabled, then kernel reports a #GP exception.
>
> The idea is taken from articles:
> https://lwn.net/Articles/738209/
> https://lwn.net/Articles/694385/
>
> Enable the feature if supported on bare metal and emulate instructions
> to return dummy values for certain cases.
>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/kvm/svm.c |   21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 4153ca8cddb7..79abbdeca148 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2533,6 +2533,11 @@ static void svm_decache_cr4_guest_bits(struct kvm_vcpu *vcpu)
>  {
>  }
>
> +static bool svm_umip_emulated(void)
> +{
> +       return boot_cpu_has(X86_FEATURE_UMIP);
> +}

This makes no sense to me. If the hardware actually supports UMIP,
then it doesn't have to be emulated.

To the extent that kvm emulates UMIP on Intel CPUs without hardware
UMIP (i.e. smsw is still allowed at CPL>0), we can always do the same
emulation on AMD, because SVM has always offered intercepts of sgdt,
sidt, sldt, and str. So, if you really want to offer this emulation on
pre-EPYC 2 CPUs, this function should just return true. But, I have to
ask, "why?"

*Virtualization* of UMIP on EPYC 2 already works without any of these changes.

>  static void update_cr0_intercept(struct vcpu_svm *svm)
>  {
>         ulong gcr0 = svm->vcpu.arch.cr0;
> @@ -4438,6 +4443,13 @@ static int interrupt_window_interception(struct vcpu_svm *svm)
>         return 1;
>  }
>
> +static int umip_interception(struct vcpu_svm *svm)
> +{
> +       struct kvm_vcpu *vcpu = &svm->vcpu;
> +
> +       return kvm_emulate_instruction(vcpu, 0);
> +}
> +
>  static int pause_interception(struct vcpu_svm *svm)
>  {
>         struct kvm_vcpu *vcpu = &svm->vcpu;
> @@ -4775,6 +4787,10 @@ static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
>         [SVM_EXIT_SMI]                          = nop_on_interception,
>         [SVM_EXIT_INIT]                         = nop_on_interception,
>         [SVM_EXIT_VINTR]                        = interrupt_window_interception,
> +       [SVM_EXIT_IDTR_READ]                    = umip_interception,
> +       [SVM_EXIT_GDTR_READ]                    = umip_interception,
> +       [SVM_EXIT_LDTR_READ]                    = umip_interception,
> +       [SVM_EXIT_TR_READ]                      = umip_interception,
>         [SVM_EXIT_RDPMC]                        = rdpmc_interception,
>         [SVM_EXIT_CPUID]                        = cpuid_interception,
>         [SVM_EXIT_IRET]                         = iret_interception,
> @@ -5976,11 +5992,6 @@ static bool svm_xsaves_supported(void)
>         return boot_cpu_has(X86_FEATURE_XSAVES);
>  }
>
> -static bool svm_umip_emulated(void)
> -{
> -       return false;
> -}
> -
>  static bool svm_pt_supported(void)
>  {
>         return false;
>