Re: [Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables
> -Original Message- > From: Jan Beulich > Sent: 29 August 2019 14:39 > To: Paul Durrant > Cc: xen-devel@lists.xenproject.org; Julien Grall ; > Alexandru Isaila > ; Petre Pircalabu ; > Razvan Cojocaru > ; Andrew Cooper ; Roger > Pau Monne > ; Volodymyr Babchuk ; > George Dunlap > ; Ian Jackson ; Stefano > Stabellini > ; KonradRzeszutek Wilk ; > Tamas K Lengyel > ; Tim (Xen.org) ; Wei Liu > Subject: Re: [PATCH v6 08/10] remove late (on-demand) construction of IOMMU > page tables > > On 16.08.2019 19:19, Paul Durrant wrote: > > --- a/xen/drivers/passthrough/iommu.c > > +++ b/xen/drivers/passthrough/iommu.c > > @@ -146,6 +146,17 @@ static int __init parse_dom0_iommu_param(const char *s) > > } > > custom_param("dom0-iommu", parse_dom0_iommu_param); > > > > +static void __hwdom_init check_hwdom_reqs(struct domain *d) > > This really should have const, but I realize ... > > > +{ > > +if ( iommu_hwdom_none || !paging_mode_translate(d) ) > > +return; > > + > > +arch_iommu_check_autotranslated_hwdom(d); > > ... this one wants non-const (for - afaict - no reason). > > > @@ -159,129 +170,44 @@ int iommu_domain_init(struct domain *d) > > return ret; > > > > hd->platform_ops = iommu_get_ops(); > > -return hd->platform_ops->init(d); > > -} > > +ret = hd->platform_ops->init(d); > > +if ( ret ) > > +return ret; > > > > -static void __hwdom_init check_hwdom_reqs(struct domain *d) > > -{ > > -if ( iommu_hwdom_none || !paging_mode_translate(d) ) > > -return; > > +/* > > + * NB: 'relaxed' h/w domains don't need the IOMMU mappings to be kept > > + * in-sync with their assigned pages because all host RAM will be > > + * mapped during hwdom_init(). > > + */ > > Doesn't this comment belong to ... > > > +if ( is_hardware_domain(d) ) > > +check_hwdom_reqs(d); /* may modify iommu_hwdom_strict */ > > > > -arch_iommu_check_autotranslated_hwdom(d); > > +if ( !is_hardware_domain(d) || iommu_hwdom_strict ) > > +hd->need_sync = !iommu_use_hap_pt(d); > > ... this if()? Yes, it's probably adrift from where it should be now. > > > @@ -629,8 +552,7 @@ static void iommu_dump_p2m_table(unsigned char key) > > ops = iommu_get_ops(); > > for_each_domain(d) > > { > > -if ( is_hardware_domain(d) || > > - dom_iommu(d)->status < IOMMU_STATUS_initialized ) > > +if ( !is_iommu_enabled(d) ) > > continue; > > Didn't you agree to retain the hwdom part of the condition here? > Indeed I have as of today, but this was posted 2 weeks ago :-) Paul > Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables
On 16.08.2019 19:19, Paul Durrant wrote: > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -146,6 +146,17 @@ static int __init parse_dom0_iommu_param(const char *s) > } > custom_param("dom0-iommu", parse_dom0_iommu_param); > > +static void __hwdom_init check_hwdom_reqs(struct domain *d) This really should have const, but I realize ... > +{ > +if ( iommu_hwdom_none || !paging_mode_translate(d) ) > +return; > + > +arch_iommu_check_autotranslated_hwdom(d); ... this one wants non-const (for - afaict - no reason). > @@ -159,129 +170,44 @@ int iommu_domain_init(struct domain *d) > return ret; > > hd->platform_ops = iommu_get_ops(); > -return hd->platform_ops->init(d); > -} > +ret = hd->platform_ops->init(d); > +if ( ret ) > +return ret; > > -static void __hwdom_init check_hwdom_reqs(struct domain *d) > -{ > -if ( iommu_hwdom_none || !paging_mode_translate(d) ) > -return; > +/* > + * NB: 'relaxed' h/w domains don't need the IOMMU mappings to be kept > + * in-sync with their assigned pages because all host RAM will be > + * mapped during hwdom_init(). > + */ Doesn't this comment belong to ... > +if ( is_hardware_domain(d) ) > +check_hwdom_reqs(d); /* may modify iommu_hwdom_strict */ > > -arch_iommu_check_autotranslated_hwdom(d); > +if ( !is_hardware_domain(d) || iommu_hwdom_strict ) > +hd->need_sync = !iommu_use_hap_pt(d); ... this if()? > @@ -629,8 +552,7 @@ static void iommu_dump_p2m_table(unsigned char key) > ops = iommu_get_ops(); > for_each_domain(d) > { > -if ( is_hardware_domain(d) || > - dom_iommu(d)->status < IOMMU_STATUS_initialized ) > +if ( !is_iommu_enabled(d) ) > continue; Didn't you agree to retain the hwdom part of the condition here? Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables
> -Original Message- > From: Roger Pau Monne > Sent: 23 August 2019 12:34 > To: Paul Durrant > Cc: xen-devel@lists.xenproject.org; Alexandru Isaila > ; Stefano Stabellini > ; Julien Grall ; Volodymyr > Babchuk > ; Andrew Cooper ; > George Dunlap > ; Ian Jackson ; Jan Beulich > ; > Konrad Rzeszutek Wilk ; Tim (Xen.org) ; > Wei Liu ; > Tamas K Lengyel ; Razvan Cojocaru > ; Petre Pircalabu > > Subject: Re: [PATCH v6 08/10] remove late (on-demand) construction of IOMMU > page tables > > On Fri, Aug 16, 2019 at 06:19:59PM +0100, Paul Durrant wrote: > > Now that there is a per-domain IOMMU enable flag, which should be enabled if > > any device is going to be passed through, stop deferring page table > > construction until the assignment is done. Also don't tear down the tables > > again when the last device is de-assigned; defer that task until domain > > destruction. > > > > This allows the has_iommu_pt() helper and iommu_status enumeration to be > > removed. Calls to has_iommu_pt() are simply replaced by calls to > > is_iommu_enabled(). Remaining open-code tests of iommu_hap_pt_share can also > > be replaced by calls to iommu_use_hap_pt(). > > The arch_iommu_populate_page_table() and iommu_construct() functions become > > redundant, as does the 'strict mode' dom0 page_list mapping code in > > iommu_hwdom_init(), and iommu_teardown() can be made static is its only > > remaining caller, iommu_domain_destroy(), is within the same source > > module. > > > > All in all, about 220 lines of code are removed. > > > > NOTE: This patch will cause a small amount of extra resource to be used > > to accommodate IOMMU page tables that may never be used, since the > > per-domain IOMMU flag enable flag is currently set to the value > > of the global iommu_enable flag. A subsequent patch will add an > > option to the toolstack to allow it to be turned off if there is > > no intention to assign passthrough hardware to the domain. > > > > Signed-off-by: Paul Durrant > > Reviewed-by: Alexandru Isaila > > --- > > Cc: Stefano Stabellini > > Cc: Julien Grall > > Cc: Volodymyr Babchuk > > Cc: Andrew Cooper > > Cc: George Dunlap > > Cc: Ian Jackson > > Cc: Jan Beulich > > Cc: Konrad Rzeszutek Wilk > > Cc: Tim Deegan > > Cc: Wei Liu > > Cc: "Roger Pau Monné" > > Cc: Tamas K Lengyel > > Cc: George Dunlap > > Cc: Razvan Cojocaru > > Cc: Petre Pircalabu > > > > Previously part of series > > https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html > > > > v5: > > - Minor style fixes > > --- > > xen/arch/arm/p2m.c| 2 +- > > xen/arch/x86/dom0_build.c | 2 +- > > xen/arch/x86/hvm/mtrr.c | 5 +- > > xen/arch/x86/mm/mem_sharing.c | 2 +- > > xen/arch/x86/mm/paging.c | 2 +- > > xen/arch/x86/x86_64/mm.c | 2 +- > > xen/common/memory.c | 4 +- > > xen/common/vm_event.c | 2 +- > > xen/drivers/passthrough/device_tree.c | 11 --- > > xen/drivers/passthrough/iommu.c | 134 ++ > > xen/drivers/passthrough/pci.c | 12 --- > > xen/drivers/passthrough/vtd/iommu.c | 10 +- > > xen/drivers/passthrough/x86/iommu.c | 95 -- > > xen/include/asm-arm/iommu.h | 2 +- > > xen/include/asm-x86/iommu.h | 2 +- > > xen/include/xen/iommu.h | 16 --- > > xen/include/xen/sched.h | 2 - > > 17 files changed, 42 insertions(+), 263 deletions(-) > > > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > > index 7f1442932a..692565757e 100644 > > --- a/xen/arch/arm/p2m.c > > +++ b/xen/arch/arm/p2m.c > > @@ -1056,7 +1056,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m, > > !mfn_eq(lpae_get_mfn(*entry), lpae_get_mfn(orig_pte)) ) > > p2m_free_entry(p2m, orig_pte, level); > > > > -if ( has_iommu_pt(p2m->domain) && > > +if ( is_iommu_enabled(p2m->domain) && > > (lpae_is_valid(orig_pte) || lpae_is_valid(*entry)) ) > > { > > unsigned int flush_flags = 0; > > diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c > > index d381784edd..7cfab2dc25 100644 > > --- a/xen/arch/x86/dom0_build.c > > +++ b/xen/arch/x86/dom0_build.c > > @@ -365,7 +365,7 @@ unsigned long __init dom0_compute_nr_pages( > > } > > > > need_paging = is_hvm_domain(d) && > > -(!iommu_hap_pt_share || !paging_mode_hap(d)); > > +(!iommu_use_hap_pt(d) || !paging_mode_hap(d)); > > I'm not sure this is correct, at the point where dom0_compute_nr_pages > gets called the iommu has not been initialized yet (the call to > iommu_hwdom_init is done afterwards), so the iommu status field which > is used by iommu_use_hap_pt is not yet initialized. Note that this patch removes the iommu status field. > > > for ( ; ; need_paging = false ) > > { > > nr_pages = get_memsize(_size,
Re: [Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables
On Fri, Aug 16, 2019 at 06:19:59PM +0100, Paul Durrant wrote: > Now that there is a per-domain IOMMU enable flag, which should be enabled if > any device is going to be passed through, stop deferring page table > construction until the assignment is done. Also don't tear down the tables > again when the last device is de-assigned; defer that task until domain > destruction. > > This allows the has_iommu_pt() helper and iommu_status enumeration to be > removed. Calls to has_iommu_pt() are simply replaced by calls to > is_iommu_enabled(). Remaining open-code tests of iommu_hap_pt_share can also > be replaced by calls to iommu_use_hap_pt(). > The arch_iommu_populate_page_table() and iommu_construct() functions become > redundant, as does the 'strict mode' dom0 page_list mapping code in > iommu_hwdom_init(), and iommu_teardown() can be made static is its only > remaining caller, iommu_domain_destroy(), is within the same source > module. > > All in all, about 220 lines of code are removed. > > NOTE: This patch will cause a small amount of extra resource to be used > to accommodate IOMMU page tables that may never be used, since the > per-domain IOMMU flag enable flag is currently set to the value > of the global iommu_enable flag. A subsequent patch will add an > option to the toolstack to allow it to be turned off if there is > no intention to assign passthrough hardware to the domain. > > Signed-off-by: Paul Durrant > Reviewed-by: Alexandru Isaila > --- > Cc: Stefano Stabellini > Cc: Julien Grall > Cc: Volodymyr Babchuk > Cc: Andrew Cooper > Cc: George Dunlap > Cc: Ian Jackson > Cc: Jan Beulich > Cc: Konrad Rzeszutek Wilk > Cc: Tim Deegan > Cc: Wei Liu > Cc: "Roger Pau Monné" > Cc: Tamas K Lengyel > Cc: George Dunlap > Cc: Razvan Cojocaru > Cc: Petre Pircalabu > > Previously part of series > https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html > > v5: > - Minor style fixes > --- > xen/arch/arm/p2m.c| 2 +- > xen/arch/x86/dom0_build.c | 2 +- > xen/arch/x86/hvm/mtrr.c | 5 +- > xen/arch/x86/mm/mem_sharing.c | 2 +- > xen/arch/x86/mm/paging.c | 2 +- > xen/arch/x86/x86_64/mm.c | 2 +- > xen/common/memory.c | 4 +- > xen/common/vm_event.c | 2 +- > xen/drivers/passthrough/device_tree.c | 11 --- > xen/drivers/passthrough/iommu.c | 134 ++ > xen/drivers/passthrough/pci.c | 12 --- > xen/drivers/passthrough/vtd/iommu.c | 10 +- > xen/drivers/passthrough/x86/iommu.c | 95 -- > xen/include/asm-arm/iommu.h | 2 +- > xen/include/asm-x86/iommu.h | 2 +- > xen/include/xen/iommu.h | 16 --- > xen/include/xen/sched.h | 2 - > 17 files changed, 42 insertions(+), 263 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 7f1442932a..692565757e 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1056,7 +1056,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m, > !mfn_eq(lpae_get_mfn(*entry), lpae_get_mfn(orig_pte)) ) > p2m_free_entry(p2m, orig_pte, level); > > -if ( has_iommu_pt(p2m->domain) && > +if ( is_iommu_enabled(p2m->domain) && > (lpae_is_valid(orig_pte) || lpae_is_valid(*entry)) ) > { > unsigned int flush_flags = 0; > diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c > index d381784edd..7cfab2dc25 100644 > --- a/xen/arch/x86/dom0_build.c > +++ b/xen/arch/x86/dom0_build.c > @@ -365,7 +365,7 @@ unsigned long __init dom0_compute_nr_pages( > } > > need_paging = is_hvm_domain(d) && > -(!iommu_hap_pt_share || !paging_mode_hap(d)); > +(!iommu_use_hap_pt(d) || !paging_mode_hap(d)); I'm not sure this is correct, at the point where dom0_compute_nr_pages gets called the iommu has not been initialized yet (the call to iommu_hwdom_init is done afterwards), so the iommu status field which is used by iommu_use_hap_pt is not yet initialized. > for ( ; ; need_paging = false ) > { > nr_pages = get_memsize(_size, avail); > diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c > index 7ccd85bcea..5ad15eafe0 100644 > --- a/xen/arch/x86/hvm/mtrr.c > +++ b/xen/arch/x86/hvm/mtrr.c > @@ -783,7 +783,8 @@ HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, > hvm_load_mtrr_msr, 1, > > void memory_type_changed(struct domain *d) > { > -if ( (has_iommu_pt(d) || cache_flush_permitted(d)) && d->vcpu && > d->vcpu[0] ) > +if ( (is_iommu_enabled(d) || cache_flush_permitted(d)) && > + d->vcpu && d->vcpu[0] ) > { > p2m_memory_type_changed(d); > flush_all(FLUSH_CACHE); > @@ -831,7 +832,7 @@ int epte_get_entry_emt(struct domain *d, unsigned long > gfn, mfn_t mfn, > return MTRR_TYPE_UNCACHABLE; > } > > -if
Re: [Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables
On 8/16/19 8:19 PM, Paul Durrant wrote: > Now that there is a per-domain IOMMU enable flag, which should be enabled if > any device is going to be passed through, stop deferring page table > construction until the assignment is done. Also don't tear down the tables > again when the last device is de-assigned; defer that task until domain > destruction. > > This allows the has_iommu_pt() helper and iommu_status enumeration to be > removed. Calls to has_iommu_pt() are simply replaced by calls to > is_iommu_enabled(). Remaining open-code tests of iommu_hap_pt_share can also > be replaced by calls to iommu_use_hap_pt(). > The arch_iommu_populate_page_table() and iommu_construct() functions become > redundant, as does the 'strict mode' dom0 page_list mapping code in > iommu_hwdom_init(), and iommu_teardown() can be made static is its only > remaining caller, iommu_domain_destroy(), is within the same source > module. > > All in all, about 220 lines of code are removed. > > NOTE: This patch will cause a small amount of extra resource to be used > to accommodate IOMMU page tables that may never be used, since the > per-domain IOMMU flag enable flag is currently set to the value > of the global iommu_enable flag. A subsequent patch will add an > option to the toolstack to allow it to be turned off if there is > no intention to assign passthrough hardware to the domain. This has slipped under my radar, sorry. Acked-by: Razvan Cojocaru Thanks, Razvan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables
Now that there is a per-domain IOMMU enable flag, which should be enabled if any device is going to be passed through, stop deferring page table construction until the assignment is done. Also don't tear down the tables again when the last device is de-assigned; defer that task until domain destruction. This allows the has_iommu_pt() helper and iommu_status enumeration to be removed. Calls to has_iommu_pt() are simply replaced by calls to is_iommu_enabled(). Remaining open-code tests of iommu_hap_pt_share can also be replaced by calls to iommu_use_hap_pt(). The arch_iommu_populate_page_table() and iommu_construct() functions become redundant, as does the 'strict mode' dom0 page_list mapping code in iommu_hwdom_init(), and iommu_teardown() can be made static is its only remaining caller, iommu_domain_destroy(), is within the same source module. All in all, about 220 lines of code are removed. NOTE: This patch will cause a small amount of extra resource to be used to accommodate IOMMU page tables that may never be used, since the per-domain IOMMU flag enable flag is currently set to the value of the global iommu_enable flag. A subsequent patch will add an option to the toolstack to allow it to be turned off if there is no intention to assign passthrough hardware to the domain. Signed-off-by: Paul Durrant Reviewed-by: Alexandru Isaila --- Cc: Stefano Stabellini Cc: Julien Grall Cc: Volodymyr Babchuk Cc: Andrew Cooper Cc: George Dunlap Cc: Ian Jackson Cc: Jan Beulich Cc: Konrad Rzeszutek Wilk Cc: Tim Deegan Cc: Wei Liu Cc: "Roger Pau Monné" Cc: Tamas K Lengyel Cc: George Dunlap Cc: Razvan Cojocaru Cc: Petre Pircalabu Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html v5: - Minor style fixes --- xen/arch/arm/p2m.c| 2 +- xen/arch/x86/dom0_build.c | 2 +- xen/arch/x86/hvm/mtrr.c | 5 +- xen/arch/x86/mm/mem_sharing.c | 2 +- xen/arch/x86/mm/paging.c | 2 +- xen/arch/x86/x86_64/mm.c | 2 +- xen/common/memory.c | 4 +- xen/common/vm_event.c | 2 +- xen/drivers/passthrough/device_tree.c | 11 --- xen/drivers/passthrough/iommu.c | 134 ++ xen/drivers/passthrough/pci.c | 12 --- xen/drivers/passthrough/vtd/iommu.c | 10 +- xen/drivers/passthrough/x86/iommu.c | 95 -- xen/include/asm-arm/iommu.h | 2 +- xen/include/asm-x86/iommu.h | 2 +- xen/include/xen/iommu.h | 16 --- xen/include/xen/sched.h | 2 - 17 files changed, 42 insertions(+), 263 deletions(-) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 7f1442932a..692565757e 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1056,7 +1056,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m, !mfn_eq(lpae_get_mfn(*entry), lpae_get_mfn(orig_pte)) ) p2m_free_entry(p2m, orig_pte, level); -if ( has_iommu_pt(p2m->domain) && +if ( is_iommu_enabled(p2m->domain) && (lpae_is_valid(orig_pte) || lpae_is_valid(*entry)) ) { unsigned int flush_flags = 0; diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c index d381784edd..7cfab2dc25 100644 --- a/xen/arch/x86/dom0_build.c +++ b/xen/arch/x86/dom0_build.c @@ -365,7 +365,7 @@ unsigned long __init dom0_compute_nr_pages( } need_paging = is_hvm_domain(d) && -(!iommu_hap_pt_share || !paging_mode_hap(d)); +(!iommu_use_hap_pt(d) || !paging_mode_hap(d)); for ( ; ; need_paging = false ) { nr_pages = get_memsize(_size, avail); diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c index 7ccd85bcea..5ad15eafe0 100644 --- a/xen/arch/x86/hvm/mtrr.c +++ b/xen/arch/x86/hvm/mtrr.c @@ -783,7 +783,8 @@ HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, hvm_load_mtrr_msr, 1, void memory_type_changed(struct domain *d) { -if ( (has_iommu_pt(d) || cache_flush_permitted(d)) && d->vcpu && d->vcpu[0] ) +if ( (is_iommu_enabled(d) || cache_flush_permitted(d)) && + d->vcpu && d->vcpu[0] ) { p2m_memory_type_changed(d); flush_all(FLUSH_CACHE); @@ -831,7 +832,7 @@ int epte_get_entry_emt(struct domain *d, unsigned long gfn, mfn_t mfn, return MTRR_TYPE_UNCACHABLE; } -if ( !has_iommu_pt(d) && !cache_flush_permitted(d) ) +if ( !is_iommu_enabled(d) && !cache_flush_permitted(d) ) { *ipat = 1; return MTRR_TYPE_WRBACK; diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c index a5fe89e339..efb8821768 100644 --- a/xen/arch/x86/mm/mem_sharing.c +++ b/xen/arch/x86/mm/mem_sharing.c @@ -1664,7 +1664,7 @@ int mem_sharing_domctl(struct domain *d, struct xen_domctl_mem_sharing_op *mec) case XEN_DOMCTL_MEM_SHARING_CONTROL: { rc = 0; -if (