On Wed, Jun 01, 2022 at 09:10:09AM +0200, Jan Beulich wrote:
> On 31.05.2022 18:15, Roger Pau Monné wrote:
> > On Tue, May 31, 2022 at 05:40:03PM +0200, Jan Beulich wrote:
> >> On 31.05.2022 16:40, Roger Pau Monné wrote:
> >>> On Fri, May 27, 2022 at 01:12:06PM +0200, Jan Beulich wrote:
> >>>> @@ -289,44 +290,75 @@ static bool __hwdom_init hwdom_iommu_map
> >>>> * that fall in unusable ranges for PV Dom0.
> >>>> */
> >>>> if ( (pfn > max_pfn && !mfn_valid(mfn)) || xen_in_range(pfn) )
> >>>> - return false;
> >>>> + return 0;
> >>>>
> >>>> switch ( type = page_get_ram_type(mfn) )
> >>>> {
> >>>> case RAM_TYPE_UNUSABLE:
> >>>> - return false;
> >>>> + return 0;
> >>>>
> >>>> case RAM_TYPE_CONVENTIONAL:
> >>>> if ( iommu_hwdom_strict )
> >>>> - return false;
> >>>> + return 0;
> >>>> break;
> >>>>
> >>>> default:
> >>>> if ( type & RAM_TYPE_RESERVED )
> >>>> {
> >>>> if ( !iommu_hwdom_inclusive && !iommu_hwdom_reserved )
> >>>> - return false;
> >>>> + perms = 0;
> >>>> }
> >>>> - else if ( is_hvm_domain(d) || !iommu_hwdom_inclusive || pfn >
> >>>> max_pfn )
> >>>> - return false;
> >>>> + else if ( is_hvm_domain(d) )
> >>>> + return 0;
> >>>> + else if ( !iommu_hwdom_inclusive || pfn > max_pfn )
> >>>> + perms = 0;
> >>>> }
> >>>>
> >>>> /* Check that it doesn't overlap with the Interrupt Address Range.
> >>>> */
> >>>> if ( pfn >= 0xfee00 && pfn <= 0xfeeff )
> >>>> - return false;
> >>>> + return 0;
> >>>> /* ... or the IO-APIC */
> >>>> - for ( i = 0; has_vioapic(d) && i < d->arch.hvm.nr_vioapics; i++ )
> >>>> - if ( pfn == PFN_DOWN(domain_vioapic(d, i)->base_address) )
> >>>> - return false;
> >>>> + if ( has_vioapic(d) )
> >>>> + {
> >>>> + for ( i = 0; i < d->arch.hvm.nr_vioapics; i++ )
> >>>> + if ( pfn == PFN_DOWN(domain_vioapic(d, i)->base_address) )
> >>>> + return 0;
> >>>> + }
> >>>> + else if ( is_pv_domain(d) )
> >>>> + {
> >>>> + /*
> >>>> + * Be consistent with CPU mappings: Dom0 is permitted to
> >>>> establish r/o
> >>>> + * ones there (also for e.g. HPET in certain cases), so it
> >>>> should also
> >>>> + * have such established for IOMMUs.
> >>>> + */
> >>>> + if ( iomem_access_permitted(d, pfn, pfn) &&
> >>>> + rangeset_contains_singleton(mmio_ro_ranges, pfn) )
> >>>> + perms = IOMMUF_readable;
> >>>> + }
> >>>> /*
> >>>> * ... or the PCIe MCFG regions.
> >>
> >> With this comment (which I leave alone) ...
> >>
> >>>> * TODO: runtime added MMCFG regions are not checked to make sure
> >>>> they
> >>>> * don't overlap with already mapped regions, thus preventing
> >>>> trapping.
> >>>> */
> >>>> if ( has_vpci(d) && vpci_is_mmcfg_address(d, pfn_to_paddr(pfn)) )
> >>>> - return false;
> >>>> + return 0;
> >>>> + else if ( is_pv_domain(d) )
> >>>> + {
> >>>> + /*
> >>>> + * Don't extend consistency with CPU mappings to PCI MMCFG
> >>>> regions.
> >>>> + * These shouldn't be accessed via DMA by devices.
> >>>
> >>> Could you expand the comment a bit to explicitly mention the reason
> >>> why MMCFG regions shouldn't be accessible from device DMA operations?
> >>
> >> ... it's hard to tell what I should write here. I'd expect extended
> >> reasoning to go there (if anywhere). I'd be okay adjusting the earlier
> >> comment, if only I knew what to write. "We don't want them to be
> >> accessed that way" seems a little blunt. I could say "Devices have
> >> other means to access PCI config space", but this not being said there
> >> I took as being implied.
> >
> > But we could likely say the same about IO-APIC or HPET MMIO regions.
> > I don't think we expect them to be accessed by devices, yet we provide
> > them for coherency with CPU side mappings in the PV case.
>
> As to "say the same" - yes for the first part of my earlier reply, but
> no for the latter part.
Yes, obviously devices cannot access the HPET or the IO-APIC MMIO from
the PCI config space :).
> >> Or else what was the reason to exclude these
> >> for PVH Dom0?
> >
> > The reason for PVH is because the config space is (partially) emulated
> > for the hardware domain, so we don't allow untrapped access by the CPU
> > either.
>
> Hmm, right - there's read emulation there as well, while for PV we
> only intercept writes.
>
> So overall should we perhaps permit r/o access to MMCFG for PV? Of
> course that would only end up consistent once we adjust mappings
> dynamically when MMCFG ranges are put in use (IOW if we can't verify
> an MMCFG range is suitably reserved, we'd not find in
> mmio_ro_ranges just yet, and hence we still wouldn't have an IOMMU
> side mapping even if CPU side mappings are permitted). But for the
> patch here it would simply mean dropping some of the code I did add
> for v5.
I would be OK with that, as I think we would then be consistent with
how IO-APIC and HPET MMIO regions are handled. We would have to add
some small helper/handling in PHYSDEVOP_pci_mmcfg_reserved for PV.
> Otherwise, i.e. if the code is to remain as is, I'm afraid I still
> wouldn't see what to put usefully in the comment.
IMO the important part is to note whether there's a reason or not why
the handling of IO-APIC, HPET vs MMCFG RO regions differ in PV mode.
Ie: if we don't want to handle MMCFG in RO mode for device mappings
because of the complication with handling dynamic changes as a result
of PHYSDEVOP_pci_mmcfg_reserved we should just note it.
Thanks, Roger.
