On 11/10/17 13:42, Alex Williamson wrote:
> 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.
Who does? :) Let's cc: him.
>
>
> 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,
Oopsie. This is because I overlooked it. Others do not use it. So I do need
a file. But in the current scheme where all BARs share one fd - it won't
work - I simply cannot allow WC on non-prefetchable BARs :-/
--
Alexey
