DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 9CAF27ACB0
Subject: Re: [PATCH v1 3/4] KVM: MMU: Add 5 level EPT & Shadow page table
 support.
To: Yu Zhang <yu.c.zhang@linux.intel.com>, kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, rkrcmar@redhat.com, tglx@linutronix.de,
        mingo@redhat.com, hpa@zytor.com, xiaoguangrong@tencent.com,
        joro@8bytes.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>
 <22052f76-f8b3-d18c-f21a-4cb367f90993@linux.intel.com>
 <d907d814-d346-15e8-e281-f180f0fce1eb@redhat.com>
 <2ab782d9-f1cc-f9d0-97d0-876a97f28fe2@linux.intel.com>
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <f0ba814b-5516-f2b6-a465-f0f1cb18aec9@redhat.com>
Date: Mon, 14 Aug 2017 17:02:58 +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: <2ab782d9-f1cc-f9d0-97d0-876a97f28fe2@linux.intel.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3134
Lines: 77

On 14/08/2017 16:32, Yu Zhang wrote:
> 
> 
> On 8/14/2017 10:13 PM, Paolo Bonzini wrote:
>> On 14/08/2017 13:37, Yu Zhang wrote:
>>> 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. :-)
>> Yes, you're right.  I think the solution is to define
>> PT64_ROOT_MAX_LEVEL in kvm_host.h.
> 
> Thanks, Paolo. How about we also move the definition of PT64_ROOT_4LEVEL/
> PT32E_ROOT_LEVEL/PT32_ROOT_LEVEL from mmu.h to kvm_host.h? Then we
> can define PT64_ROOT_MAX_LEVEL as PT64_ROOT_4LEVEL instead of 4 in
> kvm_host.h.

No, I think those are best left in mmu.h.  They are only used in mmu
files, except for two occurrences in svm.c.

kvm_host.h would have PT64_ROOT_MAX_LEVEL just because it is slightly
better than "4" or "5".

Paolo

>>>>> @@ -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?
>> That was my idea, but setting both to 4 should be fine too as you
>> suggest below.
>>
>>> 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?
>> Nothing else, I think.
> 
> Thanks. I'll try to keep both values fixed to PT64_ROOT_4LEVEL then. :-)
> For nested EPT5, we can enable it later(should be a quite simple patch,
> but need to
> be verified in our simics environment, which I am not sure if nested
> scenario works).
> 
> B.R.
> Yu