On 02.08.2019 11:22, Roger Pau Monne wrote:
> Switch rmrr_identity_mapping to use iommu_{un}map in order to
> establish RMRR mappings for PV domains, like it's done in
> arch_iommu_hwdom_init. This solves the issue of a PV hardware domain
> not getting RMRR mappings because {set/clear}_identity_p2m_entry was
> not properly updating the iommu page tables.
In which was this was not happening properly is still not really
understood, yet I think this (or at least a plausible theory) is
a prereq for any kind of fix. The code paths for PV and HVM are
probably different enough such that we don't need to be afraid
of there being a similar problem on the HVM side, but what if
there's a more general problem that we would better take care of,
rather perhaps than getting into a similarly puzzling situation
again later on.
> As rmrr_identity_mapping was the last user of
> {set/clear}_identity_p2m_entry against PV domains modify the function
> so it's only usable against translated domains, as the other p2m
> related functions.
Looking at the resulting code I'm actually not convinced that
this is a good move. I would, however, specifically like to
hear George's opinion here.
> @@ -1995,13 +1996,20 @@ static int rmrr_identity_mapping(struct domain *d,
> bool_t map,
>
> while ( base_pfn < end_pfn )
> {
> - if ( clear_identity_p2m_entry(d, base_pfn) )
> - ret = -ENXIO;
> + if ( paging_mode_translate(d) )
> + ret = clear_identity_p2m_entry(d, base_pfn);
> + else
> + ret = iommu_unmap(d, _dfn(base_pfn), PAGE_ORDER_4K,
> + &flush_flags);
> base_pfn++;
> }
>
> list_del(&mrmrr->list);
> xfree(mrmrr);
> + /* Keep the previous error code if there's one. */
> + err = iommu_iotlb_flush_all(d, flush_flags);
> + if ( !ret )
> + ret = err;
> return ret;
> }
> }
> @@ -2011,8 +2019,13 @@ static int rmrr_identity_mapping(struct domain *d,
> bool_t map,
>
> while ( base_pfn < end_pfn )
> {
> - int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
> + int err;
>
> + if ( paging_mode_translate(d) )
> + err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
> + else
> + err = iommu_map(d, _dfn(base_pfn), _mfn(base_pfn), PAGE_ORDER_4K,
> + IOMMUF_readable | IOMMUF_writable, &flush_flags);
> if ( err )
> return err;
> base_pfn++;
> @@ -2026,7 +2039,7 @@ static int rmrr_identity_mapping(struct domain *d,
> bool_t map,
> mrmrr->count = 1;
> list_add_tail(&mrmrr->list, &hd->arch.mapped_rmrrs);
>
> - return 0;
> + return iommu_iotlb_flush_all(d, flush_flags);
This is VT-d code - is there a particular reason why you go through
the generic code wrappers everywhere here?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
