Re: [PATCH 1/3] amd-vi: use the same IOMMU page table levels for PV and HVM

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Nov 20, 2023 at 12:34:45PM +0100, Jan Beulich wrote:
> On 20.11.2023 11:50, Roger Pau Monné wrote:
> > On Mon, Nov 20, 2023 at 11:37:43AM +0100, Jan Beulich wrote:
> >> On 20.11.2023 11:27, Roger Pau Monné wrote:
> >>> On Mon, Nov 20, 2023 at 10:45:29AM +0100, Jan Beulich wrote:
> >>>> On 17.11.2023 12:55, Andrew Cooper wrote:
> >>>>> On 17/11/2023 9:47 am, Roger Pau Monne wrote:
> >>>>>>      /*
> >>>>>> -     * Choose the number of levels for the IOMMU page tables.
> >>>>>> -     * - PV needs 3 or 4, depending on whether there is RAM 
> >>>>>> (including hotplug
> >>>>>> -     *   RAM) above the 512G boundary.
> >>>>>> -     * - HVM could in principle use 3 or 4 depending on how much guest
> >>>>>> -     *   physical address space we give it, but this isn't known yet 
> >>>>>> so use 4
> >>>>>> -     *   unilaterally.
> >>>>>> -     * - Unity maps may require an even higher number.
> >>>>>> +     * Choose the number of levels for the IOMMU page tables, taking 
> >>>>>> into
> >>>>>> +     * account unity maps.
> >>>>>>       */
> >>>>>> -    hd->arch.amd.paging_mode = max(amd_iommu_get_paging_mode(
> >>>>>> -            is_hvm_domain(d)
> >>>>>> -            ? 1UL << (DEFAULT_DOMAIN_ADDRESS_WIDTH - PAGE_SHIFT)
> >>>>>> -            : get_upper_mfn_bound() + 1),
> >>>>>> -        amd_iommu_min_paging_mode);
> >>>>>> +    hd->arch.amd.paging_mode = max(pgmode, amd_iommu_min_paging_mode);
> >>>>>
> >>>>> I think these min/max variables can be dropped now we're not doing
> >>>>> variable height IOMMU pagetables, which further simplifies this 
> >>>>> expression.
> >>>>
> >>>> Did you take unity maps into account? At least $subject and comment looks
> >>>> to not be consistent in this regard: Either unity maps need considering
> >>>> specially (and then we don't uniformly use the same depth), or they don't
> >>>> need mentioning in the comment (anymore).
> >>>
> >>> Unity maps that require an address width > DEFAULT_DOMAIN_ADDRESS_WIDTH
> >>> will currently only work on PV at best, as HVM p2m code is limited to
> >>> 4 level page tables, so even if the IOMMU page tables support a
> >>> greater address width the call to map such regions will trigger an
> >>> error in the p2m code way before attempting to create any IOMMU
> >>> mappings.
> >>>
> >>> We could do:
> >>>
> >>> hd->arch.amd.paging_mode =
> >>>     is_hvm_domain(d) ? pgmode : max(pgmode, amd_iommu_min_paging_mode);
> >>>
> >>> Putting IVMD/RMRR regions that require the usage of 5 level page
> >>> tables would be a very short sighted move by vendors IMO.
> >>>
> >>> And will put us back in a situation where PV vs HVM can get different
> >>> IOMMU page table levels, which is undesirable.  It might be better to
> >>> just assume all domains use DEFAULT_DOMAIN_ADDRESS_WIDTH and hide
> >>> devices that have IVMD/RMRR regions above that limit.
> >>
> >> That's a possible approach, yes. To be honest, I was actually hoping we'd
> >> move in a different direction: Do away with the entirely arbitrary
> >> DEFAULT_DOMAIN_ADDRESS_WIDTH, and use actual system properties instead.
> > 
> > Hm, yes, that might be a sensible approach, but right now I don't want
> > to block this series on such (likely big) piece of work.  I think we
> > should aim for HVM and PV to have the same IOMMU page table levels,
> > and that's currently limited by the p2m code only supporting 4 levels.
> 
> No, I certainly don't mean to introduce a dependency there. Yet what
> you do here goes actively against that possible movement in the other
> direction: What "actual system properties" are differs between PV and
> HVM (host properties vs guest properties), and hence there would
> continue to be a (possible) difference in depth between the two.

Might be.  Overall seems like more complexity for a little win.

The simplest option would be to unconditionally use the maximum page
table levels supported by both the CPU and the IOMMU.

> >> Whether having PV and HVM have uniform depth is indeed desirable is also
> >> not entirely obvious to me. Having looked over patch 3 now, it also
> >> hasn't become clear to me why the change here is actually a (necessary)
> >> prereq.
> > 
> > Oh, it's a prereq because I've found AMD systems that have reserved
> > regions > 512GB, but no RAM past that region.  arch_iommu_hwdom_init()
> > would fail on those systems when patch 3/3 was applied, as then
> > reserved regions past the last RAM address are also mapped in
> > arch_iommu_hwdom_init().
> 
> Hmm, interesting. I can't bring together "would fail" and "are also
> mapped" though, unless the latter was meant to say "are attempted to
> also be mapped", in which case I could at least see room for failure.

Yes, "are attempted to also be mapped", and that attempt fails.  I
would assume that "would fail" was already connected to "also mapped",
but maybe it's not clear enough.

> Yet still this would then feel like an issue with the last patch alone,
> which the change here is merely avoiding (without this being a strict
> prereq). Instead I'd expect us to use 4 levels whenever there are any
> kind of regions (reserved or not) above 512G. Without disallowing use
> of 3 levels on other (smaller) systems.

While reserved regions are the ones that made me realize about this
IOMMU page table difference, what about device MMIO regions?

There's no limitation that avoids MMIO regions from living past the
last RAM address, and possibly above the 512GB mark.

If anything for PV we should limit page table levels based on the
supported paddr bits reported by the CPU, but limiting it based on the
memory map seems plain bogus.

Thanks, Roger.