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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 9/26/19 9:39 AM, Paul Durrant wrote:
> > -----Original Message-----
> > From: Julien Grall <Julien.Grall@xxxxxxx>
> > Sent: 25 September 2019 22:34
> > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; 'Oleksandr' <olekstysh@xxxxxxxxx>; 
> > 'Jan Beulich'
> > <jbeulich@xxxxxxxx>
> > Cc: nd <nd@xxxxxxx>; Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>; Stefano 
> > Stabellini
> > <sstabellini@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; KonradRzeszutek Wilk 
> > <konrad.wilk@xxxxxxxxxx>; Andrew
> > Cooper <Andrew.Cooper3@xxxxxxxxxx>; David Scott <dave@xxxxxxxxxx>; Tim (Xen.org) 
> > <tim@xxxxxxx>; George
> > Dunlap <George.Dunlap@xxxxxxxxxx>; Tamas K Lengyel <tamas@xxxxxxxxxxxxx>; Ian 
> > Jackson
> > <Ian.Jackson@xxxxxxxxxx>; Anthony Perard <anthony.perard@xxxxxxxxxx>; 
> > xen-devel@xxxxxxxxxxxxxxxxxxxx;
> > Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; Roger Pau Monne 
> > <roger.pau@xxxxxxxxxx>
> > 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 <olekstysh@xxxxxxxxx>
> > > > Sent: 25 September 2019 16:50
> > > > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; 'Jan Beulich' <jbeulich@xxxxxxxx>
> > > > Cc: Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>; Stefano Stabellini 
> > > > <sstabellini@xxxxxxxxxx>; Wei
> > Liu
> > > > <wl@xxxxxxx>; KonradRzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Andrew Cooper
> > > > <Andrew.Cooper3@xxxxxxxxxx>; David Scott <dave@xxxxxxxxxx>; Tim (Xen.org) 
> > > > <tim@xxxxxxx>; George
> > Dunlap
> > > > <George.Dunlap@xxxxxxxxxx>; Tamas K Lengyel <tamas@xxxxxxxxxxxxx>; Ian Jackson
> > > > <Ian.Jackson@xxxxxxxxxx>; Anthony Perard <anthony.perard@xxxxxxxxxx>; xen-
> > devel@xxxxxxxxxxxxxxxxxxxx;
> > > > Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; Roger Pau Monne 
> > > > <roger.pau@xxxxxxxxxx>; Julien
> > Grall
> > > > <julien.grall@xxxxxxx>
> > > > 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?
> 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? 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.
Cheers,

--
Julien Grall

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