Re: [PATCH 25/25] [PATCH] add paravirtualization support for x86_64

[Jeremy Fitzhardinge <jeremy@goop.org>]

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