From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v4 7/8] xen/iommu: smmu: Advertise when the SMMU support coherent table walk Date: Mon, 2 Mar 2015 13:10:09 +0000 Message-ID: <1425301809.21151.9.camel@citrix.com> References: <1424890381-4225-1-git-send-email-julien.grall@linaro.org> <1424890381-4225-8-git-send-email-julien.grall@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YSQ6v-00018n-QJ for xen-devel@lists.xenproject.org; Mon, 02 Mar 2015 13:10:13 +0000 In-Reply-To: <1424890381-4225-8-git-send-email-julien.grall@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: xen-devel@lists.xenproject.org, manish.jaggi@caviumnetworks.com, stefano.stabellini@citrix.com, tim@xen.org List-Id: xen-devel@lists.xenproject.org On Wed, 2015-02-25 at 18:53 +0000, Julien Grall wrote: > When SMMU doesn't support coherent table walk, Xen may need to clean > updated PT (see commit 4c5f4cb "xen/arm: p2m: Clean cache PT when the > IOMMU doesn't support coherent walk"). > > If one SMMU of the platform doesn't support coherent table walk, the > feature is disabled for the whole platform. This is because device is > assigned to a domain after the page table are populated. > > This could impact performance on domain which doesn't use device > passthrough. But, as the spec strongly recommends the support of this > feature for maintstream platform, I expect server will always have SMMUs "mainstream". > supporting coherent table walk. If not, we may need to enable this feature > per-domain. > > Signed-off-by: Julien Grall > > --- > I've just noticed that the support on the previous driver (i.e in > Xen 4.5) may incorrectly expose this feature when all the SMMUs is > not supporting coherent table walk. > > I'm not sure, if I should send a patch for it. > > Also I didn't squash this patch into "xen/iommu: smmu: Add Xen specific > code to be able to use the driver" to help for review and to catch > possible error in this patch. > > Changes in v4: > - Browse the list to retrieve the last SMMU added. > - Take the spinlock > > Changes in v3: > - Patch added > --- > xen/drivers/passthrough/arm/smmu.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c > index d01a26a..b478463 100644 > --- a/xen/drivers/passthrough/arm/smmu.c > +++ b/xen/drivers/passthrough/arm/smmu.c > @@ -2531,6 +2531,13 @@ MODULE_LICENSE("GPL v2"); > /* Xen only supports stage-2 translation, so force the value to 2. */ > static int force_stage = 2; > > +/* > + * Platform features. It indicates the list of features supported by all the > + * SMMUs. s/the// > + * Actually we only care about coherent table walk. > + */ > +static u32 platform_features = ARM_SMMU_FEAT_COHERENT_WALK; > + > static void arm_smmu_iotlb_flush_all(struct domain *d) > { > struct arm_smmu_xen_domain *smmu_domain = domain_hvm_iommu(d)->arch.priv; > @@ -2668,6 +2675,13 @@ static int arm_smmu_iommu_domain_init(struct domain *d) > > domain_hvm_iommu(d)->arch.priv = xen_domain; > > + /* > + * The feature coherent walk can be enabled only when all SMMUs > + * support it. s/The feature c/C/ or "The coherent walk feature..." (I prefer the former since it is briefer) > + */ > + if (platform_features & ARM_SMMU_FEAT_COHERENT_WALK) > + iommu_set_feature(d, IOMMU_FEAT_COHERENT_WALK); > + > return 0; > } > > @@ -2742,6 +2756,7 @@ static __init int arm_smmu_dt_init(struct dt_device_node *dev, > const void *data) > { > int rc; > + struct arm_smmu_device *smmu; > > /* > * Even if the device can't be initialized, we don't want to > @@ -2755,6 +2770,20 @@ static __init int arm_smmu_dt_init(struct dt_device_node *dev, > > iommu_set_ops(&arm_smmu_iommu_ops); > > + /* Find the last SMMU added and retrieve its features. */ This comment no longer applies, I think? > + spin_lock(&arm_smmu_devices_lock); > + list_for_each_entry(smmu, &arm_smmu_devices, list) { > + if (smmu->dev == &dev->dev) > + goto found; Please try and avoid goto. In this case I think { platform_features &= smmu->features; break; } within the if would be fine, combined with dropping the following BUG(). Alternatively you might prefer to provide a helper function to lookup an smmu by &dev->dev. > + } > + > + BUG(); > + > +found: > + platform_features &= smmu->features; > + > + spin_unlock(&arm_smmu_devices_lock); > + > return 0; > } >