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 <bzImage> -initrd <initrd> \
-drive media=disk,if=xen,file=<diskimage> \
-net nic,model=xen,macaddr=<addr> \
-serial <yourconsolehere>
> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home