| Message ID | 1422643768-23614-14-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Stefano, On 06/02/2015 22:06, Stefano Stabellini wrote: > On Fri, 30 Jan 2015, 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 >> 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 v3: >> - Patch added >> --- >> xen/drivers/passthrough/arm/smmu.c | 25 +++++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c >> index 373eee8..f4c7e49 100644 >> --- a/xen/drivers/passthrough/arm/smmu.c >> +++ b/xen/drivers/passthrough/arm/smmu.c >> @@ -577,6 +577,13 @@ struct arm_smmu_domain { >> spinlock_t lock; >> }; >> >> +/* >> + * Xen: Platform features. It indicates the list of features support by all the >> + * SMMUs. >> + * Actually we only care about coherent table walk. >> + */ >> +static u32 platform_features = ARM_SMMU_FEAT_COHERENT_WALK; >> + >> /* Xen: Dummy iommu_domain */ >> struct iommu_domain >> { >> @@ -2810,6 +2817,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; >> } > > As long as you are sure that arm_smmu_iommu_domain_init is called after > all the iommus have been initialized by arm_smmu_dt_init, then this > patch is correct. We don't support SMMU hotplug on Xen :). All the SMMUs are initialize via iommu_setup before DOM0 is created. Regards,
On 10/02/2015 09:36, Julien Grall wrote: > Hi Stefano, > > On 06/02/2015 22:06, Stefano Stabellini wrote: >> On Fri, 30 Jan 2015, 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 >>> 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 v3: >>> - Patch added >>> --- >>> xen/drivers/passthrough/arm/smmu.c | 25 +++++++++++++++++++++++++ >>> 1 file changed, 25 insertions(+) >>> >>> diff --git a/xen/drivers/passthrough/arm/smmu.c >>> b/xen/drivers/passthrough/arm/smmu.c >>> index 373eee8..f4c7e49 100644 >>> --- a/xen/drivers/passthrough/arm/smmu.c >>> +++ b/xen/drivers/passthrough/arm/smmu.c >>> @@ -577,6 +577,13 @@ struct arm_smmu_domain { >>> spinlock_t lock; >>> }; >>> >>> +/* >>> + * Xen: Platform features. It indicates the list of features support >>> by all the >>> + * SMMUs. >>> + * Actually we only care about coherent table walk. >>> + */ >>> +static u32 platform_features = ARM_SMMU_FEAT_COHERENT_WALK; >>> + >>> /* Xen: Dummy iommu_domain */ >>> struct iommu_domain >>> { >>> @@ -2810,6 +2817,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; >>> } >> >> As long as you are sure that arm_smmu_iommu_domain_init is called after >> all the iommus have been initialized by arm_smmu_dt_init, then this >> patch is correct. > > We don't support SMMU hotplug on Xen :). All the SMMUs are initialize > via iommu_setup before DOM0 is created. Though, I could update the commit message to explain it.
On 20/02/15 13:34, Ian Campbell wrote: > On Fri, 2015-01-30 at 18:49 +0000, Julien Grall wrote: >> @@ -2896,6 +2911,16 @@ static __init int arm_smmu_dt_init(struct dt_device_node *dev, >> if ( !rc ) >> iommu_set_ops(&arm_smmu_iommu_ops); >> >> + /* >> + * The last added SMMU is the first element of arm_smmu_devices. >> + * It's not necessary to take the lock because only the boot CPU is >> + * initialized the SMMU devices. > > Why is only the last added SMMU of interest? Do we not need to take the > union and/or intersection of them all? It's already the case. The function arm_smmu_dt_init is called on every SMMU. So the last added SMMU is the one we are currently added. > Perhaps the code which calls iommu_set_feature should gain an else which > calls iommu_clear_feature, and between them they can ensure that > platform_features is correctly updated? iommu_{set,clear}_feature is a generic code and per-domain. The main issue is if we add device protected by SMMU which doesn't have coherent walk, we have to browse the page table and clean the cache. So we have to know at boot time that one of the SMMU doesn't have coherent walk. Regards,
On 20/02/15 14:13, Ian Campbell wrote: > On Fri, 2015-02-20 at 14:07 +0000, Julien Grall wrote: >> On 20/02/15 13:34, Ian Campbell wrote: >>> On Fri, 2015-01-30 at 18:49 +0000, Julien Grall wrote: >>>> @@ -2896,6 +2911,16 @@ static __init int arm_smmu_dt_init(struct dt_device_node *dev, >>>> if ( !rc ) >>>> iommu_set_ops(&arm_smmu_iommu_ops); >>>> >>>> + /* >>>> + * The last added SMMU is the first element of arm_smmu_devices. >>>> + * It's not necessary to take the lock because only the boot CPU is >>>> + * initialized the SMMU devices. >>> >>> Why is only the last added SMMU of interest? Do we not need to take the >>> union and/or intersection of them all? >> >> It's already the case. The function arm_smmu_dt_init is called on every >> SMMU. So the last added SMMU is the one we are currently added. > > Why do we not just have it in our hand and have to go scrobbling round > in a list then? [..] > Rather than making assumptions about the list ordering and if we can't > just get hold of the smmu pointer directly from arm_smmu_dt_init then > I'd prefer an explicit walk of the list at some appropriate point after > everything has been registered up. Because that would require to modify more heavily arm_smmu_dt_init. Given that we control the way we add the SMMU in the list, the explicit walk sounds pointless. Regards,
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index 373eee8..f4c7e49 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -577,6 +577,13 @@ struct arm_smmu_domain { spinlock_t lock; }; +/* + * Xen: Platform features. It indicates the list of features support by all the + * SMMUs. + * Actually we only care about coherent table walk. + */ +static u32 platform_features = ARM_SMMU_FEAT_COHERENT_WALK; + /* Xen: Dummy iommu_domain */ struct iommu_domain { @@ -2810,6 +2817,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; } @@ -2885,6 +2899,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 @@ -2896,6 +2911,16 @@ static __init int arm_smmu_dt_init(struct dt_device_node *dev, if ( !rc ) iommu_set_ops(&arm_smmu_iommu_ops); + /* + * The last added SMMU is the first element of arm_smmu_devices. + * It's not necessary to take the lock because only the boot CPU is + * initialized the SMMU devices. + */ + smmu = list_entry(arm_smmu_devices.next, typeof(*smmu), list); + ASSERT(smmu != NULL); + + platform_features &= smmu->features; + return rc; }
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 v3: - Patch added --- xen/drivers/passthrough/arm/smmu.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)