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

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

On Wed, Jul 03, 2019 at 12:18:45PM +0000, Jan Beulich wrote:
> There are currently three more or less different checks:
> - _get_page_type() adjusts the IOMMU mappings according to the new type
>    alone,
> - arch_iommu_populate_page_table() wants just the type to be
>    PGT_writable_page,
> - iommu_hwdom_init() additionally permits all other types with a type
>    refcount of zero.
> The canonical one is in _get_page_type(), and as of XSA-288
> arch_iommu_populate_page_table() also has no need anymore to deal with
> PGT_none pages. In the PV Dom0 case, however, PGT_none pages are still
> necessary to consider, since in that case pages don't get handed to
> guest_physmap_add_entry(). Furthermore, the function so far also
> established r/o mappings, which is not in line with the rules set forth
> by the XSA-288 change.
> 
> For arch_iommu_populate_page_table() to not encounter PGT_none pages
> anymore even in cases where the IOMMU gets enabled for a domain only
> when it is already running, replace the IOMMU dependency in
> guest_physmap_add_entry()'s handling of PV guests to check just the
> system wide state instead of the domain property.
> 
> Unfortunately (partially) replacing the iommu_map() call in
> iommu_hwdom_init() implies resurrecting the flush suppression that got
> previously eliminated. Note that the call to iommu_map() can't be
> removed at this point in time - Dom0's initial allocation gets its page
> types set before iommu_hwdom_init() runs.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v3: Re-base.
> v2: Fix IOTLB flushing. Exclude PVH. Use type safe local variables.
> 
> --- 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?

>                   /* nothing */;
>               else if ( get_page_and_type(page, d, PGT_writable_page) )
>                   put_page_and_type(page);
> --- 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?

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

Thanks, Roger.

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