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]

Date:	Wed, 8 Aug 2007 12:00:12 +0200
From:	Andi Kleen <ak@...e.de>
To:	Glauber de Oliveira Costa <gcosta@...hat.com>
Cc:	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
	rusty@...tcorp.com.au, mingo@...e.hu, chrisw@...s-sol.org,
	jeremy@...p.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


> +config PARAVIRT
> +       bool "Paravirtualization support (EXPERIMENTAL)"

This should be hidden and selected by the clients as needed
(I already did this change on i386) 

Users know nothing about paravirt, they just know about Xen, lguest
etc.

Strictly you would at least need a !X86_VSMP dependency, but
with the vsmp change i requested that will be unnecessary

Is this really synced with the latest version of the i386 code?


> +#ifdef CONFIG_PARAVIRT
> +#include <asm/paravirt.h>
> +#endif


> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/efi.h>
> +#include <linux/bcd.h>
> +#include <linux/start_kernel.h>
> +
> +#include <asm/bug.h>
> +#include <asm/paravirt.h>
> +#include <asm/desc.h>
> +#include <asm/setup.h>
> +#include <asm/irq.h>
> +#include <asm/delay.h>
> +#include <asm/fixmap.h>
> +#include <asm/apic.h>
> +#include <asm/tlbflush.h>
> +#include <asm/msr.h>
> +#include <asm/page.h>
> +#include <asm/pgtable.h>
> +#include <asm/proto.h>
> +#include <asm/e820.h>
> +#include <asm/time.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/smp.h>
> +#include <asm/irqflags.h>


Are the includes really all needed?


> +	if (opfunc == NULL)
> +		/* If there's no function, patch it with a ud2a (BUG) */
> +		ret = paravirt_patch_insns(site, len, start_ud2a, end_ud2a);

This will actually give corrupted BUGs because you don't supply
the full inline BUG header. Perhaps another trap would be better.


> +EXPORT_SYMBOL(paravirt_ops);

Definitely _GPL at least.



> +extern struct paravirt_ops paravirt_ops;

Should be native_paravirt_ops I guess

> +
> + * This generates an indirect call based on the operation type number.

The macros here don't

> +static inline unsigned long read_msr(unsigned int msr)
> +{
> +	int __err;

No need for __ in inlines

> +/* The paravirtualized I/O functions */
> +static inline void slow_down_io(void) {

I doubt this needs to be inline and it's large

> +	__asm__ __volatile__(paravirt_alt(PARAVIRT_CALL)

No __*__ in new code please

> +			     : "=a"(f)
> +			     : paravirt_type(save_fl),
> +			       paravirt_clobber(CLBR_RAX)
> +			     : "memory", "cc");
> +	return f;
> +}
> +
> +static inline void raw_local_irq_restore(unsigned long f)
> +{
> +	__asm__ __volatile__(paravirt_alt(PARAVIRT_CALL)
> +			     :
> +			     : "D" (f),

Have you investigated if a different input register generates better/smaller 
code? I would assume rdi to be usually used already for the caller's 
arguments so it will produce spilling

Similar for the rax return in the other functions.



-Andi
-
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/