| Message ID | 20170209082738.GA15352@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 09, 2017 at 01:56:14AM -0700, Jan Beulich wrote: >>>> On 09.02.17 at 09:27, <chao.gao@intel.com> wrote: >> On Wed, Feb 08, 2017 at 10:05:38AM -0700, Jan Beulich wrote: >>>>>> On 08.02.17 at 08:31, <kevin.tian@intel.com> wrote: >>>>> From: Jan Beulich [mailto:JBeulich@suse.com] >>>>> Sent: Monday, February 06, 2017 11:38 PM >>>>> >>>>> >>> On 06.02.17 at 15:48, <dwmw2@infradead.org> wrote: >>>>> > On Mon, 2017-02-06 at 07:26 -0700, Jan Beulich wrote: >>>>> >> > >>>>> >> > > >>>>> >> > > > >>>>> >> > > > On 06.02.17 at 14:55, <andrew.cooper3@citrix.com> 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@citrix.com> >>>>> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>>>> >> >>>>> >> 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 is right. When the assertion fails, the value of gfn is 0xfee00. Do you mean that is_iomem_page() is equal to direct_mmio except some corner cases such as APIC access MFN and INVALID_MFN( any others? ) ? Thanks Chao >Jan
>>> On 09.02.17 at 09:27, <chao.gao@intel.com> wrote: > On Wed, Feb 08, 2017 at 10:05:38AM -0700, Jan Beulich wrote: >>>>> On 08.02.17 at 08:31, <kevin.tian@intel.com> wrote: >>>> From: Jan Beulich [mailto:JBeulich@suse.com] >>>> Sent: Monday, February 06, 2017 11:38 PM >>>> >>>> >>> On 06.02.17 at 15:48, <dwmw2@infradead.org> wrote: >>>> > On Mon, 2017-02-06 at 07:26 -0700, Jan Beulich wrote: >>>> >> > >>>> >> > > >>>> >> > > > >>>> >> > > > On 06.02.17 at 14:55, <andrew.cooper3@citrix.com> 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@citrix.com> >>>> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>>> >> >>>> >> 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
On Thu, 2017-02-09 at 01:56 -0700, Jan Beulich wrote: > > > 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. You'll probably hit it for INVALID_MFN too on an unmap. And *do* make it print the offending address when it triggers. :)
On Thu, 2017-02-09 at 11:54 +0800, Chao Gao wrote: > > Jan is right. When the assertion fails, the value of gfn is 0xfee00. > > Do you mean that is_iomem_page() is equal to direct_mmio except some > corner cases such as APIC access MFN and INVALID_MFN( any others? ) ? That was the hypothesis, and it seems reasonable. But I also note you're only testing that hypothesis in the direct_mmio==1 case. You don't test the hypothesis that if direct_mmio==0, then so is is_iomem_page().
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 )