[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 5/6] x86/iommu: switch hwdom IOMMU to use a rangeset
On Tue, Dec 05, 2023 at 04:47:15PM +0100, Jan Beulich wrote: > On 05.12.2023 16:43, Roger Pau Monné wrote: > > On Tue, Dec 05, 2023 at 04:27:05PM +0100, Jan Beulich wrote: > >> On 04.12.2023 10:43, Roger Pau Monne wrote: > >>> @@ -476,58 +406,55 @@ void __hwdom_init arch_iommu_hwdom_init(struct > >>> domain *d) > >>> if ( !map ) > >>> panic("IOMMU init: unable to allocate rangeset\n"); > >>> > >>> - max_pfn = (GB(4) >> PAGE_SHIFT) - 1; > >>> - top = max(max_pdx, pfn_to_pdx(max_pfn) + 1); > >>> + if ( iommu_hwdom_inclusive ) > >>> + { > >>> + /* Add the whole range below 4GB, UNUSABLE regions will be > >>> removed. */ > >>> + rc = rangeset_add_range(map, 0, max_pfn); > >>> + if ( rc ) > >>> + panic("IOMMU inclusive mappings can't be added: %d\n", > >>> + rc); > >>> + } > >>> > >>> - for ( i = 0, start = 0, count = 0; i < top; ) > >>> + for ( i = 0; i < e820.nr_map; i++ ) > >>> { > >>> - unsigned long pfn = pdx_to_pfn(i); > >>> - unsigned int perms = hwdom_iommu_map(d, pfn, max_pfn); > >>> + struct e820entry entry = e820.map[i]; > >>> > >>> - if ( !perms ) > >>> - /* nothing */; > >>> - else if ( paging_mode_translate(d) ) > >>> + switch ( entry.type ) > >>> { > >>> - int rc; > >>> + case E820_UNUSABLE: > >>> + if ( !iommu_hwdom_inclusive || PFN_DOWN(entry.addr) > > >>> max_pfn ) > >>> + continue; > >> > >> The !iommu_hwdom_inclusive part isn't really needed here, is it? The ... > > > > Nor the PFN_DOWN(entry.addr) > max_pfn. > > Hmm, I couldn't convince myself that could also be dropped. We never map unusable regions, so it's always fine to remove them from the rangeset. The condition was just a way to exit early and avoid the rangeset_remove_range() call. > >>> - rc = p2m_add_identity_entry(d, pfn, > >>> - perms & IOMMUF_writable ? > >>> p2m_access_rw > >>> - : > >>> p2m_access_r, > >>> - 0); > >>> + rc = rangeset_remove_range(map, PFN_DOWN(entry.addr), > >>> + PFN_DOWN(entry.addr + entry.size > >>> - 1)); > >> > >> ... call here would then simply be a no-op, as it looks. And things would > >> overall look more safe if the removal was skipped for fewer reasons. > > > > OK, the removal can be done unconditionally if so desired. > > > >>> @@ -605,7 +532,7 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain > >>> *d) > >>> rangeset_destroy(map); > >>> > >>> /* Use if to avoid compiler warning */ > >>> - if ( iommu_iotlb_flush_all(d, flush_flags) ) > >>> + if ( iommu_iotlb_flush_all(d, map_data.flush_flags) ) > >>> return; > >>> } > >> > >> Ah yes, here is said change. But I think for correctness this wants > >> moving to the earlier patch. > > > > OK, so something like: > > > > map_data.flush_flags |= flush_flags; > > Or simply drop flush_flags here right away (read: replace by map.flush_flags). Right, OK, that will lead to some small changes to existing code which I wanted to avoid in the context of just adding new code to deal with a rangeset, but anyway, if that's preferred I will adjust. Thanks, Roger.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |