Re: [PATCH v3 13/13] drivers/passthrough: arm: Add support for SMMU drivers

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

Hi Ian,

On 03/18/2014 04:54 PM, Ian Campbell wrote:
> On Tue, 2014-03-11 at 15:49 +0000, Julien Grall wrote:
>> This patch add support for ARM architected SMMU driver. It's based on the
>> linux drivers (drivers/iommu/arm-smmu) commit 89ac23cd.
>>
>> The major differences with the Linux driver are:
>>     - Fault by default if the SMMU is enabled to translate an
>>     address (Linux is bypassing the SMMU)
>>     - Using P2M page table instead of creating new one
>>     - Dropped stage-1 support
>>     - Dropped chained SMMUs support for now
>>     - Reworking device assignment and the different structures
>>
>> Xen is programming each IOMMU by:
>>     - Using stage-2 mode translation
>>     - Sharing the page table with the processor
>>     - Injecting a fault if the device has made a wrong translation
>>
>> Signed-off-by: Julien Grall<julien.grall@linaro.org>
>> Cc: Xiantao Zhang <xiantao.zhang@intel.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
> 
> I don't think it is sensible to try and review this code in great detail
> given you no doubt did so as you imported it. I looked through the bits
> which seemed like they were new Xen code rather than imported Linux
> code. It mostly looks good, one question and when grammar nit.
> 
>> +static __init void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>> +{
>> +[...]
>> +    /* Don't upgrade barriers */
>> +    reg &= ~(SMMU_sCR0_BSU_MASK << SMMU_sCR0_BSU_SHIFT);
> 
> No? Is that safe when a vcpu migrates around pCPUs?

>From the SMMU doc 9.6.3, this field is only used when client devices are
not mapped to a translation context banks.

By default, the policy in Xen is to deny every transaction that doesn't
have a valid mapping. So we are safe.

I can update the comment if you want, or even better removing this code.

>> +
>> +static int __init smmu_init(struct dt_device_node *dev,
>> +                            const void *data)
>> +{
>> +    struct arm_smmu_device *smmu;
>> +    int res;
>> +    u64 addr, size;
>> +    unsigned int num_irqs, i;
>> +    struct dt_phandle_args masterspec;
>> +    struct rb_node *node;
>> +
>> +    /* Even if the device can't be initialized, we don't want to give to
>> +     * dom0 the smmu device
> 
> "we don't want to give the smmu device to dom0"

Will do the modification in the next version.

Regards,

-- 
Julien Grall