Glauber de Oliveira Costa wrote:
> +static unsigned native_patch(u8 type, u16 clobbers, void *insns, unsigned
> len)
> +{
> + const unsigned char *start, *end;
> + unsigned ret;
> +
> + switch(type) {
> +#define SITE(x) case PARAVIRT_PATCH(x): start = start_##x; end =
> end_##x; goto patch_site
> + SITE(irq_disable);
> + SITE(irq_enable);
> + SITE(restore_fl);
> + SITE(save_fl);
> + SITE(iret);
> + SITE(sysret);
> + SITE(swapgs);
> + SITE(read_cr2);
> + SITE(read_cr3);
> + SITE(write_cr3);
> + SITE(clts);
> + SITE(flush_tlb_single);
> + SITE(wbinvd);
> +#undef SITE
> +
> + patch_site:
> + ret = paravirt_patch_insns(insns, len, start, end);
> + break;
> +
> + case PARAVIRT_PATCH(make_pgd):
> + case PARAVIRT_PATCH(pgd_val):
> + case PARAVIRT_PATCH(make_pte):
> + case PARAVIRT_PATCH(pte_val):
> + case PARAVIRT_PATCH(make_pmd):
> + case PARAVIRT_PATCH(pmd_val):
> + case PARAVIRT_PATCH(make_pud):
> + case PARAVIRT_PATCH(pud_val):
> + /* These functions end up returning what
> + they're passed in the first argument */
>
Is this still true with 64-bit? Either way, I don't think its worth
having this here. The damage to codegen around all those sites has
already happened, and the additional cost of a noop direct call is
pretty trivial. I think this is a nanooptimisation which risks more
problems than it could possibly be worth.
> + case PARAVIRT_PATCH(set_pte):
> + case PARAVIRT_PATCH(set_pmd):
> + case PARAVIRT_PATCH(set_pud):
> + case PARAVIRT_PATCH(set_pgd):
> + /* These functions end up storing the second
> + * argument in the location pointed by the first */
> + ret = paravirt_patch_store_reg(insns, len);
> + break;
>
Ditto, really. Do this in a later patch if it actually seems to help.
> +unsigned paravirt_patch_copy_reg(void *site, unsigned len)
> +{
> + unsigned char *mov = site;
> + if (len < 3)
> + return len;
> +
> + /* This is mov %rdi, %rax */
> + *mov++ = 0x48;
> + *mov++ = 0x89;
> + *mov = 0xf8;
> + return 3;
> +}
> +
> +unsigned paravirt_patch_store_reg(void *site, unsigned len)
> +{
> + unsigned char *mov = site;
> + if (len < 3)
> + return len;
> +
> + /* This is mov %rsi, (%rdi) */
> + *mov++ = 0x48;
> + *mov++ = 0x89;
> + *mov = 0x37;
> + return 3;
> +}
>
These seem excessively special-purpose. Are their only uses the ones I
commented on above.
> +/*
> + * integers must be use with care here. They can break the PARAVIRT_PATCH(x)
> + * macro, that divides the offset in the structure by 8, to get a number
> + * associated with the hook. Dividing by four would be a solution, but it
> + * would limit the future growth of the structure if needed.
>
Why not just stick them at the end of the structure?
J
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
