DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 04E9072F1F
Subject: Re: [PATCH] kvm: x86: disable KVM_FAST_MMIO_BUS
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= <rkrcmar@redhat.com>,
        stable@vger.kernel.org
References: <20170816112249.28939-1-pbonzini@redhat.com>
 <20170816155132-mutt-send-email-mst@kernel.org>
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <9de5ebf5-457d-2a34-0314-c6c612ddb2e9@redhat.com>
Date: Wed, 16 Aug 2017 15:05:51 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <20170816155132-mutt-send-email-mst@kernel.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3311
Lines: 86

On 16/08/2017 14:58, Michael S. Tsirkin wrote:
> On Wed, Aug 16, 2017 at 01:22:49PM +0200, Paolo Bonzini wrote:
>> Microsoft pointed out privately to me that KVM's handling of
>> KVM_FAST_MMIO_BUS is invalid.  Using skip_emulation_instruction is invalid
>> in EPT misconfiguration vmexit handlers, because neither EPT violations
>> nor misconfigurations are listed in the manual among the VM exits that
>> set the VM-exit instruction length field.
>>
>> While physical processors seem to set the field, this is not architectural
>> and is just a side effect of the implementation.  I couldn't convince
>> myself of any condition on the exit qualification where VM-exit
>> instruction length "has" to be defined; there are no trap-like VM-exits
>> that can be repurposed; and fault-like VM-exits such as descriptor-table
>> exits provide no decoding information.  So I don't really see any elegant
>> way to fix it except by disabling KVM_FAST_MMIO_BUS, which means virtio
>> 1 will go slower.
> 
> How about I will try asking Intel about it? If they can commit to length
> being there in the future, we are all set.

Nope, "I couldn't convince myself of any condition on the exit
qualification where VM-exit instruction length "has" to be defined".  So
assuming Intel can do it, it would only apply to future processors (2
years+ for server SKUs).

Plus of course it wouldn't be guaranteed to work on nested.

>> Adding a hypercall or MSR write that does a fast MMIO write to a physical
>> address would do it, but it adds hypervisor knowledge in virtio, including
>> CPUID handling.
> 
> Another issue is that it will break DPDK on virtio.

Not break, just make it slower.

Paolo

> Hmm that's quite unfortunate as we have just completed rolling out MMIO
> signalling across the board. We did measure a significant slowdown
> before enabling fast mmio. 
> 
>     Guest TX:(TCP)
>     size/session/+throughput%/+cpu%/-+per cpu%/
>     64/1/[+18.9183%]/-0.2823%/[+19.2550%]/
>     64/2/[+13.5714%]/[+2.2675%]/[+11.0533%]/
>     64/4/[+13.1070%]/[+2.1817%]/[+10.6920%]/
>     64/8/[+13.0426%]/[+2.0887%]/[+10.7299%]/
>     256/1/[+36.2761%]/+6.3434%/[+28.1471%]/
>     ...
>     1024/1/[+44.8873%]/+2.0811%/[+41.9335%]/
>     ...
>     1024/4/+0.0228%/[-2.2044%]/[+2.2774%]/
>     ...
>     16384/2/+0.0127%/[-5.0346%]/[+5.3148%]/
>     ...
>     65535/1/[+0.0062%]/[-4.1183%]/[+4.3017%]/
>     65535/2/+0.0004%/[-4.2311%]/[+4.4185%]/
>     65535/4/+0.0107%/[-4.6106%]/[+4.8446%]/
>     65535/8/-0.0090%/[-5.5178%]/[+5.8306%]/
>     
> 
> See commit bc85ccfdf5cc045588f665c84b5707d7364c8a6c for more numbers.
> 
> 
>> ---
>>  arch/x86/kvm/vmx.c | 5 -----
>>  1 file changed, 5 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 375dca24cf42..b3eaeb20670d 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -6320,11 +6320,6 @@ static int handle_ept_misconfig(struct kvm_vcpu *vcpu)
>>  	gpa_t gpa;
>>  
>>  	gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
>> -	if (!kvm_io_bus_write(vcpu, KVM_FAST_MMIO_BUS, gpa, 0, NULL)) {
>> -		trace_kvm_fast_mmio(gpa);
>> -		return kvm_skip_emulated_instruction(vcpu);
>> -	}
>> -
>>  	ret = handle_mmio_page_fault(vcpu, gpa, true);
>>  	vcpu->arch.gpa_available = true;
>>  	if (likely(ret == RET_MMIO_PF_EMULATE))
>> -- 
>> 2.13.5