* [PATCH v2 0/2] kexec: Use hypercall_create_continuation to protect KEXEC ops @ 2017-04-17 19:09 Eric DeVolder 2017-04-17 19:09 ` [PATCH v2 1/2] kexec: use " Eric DeVolder 2017-04-17 19:09 ` [PATCH v2 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level Eric DeVolder 0 siblings, 2 replies; 15+ messages in thread From: Eric DeVolder @ 2017-04-17 19:09 UTC (permalink / raw) To: xen-devel, andrew.cooper3 Cc: daniel.kiper, eric.devolder, bhavesh.davda, JBeulich During testing (using the script below) we found that multiple invocations of kexec of unload/load are not safe. This does not exist in classic Xen kernels in which the kexec-tools did the kexec via Linux kernel syscall (which in turn made the hypercall), as the Linux code has a mutex_trylock which would inhibit multiple concurrent calls. But with the kexec-tools utilizing xc_kexec_* that is no longer the case and we need to protect against multiple concurrent invocations. Please see the patches and review at your convenience! ==== try-crash.pl from bhavesh.davda@oracle.com ==== #!/usr/bin/perl -w use strict; use warnings; use threads; sub threaded_task { threads->create(sub { my $thr_id = threads->self->tid; #print "Starting load thread $thr_id\n"; system("/sbin/kexec -p --command-line=\"placeholder root=/dev/mapper/nimbula-root ro rhbg console=tty0 console=hvc0 earlyprintk=xen nomodeset printk.time=1 irqpoll maxcpus=1 nr_cpus=1 reset_devices cgroup_disable=memory mce=off selinux=0 console=ttyS1,115200n8\" --initrd=/boot/initrd-4.1.12-61.1.9.el6uek.x86_64kdump.img /boot/vmlinuz-4.1.12-61.1.9.el6uek.x86_64"); #print "Ending load thread $thr_id\n"; threads->detach(); #End thread. }); threads->create(sub { my $thr_id = threads->self->tid; #print "Starting unload thread $thr_id\n"; system("/sbin/kexec -p -u"); #print "Ending unload thread $thr_id\n"; threads->detach(); #End thread. }); } for my $i (0..99) { threaded_task(); } Eric DeVolder (2): kexec: use hypercall_create_continuation to protect KEXEC ops kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level xen/common/kexec.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/2] kexec: use hypercall_create_continuation to protect KEXEC ops 2017-04-17 19:09 [PATCH v2 0/2] kexec: Use hypercall_create_continuation to protect KEXEC ops Eric DeVolder @ 2017-04-17 19:09 ` Eric DeVolder 2017-04-18 10:48 ` Jan Beulich 2017-04-17 19:09 ` [PATCH v2 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level Eric DeVolder 1 sibling, 1 reply; 15+ messages in thread From: Eric DeVolder @ 2017-04-17 19:09 UTC (permalink / raw) To: xen-devel, andrew.cooper3 Cc: daniel.kiper, eric.devolder, bhavesh.davda, JBeulich When we concurrently try to unload and load crash images we eventually get: Xen call trace: [<ffff82d08018b04f>] machine_kexec_add_page+0x3a0/0x3fa [<ffff82d08018b184>] machine_kexec_load+0xdb/0x107 [<ffff82d080116e8d>] kexec.c#kexec_load_slot+0x11/0x42 [<ffff82d08011724f>] kexec.c#kexec_load+0x119/0x150 [<ffff82d080117c1e>] kexec.c#do_kexec_op_internal+0xab/0xcf [<ffff82d080117c60>] do_kexec_op+0xe/0x1e [<ffff82d08025c620>] pv_hypercall+0x20a/0x44a [<ffff82d080260116>] cpufreq.c#test_all_events+0/0x30 Pagetable walk from ffff820040088320: L4[0x104] = 00000002979d1063 ffffffffffffffff L3[0x001] = 00000002979d0063 ffffffffffffffff L2[0x000] = 00000002979c7063 ffffffffffffffff L1[0x088] = 80037a91ede97063 ffffffffffffffff The interesting thing is that the page bits (063) look legit. The operation on which we blow up is us trying to write in the L1 and finding that the L2 entry points to some bizzare MFN. It stinks of a race, and it looks like the issue is due to no concurrency locks when dealing with the crash kernel space. Specifically we concurrently call kimage_alloc_crash_control_page which iterates over the kexec_crash_area.start -> kexec_crash_area.size and once found: if ( page ) { image->next_crash_page = hole_end; clear_domain_page(_mfn(page_to_mfn(page))); } clears. Since the parameters of what MFN to use are provided by the callers (and the area to search is bounded) the the 'page' is probably the same. So #1 we concurrently clear the 'control_code_page'. The next step is us passing this 'control_code_page' to machine_kexec_add_page. This function requires the MFNs: page_to_maddr(image->control_code_page). And this would always return the same virtual address, as the MFN of the control_code_page is inside of the kexec_crash_area.start -> kexec_crash_area.size area. Then machine_kexec_add_page updates the L1 .. which can be done concurrently and on subsequent calls we mangle it up. This is all a theory at this time, but testing reveals that adding the hypercall_create_continuation() at the kexec hypercall fixes the crash. NOTE: This patch follows 5c5216 (kexec: clear kexec_image slot when unloading kexec image) to prevent crashes during simultaneous load/unloads. NOTE: Consideration was given to using the existing flag KEXEC_FLAG_IN_PROGRESS to denote a kexec hypercall in progress. This, however, overloads the original intent of the flag which is to denote that we are about-to/have made the jump to the crash path. The overloading would lead to failures in existing checks on this flag as the flag would always be set at the top level in do_kexec_op_internal(). For this reason, the new flag KEXEC_FLAG_HC_IN_PROGRESS was introduced. While at it, fixed the #define mismatched spacing Signed-off-by: Eric DeVolder <eric.devolder@oracle.com> Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- v2: 04/17/2017 - Patch titled 'kexec: use hypercall_create_continuation to protect KEXEC ops' - Jan Beulich directed me to use a continuation method instead of spinlock. - Incorporated feedback from Daniel Kiper <daniel.kiper@oracle.com> - Incorporated feedback from Konrad Wilk <konrad.wilk@oracle.com> v1: 04/10/2017 - Patch titled 'kexec: Add spinlock for the whole hypercall' - Used spinlock in do_kexec_op_internal() --- xen/common/kexec.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/xen/common/kexec.c b/xen/common/kexec.c index 072cc8e..3f96eb2 100644 --- a/xen/common/kexec.c +++ b/xen/common/kexec.c @@ -50,9 +50,10 @@ static cpumask_t crash_saved_cpus; static struct kexec_image *kexec_image[KEXEC_IMAGE_NR]; -#define KEXEC_FLAG_DEFAULT_POS (KEXEC_IMAGE_NR + 0) -#define KEXEC_FLAG_CRASH_POS (KEXEC_IMAGE_NR + 1) -#define KEXEC_FLAG_IN_PROGRESS (KEXEC_IMAGE_NR + 2) +#define KEXEC_FLAG_DEFAULT_POS (KEXEC_IMAGE_NR + 0) +#define KEXEC_FLAG_CRASH_POS (KEXEC_IMAGE_NR + 1) +#define KEXEC_FLAG_IN_PROGRESS (KEXEC_IMAGE_NR + 2) +#define KEXEC_FLAG_HC_IN_PROGRESS (KEXEC_IMAGE_NR + 3) static unsigned long kexec_flags = 0; /* the lowest bits are for KEXEC_IMAGE... */ @@ -1193,6 +1194,9 @@ static int do_kexec_op_internal(unsigned long op, if ( ret ) return ret; + if ( test_and_set_bit(KEXEC_FLAG_HC_IN_PROGRESS, &kexec_flags)) + return hypercall_create_continuation(__HYPERVISOR_kexec_op, "lh", op, uarg); + switch ( op ) { case KEXEC_CMD_kexec_get_range: @@ -1227,6 +1231,8 @@ static int do_kexec_op_internal(unsigned long op, break; } + clear_bit(KEXEC_FLAG_HC_IN_PROGRESS, &kexec_flags); + return ret; } -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] kexec: use hypercall_create_continuation to protect KEXEC ops 2017-04-17 19:09 ` [PATCH v2 1/2] kexec: use " Eric DeVolder @ 2017-04-18 10:48 ` Jan Beulich 2017-04-19 11:00 ` Daniel Kiper 0 siblings, 1 reply; 15+ messages in thread From: Jan Beulich @ 2017-04-18 10:48 UTC (permalink / raw) To: Eric DeVolder; +Cc: andrew.cooper3, bhavesh.davda, daniel.kiper, xen-devel >>> On 17.04.17 at 21:09, <eric.devolder@oracle.com> wrote: > --- a/xen/common/kexec.c > +++ b/xen/common/kexec.c > @@ -50,9 +50,10 @@ static cpumask_t crash_saved_cpus; > > static struct kexec_image *kexec_image[KEXEC_IMAGE_NR]; > > -#define KEXEC_FLAG_DEFAULT_POS (KEXEC_IMAGE_NR + 0) > -#define KEXEC_FLAG_CRASH_POS (KEXEC_IMAGE_NR + 1) > -#define KEXEC_FLAG_IN_PROGRESS (KEXEC_IMAGE_NR + 2) > +#define KEXEC_FLAG_DEFAULT_POS (KEXEC_IMAGE_NR + 0) > +#define KEXEC_FLAG_CRASH_POS (KEXEC_IMAGE_NR + 1) > +#define KEXEC_FLAG_IN_PROGRESS (KEXEC_IMAGE_NR + 2) > +#define KEXEC_FLAG_HC_IN_PROGRESS (KEXEC_IMAGE_NR + 3) Perhaps KEXEC_FLAG_IN_HYPERCALL? Other than that (and this clearly is subject to Andrew's opinion) Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] kexec: use hypercall_create_continuation to protect KEXEC ops 2017-04-18 10:48 ` Jan Beulich @ 2017-04-19 11:00 ` Daniel Kiper 2017-04-19 11:48 ` Andrew Cooper 0 siblings, 1 reply; 15+ messages in thread From: Daniel Kiper @ 2017-04-19 11:00 UTC (permalink / raw) To: Jan Beulich; +Cc: andrew.cooper3, bhavesh.davda, Eric DeVolder, xen-devel On Tue, Apr 18, 2017 at 04:48:06AM -0600, Jan Beulich wrote: > >>> On 17.04.17 at 21:09, <eric.devolder@oracle.com> wrote: > > --- a/xen/common/kexec.c > > +++ b/xen/common/kexec.c > > @@ -50,9 +50,10 @@ static cpumask_t crash_saved_cpus; > > > > static struct kexec_image *kexec_image[KEXEC_IMAGE_NR]; > > > > -#define KEXEC_FLAG_DEFAULT_POS (KEXEC_IMAGE_NR + 0) > > -#define KEXEC_FLAG_CRASH_POS (KEXEC_IMAGE_NR + 1) > > -#define KEXEC_FLAG_IN_PROGRESS (KEXEC_IMAGE_NR + 2) > > +#define KEXEC_FLAG_DEFAULT_POS (KEXEC_IMAGE_NR + 0) > > +#define KEXEC_FLAG_CRASH_POS (KEXEC_IMAGE_NR + 1) > > +#define KEXEC_FLAG_IN_PROGRESS (KEXEC_IMAGE_NR + 2) > > +#define KEXEC_FLAG_HC_IN_PROGRESS (KEXEC_IMAGE_NR + 3) > > Perhaps KEXEC_FLAG_IN_HYPERCALL? Other than that (and this Make sense for me. > clearly is subject to Andrew's opinion) > Reviewed-by: Jan Beulich <jbeulich@suse.com> Otherwise Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] kexec: use hypercall_create_continuation to protect KEXEC ops 2017-04-19 11:00 ` Daniel Kiper @ 2017-04-19 11:48 ` Andrew Cooper 2017-04-19 12:16 ` Daniel Kiper 2017-04-19 15:48 ` Eric DeVolder 0 siblings, 2 replies; 15+ messages in thread From: Andrew Cooper @ 2017-04-19 11:48 UTC (permalink / raw) To: Daniel Kiper, Jan Beulich Cc: bhavesh.davda, Eric DeVolder, Julien Grall, xen-devel On 19/04/17 12:00, Daniel Kiper wrote: > On Tue, Apr 18, 2017 at 04:48:06AM -0600, Jan Beulich wrote: >>>>> On 17.04.17 at 21:09, <eric.devolder@oracle.com> wrote: >>> --- a/xen/common/kexec.c >>> +++ b/xen/common/kexec.c >>> @@ -50,9 +50,10 @@ static cpumask_t crash_saved_cpus; >>> >>> static struct kexec_image *kexec_image[KEXEC_IMAGE_NR]; >>> >>> -#define KEXEC_FLAG_DEFAULT_POS (KEXEC_IMAGE_NR + 0) >>> -#define KEXEC_FLAG_CRASH_POS (KEXEC_IMAGE_NR + 1) >>> -#define KEXEC_FLAG_IN_PROGRESS (KEXEC_IMAGE_NR + 2) >>> +#define KEXEC_FLAG_DEFAULT_POS (KEXEC_IMAGE_NR + 0) >>> +#define KEXEC_FLAG_CRASH_POS (KEXEC_IMAGE_NR + 1) >>> +#define KEXEC_FLAG_IN_PROGRESS (KEXEC_IMAGE_NR + 2) >>> +#define KEXEC_FLAG_HC_IN_PROGRESS (KEXEC_IMAGE_NR + 3) >> Perhaps KEXEC_FLAG_IN_HYPERCALL? Other than that (and this > Make sense for me. Agreed. (I can fix on commit). > >> clearly is subject to Andrew's opinion) >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > Otherwise Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> CC'ing Julien. This is clearly a good bugfix for 4.9 ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] kexec: use hypercall_create_continuation to protect KEXEC ops 2017-04-19 11:48 ` Andrew Cooper @ 2017-04-19 12:16 ` Daniel Kiper 2017-04-19 15:48 ` Eric DeVolder 1 sibling, 0 replies; 15+ messages in thread From: Daniel Kiper @ 2017-04-19 12:16 UTC (permalink / raw) To: Andrew Cooper Cc: Julien Grall, xen-devel, bhavesh.davda, Jan Beulich, Eric DeVolder On Wed, Apr 19, 2017 at 12:48:56PM +0100, Andrew Cooper wrote: > On 19/04/17 12:00, Daniel Kiper wrote: > > On Tue, Apr 18, 2017 at 04:48:06AM -0600, Jan Beulich wrote: > >>>>> On 17.04.17 at 21:09, <eric.devolder@oracle.com> wrote: > >>> --- a/xen/common/kexec.c > >>> +++ b/xen/common/kexec.c > >>> @@ -50,9 +50,10 @@ static cpumask_t crash_saved_cpus; > >>> > >>> static struct kexec_image *kexec_image[KEXEC_IMAGE_NR]; > >>> > >>> -#define KEXEC_FLAG_DEFAULT_POS (KEXEC_IMAGE_NR + 0) > >>> -#define KEXEC_FLAG_CRASH_POS (KEXEC_IMAGE_NR + 1) > >>> -#define KEXEC_FLAG_IN_PROGRESS (KEXEC_IMAGE_NR + 2) > >>> +#define KEXEC_FLAG_DEFAULT_POS (KEXEC_IMAGE_NR + 0) > >>> +#define KEXEC_FLAG_CRASH_POS (KEXEC_IMAGE_NR + 1) > >>> +#define KEXEC_FLAG_IN_PROGRESS (KEXEC_IMAGE_NR + 2) > >>> +#define KEXEC_FLAG_HC_IN_PROGRESS (KEXEC_IMAGE_NR + 3) > >> Perhaps KEXEC_FLAG_IN_HYPERCALL? Other than that (and this > > Make sense for me. > > Agreed. (I can fix on commit). > > > > >> clearly is subject to Andrew's opinion) > >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Otherwise Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > CC'ing Julien. This is clearly a good bugfix for 4.9 Yep and I think that it is stable material too. Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] kexec: use hypercall_create_continuation to protect KEXEC ops 2017-04-19 11:48 ` Andrew Cooper 2017-04-19 12:16 ` Daniel Kiper @ 2017-04-19 15:48 ` Eric DeVolder 1 sibling, 0 replies; 15+ messages in thread From: Eric DeVolder @ 2017-04-19 15:48 UTC (permalink / raw) To: Andrew Cooper, Daniel Kiper, Jan Beulich Cc: bhavesh.davda, Julien Grall, xen-devel On 04/19/2017 06:48 AM, Andrew Cooper wrote: > On 19/04/17 12:00, Daniel Kiper wrote: >> On Tue, Apr 18, 2017 at 04:48:06AM -0600, Jan Beulich wrote: >>>>>> On 17.04.17 at 21:09, <eric.devolder@oracle.com> wrote: >>>> --- a/xen/common/kexec.c >>>> +++ b/xen/common/kexec.c >>>> @@ -50,9 +50,10 @@ static cpumask_t crash_saved_cpus; >>>> >>>> static struct kexec_image *kexec_image[KEXEC_IMAGE_NR]; >>>> >>>> -#define KEXEC_FLAG_DEFAULT_POS (KEXEC_IMAGE_NR + 0) >>>> -#define KEXEC_FLAG_CRASH_POS (KEXEC_IMAGE_NR + 1) >>>> -#define KEXEC_FLAG_IN_PROGRESS (KEXEC_IMAGE_NR + 2) >>>> +#define KEXEC_FLAG_DEFAULT_POS (KEXEC_IMAGE_NR + 0) >>>> +#define KEXEC_FLAG_CRASH_POS (KEXEC_IMAGE_NR + 1) >>>> +#define KEXEC_FLAG_IN_PROGRESS (KEXEC_IMAGE_NR + 2) >>>> +#define KEXEC_FLAG_HC_IN_PROGRESS (KEXEC_IMAGE_NR + 3) >>> Perhaps KEXEC_FLAG_IN_HYPERCALL? Other than that (and this >> Make sense for me. > > Agreed. (I can fix on commit). > >> >>> clearly is subject to Andrew's opinion) >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> Otherwise Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > CC'ing Julien. This is clearly a good bugfix for 4.9 I've just posted v3 of this patch with the flag name change. Eric _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level 2017-04-17 19:09 [PATCH v2 0/2] kexec: Use hypercall_create_continuation to protect KEXEC ops Eric DeVolder 2017-04-17 19:09 ` [PATCH v2 1/2] kexec: use " Eric DeVolder @ 2017-04-17 19:09 ` Eric DeVolder 2017-04-18 10:49 ` Jan Beulich 1 sibling, 1 reply; 15+ messages in thread From: Eric DeVolder @ 2017-04-17 19:09 UTC (permalink / raw) To: xen-devel, andrew.cooper3 Cc: daniel.kiper, eric.devolder, bhavesh.davda, JBeulich The spinlock in kexec_swap_images() was removed as this function is only reachable on the kexec hypercall, which is now protected at the top-level in do_kexec_op_internal(), thus the local spinlock is no longer necessary. Signed-off-by: Eric DeVolder <eric.devolder@oracle.com> Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- xen/common/kexec.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/xen/common/kexec.c b/xen/common/kexec.c index 3f96eb2..efecf60 100644 --- a/xen/common/kexec.c +++ b/xen/common/kexec.c @@ -820,7 +820,6 @@ static int kexec_exec(XEN_GUEST_HANDLE_PARAM(void) uarg) static int kexec_swap_images(int type, struct kexec_image *new, struct kexec_image **old) { - static DEFINE_SPINLOCK(kexec_lock); int base, bit, pos; int new_slot, old_slot; @@ -832,8 +831,6 @@ static int kexec_swap_images(int type, struct kexec_image *new, if ( kexec_load_get_bits(type, &base, &bit) ) return -EINVAL; - spin_lock(&kexec_lock); - pos = (test_bit(bit, &kexec_flags) != 0); old_slot = base + pos; new_slot = base + !pos; @@ -846,8 +843,6 @@ static int kexec_swap_images(int type, struct kexec_image *new, clear_bit(old_slot, &kexec_flags); *old = kexec_image[old_slot]; - spin_unlock(&kexec_lock); - return 0; } -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level 2017-04-17 19:09 ` [PATCH v2 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level Eric DeVolder @ 2017-04-18 10:49 ` Jan Beulich 2017-04-19 10:56 ` Daniel Kiper 0 siblings, 1 reply; 15+ messages in thread From: Jan Beulich @ 2017-04-18 10:49 UTC (permalink / raw) To: Eric DeVolder; +Cc: andrew.cooper3, bhavesh.davda, daniel.kiper, xen-devel >>> On 17.04.17 at 21:09, <eric.devolder@oracle.com> wrote: > The spinlock in kexec_swap_images() was removed as > this function is only reachable on the kexec hypercall, which is > now protected at the top-level in do_kexec_op_internal(), > thus the local spinlock is no longer necessary. But perhaps leave an ASSERT() there, making sure the in-hypercall flag is set? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level 2017-04-18 10:49 ` Jan Beulich @ 2017-04-19 10:56 ` Daniel Kiper 2017-04-19 11:20 ` Jan Beulich 0 siblings, 1 reply; 15+ messages in thread From: Daniel Kiper @ 2017-04-19 10:56 UTC (permalink / raw) To: Jan Beulich; +Cc: andrew.cooper3, bhavesh.davda, Eric DeVolder, xen-devel On Tue, Apr 18, 2017 at 04:49:48AM -0600, Jan Beulich wrote: > >>> On 17.04.17 at 21:09, <eric.devolder@oracle.com> wrote: > > The spinlock in kexec_swap_images() was removed as > > this function is only reachable on the kexec hypercall, which is > > now protected at the top-level in do_kexec_op_internal(), > > thus the local spinlock is no longer necessary. > > But perhaps leave an ASSERT() there, making sure the in-hypercall > flag is set? I am not sure why but if at all I think that we should also consider other key kexec functions then. Or put ASSERT() into do_kexec_op_internal() just before "switch ( op )". Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level 2017-04-19 10:56 ` Daniel Kiper @ 2017-04-19 11:20 ` Jan Beulich 2017-04-19 11:52 ` Andrew Cooper 2017-04-19 12:13 ` Daniel Kiper 0 siblings, 2 replies; 15+ messages in thread From: Jan Beulich @ 2017-04-19 11:20 UTC (permalink / raw) To: Daniel Kiper; +Cc: andrew.cooper3, bhavesh.davda, Eric DeVolder, xen-devel >>> On 19.04.17 at 12:56, <daniel.kiper@oracle.com> wrote: > On Tue, Apr 18, 2017 at 04:49:48AM -0600, Jan Beulich wrote: >> >>> On 17.04.17 at 21:09, <eric.devolder@oracle.com> wrote: >> > The spinlock in kexec_swap_images() was removed as >> > this function is only reachable on the kexec hypercall, which is >> > now protected at the top-level in do_kexec_op_internal(), >> > thus the local spinlock is no longer necessary. >> >> But perhaps leave an ASSERT() there, making sure the in-hypercall >> flag is set? > > I am not sure why but if at all I think that we should also consider > other key kexec functions then. Or put ASSERT() into do_kexec_op_internal() > just before "switch ( op )". The point of my placement suggestion was that the ASSERT() effectively replaces the lock acquire - the places you name didn't previously require any synchronization. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level 2017-04-19 11:20 ` Jan Beulich @ 2017-04-19 11:52 ` Andrew Cooper 2017-04-19 12:13 ` Daniel Kiper 1 sibling, 0 replies; 15+ messages in thread From: Andrew Cooper @ 2017-04-19 11:52 UTC (permalink / raw) To: Jan Beulich, Daniel Kiper; +Cc: bhavesh.davda, Eric DeVolder, xen-devel On 19/04/17 12:20, Jan Beulich wrote: >>>> On 19.04.17 at 12:56, <daniel.kiper@oracle.com> wrote: >> On Tue, Apr 18, 2017 at 04:49:48AM -0600, Jan Beulich wrote: >>>>>> On 17.04.17 at 21:09, <eric.devolder@oracle.com> wrote: >>>> The spinlock in kexec_swap_images() was removed as >>>> this function is only reachable on the kexec hypercall, which is >>>> now protected at the top-level in do_kexec_op_internal(), >>>> thus the local spinlock is no longer necessary. >>> But perhaps leave an ASSERT() there, making sure the in-hypercall >>> flag is set? >> I am not sure why but if at all I think that we should also consider >> other key kexec functions then. Or put ASSERT() into do_kexec_op_internal() >> just before "switch ( op )". > The point of my placement suggestion was that the ASSERT() > effectively replaces the lock acquire - the places you name > didn't previously require any synchronization. I'd recommend adding the ASSERT(), just to be on the safe side. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level 2017-04-19 11:20 ` Jan Beulich 2017-04-19 11:52 ` Andrew Cooper @ 2017-04-19 12:13 ` Daniel Kiper 2017-04-19 13:37 ` Jan Beulich 1 sibling, 1 reply; 15+ messages in thread From: Daniel Kiper @ 2017-04-19 12:13 UTC (permalink / raw) To: Jan Beulich; +Cc: andrew.cooper3, bhavesh.davda, Eric DeVolder, xen-devel On Wed, Apr 19, 2017 at 05:20:50AM -0600, Jan Beulich wrote: > >>> On 19.04.17 at 12:56, <daniel.kiper@oracle.com> wrote: > > On Tue, Apr 18, 2017 at 04:49:48AM -0600, Jan Beulich wrote: > >> >>> On 17.04.17 at 21:09, <eric.devolder@oracle.com> wrote: > >> > The spinlock in kexec_swap_images() was removed as > >> > this function is only reachable on the kexec hypercall, which is > >> > now protected at the top-level in do_kexec_op_internal(), > >> > thus the local spinlock is no longer necessary. > >> > >> But perhaps leave an ASSERT() there, making sure the in-hypercall > >> flag is set? > > > > I am not sure why but if at all I think that we should also consider > > other key kexec functions then. Or put ASSERT() into do_kexec_op_internal() > > just before "switch ( op )". > > The point of my placement suggestion was that the ASSERT() > effectively replaces the lock acquire - the places you name > didn't previously require any synchronization. After the first patch of this series kexec_swap_images() cannot be started twice in parallel. So, I do not see the point of ASSERT() here. Or let's say we wish to have it to double check that "the in-hypercall flag is set". AIUI, it is your original idea. However, then I think that we should have an ASSERT() at least in kexec_load_slot() because parallel loads make issues too. And we can go higher to feel more safe. That is why I suggested do_kexec_op_internal() as the final resting place for an ASSERT(). Simply it looks to me the safest approach. Am I missing something? Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level 2017-04-19 12:13 ` Daniel Kiper @ 2017-04-19 13:37 ` Jan Beulich 2017-04-19 15:49 ` Eric DeVolder 0 siblings, 1 reply; 15+ messages in thread From: Jan Beulich @ 2017-04-19 13:37 UTC (permalink / raw) To: Daniel Kiper; +Cc: andrew.cooper3, bhavesh.davda, Eric DeVolder, xen-devel >>> On 19.04.17 at 14:13, <daniel.kiper@oracle.com> wrote: > On Wed, Apr 19, 2017 at 05:20:50AM -0600, Jan Beulich wrote: >> >>> On 19.04.17 at 12:56, <daniel.kiper@oracle.com> wrote: >> > On Tue, Apr 18, 2017 at 04:49:48AM -0600, Jan Beulich wrote: >> >> >>> On 17.04.17 at 21:09, <eric.devolder@oracle.com> wrote: >> >> > The spinlock in kexec_swap_images() was removed as >> >> > this function is only reachable on the kexec hypercall, which is >> >> > now protected at the top-level in do_kexec_op_internal(), >> >> > thus the local spinlock is no longer necessary. >> >> >> >> But perhaps leave an ASSERT() there, making sure the in-hypercall >> >> flag is set? >> > >> > I am not sure why but if at all I think that we should also consider >> > other key kexec functions then. Or put ASSERT() into do_kexec_op_internal() >> > just before "switch ( op )". >> >> The point of my placement suggestion was that the ASSERT() >> effectively replaces the lock acquire - the places you name >> didn't previously require any synchronization. > > After the first patch of this series kexec_swap_images() cannot be > started twice in parallel. So, I do not see the point of ASSERT() here. > Or let's say we wish to have it to double check that "the in-hypercall > flag is set". AIUI, it is your original idea. However, then I think that > we should have an ASSERT() at least in kexec_load_slot() because parallel > loads make issues too. And we can go higher to feel more safe. That is > why I suggested do_kexec_op_internal() as the final resting place for > an ASSERT(). Simply it looks to me the safest approach. Am I missing > something? The point you're missing is - why don't you then move the ASSERT() yet one more level up, right next to where the flag is being set? IOW what you suggest would imo rather mean not adding any assertion at all. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level 2017-04-19 13:37 ` Jan Beulich @ 2017-04-19 15:49 ` Eric DeVolder 0 siblings, 0 replies; 15+ messages in thread From: Eric DeVolder @ 2017-04-19 15:49 UTC (permalink / raw) To: Jan Beulich, Daniel Kiper; +Cc: andrew.cooper3, bhavesh.davda, xen-devel On 04/19/2017 08:37 AM, Jan Beulich wrote: >>>> On 19.04.17 at 14:13, <daniel.kiper@oracle.com> wrote: >> On Wed, Apr 19, 2017 at 05:20:50AM -0600, Jan Beulich wrote: >>>>>> On 19.04.17 at 12:56, <daniel.kiper@oracle.com> wrote: >>>> On Tue, Apr 18, 2017 at 04:49:48AM -0600, Jan Beulich wrote: >>>>>>>> On 17.04.17 at 21:09, <eric.devolder@oracle.com> wrote: >>>>>> The spinlock in kexec_swap_images() was removed as >>>>>> this function is only reachable on the kexec hypercall, which is >>>>>> now protected at the top-level in do_kexec_op_internal(), >>>>>> thus the local spinlock is no longer necessary. >>>>> >>>>> But perhaps leave an ASSERT() there, making sure the in-hypercall >>>>> flag is set? >>>> >>>> I am not sure why but if at all I think that we should also consider >>>> other key kexec functions then. Or put ASSERT() into do_kexec_op_internal() >>>> just before "switch ( op )". >>> >>> The point of my placement suggestion was that the ASSERT() >>> effectively replaces the lock acquire - the places you name >>> didn't previously require any synchronization. >> >> After the first patch of this series kexec_swap_images() cannot be >> started twice in parallel. So, I do not see the point of ASSERT() here. >> Or let's say we wish to have it to double check that "the in-hypercall >> flag is set". AIUI, it is your original idea. However, then I think that >> we should have an ASSERT() at least in kexec_load_slot() because parallel >> loads make issues too. And we can go higher to feel more safe. That is >> why I suggested do_kexec_op_internal() as the final resting place for >> an ASSERT(). Simply it looks to me the safest approach. Am I missing >> something? > > The point you're missing is - why don't you then move the ASSERT() > yet one more level up, right next to where the flag is being set? IOW > what you suggest would imo rather mean not adding any assertion > at all. I've just posted v3 of this patch with the ASSERT in lieu of the spin_lock(). Eric _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-04-19 15:49 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-17 19:09 [PATCH v2 0/2] kexec: Use hypercall_create_continuation to protect KEXEC ops Eric DeVolder 2017-04-17 19:09 ` [PATCH v2 1/2] kexec: use " Eric DeVolder 2017-04-18 10:48 ` Jan Beulich 2017-04-19 11:00 ` Daniel Kiper 2017-04-19 11:48 ` Andrew Cooper 2017-04-19 12:16 ` Daniel Kiper 2017-04-19 15:48 ` Eric DeVolder 2017-04-17 19:09 ` [PATCH v2 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level Eric DeVolder 2017-04-18 10:49 ` Jan Beulich 2017-04-19 10:56 ` Daniel Kiper 2017-04-19 11:20 ` Jan Beulich 2017-04-19 11:52 ` Andrew Cooper 2017-04-19 12:13 ` Daniel Kiper 2017-04-19 13:37 ` Jan Beulich 2017-04-19 15:49 ` Eric DeVolder
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.