WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] Re: pv_ops & gntdev?

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

<Prev in Thread] Current Thread [Next in Thread>