Subject: Re: [PATCH] KVM: vmx: speed up MSR bitmap merge
To: Jim Mattson <jmattson@google.com>
Cc: LKML <linux-kernel@vger.kernel.org>, kvm list <kvm@vger.kernel.org>,
        David Hildenbrand <david@redhat.com>, Bandan Das <bsd@redhat.com>
References: <1513171828-5130-1-git-send-email-pbonzini@redhat.com>
 <CALMp9eTgA=T_n1d_gUi9d0_igZZXczjqziS323iyURHa0z1_qg@mail.gmail.com>
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <944389d2-07f0-1291-77dc-8a99588a5bb4@redhat.com>
Date: Tue, 19 Dec 2017 22:13:14 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <CALMp9eTgA=T_n1d_gUi9d0_igZZXczjqziS323iyURHa0z1_qg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2671
Lines: 57

On 19/12/2017 20:58, Jim Mattson wrote:
>> +               /* Disable read intercept for all MSRs between 0x800 and 0x8ff.  */
> Aren't we actually adopting the read intercepts from VMCS12 and
> *enabling* the *write* intercepts?

Yeah, the comment isn't the best.  What I mean is that L0 doesn't care
about intercepting the reads, so the default all-set msr_bitmap_l0 is
changed as if the read intercept was disabled.  This leaves
msr_bitmap_l1 as the result.

>> +               for (msr = 0x800; msr <= 0x8ff; msr += BITS_PER_LONG) {
>> +                       unsigned word = msr / BITS_PER_LONG;
>> +                       msr_bitmap_l0[word] = msr_bitmap_l1[word];
>> +                       msr_bitmap_l0[word + (0x800 / sizeof(long))] = ~0;
>
> The indexing above seems a bit obtuse, but maybe it will be clear
> enough after the above comment is fixed up.
> 
>> +               }
>> +       } else {
>> +               for (msr = 0x800; msr <= 0x8ff; msr += BITS_PER_LONG) {
>> +                       unsigned word = msr / BITS_PER_LONG;
>> +                       msr_bitmap_l0[word] = ~0;
>> +                       msr_bitmap_l0[word + (0x800 / sizeof(long))] = ~0;
>> +               }
>> +       }
>>
>> -       if (nested_cpu_has_virt_x2apic_mode(vmcs12)) {
>> -               if (nested_cpu_has_apic_reg_virt(vmcs12))
>> -                       for (msr = 0x800; msr <= 0x8ff; msr++)
>> -                               nested_vmx_disable_intercept_for_msr(
>> -                                       msr_bitmap_l1, msr_bitmap_l0,
>> -                                       msr, MSR_TYPE_R);
>> +       nested_vmx_disable_intercept_for_msr(
>> +               msr_bitmap_l1, msr_bitmap_l0,
>> +               APIC_BASE_MSR + (APIC_TASKPRI >> 4),
> Perhaps you could #define X2APIC_MSR(reg) (APIC_BASE_MSR + ((reg) >>
> 4)) somewhere appropriate (e.g. arch/x86/include/asm/apicdef.h) and
> use that here (and below) for brevity?

Good idea.

> Should we also think about letting L1 control pass-through of some of
> the more mundane MSRs, like FS_BASE, GS_BASE, and KERNEL_GS_BASE?

Rhetorical question? :)  Yes, it probably gives a small performance
improvement on real-world multi-process (or even just multi-threaded)
workloads.  It's a separate thing to do though.

The next obvious step, to me, is to split prepare_vmcs02 in two parts.
Most of the work can be skipped in the common case where we didn't get
any vmread/vmwrite exit and all guest accesses between VMRESUMEs go
through the shadow VMCS.  This gives both an easy place to draw the line
between the two parts, and an easy way to detect the need to go down
slow path.  Any takers? :)

Paolo