Re: [Xen-devel] [PATCH] xen/mm: Alter is_iomem_page() to use mfn_t

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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