Re: [RFC for-4.5 12/12] drivers/passthrough: arm: Add support for SMMU drivers

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

On 02/19/2014 02:15 PM, Ian Campbell wrote:
> On Fri, 2014-02-07 at 17:43 +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.
> 
> Aha, here comes all the hard stuff ;-)
> 
> Could you try and briefly enumerate the areas which you had to change
> please.

The main changes 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 structure

> Some comments on e.g. which translation context type we are using and
> how we are configuring things etc might also be helpful here in
> understanding what is going on.

Honestly, the configuration part was mostly copied from Linux. Some bits
are still fuzzy for me. I will try to improve the commit message.

> Also could you give details of the test setup you used, was it just
> booting dom0 on Midway with these patches? Was the DTB complete etc?

I had to modify the device tree by applying this following patch:
http://www.spinics.net/lists/arm-kernel/msg301163.html

I've tried to boot dom0 and a guest with LVM (a patch is coming to
remove swiotlb when the device is protected by an IOMMU).

> 
>> + * This driver currently supports:
>> + *  - SMMUv1 and v2 implementations (didn't try v2 SMMU)
> 
> I guess this is Will's original comment, I thought SMMU-400, which
> you've tried, was v2?

No it's SMMU v2. As I understand:
	- SMMU-400 => SMMU v1
        - SMMU-500 => SMMU v2

The words in parenthesis was added by me because I don't have a test box
with SMMU v2.

> 
>> + *  - Stream-matching and stream-indexing
>> + *  - v7/v8 long-descriptor format
>> + *  - Non-secure access to the SMMU
>> + *  - 4k pages, p2m shared with the processor
>> + *  - Up to 40-bit addressing
>> + *  - Context fault reporting
>> + */
>> +
>> +#include <xen/config.h>
>> +#include <xen/delay.h>
>> +#include <xen/errno.h>
>> +#include <xen/irq.h>
>> +#include <xen/lib.h>
>> +#include <xen/list.h>
>> +#include <xen/mm.h>
>> +#include <xen/rbtree.h>
>> +#include <xen/sched.h>
>> +#include <asm/atomic.h>
>> +#include <asm/device.h>
>> +#include <asm/io.h>
>> +#include <asm/platform.h>
>> +
>> +#define SZ_4K                               (1 << 12)
>> +#define SZ_64K                              (1 << 16)
>> +
>> +/* Driver options */
>> +#define SMMU_OPT_SECURE_CONFIG_ACCESS   (1 << 0)
> 
> Is this just retained to reduce the deviation from the Linux driver?
> It's no use to us I think. (I suppose that goes for a bunch of other
> stuff, eg.. the PGSZ_4K stuff, which I will avoid commenting on
> further).

SZ_4K and SZ_64K is used later in the code. The
SMMU_OPT_SECURE_CONFIG_ACCESS is used for midway as the SMMU is broken.

> 
>> +
>> +void arm_smmu_iommu_dom0_init(struct domain *d)
>> +{
>> +    struct arm_smmu_device *smmu;
>> +    struct rb_node *node;
>> +
>> +    printk(XENLOG_DEBUG "arm-smmu: Initialize dom0\n");
>> +
>> +    list_for_each_entry( smmu, &arm_smmu_devices, list )
>> +    {
>> +        for ( node = rb_first(&smmu->masters); node; node = rb_next(node) )
>> +        {
>> +            struct arm_smmu_master *master;
>> +
>> +            master = container_of(node, struct arm_smmu_master, node);
>> +
>> +            if ( dt_device_used_by(master->dt_node) == DOMID_XEN ||
>> +                 platform_device_is_blacklisted(master->dt_node) )
>> +                continue;
>> +
>> +            arm_smmu_attach_dev(d, master->dt_node);
> 
> Should this not be driven from the same loop as the MMIO mapping setup
> in dom0 build? Otherwise isn't there a danger that they won't
> correspond?

On my second version of the patch series I moved out this code in
map_device.

> 
> A "master" here is a "bus master" i.e. a device, right?



>> +    /* Even if the device can't be initialized, we don't want to give to
>> +     * dom0 the smmu device
> 
> "give the smmu device to dom0"
> 
> 
> I obviously haven't reviewed all this code in detail, but I have skimmed
> it and nothing leapt out.

Thanks for the review!

Cheers,

-- 
Julien Grall