Re: [PATCH v4 17/21] AMD/IOMMU: replace all-contiguous page tables by superpage mappings

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

On Mon, Apr 25, 2022 at 10:43:16AM +0200, Jan Beulich wrote:
> When a page table ends up with all contiguous entries (including all
> identical attributes), it can be replaced by a superpage entry at the
> next higher level. The page table itself can then be scheduled for
> freeing.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Unlike the freeing of all-empty page tables, this causes quite a bit of
> back and forth for PV domains, due to their mapping/unmapping of pages
> when they get converted to/from being page tables. It may therefore be
> worth considering to delay re-coalescing a little, to avoid doing so
> when the superpage would otherwise get split again pretty soon. But I
> think this would better be the subject of a separate change anyway.
> 
> Of course this could also be helped by more "aware" kernel side
> behavior: They could avoid immediately mapping freed page tables
> writable again, in anticipation of re-using that same page for another
> page table elsewhere.
> ---
> v4: Re-base over changes earlier in the series.
> v3: New.
> 
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -81,7 +81,8 @@ static union amd_iommu_pte set_iommu_pte
>                                                   unsigned long dfn,
>                                                   unsigned long next_mfn,
>                                                   unsigned int level,
> -                                                 bool iw, bool ir)
> +                                                 bool iw, bool ir,
> +                                                 bool *contig)
>  {
>      union amd_iommu_pte *table, *pde, old;
>  
> @@ -94,11 +95,15 @@ static union amd_iommu_pte set_iommu_pte
>           old.iw != iw || old.ir != ir )
>      {
>          set_iommu_pde_present(pde, next_mfn, 0, iw, ir);
> -        pt_update_contig_markers(&table->raw, pfn_to_pde_idx(dfn, level),
> -                                 level, PTE_kind_leaf);
> +        *contig = pt_update_contig_markers(&table->raw,
> +                                           pfn_to_pde_idx(dfn, level),
> +                                           level, PTE_kind_leaf);
>      }
>      else
> +    {
>          old.pr = false; /* signal "no change" to the caller */
> +        *contig = false;

So we assume that any caller getting contig == true must have acted
and coalesced the page table?

Might be worth a comment, to note that the function assumes that a
previous return of contig == true will have coalesced the page table
and hence a "no change" PTE write is not expected to happen on a
contig page table.

Thanks, Roger.