Re: [PATCH v2 01/18] AMD/IOMMU: have callers specify the target level for page table walks

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.09.2021 12:58, Roger Pau Monné wrote:
> On Fri, Sep 24, 2021 at 11:41:14AM +0200, Jan Beulich wrote:
>> --- a/xen/drivers/passthrough/amd/iommu_map.c
>> +++ b/xen/drivers/passthrough/amd/iommu_map.c
>> @@ -178,7 +178,8 @@ void __init iommu_dte_add_device_entry(s
>>   * page tables.
>>   */
>>  static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
>> -                              unsigned long *pt_mfn, bool map)
>> +                              unsigned int target, unsigned long *pt_mfn,
>> +                              bool map)
>>  {
>>      union amd_iommu_pte *pde, *next_table_vaddr;
>>      unsigned long  next_table_mfn;
>> @@ -189,7 +190,8 @@ static int iommu_pde_from_dfn(struct dom
>>      table = hd->arch.amd.root_table;
>>      level = hd->arch.amd.paging_mode;
>>  
>> -    BUG_ON( table == NULL || level < 1 || level > 6 );
>> +    if ( !table || target < 1 || level < target || level > 6 )
>> +        return 1;
> 
> I would consider adding an ASSERT_UNREACHABLE here, since there should
> be no callers passing those parameters, and we shouldn't be
> introducing new ones. Unless you believe there could be valid callers
> passing level < target parameter.

Ah yes - added.

>> @@ -200,7 +202,7 @@ static int iommu_pde_from_dfn(struct dom
>>  
>>      next_table_mfn = mfn_x(page_to_mfn(table));
>>  
>> -    while ( level > 1 )
>> +    while ( level > target )
>>      {
>>          unsigned int next_level = level - 1;
> 
> There's a comment at the bottom of iommu_pde_from_dfn that needs to be
> adjusted to no longer explicitly mention level 1.

Oh, thanks for noticing. I recall spotting that comment as in
need of updating before starting any of this work. And then I
forgot ...

> With that adjusted:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

> FWIW, I always get confused with AMD and shadow code using level 1 to
> denote the smaller page size level while Intel uses 0.

Wait - with "Intel" you mean just EPT here, don't you? VT-d
code is using 1-based numbering again from all I can tell.

Jan