Re: [PATCH/RFC] iommu/ipmmu-vmsa: Update ->add_device() return value

2016-09-22 Thread Robin Murphy 
On 20/09/16 16:03, Magnus Damm wrote:
> Hi Robin,
> 
> Thanks for your feedback!!
> 
> On Tue, Sep 20, 2016 at 10:18 PM, Robin Murphy  wrote:
>> Hi Magnus,
>>
>> On 20/09/16 13:41, Magnus Damm wrote:
>>> From: Magnus Damm 
>>>
>>> Update the IPMMU driver to return -ENODEV when adding devices
>>> not hooked up a particular IPMMU instance.
>>>
>>> Currently the ->add_device() callback implementation in the IPMMU
>>> driver returns -ENODEV for devices with no "iommus" property,
>>> however the function ipmmu_find_utlbs() may return -EINVAL.
>>
>> If there were no "iommus" property at all, of_parse_phandle_with_args()
>> should return -ENOENT - that probably does want to be caught and passed
>> back as -ENODEV to imply an untranslated device. On the other hand,
>> -EINVAL would stem from the existence of the property, but in a somehow
>> erroneous manner - other than the "args.np != mmu->dev->of_node" check
>> (which could legitimately fail and be safely ignored if there are
>> multiple IOMMUs in the system), any other reason implies a DT error
>> which probably shouldn't be papered over.
> 
> Regarding -ENOENT to -ENODEV, I agree but I believe this case is
> already handled. The ->add_device() callback seems to be using
> of_count_phandle_with_args() to check for the presence of "iommus"
> property before calling ipmmu_find_utlbs(). So for the case with no
> "iommus" property at all it is returned as -ENODEV as you suggest.

Ah, right you are, I missed that there was a separate check earlier.

> The ->add_device() callback has the ret variable initialised to
> -ENODEV by default. In case there is only one IPMMU device on the
> ipmmu_device list and it matches the struct device passed to the
> ipmmu_add_device() function then all is fine. However when there are
> more than one IPMMU device on the ipmmu_device list then
> ipmmu_find_utlbs() may return -EINVAL. Judging by the code this seems
> to happen when the order of the IPMMU devices on the "iommus" property
> does not match the IPMMU order on the ipmmu_device list.
> 
> Hm, I wonder if all this should be replaced with ->xlate() somehow?

Ideally, yes - the core code already has most of this covered, so taking
advantage of it would be good. I think the only slight hiccup is that
the 32-bit DMA code is then going to call attach_dev() with a domain you
probably don't want, before you get your add_device() call. Other than
handling that vs. group-based default domains for 64-bit, though, there
shouldn't be anything else to special-case, I don't think.

I'm finally starting to have a look into converting the arch/arm code
over to use groups and default domains sensibly, but I suspect that's
ultimately going to have some dependency on the probe deferral stuff,
rather than introduce the same bus notifier bodge we currently have on
arm64.

Robin.

> 
> Thanks,
> 
> / magnus
> 

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [GIT PULL] iommu/arm-smmu: Updates for 4.9

2016-09-22 Thread Joerg Roedel 
On Tue, Sep 20, 2016 at 11:25:34AM +0100, Will Deacon wrote:
> Hi Joerg,
> 
> Please pull these significant arm-smmu updates for 4.9. The vast majority
> of the code here is Robin's series to move the drivers over to the
> generic DT bindings, which finally hooks up the DMA API and MSI mapping
> for host machines. This has been stewing for a while and is now at the
> point where I feel it's much better in mainline than languishing outside.
> 
> A consequence of that is that there are some non-trivial changes outside
> of the drivers, but these are all acked by the relevant subsystem
> maintainers.
> 
> Other stuff here includes some non-critical fixes to the queue handling
> on SMMUv3 and a small devm cleanup that missed the boat last time around.
> 
> Cheers,
> 
> Will
> 
> --->8
> 
> The following changes since commit 3eab887a55424fc2c27553b7bfe32330df83f7b8:
> 
>   Linux 4.8-rc4 (2016-08-28 15:04:33 -0700)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git 
> for-joerg/arm-smmu/updates

Pulled, thanks Will.

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 00/07] iommu/ipmmu-vmsa: IPMMU multi-arch update V5

2016-09-22 Thread Joerg Roedel 
On Tue, Sep 20, 2016 at 10:43:55PM +0900, Magnus Damm wrote:
> iommu/ipmmu-vmsa: IPMMU multi-arch update V5
> 
> [PATCH v5 01/07] iommu/ipmmu-vmsa: Remove platform data handling
> [PATCH v5 02/07] iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for 
> context
> [PATCH v5 03/07] iommu/ipmmu-vmsa: Break out utlb parsing code
> [PATCH v5 04/07] iommu/ipmmu-vmsa: Break out domain allocation code
> [PATCH v5 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops
> [PATCH v5 06/07] iommu/ipmmu-vmsa: ARM and ARM64 archdata access
> [PATCH v5 07/07] iommu/ipmmu-vmsa: Drop LPAE Kconfig dependency

So except for patch 5 this looks good to me. Please add

Reviewed-by: Joerg Roedel 

to all patches, except patch 5, when you resend.


Thanks,

Joerg

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

2016-09-22 Thread Joerg Roedel 
On Tue, Sep 20, 2016 at 10:44:46PM +0900, Magnus Damm wrote:
> +#ifdef CONFIG_IOMMU_DMA
> +
> +static struct iommu_domain *ipmmu_domain_alloc_dma(unsigned type)
> +{
> + struct iommu_domain *io_domain;
> +
> + if (type != IOMMU_DOMAIN_DMA)
> + return NULL;
> +
> + io_domain = __ipmmu_domain_alloc(type);
> + if (io_domain)
> + iommu_get_dma_cookie(io_domain);
> +
> + return io_domain;
> +}
> +
> +static void ipmmu_domain_free_dma(struct iommu_domain *io_domain)
> +{
> + iommu_put_dma_cookie(io_domain);
> + ipmmu_domain_free(io_domain);
> +}

> [...]

> +static const struct iommu_ops ipmmu_ops = {
> + .domain_alloc = ipmmu_domain_alloc_dma,
> + .domain_free = ipmmu_domain_free_dma,

Okay, so when CONFIG_IOMMU_DMA is enabled, you only support allocation
of DMA domains, not UNMANAGED domains anymore. Is there a reason for
that?

You can reduce the #ifdef'ed coded by supporting both types of domains
and call into allocation-subfunctions for DMA and UNMANAGED domains. The
#ifdef could then only let the dma-allocation function return NULL.

This would be much more compatible to what other IOMMU drivers do and
will allow VFIO usage in the future.



Joerg

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu