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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

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

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