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

> 
> >> --- a/xen/drivers/passthrough/iommu.c
> >> +++ b/xen/drivers/passthrough/iommu.c
> >> @@ -192,28 +192,46 @@ void __hwdom_init iommu_hwdom_init(struc
> >>           unsigned int i = 0, flush_flags = 0;
> >>           int rc = 0;
> >>   
> >> +        this_cpu(iommu_dont_flush_iotlb) = true;
> >> +
> >>           page_list_for_each ( page, &d->page_list )
> >>           {
> >> -            unsigned long mfn = mfn_x(page_to_mfn(page));
> >> -            unsigned long dfn = mfn_to_gmfn(d, mfn);
> >> -            unsigned int mapping = IOMMUF_readable;
> >> -            int ret;
> >> -
> >> -            if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) ||
> >> -                 ((page->u.inuse.type_info & PGT_type_mask)
> >> -                  == PGT_writable_page) )
> >> -                mapping |= IOMMUF_writable;
> >> -
> >> -            ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0, mapping,
> >> -                            &flush_flags);
> >> -
> >> -            if ( !rc )
> >> -                rc = ret;
> >> +            switch ( page->u.inuse.type_info & PGT_type_mask )
> >> +            {
> >> +            case PGT_none:
> >> +                if ( is_pv_domain(d) )
> >> +                {
> >> +                    ASSERT(!(page->u.inuse.type_info & PGT_count_mask));
> >> +                    if ( get_page_and_type(page, d, PGT_writable_page) )
> > 
> > Could you add a comment that get_page_and_type will add an iommu
> > entry if succeeding?
> 
> Well, yes, I could, but this would just re-state what the respective
> section of the big comment at the top of mm.c effectively says.

Oh, never mind then, it just took me some time to figure out why you
set iommu_dont_flush_iotlb without doing any _legacy iommu calls. I
then realized get_page_and_type was doing such calls.

> Anyway - as long as Paul's "remove late (on-demand) construction of
> IOMMU page tables" would go in any time soon, this will all become
> unnecessary anyway.
> 
> >> +                    {
> >> +                        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.

Thanks, Roger.

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