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) )
         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)
-- 
2.20.1.2.gb21ebb671


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel