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....@citrix.com>
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
