From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [RFC for-4.5 12/12] drivers/passthrough: arm: Add support for SMMU drivers Date: Wed, 19 Feb 2014 17:21:30 +0000 Message-ID: <5304E81A.3050703@linaro.org> References: <1391794991-5919-1-git-send-email-julien.grall@linaro.org> <1391794991-5919-13-git-send-email-julien.grall@linaro.org> <1392819327.29739.85.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WGApz-0004IK-74 for xen-devel@lists.xenproject.org; Wed, 19 Feb 2014 17:21:35 +0000 Received: by mail-ee0-f45.google.com with SMTP id b15so370530eek.18 for ; Wed, 19 Feb 2014 09:21:33 -0800 (PST) In-Reply-To: <1392819327.29739.85.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: patches@linaro.org, tim@xen.org, stefano.stabellini@citrix.com, Jan Beulich , xen-devel@lists.xenproject.org, Xiantao Zhang List-Id: xen-devel@lists.xenproject.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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#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