All of lore.kernel.org
 help / color / mirror / Atom feed
From: Russell King - ARM Linux <linux@arm.linux.org.uk>
To: Dave P Martin <Dave.Martin@arm.com>
Cc: Christoffer Dall <christoffer.dall@linaro.org>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	KVM devel mailing list <kvm@vger.kernel.org>,
	Nicolas Pitre <nico@linaro.org>,
	Marc Zyngier <Marc.Zyngier@arm.com>,
	"kvmarm@lists.cs.columbia.edu" <kvmarm@lists.cs.columbia.edu>,
	Gleb Natapov <gleb@kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 1/2] ARM: kvm: fix a bad BSYM() usage
Date: Mon, 11 May 2015 11:17:39 +0100	[thread overview]
Message-ID: <20150511101738.GC2067@n2100.arm.linux.org.uk> (raw)
In-Reply-To: <20150511095657.GA5856@e103592.cambridge.arm.com>

On Mon, May 11, 2015 at 10:56:57AM +0100, Dave P Martin wrote:
> ldr= will do the right thing *if* the target symbol's type is correctly
> annotated.  This means that ldr =some_local_code_symbol does the right
> thing for branch target addresses if and only if some_local_code_symbol
> is marked with .type %function (or ENDPROC).
> 
> The fact that a symbol is in a code section is *not* enough.
> 
> For ARM code this never mattered, so local symbols in .S files are
> probably under-annotated in general.  BSYM() might have been used to
> work around this in some cases.
> 
> We should check that all the BSYMs removed by this series from ldr=
> and .long/.word etc. point to a correctly annotated symbol, and add
> the annotations if not.

I guess the problem here is that people forget what a patch series is
actually doing and then start making comments based on hypothetical
stuff rather than what the series is actually doing.

To recap, this series:

1. Removes the wrong usage of BSYM() in the KVM code.  This is used with
   the symbol "panic" which is a function declared by generic C code.
   Therefore, panic will have the appropriate annotations attached to
   this symbol, unless GCC itself is buggy.

   This is the _only_ case of "ldr rd, =BSYM(sym)".

2. It replaces the remaining "adr rd, BSYM(sym)" with a new "badr" macro,
   and removes the BSYM() preprocessor macro.  The new "badr" macro which
   is identical in behaviour to the former, but ensures that BSYM()
   doesn't get mis-used with "ldr".

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

WARNING: multiple messages have this Message-ID (diff)
From: linux@arm.linux.org.uk (Russell King - ARM Linux)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/2] ARM: kvm: fix a bad BSYM() usage
Date: Mon, 11 May 2015 11:17:39 +0100	[thread overview]
Message-ID: <20150511101738.GC2067@n2100.arm.linux.org.uk> (raw)
In-Reply-To: <20150511095657.GA5856@e103592.cambridge.arm.com>

On Mon, May 11, 2015 at 10:56:57AM +0100, Dave P Martin wrote:
> ldr= will do the right thing *if* the target symbol's type is correctly
> annotated.  This means that ldr =some_local_code_symbol does the right
> thing for branch target addresses if and only if some_local_code_symbol
> is marked with .type %function (or ENDPROC).
> 
> The fact that a symbol is in a code section is *not* enough.
> 
> For ARM code this never mattered, so local symbols in .S files are
> probably under-annotated in general.  BSYM() might have been used to
> work around this in some cases.
> 
> We should check that all the BSYMs removed by this series from ldr=
> and .long/.word etc. point to a correctly annotated symbol, and add
> the annotations if not.

I guess the problem here is that people forget what a patch series is
actually doing and then start making comments based on hypothetical
stuff rather than what the series is actually doing.

To recap, this series:

1. Removes the wrong usage of BSYM() in the KVM code.  This is used with
   the symbol "panic" which is a function declared by generic C code.
   Therefore, panic will have the appropriate annotations attached to
   this symbol, unless GCC itself is buggy.

   This is the _only_ case of "ldr rd, =BSYM(sym)".

2. It replaces the remaining "adr rd, BSYM(sym)" with a new "badr" macro,
   and removes the BSYM() preprocessor macro.  The new "badr" macro which
   is identical in behaviour to the former, but ensures that BSYM()
   doesn't get mis-used with "ldr".

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

  reply	other threads:[~2015-05-11 10:17 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-05-08 16:08 [PATCH 1/2] ARM: kvm: fix a bad BSYM() usage Russell King
2015-05-08 16:08 ` Russell King
2015-05-08 16:21 ` Nicolas Pitre
2015-05-08 16:21   ` Nicolas Pitre
2015-05-08 16:31 ` Dave P Martin
2015-05-08 16:31   ` Dave P Martin
2015-05-09 20:07 ` Christoffer Dall
2015-05-09 20:07   ` Christoffer Dall
2015-05-09 20:10   ` Ard Biesheuvel
2015-05-09 20:10     ` Ard Biesheuvel
2015-05-11  9:05     ` Christoffer Dall
2015-05-11  9:05       ` Christoffer Dall
2015-05-11  9:44       ` Ard Biesheuvel
2015-05-11  9:44         ` Ard Biesheuvel
2015-05-11 10:07         ` Dave P Martin
2015-05-11 10:07           ` Dave P Martin
2015-05-11  9:56       ` Dave P Martin
2015-05-11  9:56         ` Dave P Martin
2015-05-11 10:17         ` Russell King - ARM Linux [this message]
2015-05-11 10:17           ` Russell King - ARM Linux
2015-05-11 10:27           ` Dave P Martin
2015-05-11 10:27             ` Dave P Martin
2015-05-09 20:10   ` Russell King - ARM Linux
2015-05-09 20:10     ` Russell King - ARM Linux
2015-05-11  9:00     ` Christoffer Dall
2015-05-11  9:00       ` Christoffer Dall

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=20150511101738.GC2067@n2100.arm.linux.org.uk \
    --to=linux@arm.linux.org.uk \
    --cc=Dave.Martin@arm.com \
    --cc=Marc.Zyngier@arm.com \
    --cc=ard.biesheuvel@linaro.org \
    --cc=christoffer.dall@linaro.org \
    --cc=gleb@kernel.org \
    --cc=kvm@vger.kernel.org \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=nico@linaro.org \
    --cc=pbonzini@redhat.com \
    /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.