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

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: <paul@xen.org>
Cc: <xen-devel@lists.xenproject.org>,
	'Jan Beulich' <jbeulich@suse.com>,
	'Andrew Cooper' <andrew.cooper3@citrix.com>,
	'Wei Liu' <wl@xen.org>
Subject: Re: [PATCH] Revert "x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()"
Date: Tue, 8 Sep 2020 09:45:51 +0200	[thread overview]
Message-ID: <20200908074551.GT753@Air-de-Roger> (raw)
In-Reply-To: <004801d685ab$e0452ce0$a0cf86a0$@xen.org>

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.