Re: [RFC PATCH kernel] vfio-pci: Allow write combining

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 11 Oct 2017 13:05:00 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 11/10/17 08:55, Alex Williamson wrote:
> > On Mon,  9 Oct 2017 13:50:00 +1100
> > Alexey Kardashevskiy <aik@ozlabs.ru> 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@ozlabs.ru>
> >> ---
> >>
> >> 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