Re: [PATCH v2 1/3] kvm: x86: Add a hook for arch specific dirty logging emulation

[Bandan Das <bsd@redhat.com>]

Hi Kai,

"Huang, Kai" <kai.huang@linux.intel.com> writes:

> On 5/6/2017 7:25 AM, Bandan Das wrote:
>> When KVM updates accessed/dirty bits, this hook can be used
>> to invoke an arch specific function that implements/emulates
>> dirty logging such as PML.
>>
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>>  arch/x86/include/asm/kvm_host.h |  2 ++
>>  arch/x86/kvm/mmu.c              | 15 +++++++++++++++
>>  arch/x86/kvm/mmu.h              |  1 +
>>  arch/x86/kvm/paging_tmpl.h      |  4 ++++
>>  4 files changed, 22 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index f5bddf92..9c761fe 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1020,6 +1020,8 @@ struct kvm_x86_ops {
>>  	void (*enable_log_dirty_pt_masked)(struct kvm *kvm,
>>  					   struct kvm_memory_slot *slot,
>>  					   gfn_t offset, unsigned long mask);
>> +	int (*write_log_dirty)(struct kvm_vcpu *vcpu);
>
> Hi,
>
> Thanks for adding PML to nested support!
>
> IMHO this callback is only used for write L2's dirty gpa to L1's PML
> buffer, so probably it's better to change the name to something like:
> nested_write_log_dirty.

The name was meant more to signify what it does: i.e. write dirty log rather
than where in the hierarchy it's being used :) But I guess, a nested_ prefix
doesn't hurt either.

Bandan

> Thanks,
> -Kai
>
>> +
>>  	/* pmu operations of sub-arch */
>>  	const struct kvm_pmu_ops *pmu_ops;
>>
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index 5586765..5d3376f 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -1498,6 +1498,21 @@ void kvm_arch_mmu_enable_log_dirty_pt_masked(struct kvm *kvm,
>>  		kvm_mmu_write_protect_pt_masked(kvm, slot, gfn_offset, mask);
>>  }
>>
>> +/**
>> + * kvm_arch_write_log_dirty - emulate dirty page logging
>> + * @vcpu: Guest mode vcpu
>> + *
>> + * Emulate arch specific page modification logging for the
>> + * nested hypervisor
>> + */
>> +int kvm_arch_write_log_dirty(struct kvm_vcpu *vcpu)
>> +{
>> +	if (kvm_x86_ops->write_log_dirty)
>> +		return kvm_x86_ops->write_log_dirty(vcpu);
>> +
>> +	return 0;
>> +}
>
> kvm_nested_arch_write_log_dirty?
>
>> +
>>  bool kvm_mmu_slot_gfn_write_protect(struct kvm *kvm,
>>  				    struct kvm_memory_slot *slot, u64 gfn)
>>  {
>> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
>> index d8ccb32..2797580 100644
>> --- a/arch/x86/kvm/mmu.h
>> +++ b/arch/x86/kvm/mmu.h
>> @@ -202,4 +202,5 @@ void kvm_mmu_gfn_disallow_lpage(struct kvm_memory_slot *slot, gfn_t gfn);
>>  void kvm_mmu_gfn_allow_lpage(struct kvm_memory_slot *slot, gfn_t gfn);
>>  bool kvm_mmu_slot_gfn_write_protect(struct kvm *kvm,
>>  				    struct kvm_memory_slot *slot, u64 gfn);
>> +int kvm_arch_write_log_dirty(struct kvm_vcpu *vcpu);
>>  #endif
>> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
>> index 314d207..5624174 100644
>> --- a/arch/x86/kvm/paging_tmpl.h
>> +++ b/arch/x86/kvm/paging_tmpl.h
>> @@ -226,6 +226,10 @@ static int FNAME(update_accessed_dirty_bits)(struct kvm_vcpu *vcpu,
>>  		if (level == walker->level && write_fault &&
>>  				!(pte & PT_GUEST_DIRTY_MASK)) {
>>  			trace_kvm_mmu_set_dirty_bit(table_gfn, index, sizeof(pte));
>> +#if PTTYPE == PTTYPE_EPT
>> +			if (kvm_arch_write_log_dirty(vcpu))
>> +				return -EINVAL;
>> +#endif
>>  			pte |= PT_GUEST_DIRTY_MASK;
>>  		}
>>  		if (pte == orig_pte)
>>