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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.

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.

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?

> + * 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?

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

> +
> +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?

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.

Ian.


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