> -----Original Message----- > From: Julien Grall <[email protected]> > Sent: 26 September 2019 10:13 > 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 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]>; xen- > [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? >
Well, I was hoping to avoid the nested if in sysctl.c. > > 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? Yes. > 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. > That seems like a bit of a long shot. If I remove the call from iommu_setup() then the only remaining callers are in x86 code, but I suppose it can stay where it is to avoid the churn. I'll spin a new test patch. Paul _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
