On Mon, Feb 24, 2020 at 12:05:38PM -0500, Peter Xu wrote:
> On Mon, Feb 24, 2020 at 11:25:58AM +0800, Jay Zhou wrote:
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 3be25ec..0deb8c3 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -7201,7 +7201,8 @@ static void vmx_sched_in(struct kvm_vcpu *vcpu, int cpu)
> > static void vmx_slot_enable_log_dirty(struct kvm *kvm,
> > struct kvm_memory_slot *slot)
> > {
> > - kvm_mmu_slot_leaf_clear_dirty(kvm, slot);
> > + if (!kvm_manual_dirty_log_init_set(kvm))
> > + kvm_mmu_slot_leaf_clear_dirty(kvm, slot);
> > kvm_mmu_slot_largepage_remove_write_access(kvm, slot);
> > }
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index fb5d64e..f816940 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -9956,7 +9956,7 @@ static void kvm_mmu_slot_apply_flags(struct kvm *kvm,
> > {
> > /* Still write protect RO slot */
> > if (new->flags & KVM_MEM_READONLY) {
> > - kvm_mmu_slot_remove_write_access(kvm, new);
> > + kvm_mmu_slot_remove_write_access(kvm, new, PT_PAGE_TABLE_LEVEL);
> > return;
> > }
> >
> > @@ -9993,8 +9993,20 @@ static void kvm_mmu_slot_apply_flags(struct kvm *kvm,
> > if (new->flags & KVM_MEM_LOG_DIRTY_PAGES) {
> > if (kvm_x86_ops->slot_enable_log_dirty)
> > kvm_x86_ops->slot_enable_log_dirty(kvm, new);
> > - else
> > - kvm_mmu_slot_remove_write_access(kvm, new);
> > + else {
Braces need to be added to the "if" part as well.
> > + int level = kvm_manual_dirty_log_init_set(kvm) ?
> > + PT_DIRECTORY_LEVEL : PT_PAGE_TABLE_LEVEL;
> > +
> > + /*
> > + * If we're with initial-all-set, we don't need
> > + * to write protect any small page because
> > + * they're reported as dirty already. However
> > + * we still need to write-protect huge pages
> > + * so that the page split can happen lazily on
> > + * the first write to the huge page.
> > + */
> > + kvm_mmu_slot_remove_write_access(kvm, new, level);
> > + }
> > } else {
> > if (kvm_x86_ops->slot_disable_log_dirty)
> > kvm_x86_ops->slot_disable_log_dirty(kvm, new);
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index e89eb67..80ada94 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -360,6 +360,13 @@ static inline unsigned long *kvm_second_dirty_bitmap(struct kvm_memory_slot *mem
> > return memslot->dirty_bitmap + len / sizeof(*memslot->dirty_bitmap);
> > }
> >
> > +#define KVM_DIRTY_LOG_MANUAL_PROTECT2 (1 << 0)
> > +#define KVM_DIRTY_LOG_INITIALLY_SET (1 << 1)
> > +#define KVM_DIRTY_LOG_MANUAL_CAPS (KVM_DIRTY_LOG_MANUAL_PROTECT2 | \
> > + KVM_DIRTY_LOG_INITIALLY_SET)
> > +
> > +bool kvm_manual_dirty_log_init_set(struct kvm *kvm);
For me, INITIALLY_SET is awkward and confusing, e.g. IMO it's not at all
obvious that kvm_manual_dirty_log_init_set() is a simple accessor.
Would something like KVM_DIRTY_LOG_START_DIRTY still be accurate?
> > +
> > struct kvm_s390_adapter_int {
> > u64 ind_addr;
> > u64 summary_addr;
> > @@ -493,7 +500,7 @@ struct kvm {
> > #endif
> > long tlbs_dirty;
> > struct list_head devices;
> > - bool manual_dirty_log_protect;
> > + u64 manual_dirty_log_protect;
> > struct dentry *debugfs_dentry;
> > struct kvm_stat_data **debugfs_stat_data;
> > struct srcu_struct srcu;
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 70f03ce..0ffb804 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -858,11 +858,17 @@ static int kvm_vm_release(struct inode *inode, struct file *filp)
> > return 0;
> > }
> >
> > +bool kvm_manual_dirty_log_init_set(struct kvm *kvm)
> > +{
> > + return kvm->manual_dirty_log_protect & KVM_DIRTY_LOG_INITIALLY_SET;
> > +}
>
> Nit: this can be put into kvm_host.h as inlined.
