From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ross Lagerwall Subject: Re: [PATCH v3 07/23] xsplice: Implement support for applying/reverting/replacing patches. (v5) Date: Fri, 19 Feb 2016 09:30:18 +0000 Message-ID: <56C6E0AA.8030603@citrix.com> References: <1455300361-13092-1-git-send-email-konrad.wilk@oracle.com> <1455300361-13092-8-git-send-email-konrad.wilk@oracle.com> <56C3744E.8000702@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aWhOI-0005b3-MJ for xen-devel@lists.xenproject.org; Fri, 19 Feb 2016 09:30:22 +0000 In-Reply-To: <56C3744E.8000702@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: xen-devel@lists.xenproject.org, Jan Beulich List-Id: xen-devel@lists.xenproject.org On 02/16/2016 07:11 PM, Andrew Cooper wrote: > On 12/02/16 18:05, Konrad Rzeszutek Wilk wrote: >> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c >> index 9d43f7b..b5995b9 100644 >> --- a/xen/arch/x86/domain.c >> +++ b/xen/arch/x86/domain.c >> @@ -36,6 +36,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -121,6 +122,7 @@ static void idle_loop(void) >> (*pm_idle)(); >> do_tasklet(); >> do_softirq(); >> + do_xsplice(); /* Must be last. */ > > Then name "do_xsplice()" is slightly misleading (although it is in equal > company here). check_for_xsplice_work() would be more accurate. > >> diff --git a/xen/arch/x86/xsplice.c b/xen/arch/x86/xsplice.c >> index 814dd52..ae35e91 100644 >> --- a/xen/arch/x86/xsplice.c >> +++ b/xen/arch/x86/xsplice.c >> @@ -10,6 +10,25 @@ >> __func__,__LINE__, x); return x; } >> #endif >> >> +#define PATCH_INSN_SIZE 5 > > Somewhere you should have a BUILD_BUG_ON() confirming that > PATCH_INSN_SIZE fits within the undo array. > > Having said that, I think all of xsplice_patch_func should be > arch-specific rather than generic. > >> + >> +void xsplice_apply_jmp(struct xsplice_patch_func *func) >> +{ >> + uint32_t val; >> + uint8_t *old_ptr; >> + >> + old_ptr = (uint8_t *)func->old_addr; >> + memcpy(func->undo, old_ptr, PATCH_INSN_SIZE); > > At least a newline here please. > >> + *old_ptr++ = 0xe9; /* Relative jump */ >> + val = func->new_addr - func->old_addr - PATCH_INSN_SIZE; > > E9 takes a rel32 parameter, which is signed. > > I think you need to explicitly cast to intptr_t and used signed > arithmetic throughout this calculation to correctly calculate a > backwards jump. According to my testing and expectations based on the spec and GCC's implementation-defined behaviour, the offset is computed correctly for backward (and forward) jumps. I'm sure the types can be improved though... > > I think there should also be some sanity checks that both old_addr and > new_addr are in the Xen 1G virtual region. > OK. Though these sanity checks should happen when loading the patch, not applying it. -- Ross Lagerwall