On Mon, Aug 15, 2016 at 05:15:28AM -0600, Jan Beulich wrote:
> >>> On 14.08.16 at 23:52, <konrad.w...@oracle.com> wrote:
> > v4..v11: Defered for v4.8
> > v12: s/xsplice/livepatch/
> > v13: Clarify the comments about spin_debug_enable
>
> (Side note: v13 here vs v3 in the subject.)
>
> > Rename one of the hooks to lower-case (Z->z) to guarantee it being
> > called last.
>
> Does lower case z really guarantee that? Wouldn't it be better to
> use a sort order independent mechanism, like using another object
> file (iirc object file order defines placement-within-sections unless
> options get handed to the linker which specifically allow it to
> shuffle things around)?
>
> > @@ -72,7 +73,11 @@ struct payload {
> > struct livepatch_build_id dep; /*
> > ELFNOTE_DESC(.livepatch.depends). */
> > void *bss; /* .bss of the payload. */
> > size_t bss_size; /* and its size. */
> > - char name[XEN_LIVEPATCH_NAME_SIZE]; /* Name of it. */
> > + livepatch_loadcall_t **load_funcs; /* The array of funcs to call
> > after */
> > + livepatch_unloadcall_t **unload_funcs;/* load and unload of the
> > payload. */
>
> Considering above you said "Learned a lot of about 'const'", where
> are they? (Interestingly, LIVEPATCH_{,UN}LOAD_HOOK() below look
> correct now, so effectively you lose constness here.)
Can't do const here at all. Any placement of them will make the compile
omit the call to them.
That is either one of:
const livepatch_loadcall_t **load_funcs;
livepatch_loadcall_t const **load_funcs;
results in (on gcc-5.2.1):
.loc 1 1152 0
call spin_debug_disable
.LVL69:
.loc 1 1153 0
movl 324(%r12), %edx
testl %edx, %edx
je .L32
movl $0, %eax
.LVL70:
.L33:
.loc 1 1153 0 is_stmt 0 discriminator 3
addl $1, %eax
.LVL71:
cmpl %edx, %eax
jne .L33
.LVL72:
.L32:
.loc 1 1155 0 is_stmt 1
call spin_debug_enable
(on older compiler 4.4.4) I get:
.L122:
.loc 1 1108 0
call spin_debug_disable
.LVL175:
.loc 1 1111 0
.p2align 4,,8
call spin_debug_enable
>
> > @@ -1065,6 +1089,18 @@ static int apply_payload(struct payload *data)
> >
> > arch_livepatch_quiesce();
> >
> > + /*
> > + * Since we are running with IRQs disabled and the hooks may call
> > common
> > + * code - which expects the spinlocks to run with IRQs enabled - we
> > temporarly
> > + * disable the spin locks IRQ state checks.
> > + */
>
> Much better, but a little further to go: I'd suggest
> s/the spinlocks/certain spinlocks/ or some such. Also - "temporarily".
>
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
