Re: [Xen-devel] [PATCH RFC 01/12] x86/paging: introduce paging_set_allocation

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Tue, Aug 2, 2016 at 5:12 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 02.08.16 at 17:49, <roger.pau@xxxxxxxxxx> wrote:
>> On Tue, Aug 02, 2016 at 11:47:22AM +0200, Roger Pau Monne wrote:
>>> On Fri, Jul 29, 2016 at 05:47:24PM +0100, Andrew Cooper wrote:
>>> > As this is for the construction of dom0, it would be better to take a
>>> > preemptible pointer, loop in construct_dom0(), with a
>>> > process_pending_softirqs() call in between.
>>>
>>> Now fixed.
>>
>> Hm, I have to stand corrected, using hypercall_preempt_check (as
>> any of the *_set_allocation function use), is not safe at this point:
>>
>> (XEN) ----[ Xen-4.8-unstable  x86_64  debug=y  Tainted:    C  ]----
>> (XEN) CPU:    0
>> (XEN) RIP:    e008:[<ffff82d08022fd47>] 
>> hap.c#local_events_need_delivery+0x27/0x40
>> (XEN) RFLAGS: 0000000000010246   CONTEXT: hypervisor
>> (XEN) rax: 0000000000000000   rbx: ffff83023f5a5000   rcx: ffff82d080312900
>> (XEN) rdx: 0000000000000001   rsi: ffff83023f5a56c8   rdi: ffff8300b213d000
>> (XEN) rbp: ffff82d080307cc8   rsp: ffff82d080307cc8   r8:  0180000000000000
>> (XEN) r9:  0000000000000000   r10: 0000000000247000   r11: ffff82d08029a5b0
>> (XEN) r12: 0000000000000011   r13: 00000000000023ac   r14: ffff82d080307d4c
>> (XEN) r15: ffff83023f5a56c8   cr0: 000000008005003b   cr4: 00000000001526e0
>> (XEN) cr3: 00000000b20fc000   cr2: 0000000000000000
>> (XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: 0000   cs: e008
>> (XEN) Xen code around <ffff82d08022fd47> 
>> (hap.c#local_events_need_delivery+0x27/0x40):
>> (XEN)  0d ad fa ff 48 8b 47 08 <80> 38 00 74 09 80 78 01 00 0f 94 c0 eb 02 
>> 31 c0
>> (XEN) Xen stack trace from rsp=ffff82d080307cc8:
>> (XEN)    ffff82d080307d08 ffff82d08022fc47 0000000000000000 ffff83023f5a5000
>> (XEN)    ffff83023f5a5648 0000000000000000 ffff82d080307d4c 0000000000002400
>> (XEN)    ffff82d080307d38 ffff82d08020008c 00000000000ffffd ffff8300b1efd000
>> (XEN)    ffff83023f5a5000 ffff82d080307d4c ffff82d080307d78 ffff82d0802cad30
>> (XEN)    0000000000203000 ffff83023f5a5000 ffff82d0802bf860 0000000000000000
>> (XEN)    0000000000000001 ffff83000008bef0 ffff82d080307de8 ffff82d0802c91e0
>> (XEN)    ffff82d080307de8 ffff82d080143900 ffff82d080307de8 0000000000000000
>> (XEN)    ffff83000008bf00 ffff82d0802eb480 ffff82d080307dc4 ffff82d08028b1cd
>> (XEN)    ffff83000008bf00 0000000000000000 0000000000000001 ffff83023f5a5000
>> (XEN)    ffff82d080307f08 ffff82d0802bf0c9 0000000000000000 0000000000000000
>> (XEN)    0000000000000000 ffff82d080307f18 ffff83000008bee0 0000000000000001
>> (XEN)    0000000000000001 0000000000000001 0000000000000000 0000000000100000
>> (XEN)    0000000000000001 0000000000247000 ffff83000008bef4 0000000000100000
>> (XEN)    ffff830100000000 0000000000247001 0000000000000014 0000000100000000
>> (XEN)    ffff8300ffffffec ffff83000008bef0 ffff82d0802e0640 ffff83000008bfb0
>> (XEN)    0000000000000000 0000000000000000 0000000000000111 0000000800000000
>> (XEN)    000000010000006e 0000000000000003 00000000000002f8 0000000000000000
>> (XEN)    00000000ad5c0bd0 0000000000000000 0000000000000001 0000000000000008
>> (XEN)    0000000000000000 ffff82d080100073 0000000000000000 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> (XEN) Xen call trace:
>> (XEN)    [<ffff82d08022fd47>] hap.c#local_events_need_delivery+0x27/0x40
>> (XEN)    [<ffff82d08022fc47>] hap_set_allocation+0x107/0x130
>> (XEN)    [<ffff82d08020008c>] paging_set_allocation+0x4c/0x80
>> (XEN)    [<ffff82d0802cad30>] domain_build.c#hvm_setup_p2m+0x70/0x1a0
>> (XEN)    [<ffff82d0802c91e0>] domain_build.c#construct_dom0_hvm+0x60/0x120
>> (XEN)    [<ffff82d0802bf0c9>] __start_xen+0x1ea9/0x23a0
>> (XEN)    [<ffff82d080100073>] __high_start+0x53/0x60
>> (XEN)
>> (XEN) Pagetable walk from 0000000000000000:
>
> Sadly you don't make clear what pointer it is that is NULL at that point.

It sounds from what he says in the following paragraph like current is NULL.

>> I've tried setting current to d->vcpu[0], but that just makes the call to
>> hypercall_preempt_check crash in some scheduler assert. In any case, I've
>> added the preempt parameter to the paging_set_allocation function, but I
>> don't plan to use it in the domain builder for the time being. Does that
>> sound right?
>
> Not really, new huge latency issues like this shouldn't be reintroduced;
> we've been fighting hard to get rid of those (and we still occasionally
> find some no-one had noticed before).

You mean latency in processing softirqs?

Maybe what we need to do is to make local_events_need_delivery() safe
to call at this point by having it return 0 if current is NULL rather
than crashing?

 -George

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