[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: <paul@xxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Tue, 8 Sep 2020 09:45:51 +0200
  • Authentication-results: esa4.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none
  • Cc: <xen-devel@xxxxxxxxxxxxxxxxxxxx>, 'Jan Beulich' <jbeulich@xxxxxxxx>, 'Andrew Cooper' <andrew.cooper3@xxxxxxxxxx>, 'Wei Liu' <wl@xxxxxxx>
  • Delivery-date: Tue, 08 Sep 2020 07:46:34 +0000
  • Ironport-sdr: sWM6UpE9DITOmZKbZy+Za8PK+3R8Sy8pZljeTem4b8H5BQz2elaVaFS+lpx9pEz3UqhuB2hDjV uOXwD1N3VPKza8zHDM6mnm9wrp0pH9voX/5RAD83AzHvPjLkSfh7ZegiV78M7/pLwFlcdvSd3m /+dAP6eL2y5hACjF6oZQWp3oyD+5C+UiOp03n5vE3/v1EF+OT0p2W3ruiIKqzLODR9iSKCAuG4 rWh2Buo10ZKPmG5htV480C/R7LSTvWgkOUv2VpVZNiU7dWTGgJ0luAqueaQ+Hw8AleaTQwYtfM APw=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Tue, Sep 08, 2020 at 07:47:09AM +0100, Paul Durrant wrote:
> > -----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:

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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.