diff mbox series

Revert "x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()"

Message ID 20200907170916.61693-1-roger.pau@citrix.com
State New
Headers show
Series Revert "x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()" | expand

Commit Message

Roger Pau Monné Sept. 7, 2020, 5:09 p.m. UTC
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'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@citrix.com>
---
Cc: Paul Durrant <paul@xen.org>
---
 xen/arch/x86/hvm/mtrr.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

Paul Durrant Sept. 8, 2020, 6:47 a.m. UTC | #1
> -----Original Message-----
> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: 07 September 2020 18:09
> To: xen-devel@lists.xenproject.org
> Cc: Roger Pau Monne <roger.pau@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>
> 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@citrix.com>
> ---
> Cc: Paul Durrant <paul@xen.org>
> ---
>  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
Roger Pau Monné Sept. 8, 2020, 7:45 a.m. UTC | #2
On Tue, Sep 08, 2020 at 07:47:09AM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Roger Pau Monne <roger.pau@citrix.com>
> > Sent: 07 September 2020 18:09
> > To: xen-devel@lists.xenproject.org
> > Cc: Roger Pau Monne <roger.pau@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> > <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>
> > 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@citrix.com>
> > ---
> > Cc: Paul Durrant <paul@xen.org>
> > ---
> >  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:

ACPI identity mapped regions in a PVH dom0 don't even get there since
they are handled by the 'if ( direct_mmio )' chunk above.

> +    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?

No, because on a PVH dom0 ACPI regions are MMIO identity mapped, and
thus direct_mmio is true for them, and hence they get handled by the
first chunk that the original patch removed.

> 
> 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.

domUs don't have ACPI tables mapped as MMIO, as the ACPI tables in
that case are created by the toolstack and reside in a RAM region.

I'm not completely convinced that using UC unconditionally for ACPI
identity mapped regions is fully correct, I will think of a better way
to handled them but in the meantime we need to keep this working.

Thanks, Roger.
Jan Beulich Sept. 8, 2020, 8:55 a.m. UTC | #3
On 07.09.2020 19:09, Roger Pau Monne wrote:
> 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.

Why is this wrong? Xen heap pages are RAM ones. Quite the opposite,
_not_ forcing them to WB is going to cause cachability conflicts,
unless their direct mapping entries also got changed to UC (which I
think we want to avoid if at all possible). The cachability we
select here is the one to be used by the hardware, not the one
perceived by a domain (be it Dom0 or a DomU).

> I've discovered this while trying to boot a PVH dom0, which fail to
> boot with this commit applied.

And what exactly is the deeper reason of the failure?

Jan
diff mbox series

Patch

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;
@@ -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 )
     {