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

[Chao Gao <chao.gao@xxxxxxxxx>]

On Thu, Feb 09, 2017 at 01:56:14AM -0700, Jan Beulich wrote:
>>>> 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 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel