Re: rough sketch of revised patching infrastructure

[Rusty Russell <rusty@rustcorp.com.au>]

On Thu, 2007-02-22 at 02:22 -0800, Jeremy Fitzhardinge wrote:
> Rusty Russell wrote:
> > On Wed, 2007-02-21 at 18:09 -0800, Jeremy Fitzhardinge wrote:
> >   
> >> Here's the new patching patch.  It compiles, but it doesn't, you know,
> >> boot, as such.
> >>     
> >
> > OK, there are several separate things here.
> > (1) Get rid of the PARAVIRT_IRQ_DISABLE etc constants in favour of
> > offsetof within the structure.
> > (2) Change (almost) all the paravirt hooks to be patchable.
> > (3) Genericise the infrastructure to table-driven.
> >
> > These should probably become separate patches, in fact.
> >   
> 
> OK, here's something which is more presentable, and it even boots (at
> least under Xen; haven't tried native).
> 
> paravirt-patch-rename-paravirt_patch.patch

Great.

> paravirt-use-offset-site-ids.patch

Cool.

> paravirt-fix-clobbers.patch
>  - misc cleanups

Cool.

> paravirt-patchable-call-wrappers.patch
>  - wrap a large number of the paravirt calls with the magic to make the
> callsites patchable.  More are possible; this is just a good first step.

Ugly, but I originally played with alternatives for a long time and mine
were no prettier.

> paravirt-patch-machinery.patch
>  - the actual patching machinery, which is function-pointer driven
> rather than table driven now.

Not quite so sure on this one.  We loop through calling
paravirt_ops.patch() for each patch.  In the native case, this calls
paravirt_patcher with a fn pointer, which is simply called by
paravirt_patcher.

I would think you want to replace paravirt_patcher's patch == NULL case
with a special "paravity_patch_default", and then have to paravirt_ops
patch function call that specifically when it wants a default.

Also, I think you can leave the native table almost as-is, eg:

static const struct native_insns
{
       const char *start, *end;
} native_insns[] = {
       [PARAVIRT_PATCH(irq_disable)] = { start_cli, end_cli },
       [PARAVIRT_PATCH(irq_enable)] = { start_sti, end_sti },
...
};
static unsigned native_patch(u8 type, u16 clobbers, void *insns, unsigned len)
{
       unsigned int insn_len;

       /* Don't touch it if we don't have a replacement */
       if (type >= ARRAY_SIZE(native_insns) || !native_insns[type].start)
               return paravirt_patch_default(type, clobbers, insns, len);
       insn_len = native_insns[type].end - native_insns[type].start;

       /* Similarly if we can't fit replacement. */
       if (len < insn_len)
               return paravirt_patch_default(type, clobbers, insns, len);

       return paravirt_patch_insns(insns, len, native_insns[type].start, native_insns[type].end);
}

Actually, your paravirt_patch_insns has similar logic anyway, so this
code could collapse (it should fall back to paravirt_patch_default tho
IMHO).

Thanks for doing this work!
Rusty.