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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: 26 September 2019 10:13
> 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 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?
> 

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
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel