Re: [RESEND PATCH v7 3/4] arm: dirty log write protect management support

Christoffer Dall
2014-06-10T09:22:45Z

On Mon, Jun 09, 2014 at 06:47:12PM -0700, Mario Smarduch wrote:
> On 06/08/2014 05:05 AM, Christoffer Dall wrote:
> > On Fri, Jun 06, 2014 at 10:33:41AM -0700, Mario Smarduch wrote:
> >> kvm_vm_ioctl_get_dirty_log() is generic used by x86, ARM. x86 recent patch
> >> changed this function, this patch picks up those changes, re-tested everything
> >> works. Applies cleanly with other patches.
> >>
> >> This patch adds support for keeping track of VM dirty pages. As dirty page log
> >> is retrieved, the pages that have been written are write protected again for
> >> next write and log read.
> >>
> >> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
> >> ---
> >> arch/arm/include/asm/kvm_host.h | 3 ++
> >> arch/arm/kvm/arm.c | 5 ---
> >> arch/arm/kvm/mmu.c | 79 +++++++++++++++++++++++++++++++++++
> >> arch/x86/kvm/x86.c | 86 ---------------------------------------
> >> virt/kvm/kvm_main.c | 86 +++++++++++++++++++++++++++++++++++++++
> >> 5 files changed, 168 insertions(+), 91 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> >> index 59565f5..b760f9c 100644
> >> --- a/arch/arm/include/asm/kvm_host.h
> >> +++ b/arch/arm/include/asm/kvm_host.h
> >> @@ -232,5 +232,8 @@ u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, u64 regid);
> >> int kvm_arm_timer_set_reg(struct kvm_vcpu *, u64 regid, u64 value);
> >>
> >> void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
> >> +void kvm_mmu_write_protect_pt_masked(struct kvm *kvm,
> >> + struct kvm_memory_slot *slot,
> >> + gfn_t gfn_offset, unsigned long mask);
> >
> > Do all other architectures implement this function? arm64?
>
> Besides arm, x86 but the function is not generic.
> >

you're now calling this from generic code, so all architecture must
implement it, and the prototype should proably be in
include/linux/kvm_host.h, not in the arch-specific headers.

> >>
> >> #endif /* __ARM_KVM_HOST_H__ */
> >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> >> index dfd63ac..f06fb21 100644
> >> --- a/arch/arm/kvm/arm.c
> >> +++ b/arch/arm/kvm/arm.c
> >> @@ -780,11 +780,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> >> }
> >> }
> >>
> >> -int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
> >> -{
> >> - return -EINVAL;
> >> -}
> >> -
> >
> > What about the other architectures implementing this function?
>
> Six architectures define this function. With this patch this
> function is generic in kvm_main.c used by x86.

But you're not defining it as a weak symbol (and I don't suspect that
you should unless other archs do this in a *very* different way), so you
need to either remove it from the other archs, make it a weak symbol (I
hope this is not the case) or do something else.

-Christoffer