All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andi Kleen <ak@suse.de>
To: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: virtualization@lists.osdl.org,
	Anthony Liguori <anthony@codemonkey.ws>,
	Andrew Morton <akpm@linux-foundation.org>,
	lkml <linux-kernel@vger.kernel.org>
Subject: Re: [patch 12/17] Consistently wrap paravirt ops callsites to make them patchable
Date: Mon, 2 Apr 2007 09:11:50 +0200	[thread overview]
Message-ID: <200704020911.50623.ak@suse.de> (raw)
In-Reply-To: <20070402055705.090656820@goop.org>

On Monday 02 April 2007 07:57, Jeremy Fitzhardinge wrote:
> Wrap a set of interesting paravirt_ops calls in a wrapper which makes
> the callsites available for patching.  Unfortunately this is pretty
> ugly because there's no way to get gcc to generate a function call,
> but also wrap just the callsite itself with the necessary labels.
> 
> This patch supports functions with 0-4 arguments, and either void or
> returning a value.  64-bit arguments must be split into a pair of
> 32-bit arguments (lower word first).  Small structures are returned in
> registers.

Can you please add some comments to the code explaining this a little?
Best would be perhaps a overview document in Documentation too.

> +#define PVOP_CALL0(__rettype, __op)					\

The __s shouldn't be needed for the macro arguments because
there is no shared name space with the caller.

> +	({								\
> +		__rettype __ret;					\
> +		if (sizeof(__rettype) > sizeof(unsigned long)) {	\
> +			unsigned long long __tmp;			\
> +			unsigned long __ecx;				\
> +			asm volatile(paravirt_alt(PARAVIRT_CALL)	\

Not having the volatile would probably generate better code, but it 
seems much safer for now.

> +				     : "=A" (__tmp), "=c" (__ecx)	\
> +				     : paravirt_type(__op),		\
> +				       paravirt_clobber(CLBR_ANY)	\
> +				     : "memory", "cc");			\

And the cc clobber is also not needed

-Andi

WARNING: multiple messages have this Message-ID (diff)
From: Andi Kleen <ak@suse.de>
To: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	virtualization@lists.osdl.org,
	lkml <linux-kernel@vger.kernel.org>,
	Rusty Russell <rusty@rustcorp.com.au>,
	Zachary Amsden <zach@vmware.com>,
	Anthony Liguori <anthony@codemonkey.ws>
Subject: Re: [patch 12/17] Consistently wrap paravirt ops callsites to make them patchable
Date: Mon, 2 Apr 2007 09:11:50 +0200	[thread overview]
Message-ID: <200704020911.50623.ak@suse.de> (raw)
In-Reply-To: <20070402055705.090656820@goop.org>

On Monday 02 April 2007 07:57, Jeremy Fitzhardinge wrote:
> Wrap a set of interesting paravirt_ops calls in a wrapper which makes
> the callsites available for patching.  Unfortunately this is pretty
> ugly because there's no way to get gcc to generate a function call,
> but also wrap just the callsite itself with the necessary labels.
> 
> This patch supports functions with 0-4 arguments, and either void or
> returning a value.  64-bit arguments must be split into a pair of
> 32-bit arguments (lower word first).  Small structures are returned in
> registers.

Can you please add some comments to the code explaining this a little?
Best would be perhaps a overview document in Documentation too.

> +#define PVOP_CALL0(__rettype, __op)					\

The __s shouldn't be needed for the macro arguments because
there is no shared name space with the caller.

> +	({								\
> +		__rettype __ret;					\
> +		if (sizeof(__rettype) > sizeof(unsigned long)) {	\
> +			unsigned long long __tmp;			\
> +			unsigned long __ecx;				\
> +			asm volatile(paravirt_alt(PARAVIRT_CALL)	\

Not having the volatile would probably generate better code, but it 
seems much safer for now.

> +				     : "=A" (__tmp), "=c" (__ecx)	\
> +				     : paravirt_type(__op),		\
> +				       paravirt_clobber(CLBR_ANY)	\
> +				     : "memory", "cc");			\

And the cc clobber is also not needed

-Andi

  reply	other threads:[~2007-04-02  7:11 UTC|newest]

Thread overview: 62+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-04-02  5:56 [patch 00/17] paravirt_ops updates Jeremy Fitzhardinge
2007-04-02  5:56 ` Jeremy Fitzhardinge
2007-04-02  5:56 ` [patch 01/17] update MAINTAINERS Jeremy Fitzhardinge
2007-04-02  5:56 ` [patch 02/17] Remove CONFIG_DEBUG_PARAVIRT Jeremy Fitzhardinge
2007-04-02  5:56 ` [patch 03/17] use paravirt_nop to consistently mark no-op operations Jeremy Fitzhardinge
2007-04-02  5:56   ` Jeremy Fitzhardinge
2007-04-02  5:56 ` [patch 04/17] Add pagetable accessors to pack and unpack pagetable entries Jeremy Fitzhardinge
2007-04-02  5:56   ` Jeremy Fitzhardinge
2007-04-02  6:12   ` Andi Kleen
2007-04-02  6:35     ` Rusty Russell
2007-04-02  6:48     ` Jeremy Fitzhardinge
2007-04-02  6:48       ` Jeremy Fitzhardinge
2007-04-04  9:25     ` Jeremy Fitzhardinge
2007-04-04  9:57       ` Ingo Molnar
2007-04-04 11:49         ` Andi Kleen
2007-04-04 15:43         ` Jeremy Fitzhardinge
2007-04-04 15:46           ` Ingo Molnar
2007-04-04 16:10             ` Jeremy Fitzhardinge
2007-04-04 11:47       ` Andi Kleen
2007-04-04 15:45         ` Jeremy Fitzhardinge
2007-04-04 15:56           ` Andi Kleen
2007-04-04 16:04             ` Jeremy Fitzhardinge
2007-04-04 22:59             ` Rusty Russell
2007-04-04 22:59               ` Rusty Russell
2007-04-04 23:39               ` Jeremy Fitzhardinge
2007-04-02  5:56 ` [patch 05/17] Hooks to set up initial pagetable Jeremy Fitzhardinge
2007-04-02  5:56   ` Jeremy Fitzhardinge
2007-04-02  5:56 ` [patch 06/17] Allocate a fixmap slot Jeremy Fitzhardinge
2007-04-02  5:56   ` Jeremy Fitzhardinge
2007-04-02  5:56 ` [patch 07/17] Allow paravirt backend to choose kernel PMD sharing Jeremy Fitzhardinge
2007-04-02  5:56   ` Jeremy Fitzhardinge
2007-04-02  5:57 ` [patch 08/17] add hooks to intercept mm creation and destruction Jeremy Fitzhardinge
2007-04-02  5:57   ` Jeremy Fitzhardinge
2007-04-02  5:57 ` [patch 09/17] rename struct paravirt_patch to paravirt_patch_site for clarity Jeremy Fitzhardinge
2007-04-02  5:57   ` Jeremy Fitzhardinge
2007-04-02  5:57 ` [patch 10/17] Use patch site IDs computed from offset in paravirt_ops structure Jeremy Fitzhardinge
2007-04-02  5:57   ` Jeremy Fitzhardinge
2007-04-02  5:57 ` [patch 11/17] Fix patch site clobbers to include return register Jeremy Fitzhardinge
2007-04-02  5:57   ` Jeremy Fitzhardinge
2007-04-02  5:57 ` [patch 12/17] Consistently wrap paravirt ops callsites to make them patchable Jeremy Fitzhardinge
2007-04-02  7:11   ` Andi Kleen [this message]
2007-04-02  7:11     ` Andi Kleen
2007-04-02  7:20     ` Jeremy Fitzhardinge
2007-04-02  5:57 ` [patch 13/17] add common patching machinery Jeremy Fitzhardinge
2007-04-02  5:57   ` Jeremy Fitzhardinge
2007-04-02  5:57 ` [patch 14/17] add flush_tlb_others paravirt_op Jeremy Fitzhardinge
2007-04-02  5:57   ` Jeremy Fitzhardinge
2007-04-02  5:57 ` [patch 15/17] revert map_pt_hook Jeremy Fitzhardinge
2007-04-02  5:57   ` Jeremy Fitzhardinge
2007-04-02  5:57 ` [patch 16/17] add kmap_atomic_pte for mapping highpte pages Jeremy Fitzhardinge
2007-04-02  7:18   ` Andi Kleen
2007-04-02  7:18     ` Andi Kleen
2007-04-02  7:22     ` Jeremy Fitzhardinge
2007-04-02  7:22       ` Jeremy Fitzhardinge
2007-04-02  5:57 ` [patch 17/17] Add a sched_clock paravirt_op Jeremy Fitzhardinge
2007-04-02  5:57   ` Jeremy Fitzhardinge
2007-04-02  6:09   ` Andi Kleen
2007-04-02  6:09     ` Andi Kleen
2007-04-02  6:47     ` Jeremy Fitzhardinge
2007-04-02  6:50       ` Andi Kleen
2007-04-02  6:50         ` Andi Kleen
2007-04-02  7:06         ` Jeremy Fitzhardinge

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=200704020911.50623.ak@suse.de \
    --to=ak@suse.de \
    --cc=akpm@linux-foundation.org \
    --cc=anthony@codemonkey.ws \
    --cc=jeremy@goop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=virtualization@lists.osdl.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.