lkml.org 
[lkml]   [2015]   [Apr]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH kernel v8 07/31] vfio: powerpc/spapr: Moving pinning/unpinning to helpers
On 04/15/2015 05:10 PM, David Gibson wrote:
> On Fri, Apr 10, 2015 at 04:30:49PM +1000, Alexey Kardashevskiy wrote:
>> This is a pretty mechanical patch to make next patches simpler.
>>
>> New tce_iommu_unuse_page() helper does put_page() now but it might skip
>> that after the memory registering patch applied.
>>
>> As we are here, this removes unnecessary checks for a value returned
>> by pfn_to_page() as it cannot possibly return NULL.
>>
>> This moves tce_iommu_disable() later to let tce_iommu_clear() know if
>> the container has been enabled because if it has not been, then
>> put_page() must not be called on TCEs from the TCE table. This situation
>> is not yet possible but it will after KVM acceleration patchset is
>> applied.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v6:
>> * tce_get_hva() returns hva via a pointer
>> ---
>> drivers/vfio/vfio_iommu_spapr_tce.c | 68 +++++++++++++++++++++++++++----------
>> 1 file changed, 50 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>> index c137bb3..ec5ee83 100644
>> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>> @@ -196,7 +196,6 @@ static void tce_iommu_release(void *iommu_data)
>> struct iommu_table *tbl = container->tbl;
>>
>> WARN_ON(tbl && !tbl->it_group);
>> - tce_iommu_disable(container);
>>
>> if (tbl) {
>> tce_iommu_clear(container, tbl, tbl->it_offset, tbl->it_size);
>> @@ -204,63 +203,96 @@ static void tce_iommu_release(void *iommu_data)
>> if (tbl->it_group)
>> tce_iommu_detach_group(iommu_data, tbl->it_group);
>> }
>> +
>> + tce_iommu_disable(container);
>> +
>> mutex_destroy(&container->lock);
>>
>> kfree(container);
>> }
>>
>> +static void tce_iommu_unuse_page(struct tce_container *container,
>> + unsigned long oldtce)
>> +{
>> + struct page *page;
>> +
>> + if (!(oldtce & (TCE_PCI_READ | TCE_PCI_WRITE)))
>> + return;
>> +
>> + /*
>> + * VFIO cannot map/unmap when a container is not enabled so
>> + * we would not need this check but KVM could map/unmap and if
>> + * this happened, we must not put pages as KVM does not get them as
>> + * it expects memory pre-registation to do this part.
>> + */
>> + if (!container->enabled)
>> + return;
>
> This worries me a bit. How can whether the contained is enabled now
> safely tell you whether get_page() at some earlier point in time?


This is a leftover, I'll remove it as after the "iommu v2" patch there will
be tce_iommu_unuse_page_v2().


>> +
>> + page = pfn_to_page(__pa(oldtce) >> PAGE_SHIFT);
>> +
>> + if (oldtce & TCE_PCI_WRITE)
>> + SetPageDirty(page);
>> +
>> + put_page(page);
>> +}
>> +
>> static int tce_iommu_clear(struct tce_container *container,
>> struct iommu_table *tbl,
>> unsigned long entry, unsigned long pages)
>> {
>> unsigned long oldtce;
>> - struct page *page;
>>
>> for ( ; pages; --pages, ++entry) {
>> oldtce = iommu_clear_tce(tbl, entry);
>> if (!oldtce)
>> continue;
>>
>> - page = pfn_to_page(oldtce >> PAGE_SHIFT);
>> - WARN_ON(!page);
>> - if (page) {
>> - if (oldtce & TCE_PCI_WRITE)
>> - SetPageDirty(page);
>> - put_page(page);
>> - }
>> + tce_iommu_unuse_page(container, (unsigned long) __va(oldtce));
>> }
>>
>> return 0;
>> }
>>
>> +static int tce_get_hva(unsigned long tce, unsigned long *hva)
>> +{
>> + struct page *page = NULL;
>> + enum dma_data_direction direction = iommu_tce_direction(tce);
>> +
>> + if (get_user_pages_fast(tce & PAGE_MASK, 1,
>> + direction != DMA_TO_DEVICE, &page) != 1)
>> + return -EFAULT;
>> +
>> + *hva = (unsigned long) page_address(page);
>> +
>> + return 0;
>> +}
>
> I'd prefer to see this called tce_iommu_use_page() for symmetry.


If I rename this one, then what would I call tce_get_hva_cached() from
"fio: powerpc/spapr: Register memory and define IOMMU v2"?


>
>> +
>> static long tce_iommu_build(struct tce_container *container,
>> struct iommu_table *tbl,
>> unsigned long entry, unsigned long tce, unsigned long pages)
>> {
>> long i, ret = 0;
>> - struct page *page = NULL;
>> + struct page *page;
>> unsigned long hva;
>> enum dma_data_direction direction = iommu_tce_direction(tce);
>>
>> for (i = 0; i < pages; ++i) {
>> - ret = get_user_pages_fast(tce & PAGE_MASK, 1,
>> - direction != DMA_TO_DEVICE, &page);
>> - if (unlikely(ret != 1)) {
>> - ret = -EFAULT;
>> + ret = tce_get_hva(tce, &hva);
>> + if (ret)
>> break;
>> - }
>>
>> + page = pfn_to_page(__pa(hva) >> PAGE_SHIFT);
>> if (!tce_page_is_contained(page, tbl->it_page_shift)) {
>> ret = -EPERM;
>> break;
>> }
>>
>> - hva = (unsigned long) page_address(page) +
>> - (tce & IOMMU_PAGE_MASK(tbl) & ~PAGE_MASK);
>> + /* Preserve offset within IOMMU page */
>> + hva |= tce & IOMMU_PAGE_MASK(tbl) & ~PAGE_MASK;
>>
>> ret = iommu_tce_build(tbl, entry + i, hva, direction);
>> if (ret) {
>> - put_page(page);
>> + tce_iommu_unuse_page(container, hva);
>> pr_err("iommu_tce: %s failed ioba=%lx, tce=%lx, ret=%ld\n",
>> __func__, entry << tbl->it_page_shift,
>> tce, ret);
>


--
Alexey


\
 
 \ /
  Last update: 2015-04-15 14:41    [W:0.434 / U:5.328 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site