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.
