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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

Jan

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