Re: [PATCH v2 2/6] amd-vi: set IOMMU page table levels based on guest reported paddr width

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.12.2023 16:11, Roger Pau Monné wrote:
> On Tue, Dec 05, 2023 at 03:32:20PM +0100, Jan Beulich wrote:
>> On 04.12.2023 10:43, Roger Pau Monne wrote:
>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>> @@ -359,21 +359,17 @@ int __read_mostly amd_iommu_min_paging_mode = 1;
>>>  static int cf_check amd_iommu_domain_init(struct domain *d)
>>>  {
>>>      struct domain_iommu *hd = dom_iommu(d);
>>> +    int pglvl = amd_iommu_get_paging_mode(
>>> +                PFN_DOWN(1UL << paging_max_paddr_bits(d)));
>>
>> This is a function in the paging subsystem, i.e. generally inapplicable
>> to system domains (specifically DomIO). If this is to remain this way,
>> the function would imo need to gain a warning. Yet better would imo be
>> if the function was avoided for system domains.
> 
> I have to admit I'm confused, won't systems domains return
> paging_mode_hap(d) == false, and thus fallback to using paddr_bits
> (host paddr width?).

True, but that check lives inside the function.

> I can avoid such domains calling into paging_max_paddr_bits() but it
> seems redundant, and would just be duplicated logic for a case that
> paging_max_paddr_bits() already handles correctly AFAICT.

Hence why I suggested a comment (warning) as alternative.

> Would it be better for me to rename paging_max_paddr_bits() to
> domain_max_paddr_bits() and move it to asm/domain.h?

Maybe. I'm not sure exactly why the function was introduced where it
is and under the name it has. It sole present caller is in cpu-policy.c,
so either name/placement would look good to me.

Jan