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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Sep 24, 2021 at 11:41:14AM +0200, Jan Beulich wrote:
> In order to be able to insert/remove super-pages we need to allow
> callers of the walking function to specify at which point to stop the
> walk. (For now at least gcc will instantiate just a variant of the
> function with the parameter eliminated, so effectively no change to
> generated code as far as the parameter addition goes.)
> 
> Instead of merely adjusting a BUG_ON() condition, convert it into an
> error return - there's no reason to crash the entire host in that case.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- 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.

>  
>      /*
>       * A frame number past what the current page tables can represent can't
> @@ -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.

With that adjusted:

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

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

Thanks, Roger.