lkml.org 
[lkml]   [2007]   [Aug]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 25/25] [PATCH] add paravirtualization support for x86_64
    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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-08-09 08:39    [W:2.105 / U:0.768 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site