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;
Subject: Re: [PATCH v3 4/4] KVM: VMX: Allow direct access to
 MSR_IA32_SPEC_CTRL
To:     KarimAllah Ahmed <karahmed@amazon.com>,
        Jim Mattson <jmattson@google.com>
Cc:     KarimAllah Ahmed <karahmed@amazon.de>,
        kvm list <kvm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        the arch/x86 maintainers <x86@kernel.org>,
        Asit Mallick <asit.k.mallick@intel.com>,
        Arjan Van De Ven <arjan.van.de.ven@intel.com>,
        Dave Hansen <dave.hansen@intel.com>,
        Andi Kleen <ak@linux.intel.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Tim Chen <tim.c.chen@linux.intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Dan Williams <dan.j.williams@intel.com>,
        Jun Nakajima <jun.nakajima@intel.com>,
        David Woodhouse <dwmw@amazon.co.uk>,
        Greg KH <gregkh@linuxfoundation.org>,
        Andy Lutomirski <luto@kernel.org>,
        Ashok Raj <ashok.raj@intel.com>
References: <1517271028-15916-1-git-send-email-karahmed@amazon.de>
 <1517271028-15916-5-git-send-email-karahmed@amazon.de>
 <CALMp9eSmjkJR466C6QuudsFNjEvmyn1hBoK8zUh9EGRNSoMu9Q@mail.gmail.com>
 <43859417-ae76-ed1f-eb4f-8a84a35998fc@amazon.com>
 <CALMp9eQsHzuMx042E50Q3c5H1twZ6-d4ojQ=EE664BGX9GGn8Q@mail.gmail.com>
 <d09301b9-4d51-ff17-17ba-eb80ee9c56e6@amazon.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <7bd999e6-bc15-2470-80fb-771161df39d6@redhat.com>
Date:   Tue, 30 Jan 2018 19:19:54 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.2
MIME-Version: 1.0
In-Reply-To: <d09301b9-4d51-ff17-17ba-eb80ee9c56e6@amazon.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 30/01/2018 18:50, KarimAllah Ahmed wrote:
> On 01/30/2018 11:49 PM, Jim Mattson wrote:
>> On Tue, Jan 30, 2018 at 1:00 PM, KarimAllah Ahmed
>> <karahmed@amazon.com> wrote:
>>> Ooops! I did not think at all about nested :)
>>>
>>> This should be addressed now, I hope:
>>>
>>> http://git.infradead.org/linux-retpoline.git/commitdiff/f7f0cbba3e0cffcee050a8a5a9597a162d57e572
>>>
>>
>> +               if (cpu_has_vmx_msr_bitmap() && data &&
>> +                   !vmx->save_spec_ctrl_on_exit) {
>> +                       vmx->save_spec_ctrl_on_exit = true;
>> +
>> +                       msr_bitmap = is_guest_mode(vcpu) ?
>> vmx->nested.vmcs02.msr_bitmap :
>> +
>> vmx->vmcs01.msr_bitmap;
>> +                       vmx_disable_intercept_for_msr(msr_bitmap,
>> +                                                     MSR_IA32_SPEC_CTRL,
>> +                                                     MSR_TYPE_RW);
>> +               }
>>
>> There are two ways to get to this point in vmx_set_msr while
>> is_guest_mode(vcpu) is true:
>> 1) L0 is processing vmcs12's VM-entry MSR load list on emulated
>> VM-entry (see enter_vmx_non_root_mode).
>> 2) L2 tried to execute WRMSR, writes to the MSR are intercepted in
>> vmcs02's MSR permission bitmap, and writes to the MSR are not
>> intercepted in vmcs12's MSR permission bitmap.
>>
>> In the first case, disabling the intercepts for the MSR in
>> vmx->nested.vmcs02.msr_bitmap is incorrect, because we haven't yet
>> determined that the intercepts are clear in vmcs12's MSR permission
>> bitmap.
>> In the second case, disabling *both* of the intercepts for the MSR in
>> vmx->nested.vmcs02.msr_bitmap is incorrect, because we don't know that
>> the read intercept is clear in vmcs12's MSR permission bitmap.
>> Furthermore, disabling the write intercept for the MSR in
>> vmx->nested.vmcs02.msr_bitmap is somewhat fruitless, because
>> nested_vmx_merge_msr_bitmap is just going to undo that change on the
>> next emulated VM-entry.
> 
> Okay, I took a second look at the code (specially
> nested_vmx_merge_msr_bitmap).
> 
> This means that I simply should not touch the MSR bitmap in set_msr in
> case of nested, I just need to properly update the l02 msr_bitmap in
> nested_vmx_merge_msr_bitmap. As in here:
> 
> http://git.infradead.org/linux-retpoline.git/commitdiff/d90eedebdd16bb00741a2c93bc13c5e444c99c2b
> 
> 
> or am I still missing something? (sorry, did not actually look at the
> nested code before!)

The new code in nested_vmx_merge_msr_bitmap should be conditional on
vmx->save_spec_ctrl_on_exit.  Also, guest_cpuid_has is pretty slow
(because of kvm_find_cpuid_entry); calling it once or twice on each and
every nested vmexit is probably not a good idea.  Apart from this, it
looks good to me.

Paolo