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

[Jan Beulich <jbeulich@xxxxxxxx>]

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