| Message ID | 1398172475-27873-19-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, 2014-04-22 at 14:14 +0100, Julien Grall wrote: > Some IOMMU doesn't suppport coherent PT walk. When the p2m is shared with > the CPU, Xen has to make sure the PT changes have reached the memory. > > Introduce new IOMMU callback that will retrieve the IOMMU feature for a > specified domain. > > On ARM, the platform can contain multiple IOMMUs. Each of them may not > have the same set of feature. The domain parameter will be used to get the > set of features for IOMMUs used by this domain. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > --- > Changes in v4: > - Patch added > --- > xen/arch/arm/p2m.c | 24 ++++++++++++++++++------ > xen/drivers/passthrough/iommu.c | 11 +++++++++++ > xen/include/xen/iommu.h | 5 +++++ > 3 files changed, 34 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 21219de..996d2bd 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -274,6 +274,18 @@ enum p2m_operation { > CACHEFLUSH, > }; > > +static void unmap_coherent_domain_page(struct domain *d, const void *va) > +{ > + /* Some IOMMU doesn't support coherent PT walk. When the p2m is "don't support" > + * shared with the CPU, Xen has to make sure that the PT changes have > + * reached the memory > + */ > + if ( need_iommu(d) && !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK) ) > + clean_xen_dcache_va_range(va, PAGE_SIZE); This is a fairly large hammer when you might only have touched a few bytes, if any, in the page. Wouldn't it be better to do this is write_pte, or just after the calls to write_pte? iommu_has_feature has a lot of callbacks -- worth calling once in apply_p2m_changes? Ian
On 04/28/2014 03:09 PM, Ian Campbell wrote: >> xen/arch/arm/p2m.c | 24 ++++++++++++++++++------ >> xen/drivers/passthrough/iommu.c | 11 +++++++++++ >> xen/include/xen/iommu.h | 5 +++++ >> 3 files changed, 34 insertions(+), 6 deletions(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index 21219de..996d2bd 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -274,6 +274,18 @@ enum p2m_operation { >> CACHEFLUSH, >> }; >> >> +static void unmap_coherent_domain_page(struct domain *d, const void *va) >> +{ >> + /* Some IOMMU doesn't support coherent PT walk. When the p2m is > > "don't support" > >> + * shared with the CPU, Xen has to make sure that the PT changes have >> + * reached the memory >> + */ >> + if ( need_iommu(d) && !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK) ) >> + clean_xen_dcache_va_range(va, PAGE_SIZE); > > This is a fairly large hammer when you might only have touched a few > bytes, if any, in the page. > > Wouldn't it be better to do this is write_pte, or just after the calls > to write_pte? I didn't think that the cache flush on a page was divided in small chunks. > iommu_has_feature has a lot of callbacks -- worth calling once in > apply_p2m_changes? I will create a write_pte_coherent helpers which will take in parameter a boolean to know if we need to flush the cache or not. Regards,
I forgot to cc Jan and Xiantao on this patch. On 04/22/2014 02:14 PM, Julien Grall wrote: > Some IOMMU doesn't suppport coherent PT walk. When the p2m is shared with > the CPU, Xen has to make sure the PT changes have reached the memory. > > Introduce new IOMMU callback that will retrieve the IOMMU feature for a > specified domain. > > On ARM, the platform can contain multiple IOMMUs. Each of them may not > have the same set of feature. The domain parameter will be used to get the > set of features for IOMMUs used by this domain. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > --- > Changes in v4: > - Patch added > --- > xen/arch/arm/p2m.c | 24 ++++++++++++++++++------ > xen/drivers/passthrough/iommu.c | 11 +++++++++++ > xen/include/xen/iommu.h | 5 +++++ > 3 files changed, 34 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 21219de..996d2bd 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -274,6 +274,18 @@ enum p2m_operation { > CACHEFLUSH, > }; > > +static void unmap_coherent_domain_page(struct domain *d, const void *va) > +{ > + /* Some IOMMU doesn't support coherent PT walk. When the p2m is > + * shared with the CPU, Xen has to make sure that the PT changes have > + * reached the memory > + */ > + if ( need_iommu(d) && !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK) ) > + clean_xen_dcache_va_range(va, PAGE_SIZE); > + > + unmap_domain_page(va); > +} > + > static int apply_p2m_changes(struct domain *d, > enum p2m_operation op, > paddr_t start_gpaddr, > @@ -301,7 +313,7 @@ static int apply_p2m_changes(struct domain *d, > { > if ( cur_first_page != p2m_first_level_index(addr) ) > { > - if ( first ) unmap_domain_page(first); > + if ( first ) unmap_coherent_domain_page(d, first); > first = p2m_map_first(p2m, addr); > if ( !first ) > { > @@ -331,7 +343,7 @@ static int apply_p2m_changes(struct domain *d, > > if ( cur_first_offset != first_table_offset(addr) ) > { > - if (second) unmap_domain_page(second); > + if (second) unmap_coherent_domain_page(d, second); > second = map_domain_page(first[first_table_offset(addr)].p2m.base); > cur_first_offset = first_table_offset(addr); > } > @@ -357,7 +369,7 @@ static int apply_p2m_changes(struct domain *d, > if ( cur_second_offset != second_table_offset(addr) ) > { > /* map third level */ > - if (third) unmap_domain_page(third); > + if (third) unmap_coherent_domain_page(d, third); > third = map_domain_page(second[second_table_offset(addr)].p2m.base); > cur_second_offset = second_table_offset(addr); > } > @@ -480,9 +492,9 @@ static int apply_p2m_changes(struct domain *d, > rc = 0; > > out: > - if (third) unmap_domain_page(third); > - if (second) unmap_domain_page(second); > - if (first) unmap_domain_page(first); > + if (third) unmap_coherent_domain_page(d, third); > + if (second) unmap_coherent_domain_page(d, second); > + if (first) unmap_coherent_domain_page(d, first); > > spin_unlock(&p2m->lock); > > diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c > index f93dc79..f24fb46 100644 > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -344,6 +344,17 @@ void iommu_crash_shutdown(void) > iommu_enabled = iommu_intremap = 0; > } > > +bool_t iommu_has_feature(struct domain *d, uint32_t feature) > +{ > + const struct iommu_ops *ops = domain_hvm_iommu(d)->platform_ops; > + uint32_t features = 0; > + > + if ( iommu_enabled && ops && ops->features ) > + features = ops->features(d); > + > + return !!(features & feature); > +} > + > static void iommu_dump_p2m_table(unsigned char key) > { > struct domain *d; > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h > index e119379..9ad909f 100644 > --- a/xen/include/xen/iommu.h > +++ b/xen/include/xen/iommu.h > @@ -67,6 +67,10 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn, > unsigned int flags); > int iommu_unmap_page(struct domain *d, unsigned long gfn); > > +#define IOMMU_FEAT_COHERENT_WALK (1U<<0) > +bool_t iommu_has_feature(struct domain *d, uint32_t feature); > + > + > #ifdef HAS_PCI > void pt_pci_init(void); > > @@ -139,6 +143,7 @@ struct iommu_ops { > void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count); > void (*iotlb_flush_all)(struct domain *d); > void (*dump_p2m_table)(struct domain *d); > + uint32_t (*features)(struct domain *d); > }; > > void iommu_suspend(void); >
>>> On 22.04.14 at 15:14, <julien.grall@linaro.org> wrote: > +bool_t iommu_has_feature(struct domain *d, uint32_t feature) > +{ > + const struct iommu_ops *ops = domain_hvm_iommu(d)->platform_ops; > + uint32_t features = 0; > + > + if ( iommu_enabled && ops && ops->features ) > + features = ops->features(d); > + > + return !!(features & feature); > +} That's slightly strange a feature check: Passing in a bit mask allows the caller to set more than one bit, with obvious ambiguity in what this would mean. I'd suggest making this a bit position (with individual bits defined via enumeration), at once hiding from the generic caller whether eventually there might be more than 32 features defined. Jan
Hi Jan, On 04/29/2014 08:40 AM, Jan Beulich wrote: >>>> On 22.04.14 at 15:14, <julien.grall@linaro.org> wrote: >> +bool_t iommu_has_feature(struct domain *d, uint32_t feature) >> +{ >> + const struct iommu_ops *ops = domain_hvm_iommu(d)->platform_ops; >> + uint32_t features = 0; >> + >> + if ( iommu_enabled && ops && ops->features ) >> + features = ops->features(d); >> + >> + return !!(features & feature); >> +} > > That's slightly strange a feature check: Passing in a bit mask allows > the caller to set more than one bit, with obvious ambiguity in what > this would mean. I'd suggest making this a bit position (with individual > bits defined via enumeration), at once hiding from the generic caller > whether eventually there might be more than 32 features defined. I will create a separate patch to add iommu_has_feature. It will be more clearer. Regards,
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 21219de..996d2bd 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -274,6 +274,18 @@ enum p2m_operation { CACHEFLUSH, }; +static void unmap_coherent_domain_page(struct domain *d, const void *va) +{ + /* Some IOMMU doesn't support coherent PT walk. When the p2m is + * shared with the CPU, Xen has to make sure that the PT changes have + * reached the memory + */ + if ( need_iommu(d) && !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK) ) + clean_xen_dcache_va_range(va, PAGE_SIZE); + + unmap_domain_page(va); +} + static int apply_p2m_changes(struct domain *d, enum p2m_operation op, paddr_t start_gpaddr, @@ -301,7 +313,7 @@ static int apply_p2m_changes(struct domain *d, { if ( cur_first_page != p2m_first_level_index(addr) ) { - if ( first ) unmap_domain_page(first); + if ( first ) unmap_coherent_domain_page(d, first); first = p2m_map_first(p2m, addr); if ( !first ) { @@ -331,7 +343,7 @@ static int apply_p2m_changes(struct domain *d, if ( cur_first_offset != first_table_offset(addr) ) { - if (second) unmap_domain_page(second); + if (second) unmap_coherent_domain_page(d, second); second = map_domain_page(first[first_table_offset(addr)].p2m.base); cur_first_offset = first_table_offset(addr); } @@ -357,7 +369,7 @@ static int apply_p2m_changes(struct domain *d, if ( cur_second_offset != second_table_offset(addr) ) { /* map third level */ - if (third) unmap_domain_page(third); + if (third) unmap_coherent_domain_page(d, third); third = map_domain_page(second[second_table_offset(addr)].p2m.base); cur_second_offset = second_table_offset(addr); } @@ -480,9 +492,9 @@ static int apply_p2m_changes(struct domain *d, rc = 0; out: - if (third) unmap_domain_page(third); - if (second) unmap_domain_page(second); - if (first) unmap_domain_page(first); + if (third) unmap_coherent_domain_page(d, third); + if (second) unmap_coherent_domain_page(d, second); + if (first) unmap_coherent_domain_page(d, first); spin_unlock(&p2m->lock); diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index f93dc79..f24fb46 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -344,6 +344,17 @@ void iommu_crash_shutdown(void) iommu_enabled = iommu_intremap = 0; } +bool_t iommu_has_feature(struct domain *d, uint32_t feature) +{ + const struct iommu_ops *ops = domain_hvm_iommu(d)->platform_ops; + uint32_t features = 0; + + if ( iommu_enabled && ops && ops->features ) + features = ops->features(d); + + return !!(features & feature); +} + static void iommu_dump_p2m_table(unsigned char key) { struct domain *d; diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index e119379..9ad909f 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -67,6 +67,10 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn, unsigned int flags); int iommu_unmap_page(struct domain *d, unsigned long gfn); +#define IOMMU_FEAT_COHERENT_WALK (1U<<0) +bool_t iommu_has_feature(struct domain *d, uint32_t feature); + + #ifdef HAS_PCI void pt_pci_init(void); @@ -139,6 +143,7 @@ struct iommu_ops { void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count); void (*iotlb_flush_all)(struct domain *d); void (*dump_p2m_table)(struct domain *d); + uint32_t (*features)(struct domain *d); }; void iommu_suspend(void);
Some IOMMU doesn't suppport coherent PT walk. When the p2m is shared with the CPU, Xen has to make sure the PT changes have reached the memory. Introduce new IOMMU callback that will retrieve the IOMMU feature for a specified domain. On ARM, the platform can contain multiple IOMMUs. Each of them may not have the same set of feature. The domain parameter will be used to get the set of features for IOMMUs used by this domain. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v4: - Patch added --- xen/arch/arm/p2m.c | 24 ++++++++++++++++++------ xen/drivers/passthrough/iommu.c | 11 +++++++++++ xen/include/xen/iommu.h | 5 +++++ 3 files changed, 34 insertions(+), 6 deletions(-)