Re: [PATCH v6 4/7] dma-reserved-iommu: alloc/free_reserved_iova_domain
From: Eric Auger
Date: Thu Apr 07 2016 - 05:34:53 EST
Hi Alex,
On 04/07/2016 01:00 AM, Alex Williamson wrote:
> On Mon, 4 Apr 2016 08:06:59 +0000
> Eric Auger <eric.auger@xxxxxxxxxx> wrote:
>
>> Introduce alloc/free_reserved_iova_domain in the IOMMU API.
>> alloc_reserved_iova_domain initializes an iova domain at a given
>> iova base address and with a given size. This iova domain will
>> be used to allocate iova within that window. Those IOVAs will be reserved
>> for special purpose, typically MSI frame binding. Allocation function
>> within the reserved iova domain will be introduced in subsequent patches.
>>
>> Those functions are implemented and exposed if CONFIG_IOMMU_DMA_RESERVED
>> is seta.
>>
>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>>
>> ---
>>
>> v5 -> v6:
>> - use spin lock instead of mutex
>>
>> v3 -> v4:
>> - formerly in "iommu/arm-smmu: implement alloc/free_reserved_iova_domain" &
>> "iommu: add alloc/free_reserved_iova_domain"
>>
>> v2 -> v3:
>> - remove iommu_alloc_reserved_iova_domain & iommu_free_reserved_iova_domain
>> static implementation in case CONFIG_IOMMU_API is not set
>>
>> v1 -> v2:
>> - moved from vfio API to IOMMU API
>> ---
>> drivers/iommu/Kconfig | 8 ++++
>> drivers/iommu/Makefile | 1 +
>> drivers/iommu/dma-reserved-iommu.c | 75 ++++++++++++++++++++++++++++++++++++++
>> include/linux/dma-reserved-iommu.h | 45 +++++++++++++++++++++++
>> 4 files changed, 129 insertions(+)
>> create mode 100644 drivers/iommu/dma-reserved-iommu.c
>> create mode 100644 include/linux/dma-reserved-iommu.h
>>
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index dd1dc39..a5a1530 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -74,6 +74,12 @@ config IOMMU_DMA
>> select IOMMU_IOVA
>> select NEED_SG_DMA_LENGTH
>>
>> +# IOMMU reserved IOVA mapping (MSI doorbell)
>> +config IOMMU_DMA_RESERVED
>> + bool
>> + select IOMMU_API
>> + select IOMMU_IOVA
>> +
>> config FSL_PAMU
>> bool "Freescale IOMMU support"
>> depends on PPC32
>> @@ -307,6 +313,7 @@ config SPAPR_TCE_IOMMU
>> config ARM_SMMU
>> bool "ARM Ltd. System MMU (SMMU) Support"
>> depends on (ARM64 || ARM) && MMU
>> + select IOMMU_DMA_RESERVED
>> select IOMMU_API
>> select IOMMU_IO_PGTABLE_LPAE
>> select ARM_DMA_USE_IOMMU if ARM
>> @@ -320,6 +327,7 @@ config ARM_SMMU
>> config ARM_SMMU_V3
>> bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support"
>> depends on ARM64 && PCI
>> + select IOMMU_DMA_RESERVED
>> select IOMMU_API
>> select IOMMU_IO_PGTABLE_LPAE
>> select GENERIC_MSI_IRQ_DOMAIN
>> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
>> index c6edb31..6c9ae99 100644
>> --- a/drivers/iommu/Makefile
>> +++ b/drivers/iommu/Makefile
>> @@ -2,6 +2,7 @@ obj-$(CONFIG_IOMMU_API) += iommu.o
>> obj-$(CONFIG_IOMMU_API) += iommu-traces.o
>> obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
>> obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
>> +obj-$(CONFIG_IOMMU_DMA_RESERVED) += dma-reserved-iommu.o
>> obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
>> obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
>> obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
>> diff --git a/drivers/iommu/dma-reserved-iommu.c b/drivers/iommu/dma-reserved-iommu.c
>> new file mode 100644
>> index 0000000..a461482
>> --- /dev/null
>> +++ b/drivers/iommu/dma-reserved-iommu.c
>> @@ -0,0 +1,75 @@
>> +/*
>> + * Reserved IOVA Management
>> + *
>> + * Copyright (c) 2015 Linaro Ltd.
>> + * www.linaro.org
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include <linux/iommu.h>
>> +#include <linux/iova.h>
>> +
>> +int iommu_alloc_reserved_iova_domain(struct iommu_domain *domain,
>> + dma_addr_t iova, size_t size,
>> + unsigned long order)
>> +{
>> + unsigned long granule, mask;
>> + struct iova_domain *iovad;
>> + unsigned long flags;
>> + int ret = 0;
>> +
>> + granule = 1UL << order;
>> + mask = granule - 1;
>> + if (iova & mask || (!size) || (size & mask))
>> + return -EINVAL;
>> +
>> + iovad = kzalloc(sizeof(struct iova_domain), GFP_KERNEL);
>> + if (!iovad)
>> + return -ENOMEM;
>> +
>> + spin_lock_irqsave(&domain->reserved_lock, flags);
>> +
>> + if (domain->reserved_iova_cookie) {
>> + ret = -EEXIST;
>> + goto free_unlock;
>> + }
>> +
>> + init_iova_domain(iovad, granule,
>> + iova >> order, (iova + size - 1) >> order);
>> + domain->reserved_iova_cookie = iovad;
>> + goto unlock;
>> +
>> +free_unlock:
>> + kfree(iovad);
>> +unlock:
>> + spin_unlock_irqrestore(&domain->reserved_lock, flags);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_alloc_reserved_iova_domain);
>> +
>> +void iommu_free_reserved_iova_domain(struct iommu_domain *domain)
>> +{
>> + struct iova_domain *iovad =
>> + (struct iova_domain *)domain->reserved_iova_cookie;
>> + unsigned long flags;
>> +
>> + if (!iovad)
>> + return;
>> +
>> + spin_lock_irqsave(&domain->reserved_lock, flags);
>> +
>> + put_iova_domain(iovad);
>> + kfree(iovad);
>> +
>> + spin_unlock_irqrestore(&domain->reserved_lock, flags);
>> +}
>
> The locking here looks wrong, reserved_iova_cookie needs to be read
and
> cleared (currently missing) within the lock, the put and free can
> probably happen outside the lock. Likewise the init_iova_domain() in
> the alloc path probably doesn't need to iccur under spinlock, but
> testing and setting reserved_iova_cookie does.
I agree with our statements. Need to rewrite this down.
Seems like a mutex
> would do for locking here, but maybe the rb-tree manipulation has
> reasons for using a spinlock.
I moved to a spinlock after Jean-Philippe spotted a call sequence
disallowing sleeps:
https://lkml.org/lkml/2016/3/10/216
... which I did not address correctly in this respin, focusing on this
wrong spinlock re-writing.
Thanks
Eric
Thanks,
>
> Alex
>
>> +EXPORT_SYMBOL_GPL(iommu_free_reserved_iova_domain);
>> diff --git a/include/linux/dma-reserved-iommu.h b/include/linux/dma-reserved-iommu.h
>> new file mode 100644
>> index 0000000..5bf863b
>> --- /dev/null
>> +++ b/include/linux/dma-reserved-iommu.h
>> @@ -0,0 +1,45 @@
>> +/*
>> + * Copyright (c) 2015 Linaro Ltd.
>> + * www.linaro.org
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +#ifndef __DMA_RESERVED_IOMMU_H
>> +#define __DMA_RESERVED_IOMMU_H
>> +
>> +#ifdef __KERNEL__
>> +#include <asm/errno.h>
>> +
>> +#ifdef CONFIG_IOMMU_DMA_RESERVED
>> +#include <linux/iommu.h>
>> +
>> +/**
>> + * iommu_alloc_reserved_iova_domain: allocate the reserved iova domain
>> + *
>> + * @domain: iommu domain handle
>> + * @iova: base iova address
>> + * @size: iova window size
>> + * @order: page order
>> + */
>> +int iommu_alloc_reserved_iova_domain(struct iommu_domain *domain,
>> + dma_addr_t iova, size_t size,
>> + unsigned long order);
>> +
>> +/**
>> + * iommu_free_reserved_iova_domain: free the reserved iova domain
>> + *
>> + * @domain: iommu domain handle
>> + */
>> +void iommu_free_reserved_iova_domain(struct iommu_domain *domain);
>> +
>> +#endif /* CONFIG_IOMMU_DMA_RESERVED */
>> +#endif /* __KERNEL__ */
>> +#endif /* __DMA_RESERVED_IOMMU_H */
>