| Message ID | 1424890381-4225-8-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Ian, On 02/03/15 13:10, Ian Campbell wrote: >> 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) I will use the former. > >> + */ >> + 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? I think it's useful to have a comment explaining why we are retrieving the SMMU. >> + 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; > } I though about this solution but it doesn't say if for some reason we miss to find the SMMU. > > 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. I would like to keep the BUG(). Because this would be a mistake to not find the last SMMU added. I will move to an helper function. Regards,
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. + * 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. + */ + 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. */ + spin_lock(&arm_smmu_devices_lock); + list_for_each_entry(smmu, &arm_smmu_devices, list) { + if (smmu->dev == &dev->dev) + goto found; + } + + BUG(); + +found: + platform_features &= smmu->features; + + spin_unlock(&arm_smmu_devices_lock); + return 0; }
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 supporting coherent table walk. If not, we may need to enable this feature per-domain. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- 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(+)