Re: [PATCH 02/10] nEPT: Add EPT tables support to paging_tmpl.h

[Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>]

On 08/03/2012 05:25 AM, Nadav Har'El wrote:
> On Thu, Aug 02, 2012, Xiao Guangrong wrote about "Re: [PATCH 02/10] nEPT: Add EPT tables support to paging_tmpl.h":
>>> +	#ifdef CONFIG_X86_64
>>> +	#define PT_MAX_FULL_LEVELS 4
>>> +	#define CMPXCHG cmpxchg
>>> +	#else
>>> +	#define CMPXCHG cmpxchg64
>>> +	#define PT_MAX_FULL_LEVELS 2
>>> +	#endif
>>
>> Missing the case of FULL_LEVELS == 3? Oh, you mentioned it
>> as PAE case in the PATCH 0.
> 
> I understood this differently (and it would not be surprising if
> wrongly...): With nested EPT, we only deal with two *EPT* tables -
> the shadowed page table and shadow page table are both EPT.
> And EPT tables cannot have three levels - even if PAE is used. Or at least,
> that's what I thought...
> 
>> Note A/D bits are supported on new intel cpus, this function should be reworked
>> for nept. I know you did not export this feather to guest, but we can reduce
>> the difference between nept and other mmu models if A/D are supported.
> 
> I'm not sure what you meant: If the access/dirty bits are supported in
> newer cpus, do you think we *should* support them also in the processor
> L1 processor, or are you saying that it would be easier to support them
> because this is what the shadow page table code normally does anyway,
> so *not* supporting them will take effort?

I mean "it would be easier to support them
 because this is what the shadow page table code normally does anyway,
 so *not* supporting them will take effort" :)

Then, we can drop "ifndef PTTYPT_EPT"...

Actuality, we can redefine some bits (like PRSENT, WRTIABLE, DRITY...) to
let the paging_tmpl code work for all models.

> 
>>> +#if PTTYPE != PTTYPE_EPT
>>>  static int FNAME(walk_addr_nested)(struct guest_walker *walker,
>>>  				   struct kvm_vcpu *vcpu, gva_t addr,
>>>  				   u32 access)
>>> @@ -335,6 +395,7 @@ static int FNAME(walk_addr_nested)(struc
>>>  	return FNAME(walk_addr_generic)(walker, vcpu, &vcpu->arch.nested_mmu,
>>>  					addr, access);
>>>  }
>>> +#endif
>>>
>>
>> Hmm, you do not need the special walking functions?
> 
> Since these functions are static, the compiler warns me on every
> function that is never used, so I had to #if them out...
> 
> 

IIUC, you did not implement the functions (like walk_addr_nested) to translate
L2's VA to L2's PA, yes? (it is needed for emulation.)