Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control

[Oleksandr <olekstysh@xxxxxxxxx>]

On 26.09.19 11:39, Paul Durrant wrote:

Hi Paul

> > > > [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.

Not all Arm based SoCs (which supported by Xen) contains SMMU (the only 
one supported driver at the moment) or IPMMU-VMSA (on review now, but, 
it will be under CONFIG_EXPERT when merged, so disabled by default). So, 
"iommu_enabled" can be false.

--
Regards,

Oleksandr Tyshchenko


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