Subject: Re: [PATCH v1 3/4] KVM: MMU: Add 5 level EPT & Shadow page table
 support.
To: Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org
References: <1502544906-1108-1-git-send-email-yu.c.zhang@linux.intel.com>
 <1502544906-1108-4-git-send-email-yu.c.zhang@linux.intel.com>
 <04271af0-e735-022b-fb22-4d39bf32b01b@redhat.com>
Cc: linux-kernel@vger.kernel.org, rkrcmar@redhat.com, tglx@linutronix.de,
        mingo@redhat.com, hpa@zytor.com, xiaoguangrong@tencent.com,
        joro@8bytes.org
From: Yu Zhang <yu.c.zhang@linux.intel.com>
Message-ID: <22052f76-f8b3-d18c-f21a-4cb367f90993@linux.intel.com>
Date: Mon, 14 Aug 2017 19:37:20 +0800
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <04271af0-e735-022b-fb22-4d39bf32b01b@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2594
Lines: 80

Thanks a lot for your comments, Paolo. :-)


On 8/14/2017 3:31 PM, Paolo Bonzini wrote:
> On 12/08/2017 15:35, Yu Zhang wrote:
>>   struct rsvd_bits_validate {
>> -	u64 rsvd_bits_mask[2][4];
>> +	u64 rsvd_bits_mask[2][5];
>>   	u64 bad_mt_xwr;
>>   };
>
> Can you change this 4 to PT64_ROOT_MAX_LEVEL in patch 2?

Well, I had tried, but failed to find a neat approach to do so.
The difficulty I have met is that PT64_ROOT_MAX_LEVEL is defined 
together with
PT64_ROOT_4LEVEL/PT32E_ROOT_LEVEL/PT32_ROOT_LEVEL in mmu.h, yet the
rsvd_bits_validate structure is defined in kvm_host.h, which are 
included in quite
a lot .c files that do not include mmu.h or include the mmu.h after 
kvm_host.h.

I guess that's the reason why the magic number 4 instead of PT64_ROOT_4LEVEL
is used in current definition of rsvd_bits_vadlidate. :-)

>
>> -	if (vcpu->arch.mmu.shadow_root_level == PT64_ROOT_4LEVEL &&
>> -	    (vcpu->arch.mmu.root_level == PT64_ROOT_4LEVEL ||
>> -	     vcpu->arch.mmu.direct_map)) {
>> +	if (vcpu->arch.mmu.root_level >= PT64_ROOT_4LEVEL ||
>> +	    vcpu->arch.mmu.direct_map) {
>>   		hpa_t root = vcpu->arch.mmu.root_hpa;
> You should keep the check on shadow_root_level (changing it to >= of
> course), otherwise you break the case where EPT is disabled, paging is
> disabled (so vcpu->arch.mmu.direct_map is true) and the host kernel is
> 32-bit.  In that case shadow pages use PAE format, and entering this
> branch is incorrect.

Oh, right. Thanks!

>
>> @@ -4444,7 +4457,7 @@ void kvm_init_shadow_ept_mmu(struct kvm_vcpu *vcpu, bool execonly,
>>   
>>   	MMU_WARN_ON(VALID_PAGE(context->root_hpa));
>>   
>> -	context->shadow_root_level = kvm_x86_ops->get_tdp_level();
>> +	context->shadow_root_level = kvm_x86_ops->get_tdp_level(vcpu);
>>   
>>   	context->nx = true;
>>   	context->ept_ad = accessed_dirty;
> Below, there is:
>
>          context->root_level = context->shadow_root_level;
>
> this should be forced to PT64_ROOT_4LEVEL until there is support for
> nested EPT 5-level page tables.

So the context->shadow_root_level could be 5 or 4, and 
context->root_level is always 4?

My understanding is that shadow ept level should be determined by the 
width of ngpa,
and that if L1 guest is not exposed with EPT5 feature, it shall only use 
4 level ept for L2
guest, and the shadow ept does not need a 5 level one. Is this 
understanding correct?
And how about we set both values to PT64_ROOT_4LEVEL for now?

Besides, if we wanna support nested EPT5, what do you think we need to 
do besides exposing
the EPT5 feature to L1 guest?

Thanks
Yu

>
> Thanks,
>
> Paolo
>