Re: [Xen-devel] [PATCH v3] IOMMU/x86: make page type checks consistent when mapping pages

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Sep 06, 2019 at 04:19:50PM +0200, Jan Beulich wrote:
> On 06.09.2019 16:08, Roger Pau Monné  wrote:
> > On Fri, Sep 06, 2019 at 02:08:09PM +0200, Jan Beulich wrote:
> >> On 06.09.2019 13:45, Roger Pau Monné  wrote:
> >>> On Fri, Sep 06, 2019 at 12:52:11PM +0200, Jan Beulich wrote:
> >>>> On 06.09.2019 11:37, Roger Pau Monné  wrote:
> >>>>> On Wed, Jul 03, 2019 at 12:18:45PM +0000, Jan Beulich wrote:
> >>>>>> --- a/xen/arch/x86/mm/p2m.c
> >>>>>> +++ b/xen/arch/x86/mm/p2m.c
> >>>>>> @@ -829,13 +829,13 @@ guest_physmap_add_page(struct domain *d,
> >>>>>>            *
> >>>>>>            * Retain this property by grabbing a writable type ref and 
> >>>>>> then
> >>>>>>            * dropping it immediately.  The result will be pages that 
> >>>>>> have a
> >>>>>> -         * writable type (and an IOMMU entry), but a count of 0 (such 
> >>>>>> that
> >>>>>> -         * any guest-requested type changes succeed and remove the 
> >>>>>> IOMMU
> >>>>>> -         * entry).
> >>>>>> +         * writable type (and an IOMMU entry if necessary), but a 
> >>>>>> count of 0
> >>>>>> +         * (such that any guest-requested type changes succeed and 
> >>>>>> remove the
> >>>>>> +         * IOMMU entry).
> >>>>>>            */
> >>>>>>           for ( i = 0; i < (1UL << page_order); ++i, ++page )
> >>>>>>           {
> >>>>>> -            if ( !need_iommu_pt_sync(d) )
> >>>>>> +            if ( !iommu_enabled )
> >>>>>
> >>>>> That's kind of a strong check for a domain that might never use the
> >>>>> iommu at all. Isn't it fine to just rely on
> >>>>> arch_iommu_populate_page_table finding non-writable pages and thus not
> >>>>> adding them to the iommu page-tables?
> >>>>
> >>>> No - the code change here is to take care of page additions to
> >>>> the domain after it has booted.
> >>>
> >>> Please bear with me, but AFAICT arch_iommu_populate_page_table could
> >>> be used after the domain has booted if a pci device is hot plugged.
> >>>
> >>> If this is to deal with additions to domains having an iommu already
> >>> enabled, isn't it enough to use need_iommu_pt_sync?
> >>>
> >>> That's going to return true for all PV domains, except for dom0 not
> >>> running in strict mode, which is expected because in that case dom0
> >>> already has the whole RAM mapped into the iommu page-tables?
> >>
> >> Well, my previous reply wasn't precise enough, I guess. The change
> >> really is about both cases: If the domain is already using an IOMMU,
> >> need_iommu_pt_sync() would be enough indeed. If IOMMU use _may_ be
> >> enabled later on, we need to transition pages out of their initial
> >> PGT_none state right away. If we deferred this until the point
> >> where the IOMMU gets enabled for the domain, we'd have to watch out
> >> for PGT_none pages there, which would be extra hassle.
> > 
> > I still think a relaxed PV dom0 should avoid the overhead of
> > get_page_and_type, and the iommu flush done afterwards, as it already
> > has all host RAM into it's iommu page-tables.
> > 
> > Ie: I think the check should be something like:
> > 
> > if ( !iommu_enabled ||
> >      (is_hardware_domain(d) && !need_iommu_pt_sync(d) )
> 
> Ah, yes, I can certainly do this (if the patch doesn't become
> unnecessary anyway).

With that please add my Reviewed-by: Roger Pau Monné
<roger.pau@xxxxxxxxxx>, albeit as you say I'm not sure whether this is
really needed in light of the upcoming iommu changes.

Thanks, Roger.

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