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

[Chao Gao <chao.gao@xxxxxxxxx>]

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.

(XEN) Assertion 'direct_mmio == is_iomem_page(mfn)' failed at mtrr.c:787
(XEN) ----[ Xen-4.9-unstable  x86_64  debug=y   Not tainted ]----
(XEN) CPU:    44
(XEN) RIP:    e008:[<ffff82d0804090bc>] epte_get_entry_emt+0xbc/0x24e 
[EPTE_EMT_v2]
(XEN) RFLAGS: 0000000000010297   CONTEXT: hypervisor (d0v39)
(XEN) rax: ffff832077d7e000   rbx: 00000000010107ba   rcx: ffff830000000000
(XEN) rdx: 0000000002077d7e   rsi: 00000000010107ba   rdi: 00000000010107ba
(XEN) rbp: ffff832076857a68   rsp: ffff832076857a18   r8:  ffff832076857af7
(XEN) r9:  0000000000000001   r10: 0000000000000000   r11: 0000000000000000
(XEN) r12: ffff832077d7e000   r13: 0000000000000000   r14: 0000000000000000
(XEN) r15: ffff832076857af7   cr0: 0000000080050033   cr4: 00000000001526e0
(XEN) cr3: 0000000899980000   cr2: 00007f35eb86d8d1
(XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: e010   cs: e008
(XEN) Xen code around <ffff82d0804090bc> (epte_get_entry_emt+0xbc/0x24e 
[EPTE_EMT_v2]):
(XEN)  37 4d d7 ff 3c 01 74 02 <0f> 0b 49 33 9c 24 40 06 00 00 44 89 e9 48 d3 eb
(XEN) Xen stack trace from rsp=ffff832076857a18:
(XEN)    ffff832076857b00 00000000000fee00 00000000010107ba 0000000100000000
(XEN)    ffff832076857a68 ffff832077da8b90 0000000000000000 ffff832076857b00
(XEN)    ffff8200403c8000 0000000000000000 ffff832076857b38 ffff82d0802101b5
(XEN)    ffff832000000005 0000000000000206 0000000000000000 ffff832076857aa8
(XEN)    ffff82d08013452b ffff831000000000 0000000000000001 0000000700000206
(XEN)    ffff832077d7e000 00000000000fee00 00000000010107ba 0000000000000005
(XEN)    ffff832077da8b90 0000000000000000 8000000000000000 00ff832077da8b90
(XEN)    0000000000000000 ffff8200403c8000 0000000000000001 00000000010107ba
(XEN)    0000000000000000 0000000000000001 00000000000fee00 ffff832077da8b90
(XEN)    ffff832076857b98 ffff82d080204d62 00000000ffffffff 0000000776857b68
(XEN)    ffff832077d7e000 0000000000000005 ffff832076857b98 ffff832077d7e000
(XEN)    00000000010107ba ffff832077da8b90 ffffffffffffffff 0000000000000007
(XEN)    ffff832076857c18 ffff82d080205408 0000000000000000 0000002800000021
(XEN)    0000000776857bc8 00000000010107ba 00000000000fee00 0000000000000005
(XEN)    000000128013452b 0000000000000004 ffff832076857c38 0000000000000000
(XEN)    00000000010107ba ffff832077d7e000 00000000000fee00 0000000000000007
(XEN)    ffff832076857c58 ffff82d080206fa5 ffff832076857c58 ffff832077d7e000
(XEN)    ffff82e02020f740 00000000010107ba 0000000000000001 0000000000000000
(XEN)    ffff832076857c88 ffff82d0801f8ff9 ffff832077d7e000 ffff832077d7e000
(XEN)    0000000000000000 0000000000000001 ffff832076857ca8 ffff82d0801d2a28
(XEN) Xen call trace:
(XEN)    [<ffff82d0804090bc>] epte_get_entry_emt+0xbc/0x24e [EPTE_EMT_v2]
(XEN)    [<ffff82d0802101b5>] p2m-ept.c#ept_set_entry+0x2f3/0x798
(XEN)    [<ffff82d080204d62>] p2m_set_entry+0xc9/0x10a
(XEN)    [<ffff82d080205408>] p2m.c#set_typed_p2m_entry+0x447/0x65f
(XEN)    [<ffff82d080206fa5>] set_mmio_p2m_entry+0x63/0x72
(XEN)    [<ffff82d0801f8ff9>] vmx.c#vmx_domain_initialise+0xb6/0xcd
(XEN)    [<ffff82d0801d2a28>] hvm_domain_initialise+0x2da/0x368
(XEN)    [<ffff82d08016b567>] arch_domain_create+0x4b0/0x68e
(XEN)    [<ffff82d08010520c>] domain_create+0x3ee/0x5a7
(XEN)    [<ffff82d080103690>] do_domctl+0x52e/0x1bae
(XEN)    [<ffff82d08017123f>] pv_hypercall+0x1e7/0x3fb
(XEN)    [<ffff82d080247026>] entry.o#test_all_events+0/0x30
(XEN) 
(XEN) 
(XEN) ****************************************
(XEN) Panic on CPU 44:
(XEN) Assertion 'direct_mmio == is_iomem_page(mfn)' failed at mtrr.c:787
(XEN) ****************************************

Thanks
Chao

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

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