RE: [PATCH] Revert "x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()"

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Sent: 07 September 2020 18:09
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; 
> Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Paul Durrant <paul@xxxxxxx>
> Subject: [PATCH] Revert "x86/hvm: simplify 'mmio_direct' check in 
> epte_get_entry_emt()"
> 
> This reverts commit 81fd0d3ca4b2cd309403c6e8da662c325dd35750.
> 
> Original commit only takes into account the APIC access page being a
> 'special' page, but when running a PVH dom0 there are other pages that
> also fulfill the 'special' page check but shouldn't have it's cache
> type set to WB.
> 
> For example the ACPI regions are identity mapped into the guest but
> are also Xen heap pages, and forcing those to WB cache type is wrong.

I don't understand why reversion of this patch alone is sufficient then...
 
> 
> I've discovered this while trying to boot a PVH dom0, which fail to
> boot with this commit applied.
> 
> Revert the commit while this is sorted out: either we settle that the
> current code is correct, or we modify the way ACPI regions are mapped
> into a PVH dom0.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Cc: Paul Durrant <paul@xxxxxxx>
> ---
>  xen/arch/x86/hvm/mtrr.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> index fb051d59c3..2bd64e8025 100644
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -815,13 +815,23 @@ int epte_get_entry_emt(struct domain *d, unsigned long 
> gfn, mfn_t mfn,
>          return -1;
>      }
> 
> +    if ( direct_mmio )
> +    {
> +        if ( (mfn_x(mfn) ^ mfn_x(d->arch.hvm.vmx.apic_access_mfn)) >> order )
> +            return MTRR_TYPE_UNCACHABLE;
> +        if ( order )
> +            return -1;
> +        *ipat = 1;
> +        return MTRR_TYPE_WRBACK;
> +    }
> +
>      if ( !mfn_valid(mfn) )
>      {
>          *ipat = 1;
>          return MTRR_TYPE_UNCACHABLE;
>      }
> 
> -    if ( !direct_mmio && !is_iommu_enabled(d) && !cache_flush_permitted(d) )
> +    if ( !is_iommu_enabled(d) && !cache_flush_permitted(d) )
>      {
>          *ipat = 1;
>          return MTRR_TYPE_WRBACK;

... since just below this hunk, commit ca24b2ffdbd9 "x86/hvm: set 'ipat' in EPT 
for special pages" introduced the check:

+    for ( i = 0; i < (1ul << order); i++ )
+    {
+        if ( is_special_page(mfn_to_page(mfn_add(mfn, i))) )
+        {
+            if ( order )
+                return -1;
+            *ipat = 1;
+            return MTRR_TYPE_WRBACK;
+        }
+    }
+

So, would that not be catching the ACPI regions?

Also, why is it ok for ACPI regions to be WB if the IOMMU is not enabled? I 
know that this will never be the case for PVH dom0 but why do domUs also have 
to suffer? I think we need a more targeted patch.

  Paul

> @@ -838,9 +848,6 @@ int epte_get_entry_emt(struct domain *d, unsigned long 
> gfn, mfn_t mfn,
>          }
>      }
> 
> -    if ( direct_mmio )
> -        return MTRR_TYPE_UNCACHABLE;
> -
>      gmtrr_mtype = hvm_get_mem_pinned_cacheattr(d, _gfn(gfn), order);
>      if ( gmtrr_mtype >= 0 )
>      {
> --
> 2.28.0