Re: [PATCH v2 7/9] x86/kvm/nVMX: introduce source data cache for kvm_init_shadow_ept_mmu()

[Vitaly Kuznetsov]

Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> writes:

> Sean Christopherson <sean.j.christopherson@xxxxxxxxx> writes:
>
>> On Tue, Sep 25, 2018 at 07:58:42PM +0200, Vitaly Kuznetsov wrote:
...
>>>  
>>> -static union kvm_mmu_page_role
>>> -kvm_calc_shadow_ept_root_page_role(struct kvm_vcpu *vcpu, bool accessed_dirty)
>>> +static union kvm_mmu_role
>>> +kvm_calc_shadow_ept_root_page_role(struct kvm_vcpu *vcpu, bool accessed_dirty,
>>> +				   bool execonly)
>>>  {
>>> -	union kvm_mmu_page_role role = vcpu->arch.mmu->mmu_role.base;
>>> +	union kvm_mmu_role role = kvm_calc_mmu_role_common(vcpu);
>>
>> kvm_calc_mmu_role_common() doesn't preserve the current mmu_role.base
>> and kvm_calc_mmu_role_common() doesn't capture all base fields.  Won't
>> @role will be incorrect for base fields that aren't set below, e.g.
>> cr4_pae, smep_andnot_wp, smap_andnot_wp, etc...
>
> Oh, I see what you mean. Actually, PATCH8 of this series adds some of
> this stuff but smep_andnot_wp and smap_andnot_wp are still not set. I
> think I'll enhance kvm_calc_mmu_role_common() and move some stuff from
> PATCH8 to this one.
> (The fact that @role is currently not fully re-initialized here is very
> unobvious so I would definitely prefer to explicitly initialize
> everything over inheriting something from previously initialized role).

On the other hand if we want to perform full re-initialization we'll
have to distinguish between shadow and TDP here and this isn't what we
want. I'm about to change my mind as it seems that inheriting base role
here is not the worst idea after all...

-- 
Vitaly