On Wed, May 04, 2022 at 02:12:58PM +0200, Jan Beulich wrote:
> On 04.05.2022 14:01, Roger Pau Monné wrote:
> > On Wed, May 04, 2022 at 12:51:25PM +0200, Jan Beulich wrote:
> >> On 04.05.2022 12:30, Roger Pau Monné wrote:
> >>> Right, ->iomem_caps is indeed too wide for our purpose. What
> >>> about using something like:
> >>>
> >>> else if ( is_pv_domain(d) )
> >>> {
> >>> if ( !iomem_access_permitted(d, pfn, pfn) )
> >>> return 0;
> >>
> >> We can't return 0 here (as RAM pages also make it here when
> >> !iommu_hwdom_strict), so I can at best take this as a vague outline
> >> of what you really mean. And I don't want to rely on RAM pages being
> >> (imo wrongly) represented by set bits in Dom0's iomem_caps.
> >
> > Well, yes, my suggestion was taking into account that ->iomem_caps for
> > dom0 has mostly holes for things that shouldn't be mapped, but
> > otherwise contains everything else as allowed (including RAM).
> >
> > We could instead do:
> >
> > else if ( is_pv_domain(d) && type != RAM_TYPE_CONVENTIONAL )
> > {
> > ...
> >
> > So that we don't rely on RAM being 'allowed' in ->iomem_caps?
>
> This would feel to me like excess special casing.
What about placing this in the 'default:' label on the type switch a
bit above?
> >>> if ( rangeset_contains_singleton(mmio_ro_ranges, pfn) )
> >>> return IOMMUF_readable;
> >>> }
> >>>
> >>> That would get us a bit closer to allowed CPU side mappings, and we
> >>> don't need to special case IO-APIC or HPET addresses as those are
> >>> already added to ->iomem_caps or mmio_ro_ranges respectively by
> >>> dom0_setup_permissions().
> >>
> >> This won't fit in a region of code framed by a (split) comment
> >> saying "Check that it doesn't overlap with ...". Hence if anything
> >> I could put something like this further down. Yet even then the
> >> question remains what to do with ranges which pass
> >> iomem_access_permitted() but
> >> - aren't really MMIO,
> >> - are inside MMCFG,
> >> - are otherwise special.
> >>
> >> Or did you perhaps mean to suggest something like
> >>
> >> else if ( is_pv_domain(d) && iomem_access_permitted(d, pfn, pfn) &&
> >> rangeset_contains_singleton(mmio_ro_ranges, pfn) )
> >> return IOMMUF_readable;
> >
> > I don't think this would be fully correct, as we would still allow
> > mappings of IO-APIC pages explicitly banned in ->iomem_caps by not
> > handling those?
>
> CPU side mappings don't deal with the IO-APICs specifically. They only
> care about iomem_caps and mmio_ro_ranges. Hence explicitly banned
> IO-APIC pages cannot be mapped there either. (Of course we only do
> such banning if IO-APIC pages weren't possible to represent in
> mmio_ro_ranges, which should effectively be never.)
I think I haven't expressed myself correctly.
This construct won't return 0 for pfns not in iomem_caps, and hence
could allow mapping of addresses not in iomem_caps?
> >> ? Then there would only remain the question of whether mapping r/o
> >> MMCFG pages is okay (I don't think it is), but that could then be
> >> special-cased similar to what's done further down for vPCI (by not
> >> returning in the "else if", but merely updating "perms").
> >
> > Well part of the point of this is to make CPU and Device mappings
> > more similar. I don't think devices have any business in poking at
> > the MMCFG range, so it's fine to explicitly ban that range. But I
> > would have also said the same for IO-APIC pages, so I'm unsure why are
> > IO-APIC pages fine to be mapped RO, but not the MMCFG range.
>
> I wouldn't have wanted to allow r/o mappings of the IO-APICs, but
> Linux plus the ACPI tables of certain vendors require us to permit
> this. If we didn't, Dom0 would crash there during boot.
Right, but those are required for the CPU only. I think it's a fine
goal to try to have similar mappings for CPU and Devices, and then
that would also cover MMCFG in the PV case. Or else it fine to assume
CPU vs Device mappings will be slightly different, and then don't add
any mappings for IO-APIC, HPET or MMCFG to the Device page tables
(likely there's more that could be added here).
Thanks, Roger.
