Re: Re: pv_ops & gntdev?

[Gerd Hoffmann <kraxel@redhat.com>]

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