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

Bandan Das
2017-05-10T15:53:09Z

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)
>>