Re: [PATCH v3 07/23] xsplice: Implement support for applying/reverting/replacing patches. (v5)

From: Ross Lagerwall <ross.lagerwall@citrix.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: xen-devel@lists.xenproject.org, Jan Beulich <jbeulich@suse.com>
Subject: Re: [PATCH v3 07/23] xsplice: Implement support for applying/reverting/replacing patches. (v5)
Date: Fri, 19 Feb 2016 09:30:18 +0000	[thread overview]
Message-ID: <56C6E0AA.8030603@citrix.com> (raw)
In-Reply-To: <56C3744E.8000702@citrix.com>

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 <xen/cpu.h>
>>   #include <xen/wait.h>
>>   #include <xen/guest_access.h>
>> +#include <xen/xsplice.h>
>>   #include <public/sysctl.h>
>>   #include <public/hvm/hvm_vcpu.h>
>>   #include <asm/regs.h>
>> @@ -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