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 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) )

> >>>> +                    {
> >>>> +                        put_page_and_type(page);
> >>>> +                        flush_flags |= IOMMUF_readable | 
> >>>> IOMMUF_writable;
> >>>> +                    }
> >>>> +                    else if ( !rc )
> >>>> +                        rc = -EBUSY;
> >>>
> >>> Is it fine to return an error here? AFAICT you could have a RO page
> >>> shared with Xen with PGT_none, and not having an iommu mapping for it
> >>> would be expected, hence returning an error seems wrong?
> >>
> >> No, pages shared with Xen don't live on d->page_list (which is what the
> >> loop iterates over).
> > 
> > So then there should be no PGT_none pages in d->page_list?
> > 
> > The only user I can find of PGT_none is share_xen_page_with_guest.
> 
> Plus the implicit use when a page gets first added to a domain (by
> setting ->u.inuse.type_info to zero).

Ack, thanks for the clarification.

Roger.

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