On Wed, May 18, 2022 at 12:40:59PM +0200, Jan Beulich wrote:
> On 10.05.2022 17:31, Roger Pau Monné wrote:
> > On Mon, Apr 25, 2022 at 10:43:16AM +0200, Jan Beulich wrote:
> >> @@ -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?
>
> Yes, except that I wouldn't use "must", but "would". It's not a
> requirement after all, functionality-wise all will be fine without
> re-coalescing.
>
> > 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.
>
> I'm not convinced, as there's effectively only one caller,
> amd_iommu_map_page(). I also don't see why "no change" would be a
> problem. "No change" can't result in a fully contiguous page table
> if the page table wasn't fully contiguous already before (at which
> point it would have been replaced by a superpage).
Right, I agree, it's just that I would have preferred the result from
set_iommu_ptes_present() to be consistent, ie: repeated calls to it
using the same PTE should set contig to the same value. Anyway,
that's not relevant to any current callers, so:
Reviewed-by: Roger Pau Monné <[email protected]>
Thanks, Roger.
