Re: [Xen-devel] [PATCH v8.1 19/27] xsplice: Add support for alternatives

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 04/14/16 12:02 AM >>>
>--- a/xen/arch/x86/Makefile
>+++ b/xen/arch/x86/Makefile
>@@ -6,7 +6,11 @@ subdir-y += mm
 >subdir-$(CONFIG_XENOPROF) += oprofile
 >subdir-y += x86_64
 >
>+ifdef CONFIG_XSPLICE
>+obj-y += alternative.o
>+else
 >obj-bin-y += alternative.init.o
>+endif
 
For now this is okay, but please try to use the list model in favor of ifdef-s 
in
the future, whenever possible.

>#ifdef K8_NOP1
>-static const unsigned char k8nops[] __initconst = {
>+static const unsigned char k8nops[] __INITCONST = {
     >K8_NOP1,
     >K8_NOP2,
     >K8_NOP3,
>@@ -52,7 +53,7 @@ static const unsigned char * const k8_nops[ASM_NOP_MAX+1] 
>__initconstrel = {
 
The context here suggests you didn't complete your conversion work: The
__initconstrel hardly can remain unchanged if the objects are now being
referenced by objects in non-init sections.

>@@ -125,9 +126,10 @@ static void __init add_nops(void *insns, unsigned int len)
  >* instructions. And on the local CPU you need to be protected again NMI or 
MCE
  >* handlers seeing an inconsistent instruction while you patch.
  >*
>- * This routine is called with local interrupt disabled.
>+ * You should run this with interrupts disabled or on code that has never
>+ * been executed.
  
The second half is misleading - there's no such requirement for early boot
patching. The requirement is that the code must not currently be executing
(which at that time is guaranteed by there only being a single CPU, which
is in a well defined region of code).

>@@ -12,6 +15,8 @@ const char *xen_hello_world(void)
 >{
     >unsigned long tmp;
     >int rc;
>+
>+    alternative(ASM_NOP1, ASM_NOP1, X86_FEATURE_LM);
     
So to cover the issue pointed out above you should be patching a longer
instruction with a shorter one here.

>--- a/xen/common/xsplice.c
>+++ b/xen/common/xsplice.c
>@@ -533,6 +533,39 @@ static int prepare_payload(struct payload *payload,
     >}
 >
 >#ifndef CONFIG_ARM
>+    sec = xsplice_elf_sec_by_name(elf, ".altinstructions");
>+    if ( sec )
>+    {
>+        struct alt_instr *a, *start, *end;
>+
>+        if ( !sec->sec->sh_size ||

Once again I don't see what harm a zero size section would do.

>+             (sec->sec->sh_size % sizeof(*a)) )
>+        {
>+            dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .alt_instr 
>(exp:%lu vs %lu)!\n",
>+                    elf->name, sizeof(*a),

sizeof() wants to be formatted with %zu and ...

>+                    sec->sec->sh_size);

... didn't you add an ELF-specific format macro for cases like this?

Also if you printed the message outside of the conditional, expected and actual
values would likely disagree anyway (as soon as there are multiple entries), so
some editing of the text would be nice too.

>+            return -EINVAL;
>+        }
>+
>+        start = sec->load_addr;
>+        end = sec->load_addr + sec->sec->sh_size;
>+
>+        for ( a = start; a < end; a++ )
>+        {
>+            unsigned long instr = (unsigned long)(&a->instr_offset + 
>a->instr_offset);
>+            unsigned long replacement = (unsigned long)(&a->repl_offset + 
>a->repl_offset);
>+
>+            if ( (instr < region->start && instr >= region->end) ||
>+                 (replacement < region->start && replacement >= region->end) )
>+            {
>+                dprintk(XENLOG_ERR, XSPLICE "%s Alt patching outside payload: 
>0x%lx!\n",

%#lx please

>@@ -555,6 +588,7 @@ static int prepare_payload(struct payload *payload,
         >region->ex = s;
         >region->ex_end = e;
     >}
>+
 >#endif
 
Stray addition of a blank line.

Jan


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