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. 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. Jan
