On 04/17/2018 03:21 PM, Razvan Cojocaru wrote:
> On 04/17/2018 04:53 PM, George Dunlap wrote:
>> On 04/17/2018 11:50 AM, Razvan Cojocaru wrote:
>>> void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>> {
>>> struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>>> + struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>> struct ept_data *ept;
>>>
>>> + p2m->max_mapped_pfn = hostp2m->max_mapped_pfn;
>>> + p2m->default_access = hostp2m->default_access;
>>> + p2m->domain = hostp2m->domain;
>>> + p2m->logdirty_ranges = hostp2m->logdirty_ranges;
>>> + p2m->global_logdirty = hostp2m->global_logdirty;
>>
>> This would certainly be one approach. But then we'd need to keep a lot
>> more of these things in sync -- for instance, we'd have to have similar
>> code to enable and disable global_logdirty on all active altp2m entries.
>>
>> I also don't think the max_mapped_pfn should be copied here; the fact
>> that updates got filtered out before was a red herring I think.
>
> I initially thought so too, and now I've commented out just that one
> line to remember why I couldn't remove, and the reason is this:
>
> (XEN) Assertion 's <= e' failed at rangeset.c:121
> (XEN) ----[ Xen-4.11-unstable x86_64 debug=y Not tainted ]----
> (XEN) CPU: 0
> (XEN) RIP: e008:[<ffff82d080228826>] rangeset_add_range+0x5/0x1e6
> (XEN) RFLAGS: 0000000000010206 CONTEXT: hypervisor (d0v0)
> (XEN) rax: 0000000000000000 rbx: ffff830b577f92e0 rcx: 00000000000f0000
> (XEN) rdx: 0000000000000000 rsi: 00000000000f0000 rdi: ffff830ad6a1ce50
> (XEN) rbp: ffff83007ce87c78 rsp: ffff83007ce87c20 r8: 0000000000000000
> (XEN) r9: 0000000000000000 r10: 000000000000006f r11: 0000000000000028
> (XEN) r12: 0000000000000002 r13: 0000000000000000 r14: 0000000000000001
> (XEN) r15: ffff830ad6ddd000 cr0: 0000000080050033 cr4: 00000000003526e0
> (XEN) cr3: 0000000ad714f000 cr2: 0000000000c12000
> (XEN) fsb: 00007f794c7b2700 gsb: ffff880276c00000 gss: 0000000000000000
> (XEN) ds: 0000 es: 0000 fs: 0000 gs: 0000 ss: e010 cs: e008
> (XEN) Xen code around <ffff82d080228826> (rangeset_add_range+0x5/0x1e6):
> (XEN) 00 5d c3 48 39 d6 76 02 <0f> 0b 55 48 89 e5 41 56 41 55 41 54 53
> 49 89 d5
> (XEN) Xen stack trace from rsp=ffff83007ce87c20:
> (XEN) ffff82d08032c644 0000000000000206 0000000000000010 0000000000000028
> (XEN) 00000000000f0000 ffff82c000217000 0000000000000000 ffff830ad6ddd000
> (XEN) 00000000000f0240 0000000000000000 0000000000000002 ffff83007ce87cc8
> (XEN) ffff82d08032c7ac 0000000000000240 00000000000f0000 ffff82c000217000
> (XEN) ffff830ad6ddd000 00000000000f0240 0000000000000048 ffff82c000217000
> (XEN) 00000000000f0000 ffff83007ce87d38 ffff82d080362ade ffff830c5bb20000
> (XEN) ffff830ad6ddd650 0000000000000000 0000000000000000 00007f794c7bd004
> (XEN) 0000000000000048 ffff830c5bb20000 0000000000000000 ffff83007ce87e00
> (XEN) ffffffffffffffea ffff82d0802e9c64 deadbeefdeadf00d ffff83007ce87de8
> (XEN) ffff82d0802e92c1 ffff830c5bb20000 ffff83007d616000 0000000000000000
> (XEN) ffff83007ce87dd8 0000000000000000 ffff83007ce87df8 ffff83007ce87df4
> (XEN) ffff82e016a5de40 ffff83007d616000 0000000000000007 0000000000000240
> (XEN) 00000000000f0000 ffff83007ce87dc8 ffff830ad6ddd000 ffff83007ce87dc8
> (XEN) 0000000000000002 0000000000000001 00007f794c7bf004 ffff82d0802e9c64
> (XEN) deadbeefdeadf00d ffff83007ce87e48 ffff82d0802e9ce1 ffff82d080374434
> (XEN) 0000000280370001 00007f794c7be004 0000000000000020 00007f794c7bd004
> (XEN) 0000000000000048 ffff82d080374434 ffff83007ce87ef8 ffff83007d616000
> (XEN) 0000000000000029 ffff83007ce87ee8 ffff82d08036d8a6 03ff82d080374434
> (XEN) 0000000000000001 0000000000000002 00007f794c7bf004 deadbeefdeadf00d
> (XEN) deadbeefdeadf00d ffff82d080374434 ffff82d080374428 ffff82d080374434
> (XEN) Xen call trace:
> (XEN) [<ffff82d080228826>] rangeset_add_range+0x5/0x1e6
> (XEN) [<ffff82d08032c7ac>] p2m_change_type_range+0x66/0x7f
> (XEN) [<ffff82d080362ade>] hap_track_dirty_vram+0x240/0x491
> (XEN) [<ffff82d0802e92c1>] dm.c#dm_op+0x45c/0xd06
> (XEN) [<ffff82d0802e9ce1>] do_dm_op+0x7d/0xb3
> (XEN) [<ffff82d08036d8a6>] pv_hypercall+0x1f4/0x440
> (XEN) [<ffff82d080374495>] lstar_enter+0x115/0x120
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) Assertion 's <= e' failed at rangeset.c:121
> (XEN) ****************************************
Right, but this is basically a bug in p2m_change_type_range(), where it
handles end > max_mapped_pfn, but not start > max_mapped_pfn. It should
check the latter just after grabbing the lock and bail if true. (This
should probably be in a separate patch, as it's really a generic bug in
p2m_change_type_range().)
>> Another approach would be to maintain the logdirty_ranges and
>> global_logdirty only for the host p2m, but to misconfigure entries for
>> all the p2ms; and then on a misconfiguration, handle the
>> misconfiguration for the hostp2m and then do a lazy propagate for the
>> altp2m. On the whole that's probably more error-prone than just doing a
>> for() loop, though, and not that much faster. :-)
> We can try that too.
If you want -- but as I said, arguably your approach is more robust;
just incomplete.
>> The other thing that seems to be missing from synchronization is that in
>> p2m-ept.c:ept_enable_pml() sets the p2m->ept.ad bit (which ends up being
>> part of the eptp). The code seems to indicate that this is required for
>> PML (hardware-assisted logdirty), but I don't see anywhere this is set
>> or copied from the host p2m.
>>
>> It might be nice to have a more structured way of keeping all these
>> changes in sync, rather than relying on this open-coding everywhere.
>
> Very true. It has also occured to me that some of these issues would be
> at least partially mitigated if altp2m was always on, but of course I
> can also see why that would be frowned upon at this time.
How would having it enabled all the time have helped in this situation?
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
