From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S965550AbcCJKJh (ORCPT <rfc822;w@1wt.eu>);
	Thu, 10 Mar 2016 05:09:37 -0500
Received: from mx1.redhat.com ([209.132.183.28]:52879 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S933050AbcCJKJb (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Thu, 10 Mar 2016 05:09:31 -0500
Subject: Re: [PATCH 1/2] KVM: MMU: fix
 ept=0/pte.u=0/pte.w=0/CR0.WP=0/CR4.SMEP=1/EFER.NX=0 combo
To: Xiao Guangrong <guangrong.xiao@linux.intel.com>,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org
References: <1457437467-65707-1-git-send-email-pbonzini@redhat.com>
 <1457437467-65707-2-git-send-email-pbonzini@redhat.com>
 <56E12FF0.90202@linux.intel.com>
Cc: stable@vger.kernel.org, Andy Lutomirski <luto@amacapital.net>
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <56E147D7.3040409@redhat.com>
Date: Thu, 10 Mar 2016 11:09:27 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <56E12FF0.90202@linux.intel.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org



On 10/03/2016 09:27, Xiao Guangrong wrote:
>>
> 
>> +    if (!enable_ept) {
>> +        guest_efer |= EFER_NX;
>> +        ignore_bits |= EFER_NX;
> 
> Update ignore_bits is not necessary i think.

More precisely, ignore_bits is only needed if guest EFER.NX=0 and we're
not in this CR0.WP=1/CR4.SMEP=0 situation.  In theory you could have
guest EFER.NX=1 and host EFER.NX=0.

This is what I came up with (plus some comments :)):

	u64 guest_efer = vmx->vcpu.arch.efer;
	u64 ignore_bits = 0;

	if (!enable_ept) {
		if (boot_cpu_has(X86_FEATURE_SMEP))
			guest_efer |= EFER_NX;
		else if (!(guest_efer & EFER_NX))
			ignore_bits |= EFER_NX;
	}

>> -        guest_efer = vmx->vcpu.arch.efer;
>>           if (!(guest_efer & EFER_LMA))
>>               guest_efer &= ~EFER_LME;
>>           if (guest_efer != host_efer)
>>               add_atomic_switch_msr(vmx, MSR_EFER,
>>                             guest_efer, host_efer);
> 
> So, why not set EFER_NX (if !ept) just in this branch to make the fix
> more simpler?

I didn't like having

	guest_efer = vmx->vcpu.arch.efer;
	...
	if (!enable_ept)
		guest_efer |= EFER_NX;
	guest_efer &= ~ignore_bits;
	guest_efer |= host_efer & ignore_bits;
	...
	if (...) {
		guest_efer = vmx->vcpu.arch.efer;
		if (!enable_ept)
			guest_efer |= EFER_NX;
		...
	}

My patch is bigger but the resulting code is smaller and easier to follow:

	guest_efer = vmx->vcpu.arch.efer;
	if (!enable_ept)
		guest_efer |= EFER_NX;
	...
	if (...) {
		...
	} else {
		guest_efer &= ~ignore_bits;
		guest_efer |= host_efer & ignore_bits;
	}

Paolo