Hi,
On 9/26/19 9:39 AM, Paul Durrant wrote:
-----Original Message-----
From: Julien Grall <[email protected]>
Sent: 25 September 2019 22:34
To: Paul Durrant <[email protected]>; 'Oleksandr' <[email protected]>;
'Jan Beulich'
<[email protected]>
Cc: nd <[email protected]>; Petre Pircalabu <[email protected]>; Stefano
Stabellini
<[email protected]>; Wei Liu <[email protected]>; KonradRzeszutek Wilk
<[email protected]>; Andrew
Cooper <[email protected]>; David Scott <[email protected]>; Tim (Xen.org)
<[email protected]>; George
Dunlap <[email protected]>; Tamas K Lengyel <[email protected]>; Ian
Jackson
<[email protected]>; Anthony Perard <[email protected]>;
[email protected];
Volodymyr Babchuk <[email protected]>; Roger Pau Monne
<[email protected]>
Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
Hi,
On 25/09/2019 17:10, Paul Durrant wrote:
-----Original Message-----
From: Oleksandr <[email protected]>
Sent: 25 September 2019 16:50
To: Paul Durrant <[email protected]>; 'Jan Beulich' <[email protected]>
Cc: Petre Pircalabu <[email protected]>; Stefano Stabellini
<[email protected]>; Wei
Liu
<[email protected]>; KonradRzeszutek Wilk <[email protected]>; Andrew Cooper
<[email protected]>; David Scott <[email protected]>; Tim (Xen.org)
<[email protected]>; George
Dunlap
<[email protected]>; Tamas K Lengyel <[email protected]>; Ian Jackson
<[email protected]>; Anthony Perard <[email protected]>; xen-
[email protected];
Volodymyr Babchuk <[email protected]>; Roger Pau Monne
<[email protected]>; Julien
Grall
<[email protected]>
Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
[CC Julien]
Hi Paul
I may mistake, but looks like
80ff3d338dc93260b41ffeeebb0f852c2edef9ce iommu: tidy up
iommu_use_hap_pt() and need_iommu_pt_sync() macros
triggers ASSERT_UNREACHABLE on Arm if no IOMMU has been found (I built
with my platform's IOMMU driver disabled: # CONFIG_IPMMU_VMSA is not set) .
So, iommu_setup() calls clear_iommu_hap_pt_share() with
iommu_hap_pt_share being set (CONFIG_IOMMU_FORCE_PT_SHARE=y) which,
actually, triggers ASSERT.
Here a minimal patch, leaving 'force pt share' in place. Does this avoid the
problem?
---8<---
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index e8763c7fdf..f88a285e7f 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -268,9 +268,11 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t)
u_sysctl)
pi->max_mfn = get_upper_mfn_bound();
arch_do_physinfo(pi);
if ( iommu_enabled )
+ {
pi->capabilities |= XEN_SYSCTL_PHYSCAP_directio;
- if ( iommu_hap_pt_share )
- pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
+ if ( iommu_hap_pt_share )
+ pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
+ }
if ( copy_to_guest(u_sysctl, op, 1) )
ret = -EFAULT;
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 7c3003f3f1..6a10a24128 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -68,8 +68,6 @@ static inline void clear_iommu_hap_pt_share(void)
{
#ifndef iommu_hap_pt_share
iommu_hap_pt_share = false;
-#elif iommu_hap_pt_share
- ASSERT_UNREACHABLE();
#endif
IHMO, calling this function is a mistake on platform only supporting
shared page-table so the ASSERT() should be kept here.
This raises the question why the function is actually called from common
code. iommu_hap_enabled() should technically not be used in any code if
the IOMMU is not enabled/present. So what are you trying to prevent here?
What I'm trying to prevent, on x86, is a situation where the iommu_enabled ==
false but iommu_hap_pt_share == true.
This is not entirely uncommon to have other variables gated by others.
So what could happen if you have iommu_enabled == false and
iommu_hap_pt_share == true on x86?
I had, mistakenly, believed that iommu_enabled would never be false for ARM but
it seems this is not the case so that situation has to be tolerated. I guess,
given the other hunk of my patch, I can actually leave the ASSERT in place and
avoid making the call from common code, in which case the function ought to
move into an x86 header as well.
By "the function", do you mean clear_iommu_hap_pt_share? If so, I think
it should stay were it is. This is a generic function that might be
re-used for other architecture in the future.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
