Re: [Xen-devel] [PATCH] iommu: avoid triggering ASSERT_UNREACHABLE() on ARM...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: 26 September 2019 12:24
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Oleksandr <olekstysh@xxxxxxxxx>; Andrew Cooper 
> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Jan Beulich 
> <jbeulich@xxxxxxxx>;
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini 
> <sstabellini@xxxxxxxxxx>; Tim
> (Xen.org) <tim@xxxxxxx>; Wei Liu <wl@xxxxxxx>
> Subject: Re: [PATCH] iommu: avoid triggering ASSERT_UNREACHABLE() on ARM...
> 
> Hi Paul,
> 
> On 9/26/19 11:03 AM, Paul Durrant wrote:
> > ...when the IOMMU is not enabled.
> >
> > 80ff3d338dc9 "iommu: tidy up iommu_use_hap_pt() and need_iommu_pt_sync()
> > macros" introduced CONFIG_IOMMU_FORCE_PT_SHARE, which causes the global
> > 'iommu_hap_pt_share' option to be replaced with a #define-d value of true.
> > In this configuration, calling clear_iommu_hap_pt_share() will result
> > trigger the aforementioned ASSERT.
> >
> > CONFIG_IOMMU_FORCE_PT_SHARE is always selected for ARM builds and,
> > because clear_iommu_hap_pt_share() is called by the common iommu_setup()
> > function if the IOMMU is not enabled, it is no longer safe to disable the
> > IOMMU on ARM systems.
> >
> > However, running with the IOMMU disabled is a valid configuration for ARM
> > systems, so this patch rectifies the problem by removing the call to
> > clear_iommu_hap_pt_share() from common code. As a side effect of this,
> > however, it becomes possible on x86 systems for iommu_enabled to be false
> > but iommu_hap_pt_share to be true. Thus the code in sysctl.c
> > needs to be changed to make sure that XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share
> > is not erroneously advertised when the IOMMU has been disabled.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > Reported-by: Oleksandr <olekstysh@xxxxxxxxx>
> 
> With one NIT below:
> 
> Acked-by: Julien Grall <julien.grall@xxxxxxx>
> 
> > ---
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Julien Grall <julien.grall@xxxxxxx>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > Cc: Tim Deegan <tim@xxxxxxx>
> > Cc: Wei Liu <wl@xxxxxxx>
> > ---
> >   xen/common/sysctl.c             | 6 ++++--
> >   xen/drivers/passthrough/iommu.c | 1 -
> >   2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > 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/drivers/passthrough/iommu.c 
> > b/xen/drivers/passthrough/iommu.c
> > index e8fddc2dc7..c33ca70ec9 100644
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -456,7 +456,6 @@ int __init iommu_setup(void)
> >       if ( !iommu_enabled )
> >       {
> >           iommu_intremap = 0;
> > -        clear_iommu_hap_pt_share();
> >       }
> 
> NIT: The {} can now be removed.
> 
> I can fix it on commit.

Good point. Thanks,

  Paul

> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel