linux-kernel - Re: [PATCH 25/25] [PATCH] add paravirtualization support for x86

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <46BAB5C8.5090906@goop.org>
Date:	Wed, 08 Aug 2007 23:35:52 -0700
From:	Jeremy Fitzhardinge <jeremy@...p.org>
To:	Glauber de Oliveira Costa <gcosta@...hat.com>
CC:	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
	rusty@...tcorp.com.au, ak@...e.de, mingo@...e.hu,
	chrisw@...s-sol.org, avi@...ranet.com, anthony@...emonkey.ws,
	virtualization@...ts.linux-foundation.org, lguest@...abs.org,
	Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/