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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.05.19 at 15:44, <george.dunlap@xxxxxxxxxx> wrote:
> On 3/5/19 1:26 PM, Jan Beulich wrote:
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -192,21 +192,27 @@ void __hwdom_init iommu_hwdom_init(struc
>>  
>>          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_type_mask) == PGT_none )
>> +            {
>> +                ASSERT(!(page->u.inuse.type_info & PGT_count_mask));
>> +                if ( get_page_and_type(page, d, PGT_writable_page) )
>> +                    put_page_and_type(page);
>> +                else if ( !rc )
>> +                    rc = -EBUSY;
>> +            }
>>  
>> -            if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) ||
>> -                 ((page->u.inuse.type_info & PGT_type_mask)
>> -                  == PGT_writable_page) )
>> -                mapping |= IOMMUF_writable;
>> +            if ( ((page->u.inuse.type_info & PGT_type_mask) ==
>> +                  PGT_writable_page) )
>> +            {
>> +                unsigned long mfn = mfn_x(page_to_mfn(page));
>> +                unsigned long dfn = mfn_to_gmfn(d, mfn);
>> +                int ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0,
>> +                                    IOMMUF_readable | IOMMUF_writable,
>> +                                    &flush_flags);
> 
> What's the idea behind calling iommu_map() here, rather than relying on
> the one in _get_page_type()?  Does need_iommu_pt_sync() not work yet at
> this point, or do you expect there to be pages that have been marked
> PGT_writable without having gone through _get_page_type()?

"iommu=dom0-strict" doesn't work without this. That's because the
PV Dom0 builder sets the pages in the intial allocation to
PGT_writable (or the necessary page table types) long before
calling iommu_hwdom_init(). Probably this could be re-arranged, but
I don't think I'm up to this right now.

But having at least made the attempt still pointed out two issues
with the patch: For PGT_none pages (i.e. get_page_and_type()
actually having the intended effect) IOTLB flushing wasn't done
(not a big problem, since Dom0 hasn't run yet, but inconsistent
with the function calling iommu_iotlb_flush_all() in the first place).
Plus I don't think the {get,put}_page_and_type() dance should
be done at all for PVH Dom0.

So a v2 is coming in any event.

Jan



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