Re: [Xen-devel] [PATCH v7 3/5] livepatch: NOP if func->new_addr is zero.

[Jan Beulich]

>>> On 21.09.16 at 18:57,  wrote:
> The NOP functionality will NOP any of the code at
> the 'old_addr' or at 'name' if the 'new_addr' is zero.
> The purpose of this is to NOP out calls, such as:
> 
>  e8 <4-bytes-offset>
> 
> (5 byte insn), or on ARM a 4 byte insn for branching.
> 
> We need the EIP of where we need to the NOP, and that can
> be provided via the `old_addr` or `name`.
> 
> If the `old_addr` is provided we will NOP 'new_size'
> amount of bytes at that location.
> 
> The amount is up to 31 instructions if desired (which is
> the size of the opaque member). If there is a need to NOP
> more then: a) more 'struct livepatch_func' structures need
> to be present, b) we have to implement a variable size
> buffer (in the future), or c) first byte an unconditional
> branch skipping the to be disabled code (of course provided
> there are no branch targets in the middle).
> 
> While at it, also unify the code on x86 patching so
> it is a bit simpler (instead of two seperate writes
> just make it one memcpy).
> 
> And introduce a general livepatch_insn_len inline function
> that would depend on platform specific instruction size
> (for a unconditional branch). As such we also rename the
> PATCH_INSN_SIZE to ARCH_PATCH_INSN_SIZE.
> 
> Signed-off-by: Konrad Rzeszutek Wilk 

Reviewed-by: Jan Beulich 


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v7 3/5] livepatch: NOP if func->new_addr is zero.

[Konrad Rzeszutek Wilk]

The NOP functionality will NOP any of the code at
the 'old_addr' or at 'name' if the 'new_addr' is zero.
The purpose of this is to NOP out calls, such as:

 e8 <4-bytes-offset>

(5 byte insn), or on ARM a 4 byte insn for branching.

We need the EIP of where we need to the NOP, and that can
be provided via the `old_addr` or `name`.

If the `old_addr` is provided we will NOP 'new_size'
amount of bytes at that location.

The amount is up to 31 instructions if desired (which is
the size of the opaque member). If there is a need to NOP
more then: a) more 'struct livepatch_func' structures need
to be present, b) we have to implement a variable size
buffer (in the future), or c) first byte an unconditional
branch skipping the to be disabled code (of course provided
there are no branch targets in the middle).

While at it, also unify the code on x86 patching so
it is a bit simpler (instead of two seperate writes
just make it one memcpy).

And introduce a general livepatch_insn_len inline function
that would depend on platform specific instruction size
(for a unconditional branch). As such we also rename the
PATCH_INSN_SIZE to ARCH_PATCH_INSN_SIZE.

Signed-off-by: Konrad Rzeszutek Wilk 

---
Cc: Konrad Rzeszutek Wilk 
Cc: Ross Lagerwall 
Cc: Jan Beulich 
Cc: Andrew Cooper 

v3: First submission
v4: Fix description - e9 -> e8
Remove the restriction of only doing 5 or 4 bytes.
Redo the patching code to deal with variable size of new_size.
Expand the amount of bytes we can NOP.
Move the PATCH_INSN_SIZE definition in platform specific headers
Move the get_len to livepatch_get_insn_len inline function.
v5: s/PATCH_INSN_SIZE/ARCH_PATCH_INSN_SIZE/
s/arch_livepatch_insn_len/livepatch_insn_len/
s/size_t len/unsigned int len/
Add in commit description the c) mechanism (insert an unconditional
branch).
v6: Expand in the documentation about the old_addr being at least 5.
v7: Added the extra entry in the MAINTAINERS file.
  - Expanded on the livepatch.markdown the description.
  - Used add_nops(insn, .. instead of add_nops(, ..)
---
 MAINTAINERS   |  1 +
 docs/misc/livepatch.markdown  | 22 ++
 xen/arch/x86/alternative.c|  2 +-
 xen/arch/x86/livepatch.c  | 48 +++
 xen/common/livepatch.c|  3 ++-
 xen/include/asm-x86/alternative.h |  1 +
 xen/include/asm-x86/livepatch.h   | 21 +
 xen/include/xen/livepatch.h   | 10 
 8 files changed, 86 insertions(+), 22 deletions(-)
 create mode 100644 xen/include/asm-x86/livepatch.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 97720a8..9b30600 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -270,6 +270,7 @@ F:  docs/misc/livepatch.markdown
 F:  tools/misc/xen-livepatch.c
 F:  xen/arch/*/livepatch*
 F:  xen/common/livepatch*
+F:  xen/include/asm-*/livepatch.h
 F:  xen/include/xen/livepatch*
 
 MACHINE CHECK (MCA) & RAS
diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 81f4fc9..37a0860 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -318,13 +318,24 @@ The size of the structure is 64 bytes on 64-bit 
hypervisors. It will be
   payload generation time if hypervisor function address is known. If unknown,
   the value *MUST* be zero and the hypervisor will attempt to resolve the 
address.
 
-* `new_addr` is the address of the function that is replacing the old
-  function. The address is filled in during relocation. The value **MUST** be
-  the address of the new function in the file.
+* `new_addr` can either have a non-zero value or be zero.
+  * If there is a non-zero value, then it is the address of the function that 
is
+replacing the old function and the address is recomputed during relocation.
+The value **MUST** be the address of the new function in the payload file.
 
-* `old_size` and `new_size` contain the sizes of the respective functions in 
bytes.
+  * If the value is zero, then we NOPing out at the `old_addr` location
+`new_size` bytes.
+
+* `old_size` contains the sizes of the respective `old_addr` function in bytes.
The value of `old_size` **MUST** not be zero.
 
+* `new_size` depends on what `new_addr` contains:
+  * If `new_addr` contains an non-zero value, then `new_size` has the size of
+the new function (which will replace the one at `old_addr`)  in bytes.
+  * If the value of `new_addr` is zero then `new_size` determines how many
+instruction bytes to NOP (up to opaque size modulo smallest platform
+instruction - 1 byte x86 and 4 bytes on ARM).
+
 * `version` is to be one.
 
 * `opaque` **MUST** be zero.
@@ -1087,7 +1098,8 @@ limit that calls the next trampoline.
 Please note there is a small limitation for trampolines in
 function entries: The target function (+ trailing padding) must be able
 to accomodate