[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] xen/mm: Alter is_iomem_page() to use mfn_t
>>> On 09.02.17 at 09:27, <chao.gao@xxxxxxxxx> wrote: > On Wed, Feb 08, 2017 at 10:05:38AM -0700, Jan Beulich wrote: >>>>> On 08.02.17 at 08:31, <kevin.tian@xxxxxxxxx> wrote: >>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx] >>>> Sent: Monday, February 06, 2017 11:38 PM >>>> >>>> >>> On 06.02.17 at 15:48, <dwmw2@xxxxxxxxxxxxx> wrote: >>>> > On Mon, 2017-02-06 at 07:26 -0700, Jan Beulich wrote: >>>> >> > >>>> >> > > >>>> >> > > > >>>> >> > > > On 06.02.17 at 14:55, <andrew.cooper3@xxxxxxxxxx> wrote: >>>> >> > Switch its return type to bool to match its use, and simplify the >>>> >> > ARM >>>> >> > implementation slightly. >>>> >> > >>>> >> > No functional change. >>>> >> > >>>> >> > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> >>>> >> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> >>>> >> >>>> >> And perhaps that's what should be used in epte_get_entry_emt() >>>> >> instead of !mfn_valid() in David's patch. David, would you be okay >>>> >> with your patch changed to that effect upon commit? >>>> > >>>> > I don't think that works, at least not literally >>>> > s/!mfn_valid()/is_iomem_page()/ >>>> > >>>> > In my patch, mfn_valid() is checked *after* we've processed the >>>> > 'direct_mmio' case that all MMIO should hit. In a sane world I think >>>> > it's *only* actually catching INVALID_MFN, and probably should never >>>> > match on any other value of mfn. >>>> > >>>> > I don't quite understand why we pass 'direct_mmio' in as a separate >>>> > argument. Perhaps there's scope for doing a sanity check that >>>> > 'direct_mmio == is_iomem_page(mfn)' — because when would that *not* be >>>> > true? >>>> >>>> Likely never. The reasons for why this was done the way it is >>>> escape me. Adding VMX maintainers once again. >>> >>> I'm not the original author of that code. Here is what I found from the >>> code: >>> >>> There are two places caring direct_mmio: resolve_misconfig and ept_ >>> set_entry. It's decided upon p2m type: >>> >>> int emt = epte_get_entry_emt(p2m->domain, gfn, _mfn(e.mfn), >>> level * EPT_TABLE_ORDER, &ipat, >>> e.sa_p2mt == p2m_mmio_direct); >>> >>> I'm not sure whether p2m_mmio_direct always makes is_iomem_page >>> true. If true then yes we may not require this parameter at all. >> >>From an abstract perspective the two should always correlate. We >>may want to take an intermediate step though and first only add >>checking, and perhaps a release later remove the extra parameter >>if the check never triggered for anyone. >> > > I add one assertion to the original code, like below. > > diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c > index 86c71be..3d9386a 100644 > --- a/xen/arch/x86/hvm/mtrr.c > +++ b/xen/arch/x86/hvm/mtrr.c > @@ -784,6 +784,8 @@ int epte_get_entry_emt(struct domain *d, unsigned long > gfn, > > if ( direct_mmio ) > { > + ASSERT(direct_mmio == is_iomem_page(mfn)); > + > if ( (mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >> order ) > return MTRR_TYPE_UNCACHABLE; > if ( order ) > > But when I try to create a hvm domain, it failed. logs are below. Considering the context of the change above, I'm not surprised: You've very likely hit the APIC access MFN. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |