> -----Original Message-----
> From: Julien Grall <julien.gr...@arm.com>
> Sent: 25 September 2019 22:34
> To: Paul Durrant <paul.durr...@citrix.com>; 'Oleksandr'
> <olekst...@gmail.com>; 'Jan Beulich'
> <jbeul...@suse.com>
> Cc: nd <n...@arm.com>; Petre Pircalabu <ppircal...@bitdefender.com>; Stefano
> Stabellini
> <sstabell...@kernel.org>; Wei Liu <w...@xen.org>; KonradRzeszutek Wilk
> <konrad.w...@oracle.com>; Andrew
> Cooper <andrew.coop...@citrix.com>; David Scott <d...@recoil.org>; Tim
> (Xen.org) <t...@xen.org>; George
> Dunlap <george.dun...@citrix.com>; Tamas K Lengyel <ta...@tklengyel.com>; Ian
> Jackson
> <ian.jack...@citrix.com>; Anthony Perard <anthony.per...@citrix.com>;
> xen-devel@lists.xenproject.org;
> Volodymyr Babchuk <volodymyr_babc...@epam.com>; Roger Pau Monne
> <roger....@citrix.com>
> 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 <olekst...@gmail.com>
> >> Sent: 25 September 2019 16:50
> >> To: Paul Durrant <paul.durr...@citrix.com>; 'Jan Beulich'
> >> <jbeul...@suse.com>
> >> Cc: Petre Pircalabu <ppircal...@bitdefender.com>; Stefano Stabellini
> >> <sstabell...@kernel.org>; Wei
> Liu
> >> <w...@xen.org>; KonradRzeszutek Wilk <konrad.w...@oracle.com>; Andrew
> >> Cooper
> >> <andrew.coop...@citrix.com>; David Scott <d...@recoil.org>; Tim (Xen.org)
> >> <t...@xen.org>; George
> Dunlap
> >> <george.dun...@citrix.com>; Tamas K Lengyel <ta...@tklengyel.com>; Ian
> >> Jackson
> >> <ian.jack...@citrix.com>; Anthony Perard <anthony.per...@citrix.com>; xen-
> de...@lists.xenproject.org;
> >> Volodymyr Babchuk <volodymyr_babc...@epam.com>; Roger Pau Monne
> >> <roger....@citrix.com>; Julien
> Grall
> >> <julien.gr...@arm.com>
> >> 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
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
