[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()"
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.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |