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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.)

> @@ -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