On 3/5/19 1:26 PM, 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. > > Signed-off-by: Jan Beulich <jbeul...@suse.com> > > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -837,11 +837,11 @@ guest_physmap_add_entry(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). > */ > - if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw ) > + if ( !iommu_enabled || t != p2m_ram_rw ) > return 0;
This looks good. One question about the next one... > > for ( i = 0; i < (1UL << page_order); ++i, ++page ) > --- 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()? -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel