From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gerd Hoffmann Subject: Re: Re: pv_ops & gntdev? Date: Wed, 04 Mar 2009 09:18:53 +0100 Message-ID: <49AE396D.2090503@redhat.com> References: <49A44030.2070709@redhat.com> <49A4640E.1000807@goop.org> <49A470DD.2000008@redhat.com> <49A517F6.30005@redhat.com> <49A58506.2020407@goop.org> <49A58FFF.3050604@redhat.com> <49A5A4BD.7080207@goop.org> <49A5B5DD.60309@redhat.com> <49A5B9AA.7010709@goop.org> <49A5BB7E.8030503@redhat.com> <49A5BC9D.5010801@goop.org> <49A5C208.2040509@redhat.com> <49A5C5FB.6080000@goop.org> <49A5C957.7060205@redhat.com> <49A5CD57.2010300@goop.org> <49AC635F.5010906@redhat.com> <49ADB656.7080404@goop.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <49ADB656.7080404@goop.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jeremy Fitzhardinge Cc: Xen Development Mailing List List-Id: xen-devel@lists.xenproject.org Jeremy Fitzhardinge wrote: > Do you have a test program? Qemu with xen support, including userspace backends for disk & nic which use gntdev. Grab it here: http://git.et.redhat.com/?p=qemu-kraxel.git;a=shortlog;h=refs/heads/xenbits.v0 usage: qemu -M xenpv -xen-create -uuid $(uuidgen) \ -kernel -initrd \ -drive media=disk,if=xen,file= \ -net nic,model=xen,macaddr= \ -serial > Comments below. There's quite a bit of whitespace damage and formatting > strangeness (NULL == tests, for example). I've checked in a couple of > followup patches to address some of the comments below, but not all. Ok, I'll grab a fresh checkout and go over it. >> + up_write(&priv->sem); >> + kfree(add); >> > Leaks ->grants, ->map_ops, ->unmap_ops? Yep. >> + u64 mpte; >> > pteval_t or pte_t Well, gnttab_set_map_op() wants u64 there, thats why I did it that way. >> + BUG_ON(pgnr >= map->count); >> + mpte = (u64)pfn_to_mfn(page_to_pfn(token)) << PAGE_SHIFT; >> + mpte |= (unsigned long)pte & ~PAGE_MASK; >> > (!) You're casting pte_t * to unsigned long, masking off the lower bits > then oring them into your pte. That doesn't look like it will mean very > much... I guess this explains your not-unmapping bug. > > This should probably be using mfn_pte() anyway: > > mfn = pfn_to_mfn(page_to_pfn(token)); > pgprot = __pgprot(pte->pte & PTE_FLAGS_MASK); > mpte = mfn_pte(mfn, pgprot); Looks better indeed. Unclean stuff carried over from 2.6.18 ... >> + BUG_ON(HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, >> + map->map_ops, map->count)); >> > WARN_ON() is better here, because we can probably limp on if it fails. Agree. >> +static long gntdev_ioctl(struct file *flip, >> + unsigned int cmd, unsigned long arg) >> +{ >> + struct gntdev_priv *priv = flip->private_data; >> + void __user *ptr = (void __user *)arg; >> + >> + switch (cmd) { >> + case IOCTL_GNTDEV_MAP_GRANT_REF: >> > ioctl switches should be split into separate functions. All of them or just the larger ones? >> + vma->vm_flags |= VM_RESERVED; >> + vma->vm_flags |= VM_DONTCOPY; >> + vma->vm_flags |= VM_DONTEXPAND; >> > VM_IO? Dunno, maybe. 2.6.18 used VM_RESERVED, thats why I sticked to that. According to mm.h there isn't a big difference between VM_IO and VM_RESERVED anyway ... >> + priv->mm = get_task_mm(current); >> >Is this to cope with the /dev/gnttab fd being inherited by/passed to >some other process, even if the mappings don't follow it? Well, sort of. Main intention is that I want to keep track of the mm I registered the mmu notifier for. But, yes, it is also used to cope with fd's inherited / passed on. It will not work (mmap -> EINVAL), but at least the driver shouldn't trip over it. cheers, Gerd