[Xen-devel] Re: [PATCH] infiniband/mthca : Fix userland mapping of mthca infiniband cards in Xen dom0

[Vivien Bernet-Rollande <vbr@xxxxxxxxxxx>]

Thanks  for your reply.

> This is kind of unappealing -- is there no way to make
> io_remap_pfn_range do the right thing under Xen?  It seems unfortunate
> to make drivers both set VM_IO and call io_remap_pfn_range.  Or maybe we
> should get rid of io_remap_pfn_range() and just have drivers set VM_IO
> and call remap_pfn_range() all the time?

Aside from IB drivers, the devices that need userland mapping are
mainly graphic cards. There has been quite a few issues with that and
the topic is discussed here :
http://wiki.xensource.com/xenwiki/XenPVOPSDRM

AFAIK, on x86 io_remap_pfn_range() is just #defined as
remap_pfn_range(). remap_pfn_range() sets the VM_IO flag, but does not
refresh the vm_page_prot field (mm/memory.c:1875). I think it could
make sense to patch remap_pfn_range(), but I'm afraid it might break a
lot of other drivers.

 > +    vma->vm_flags |= VM_IO;
 > +    vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
 >      vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);

Under Xen, when the VM_IO flag is set, the vm_get_page_prot() macro
will set the _PAGE_IOMAP bit on the the vm_page_prot.
Xen needs that bit to be set in order to understand that we actually
want the real memory space to be mapped. The use of this macro was
pointed out to me by the Xen team (see last post on
http://xen.1045712.n5.nabble.com/Kernel-Oops-when-reading-kernel-page-tables-debugfs-file-td3279750.html
), and, I assume, is the proper way of handling things.

To sum up :
 - Xen needs the _PAGE_IOMAP flag set on the vm_page_prot for the
device to work.
 - I understand the proper way to do this is to set the VM_IO flag,
and then call vm_get_page_prot()

> There are quite a few drivers under drivers/infiniband/hw that call
> io_remap_pfn_range() -- presumably they all need the analogous fix?

I only have access to Mellanox Infinihost HBAs for testing, but once
we figure out how we want this fixed, I can take a look at the other
drivers.

> Finally as a stylistic thing I would probably prefer to see the
> vm_page_prot manipulation written as
>
>        vma->vm_page_prot = pgprot_noncached(vm_get_page_prot(vma->vm_flags));
>

Makes sense.
While digging, I've also seen things like :

vma->vm_page_prot = pgprot_noncached(vm_get_page_prot(vma->vm_flags | VM_IO));

But it seems a bit dense.

> and if this fix is really the right thing to do, this should probably be
> wrapped up in a helper function; presumably every instance of
>
>        vma->vm_page_prot = pgprot_FOO(vma->vm_page_prot)
>
> under drivers/ would need the same fix for Xen.

The issue is actually only related to mappings of bus memory to
userland. In kernel space, ioremap() is generaly used, which I
understand sets the _PAGE_IOMAP flag.

-- 
Vivien Bernet-Rollande

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel