> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: 29 August 2019 14:39
> To: Paul Durrant <paul.durr...@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Julien Grall <julien.gr...@arm.com>;
> Alexandru Isaila
> <aisa...@bitdefender.com>; Petre Pircalabu <ppircal...@bitdefender.com>;
> Razvan Cojocaru
> <rcojoc...@bitdefender.com>; Andrew Cooper <andrew.coop...@citrix.com>; Roger
> Pau Monne
> <roger....@citrix.com>; Volodymyr Babchuk <volodymyr_babc...@epam.com>;
> George Dunlap
> <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Stefano
> Stabellini
> <sstabell...@kernel.org>; KonradRzeszutek Wilk <konrad.w...@oracle.com>;
> Tamas K Lengyel
> <ta...@tklengyel.com>; Tim (Xen.org) <t...@xen.org>; Wei Liu <w...@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
