The initial if() was inverted, invalidating all output from this
function. Which in turn means the mirroring of P2M mappings into the
IOMMU didn't always work as intended: Mappings may have got updated when
there was no need to. There would not have been too few (un)mappings;
what saves us is that alongside the flags comparison MFNs also get
compared, with non-present entries always having an MFN of 0 or
INVALID_MFN while present entries always have MFNs different from these
two (0 in the table also meant to cover INVALID_MFN):

OLD					NEW
P W	access	MFN			P W	access	MFN
0 0	r	0			0 0	n	0
0 1	rw	0			0 1	n	0
1 0	n	non-0			1 0	r	non-0
1 1	n	non-0			1 1	rw	non-0

present <-> non-present transitions are fine because the MFNs differ.
present -> present transitions as well as non-present -> non-present
ones are potentially causing too many map/unmap operations, but never
too few, because in that case old (bogus) and new access differ.

Fixes: d1bb6c97c31e ("IOMMU: also pass p2m_access_t to p2m_get_iommu_flags())
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -548,7 +548,7 @@ int p2m_pt_handle_deferred_changes(uint6
 /* Reconstruct a fake p2m_access_t from stored PTE flags. */
 static p2m_access_t p2m_flags_to_access(unsigned int flags)
 {
-    if ( flags & _PAGE_PRESENT )
+    if ( !(flags & _PAGE_PRESENT) )
         return p2m_access_n;
 
     /* No need to look at _PAGE_NX for now. */

On 02/09/2021 08:01, Jan Beulich wrote:
> The initial if() was inverted, invalidating all output from this
> function. Which in turn means the mirroring of P2M mappings into the
> IOMMU didn't always work as intended: Mappings may have got updated when
> there was no need to. There would not have been too few (un)mappings;
> what saves us is that alongside the flags comparison MFNs also get
> compared, with non-present entries always having an MFN of 0 or
> INVALID_MFN while present entries always have MFNs different from these
> two (0 in the table also meant to cover INVALID_MFN):
>
> OLD					NEW
> P W	access	MFN			P W	access	MFN
> 0 0	r	0			0 0	n	0
> 0 1	rw	0			0 1	n	0
> 1 0	n	non-0			1 0	r	non-0
> 1 1	n	non-0			1 1	rw	non-0
>
> present <-> non-present transitions are fine because the MFNs differ.
> present -> present transitions as well as non-present -> non-present
> ones are potentially causing too many map/unmap operations, but never
> too few, because in that case old (bogus) and new access differ.
>
> Fixes: d1bb6c97c31e ("IOMMU: also pass p2m_access_t to p2m_get_iommu_flags())
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -548,7 +548,7 @@ int p2m_pt_handle_deferred_changes(uint6
>  /* Reconstruct a fake p2m_access_t from stored PTE flags. */
>  static p2m_access_t p2m_flags_to_access(unsigned int flags)
>  {
> -    if ( flags & _PAGE_PRESENT )
> +    if ( !(flags & _PAGE_PRESENT) )
>          return p2m_access_n;
>  
>      /* No need to look at _PAGE_NX for now. */
>