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

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

On 02/19/2014 12:49 PM, Ian Campbell wrote:
> 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?

Right.

>> 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...)

In theory yes. I will wait that someone want to support a such platfomr
on Xen before implement it :).

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

I think SMMU is the name used for ARM IOMMU. Samsung and TI doesn't use
the term SMMU.

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

Oops. Will fix it.

>> {
>>     /* 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?

Yes, the only change is we need to specify DEVICE_IOMMU in DT_DEVICE_START.

> 
>> @@ -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 will do.

> 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.

Adding support for a such hardware should not need too much code.

>> 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 ;-)

Copied for x86/setup.c. I will remove it.

> 
>> 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@linaro.org>
>> + * Copyright (c) 2013 Linaro Limited.
> 
> It's not 2013 any more...

I have started the implementation in 2013 and forgot to update it.

> 
>> 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.

I will do.

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

The IOMMU framework in Xen was well designed.

cheers,

-- 
Julien Grall