> -----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. 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. Paul > > Cheers, > > -- > Julien Grall _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
