| Message ID | 20190927114554.11513-1-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu: fix PVH dom0 settings | expand |
On 27.09.2019 13:45, Paul Durrant wrote: > PVH dom0 must operate with the iommu settings in 'strict' mode i.e. only the > domain's own pages will be mapped in the IOMMU. The check_hwdom_reqs() is > supposed to ensure this. Unfortunately the test for a PVH dom0 is made > using paging_mode_translate() and, when commit f89f5558 "remove late > (on-demand) construction of IOMMU page tables" moved the call of > check_hwdom_reqs() from iommu_hwdom_init() to iommu_domain_init(), that > test became ineffective (because iommu_domain_init() is called before > paging_enable()). > > This patch replaces the test of paging_mode_translate() with a test of > hap_enabled(), and also verifies 'strict' mode is turned on in > arch_iommu_check_autotranslated_hwdom(). > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > Reported-by: Roger Pau Monne <roger.pau@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Nit: Conventionally your two tags should be the other way around, to represent events chronologically. Jan
On 27.09.2019 13:45, Paul Durrant wrote: > PVH dom0 must operate with the iommu settings in 'strict' mode i.e. only the > domain's own pages will be mapped in the IOMMU. The check_hwdom_reqs() is > supposed to ensure this. Unfortunately the test for a PVH dom0 is made > using paging_mode_translate() and, when commit f89f5558 "remove late > (on-demand) construction of IOMMU page tables" moved the call of > check_hwdom_reqs() from iommu_hwdom_init() to iommu_domain_init(), that > test became ineffective (because iommu_domain_init() is called before > paging_enable()). > > This patch replaces the test of paging_mode_translate() with a test of > hap_enabled(), and also verifies 'strict' mode is turned on in > arch_iommu_check_autotranslated_hwdom(). > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > Reported-by: Roger Pau Monne <roger.pau@citrix.com> Oh, and I guess you've also meant to Cc Jürgen for a release ack (now done)? Jan
On 27.09.19 14:02, Jan Beulich wrote: > On 27.09.2019 13:45, Paul Durrant wrote: >> PVH dom0 must operate with the iommu settings in 'strict' mode i.e. only the >> domain's own pages will be mapped in the IOMMU. The check_hwdom_reqs() is >> supposed to ensure this. Unfortunately the test for a PVH dom0 is made >> using paging_mode_translate() and, when commit f89f5558 "remove late >> (on-demand) construction of IOMMU page tables" moved the call of >> check_hwdom_reqs() from iommu_hwdom_init() to iommu_domain_init(), that >> test became ineffective (because iommu_domain_init() is called before >> paging_enable()). >> >> This patch replaces the test of paging_mode_translate() with a test of >> hap_enabled(), and also verifies 'strict' mode is turned on in >> arch_iommu_check_autotranslated_hwdom(). >> >> Signed-off-by: Paul Durrant <paul.durrant@citrix.com> >> Reported-by: Roger Pau Monne <roger.pau@citrix.com> > > Oh, and I guess you've also meant to Cc Jürgen for a release ack > (now done)? Thanks for the Cc! Release-acked-by: Juergen Gross <jgross@suse.com> Juergen
On Fri, Sep 27, 2019 at 12:45:54PM +0100, Paul Durrant wrote: > PVH dom0 must operate with the iommu settings in 'strict' mode i.e. only the > domain's own pages will be mapped in the IOMMU. The check_hwdom_reqs() is > supposed to ensure this. Unfortunately the test for a PVH dom0 is made > using paging_mode_translate() and, when commit f89f5558 "remove late > (on-demand) construction of IOMMU page tables" moved the call of > check_hwdom_reqs() from iommu_hwdom_init() to iommu_domain_init(), that > test became ineffective (because iommu_domain_init() is called before > paging_enable()). > > This patch replaces the test of paging_mode_translate() with a test of > hap_enabled(), and also verifies 'strict' mode is turned on in > arch_iommu_check_autotranslated_hwdom(). > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > Reported-by: Roger Pau Monne <roger.pau@citrix.com> > --- > Cc: Jan Beulich <jbeulich@suse.com> > --- > xen/drivers/passthrough/iommu.c | 6 +++--- > xen/drivers/passthrough/x86/iommu.c | 3 +++ > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c > index 2733b320ec..8b550f909b 100644 > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -154,13 +154,13 @@ custom_param("dom0-iommu", parse_dom0_iommu_param); > > static void __hwdom_init check_hwdom_reqs(struct domain *d) > { > - if ( iommu_hwdom_none || !paging_mode_translate(d) ) > + if ( iommu_hwdom_none || !hap_enabled(d) ) Since dom0 PVH can also be used with shadow paging (not sure how useful that is TBH), I'm not sure explicitly checking for hap is fine. What about using is_hvm_domain instead? That ought to cover both shadow and hap, and a classic PV dom0 won't be affected by it (which is the intention). Thanks, Roger.
On 27.09.2019 14:28, Roger Pau Monné wrote: > On Fri, Sep 27, 2019 at 12:45:54PM +0100, Paul Durrant wrote: >> --- a/xen/drivers/passthrough/iommu.c >> +++ b/xen/drivers/passthrough/iommu.c >> @@ -154,13 +154,13 @@ custom_param("dom0-iommu", parse_dom0_iommu_param); >> >> static void __hwdom_init check_hwdom_reqs(struct domain *d) >> { >> - if ( iommu_hwdom_none || !paging_mode_translate(d) ) >> + if ( iommu_hwdom_none || !hap_enabled(d) ) > > Since dom0 PVH can also be used with shadow paging (not sure how > useful that is TBH), I'm not sure explicitly checking for hap is fine. > What about using is_hvm_domain instead? > > That ought to cover both shadow and hap, and a classic PV dom0 won't > be affected by it (which is the intention). Oh, indeed - we shouldn't prevent shadow mode use. That'll need to be an incremental change though, as I've already committed the one here. Jan
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index 2733b320ec..8b550f909b 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -154,13 +154,13 @@ custom_param("dom0-iommu", parse_dom0_iommu_param); static void __hwdom_init check_hwdom_reqs(struct domain *d) { - if ( iommu_hwdom_none || !paging_mode_translate(d) ) + if ( iommu_hwdom_none || !hap_enabled(d) ) return; - arch_iommu_check_autotranslated_hwdom(d); - iommu_hwdom_passthrough = false; iommu_hwdom_strict = true; + + arch_iommu_check_autotranslated_hwdom(d); } int iommu_domain_init(struct domain *d, unsigned int opts) diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c index 47a3e55213..f54805babd 100644 --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -85,6 +85,9 @@ void __hwdom_init arch_iommu_check_autotranslated_hwdom(struct domain *d) { if ( !is_iommu_enabled(d) ) panic("Presently, iommu must be enabled for PVH hardware domain\n"); + + if ( !iommu_hwdom_strict ) + panic("PVH hardware domain iommu must be set in 'strict' mode\n"); } int arch_iommu_domain_init(struct domain *d)
PVH dom0 must operate with the iommu settings in 'strict' mode i.e. only the domain's own pages will be mapped in the IOMMU. The check_hwdom_reqs() is supposed to ensure this. Unfortunately the test for a PVH dom0 is made using paging_mode_translate() and, when commit f89f5558 "remove late (on-demand) construction of IOMMU page tables" moved the call of check_hwdom_reqs() from iommu_hwdom_init() to iommu_domain_init(), that test became ineffective (because iommu_domain_init() is called before paging_enable()). This patch replaces the test of paging_mode_translate() with a test of hap_enabled(), and also verifies 'strict' mode is turned on in arch_iommu_check_autotranslated_hwdom(). Signed-off-by: Paul Durrant <paul.durrant@citrix.com> Reported-by: Roger Pau Monne <roger.pau@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> --- xen/drivers/passthrough/iommu.c | 6 +++--- xen/drivers/passthrough/x86/iommu.c | 3 +++ 2 files changed, 6 insertions(+), 3 deletions(-)