On Wed, 11 Oct 2017 13:05:00 +1100
Alexey Kardashevskiy <aik@xxxxxxxxx> wrote:
> On 11/10/17 08:55, Alex Williamson wrote:
> > On Mon, 9 Oct 2017 13:50:00 +1100
> > Alexey Kardashevskiy <aik@xxxxxxxxx> wrote:
> >
> >> At the moment the protection in VFIO MMIO mappings is forced to
> >> _PAGE_NON_IDEMPOTENT which means that write combining is not really
> >> available to the userspace even for prefetchable 64bit MMIO BARs.
> >>
> >> This replaces pgprot_noncached() with a platform specific
> >> phys_mem_access_prot() when available and depending on the platform
> >> the vm_page_prot may be set to _PAGE_TOLERANT allowing to exploit
> >> the write combining feature.
> >>
> >> The guest drivers still have to use _wc versions of
> >> the ioremap/pci_ioremap API to get write combininig working.
> >>
> >> Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxxxx>
> >> ---
> >>
> >> This should allow DPDK and radix guests (x86, POWERPC, etc) to
> >> do write combining.
> >>
> >> POWERPC hash guests should not be affected by this change, it should
> >> work even without this.
> >> ---
> >> drivers/vfio/pci/vfio_pci.c | 7 ++++++-
> >> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> >> index f041b1a6cf66..014192b42724 100644
> >> --- a/drivers/vfio/pci/vfio_pci.c
> >> +++ b/drivers/vfio/pci/vfio_pci.c
> >> @@ -1156,8 +1156,13 @@ static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
> >> }
> >>
> >> vma->vm_private_data = vdev;
> >> - vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> >> vma->vm_pgoff = (pci_resource_start(pdev, index) >> PAGE_SHIFT) + pgoff;
> >> +#ifdef __HAVE_PHYS_MEM_ACCESS_PROT
> >> + vma->vm_page_prot = phys_mem_access_prot(NULL, vma->vm_pgoff,
> >> + req_len, vma->vm_page_prot);
> >> +#else
> >> + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> >> +#endif
> >>
> >> return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
> >> req_len, vma->vm_page_prot);
> >
> > Are you testing __HAVE_PHYS_MEM_ACCESS_PROT because the version of
> > phys_mem_access_prot() defined in drivers/char/mem.c can dereference
> > @file and we're hoping that platforms we care about won't both define
> > __HAVE_PHYS_MEM_ACCESS_PROT and look at @file?
>
> No.
>
> That version in mem.c is static and not exported at all and I do not
> understand why it got this name. Every other instance of
> phys_mem_access_prot is accompanied by
>
> #define __HAVE_PHYS_MEM_ACCESS_PROT
I did miss that mem.c was static there, but I think the point is still
valid, file being NULL doesn't seem to be a universally expected option.
> But 3 instances (ia64, x86, mips) are not exported (arm, arm64, ppc are)
> and v2 of this will come with 3 more single line patches, if we decide to
> proceed.
>
> The only version which actually looks at @file is in
> arch/mips/loongson64/common/mem.c and I do not know what to do about it
> (can it do VFIO at all?), I could pass a file there but no actual code
> would use it anyway.
The question is not whether this particular platform could use vfio,
but instead is whether vfio is using the function correctly. That, I
really don't know.
But also...
arch/arm64/mm/mmu.c:
pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
unsigned long size, pgprot_t vma_prot)
{
if (!pfn_valid(pfn))
return pgprot_noncached(vma_prot);
else if (file->f_flags & O_SYNC)
return pgprot_writecombine(vma_prot);
return vma_prot;
}
Why do we get to ignore dereferencing file on an arch that we
definitely care about? Thanks,
Alex
