Re: [Xen-devel] [RFC for-4.5 10/12] xen/passthrough: Introduce IOMMU ARM architure

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2014-02-07 at 17:43 +0000, Julien Grall wrote:
> This patch contains the architecture to use IOMMU on ARM. There is no

s/IOMMU/IOMMUs/

> IOMMU drivers on this patch.
> 
> The code will run through the device tree and will initialize every IOMMUs.

s/IOMMUs/IOMMU/

> It's possible to have multiple IOMMUs on the same platform, but they should

s/should/must/ I think for now?

> be handled with the same drivers.

s/drivers/driver/

> For now, there is no support for using multiple iommu drivers at runtime.

But this is in principal possible in the future, right? (I shudder to
think what the designers of an SoC with multiple different SMMUs on it
would have to be smoking...)

You should mention somewhere that on ARM these are called SMMUs not
IOMMUs.

> Each new IOMMU drivers should contain:
> 
> static const char * const myiommu_dt_compat[] __initcontst =

"__initconst".

> {
>     /* list of device compatible with the drivers. Will be matched with
>      * the "compatible" property on the device tree
>      */
>     NULL,
> };
> 
> DT_DEVICE_START(myiommu, "MY IOMMU", DEVICE_IOMMU)
>         .compatible = myiommu_compatible,
>         .init = myiommu_init,
> DT_DEVICE_END

This is the same as for any other driver, right?

> @@ -568,6 +571,10 @@ fail:
>  
>  void arch_domain_destroy(struct domain *d)
>  {
> +    /* IOMMU page table is shared with P2M, always call
> +     * iommu_domain_destroy() before p2m_teardown().

It would be worth adding some commentary on the design we are using
(decisions such as whether to share PTs with the stage 2 MMU) in the
commit message as well.

I suppose this requirement puts constraints on the SMMU hardware we can
support. I think it is fine to live with that until someone shows up
with an SMMU with pagetables that are incompatible with the stage 2
paging ones.

> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 1f6d713..5a687d1 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -725,6 +725,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>      local_irq_enable();
>      local_abort_enable();
>  
> +    iommu_setup(); /* setup iommu if available */

Comment is a bit redundant ;-)

> diff --git a/xen/drivers/passthrough/arm/iommu.c 
> b/xen/drivers/passthrough/arm/iommu.c
> new file mode 100644
> index 0000000..7cf36cd
> --- /dev/null
> +++ b/xen/drivers/passthrough/arm/iommu.c
> @@ -0,0 +1,65 @@
> +/*
> + * xen/drivers/passthrough/arm/iommu.c
> + *
> + * Generic IOMMU framework via the device tree
> + *
> + * Julien Grall <julien.grall@xxxxxxxxxx>
> + * Copyright (c) 2013 Linaro Limited.

It's not 2013 any more...

> diff --git a/xen/include/asm-arm/hvm/iommu.h b/xen/include/asm-arm/hvm/iommu.h
> new file mode 100644
> index 0000000..461c8cf
> --- /dev/null
> +++ b/xen/include/asm-arm/hvm/iommu.h
> @@ -0,0 +1,10 @@
> +#ifndef __ASM_ARM_HVM_IOMMU_H_
> +#define __ASM_ARM_HVM_IOMMU_H_
> +
> +struct arch_hvm_iommu
> +{
> +    /* Private information for the IOMMU drivers */
> +    void *priv;
> +};
> +
> +#endif /* __ASM_ARM_HVM_IOMMU_H_ */

Emacs magic block please.

That was a surprisingly small and uncontroversial patch. Well done.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel