Re: [Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables

From: Paul Durrant <Paul.Durrant@citrix.com>
To: 'Jan Beulich' <jbeulich@suse.com>
Cc: Petre Pircalabu <ppircalabu@bitdefender.com>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Razvan Cojocaru <rcojocaru@bitdefender.com>,
	KonradRzeszutek Wilk <konrad.wilk@oracle.com>,
	Andrew Cooper <Andrew.Cooper3@citrix.com>,
	"Tim \(Xen.org\)" <tim@xen.org>,
	George Dunlap <George.Dunlap@citrix.com>,
	Julien Grall <julien.grall@arm.com>,
	Tamas K Lengyel <tamas@tklengyel.com>,
	Ian Jackson <Ian.Jackson@citrix.com>,
	Alexandru Isaila <aisaila@bitdefender.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables
Date: Thu, 29 Aug 2019 13:44:59 +0000	[thread overview]
Message-ID: <6bc83685a50f44e2b7759dfbfc9c155d@AMSPEX02CL03.citrite.net> (raw)
In-Reply-To: <7ac91f71-eac4-bf13-ff01-09fe4d9b7376@suse.com>

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 29 August 2019 14:39
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Julien Grall <julien.grall@arm.com>; Alexandru Isaila
> <aisaila@bitdefender.com>; Petre Pircalabu <ppircalabu@bitdefender.com>; Razvan Cojocaru
> <rcojocaru@bitdefender.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>; KonradRzeszutek Wilk <konrad.wilk@oracle.com>; Tamas K Lengyel
> <tamas@tklengyel.com>; Tim (Xen.org) <tim@xen.org>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables
> 
> On 16.08.2019 19:19, Paul Durrant wrote:
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -146,6 +146,17 @@ static int __init parse_dom0_iommu_param(const char *s)
> >  }
> >  custom_param("dom0-iommu", parse_dom0_iommu_param);
> >
> > +static void __hwdom_init check_hwdom_reqs(struct domain *d)
> 
> This really should have const, but I realize ...
> 
> > +{
> > +    if ( iommu_hwdom_none || !paging_mode_translate(d) )
> > +        return;
> > +
> > +    arch_iommu_check_autotranslated_hwdom(d);
> 
> ... this one wants non-const (for - afaict - no reason).
> 
> > @@ -159,129 +170,44 @@ int iommu_domain_init(struct domain *d)
> >          return ret;
> >
> >      hd->platform_ops = iommu_get_ops();
> > -    return hd->platform_ops->init(d);
> > -}
> > +    ret = hd->platform_ops->init(d);
> > +    if ( ret )
> > +        return ret;
> >
> > -static void __hwdom_init check_hwdom_reqs(struct domain *d)
> > -{
> > -    if ( iommu_hwdom_none || !paging_mode_translate(d) )
> > -        return;
> > +    /*
> > +     * NB: 'relaxed' h/w domains don't need the IOMMU mappings to be kept
> > +     *     in-sync with their assigned pages because all host RAM will be
> > +     *     mapped during hwdom_init().
> > +     */
> 
> Doesn't this comment belong to ...
> 
> > +    if ( is_hardware_domain(d) )
> > +        check_hwdom_reqs(d); /* may modify iommu_hwdom_strict */
> >
> > -    arch_iommu_check_autotranslated_hwdom(d);
> > +    if ( !is_hardware_domain(d) || iommu_hwdom_strict )
> > +        hd->need_sync = !iommu_use_hap_pt(d);
> 
> ... this if()?

Yes, it's probably adrift from where it should be now.

> 
> > @@ -629,8 +552,7 @@ static void iommu_dump_p2m_table(unsigned char key)
> >      ops = iommu_get_ops();
> >      for_each_domain(d)
> >      {
> > -        if ( is_hardware_domain(d) ||
> > -             dom_iommu(d)->status < IOMMU_STATUS_initialized )
> > +        if ( !is_iommu_enabled(d) )
> >              continue;
> 
> Didn't you agree to retain the hwdom part of the condition here?
> 

Indeed I have as of today, but this was posted 2 weeks ago :-)

  Paul

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