From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH] Enable 32bit dirty log pointers on 64bit host Date: Fri, 23 Oct 2009 11:15:34 +0200 Message-ID: <4AE17436.6020107@siemens.com> References: <1256134109-14292-1-git-send-email-agraf@suse.de> <4AE16C50.1060806@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: kvm-devel , Avi Kivity , kvm-ppc , "arnd@arndb.de" To: Alexander Graf Return-path: Received: from david.siemens.de ([192.35.17.14]:18724 "EHLO david.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751021AbZJWJPp (ORCPT ); Fri, 23 Oct 2009 05:15:45 -0400 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: Alexander Graf wrote: > On 23.10.2009, at 10:41, Jan Kiszka wrote: > >> Alexander Graf wrote: >>> From: Arnd Bergmann >>> >>> With big endian userspace, we can't quite figure out if a pointer >>> is 32 bit (shifted >> 32) or 64 bit when we read a 64 bit pointer. >>> >>> This is what happens with dirty logging. To get the pointer >>> interpreted >>> correctly, we thus need Arnd's patch to implement a compat layer for >>> the ioctl: >>> >>> A better way to do this is to add a separate compat_ioctl() method >>> that >>> converts this for you. >>> >>> From: Arnd Bergmann >>> Signed-off-by: Arnd Bergmann >>> Acked-by: Alexander Graf >>> >>> --- >>> >>> Changes from Arnd's example version: >>> >>> - s/log.log/log/ (Avi) >>> - use sizeof(compat_log) (Avi) >>> - compile fixes >>> --- >>> virt/kvm/kvm_main.c | 49 +++++++++++++++++++++++++++++++++++++++++ >>> +++++++- >>> 1 files changed, 48 insertions(+), 1 deletions(-) >>> >>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >>> index cac69c4..54a272f 100644 >>> --- a/virt/kvm/kvm_main.c >>> +++ b/virt/kvm/kvm_main.c >>> @@ -43,6 +43,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> >>> #include >>> #include >>> @@ -1542,6 +1543,52 @@ out: >>> return r; >>> } >>> >>> +#ifdef CONFIG_COMPAT >>> +struct compat_kvm_dirty_log { >>> + __u32 slot; >>> + __u32 padding1; >>> + union { >>> + compat_uptr_t dirty_bitmap; /* one bit per page */ >>> + __u64 padding2; >>> + }; >>> +}; >>> + >>> +static long kvm_vm_compat_ioctl(struct file *filp, >>> + unsigned int ioctl, unsigned long arg) >>> +{ >>> + struct kvm *kvm = filp->private_data; >>> + int r; >>> + >>> + if (kvm->mm != current->mm) >>> + return -EIO; >>> + switch (ioctl) { >>> + case KVM_GET_DIRTY_LOG: { >>> + struct compat_kvm_dirty_log compat_log; >>> + struct kvm_dirty_log log; >>> + >>> + r = -EFAULT; >>> + if (copy_from_user(&compat_log, (void __user *)arg, >>> + sizeof(compat_log))) >>> + goto out; >>> + log.slot = compat_log.slot; >>> + log.padding1 = compat_log.padding1; >>> + log.padding2 = compat_log.padding2; >>> + log.dirty_bitmap = compat_ptr(compat_log.dirty_bitmap); >>> + >>> + r = kvm_vm_ioctl_get_dirty_log(kvm, &log); >>> + if (r) >>> + goto out; >>> + break; >>> + } >>> + default: >>> + r = kvm_vm_ioctl(filp, ioctl, arg); >>> + } >>> + >>> +out: >>> + return r; >>> +} >>> +#endif >>> + >>> static int kvm_vm_fault(struct vm_area_struct *vma, struct vm_fault >>> *vmf) >>> { >>> struct page *page[1]; >>> @@ -1576,7 +1623,7 @@ static int kvm_vm_mmap(struct file *file, >>> struct vm_area_struct *vma) >>> static struct file_operations kvm_vm_fops = { >>> .release = kvm_vm_release, >>> .unlocked_ioctl = kvm_vm_ioctl, >>> - .compat_ioctl = kvm_vm_ioctl, >>> + .compat_ioctl = kvm_vm_compat_ioctl, >> This fails in the absence of CONFIG_COMPAT. > > > So should it rather be > > #ifdef CONFIG_COMPAT > .compat_ioctl = kvm_vm_compat_ioctl, > #else > .compat_ioctl = kvm_vm_ioctl, > #endif > > or > > #ifdef CONFIG_COMPAT > .compat_ioctl = kvm_vm_compat_ioctl, > #endif > > ? I would say the latter as .compat_ioctl should simply be unused in case of !CONFIG_COMPAT. Jan -- Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux