Re: [Xen-devel] [PATCH v3 9/9] livepach: Add .livepatch.hooks functions and test-case

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Mon, Aug 15, 2016 at 05:15:28AM -0600, Jan Beulich wrote:
> >>> On 14.08.16 at 23:52, <konrad.wilk@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel