All of lore.kernel.org
 help / color / mirror / Atom feed
From: Palmer Dabbelt <palmer@rivosinc.com>
To: Atish Patra <atishp@rivosinc.com>
Cc: linux-riscv@lists.infradead.org,
	Paul Walmsley <paul.walmsley@sifive.com>,
	aou@eecs.berkeley.edu, anup@brainfault.org,
	Atish Patra <atishp@rivosinc.com>,
	jszhang@kernel.org, vincent.chen@sifive.com,
	sunnanyong@huawei.com, linux-riscv@lists.infradead.org,
	linux-kernel@vger.kernel.org, Atish Patra <atishp@rivosinc.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH] RISC-V: Prevent sbi_ecall() from being inlined
Date: Thu, 27 Jan 2022 15:00:21 -0800 (PST)	[thread overview]
Message-ID: <mhng-a2eafdcc-50bf-4947-a52c-e7c89a0f6c86@palmer-ri-x1c9> (raw)
In-Reply-To: <20220127195554.15705-1-palmer@rivosinc.com>

On Thu, 27 Jan 2022 11:55:55 PST (-0800), Palmer Dabbelt wrote:
> From: Palmer Dabbelt <palmer@rivosinc.com>
>
> The SBI spec defines SBI calls as following the standard calling
> convention, but we don't actually inform GCC of that when making an

As per some discussion on the SBI spec, this definition isn't internally 
consistent.  I've got some WIP to make proper inline SBI macros -- 
that'll be necessary either way the spec goes (ie, to get rid of all the 
zeros), but I'm going to leave this alone until the spec gets sorted 
out.

> ecall.  Unfortunately this does actually manifest for the more complex
> SBI call wrappers, for example sbi_s, for example sbi_send_ipi_v02()
> uses t1.
>
> This patch just marks sbi_ecall() as noinline, which implicitly enforces
> the standard calling convention.
>
> Fixes : b9dcd9e41587 ("RISC-V: Add basic support for SBI v0.2")
> Cc: stable@vger.kernel.org
> Reported-by: Atish Patra <atishp@rivosinc.com>
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> ---
> This is more of a stop-gap fix than anything else, but it's small enough
> that it should be straight-forward to back port to stable.  This bug has
> existed forever, in theory, but none of this was specified in SBI-0.1
> so the backport to the introduction of 0.2 should be sufficient.
> No extant versions OpenSBI or BBL will manifest issues here, as they
> save all registers, but the spec is quite explicit so we're better off
> getting back in line sooner rather than later.
>
> There'll be some marginal performance impact here.  I'll send a
> follow-on to clean up the SBI call wrappers in a way that allows
> inlining without violating the spec, but that'll be a bigger change and
> thus isn't really suitable for stable.
> ---
>  arch/riscv/kernel/sbi.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index f72527fcb347..7be586f5dc69 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -21,6 +21,11 @@ static int (*__sbi_rfence)(int fid, const struct cpumask *cpu_mask,
>  			   unsigned long start, unsigned long size,
>  			   unsigned long arg4, unsigned long arg5) __ro_after_init;
>
> +/*
> + * This ecall stub can't be inlined because we're relying on the presence of a
> + * function call to enforce the calling convention.
> + */
> +noinline
>  struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
>  			unsigned long arg1, unsigned long arg2,
>  			unsigned long arg3, unsigned long arg4,

WARNING: multiple messages have this Message-ID (diff)
From: Palmer Dabbelt <palmer@rivosinc.com>
To: Atish Patra <atishp@rivosinc.com>
Cc: linux-riscv@lists.infradead.org,
	Paul Walmsley <paul.walmsley@sifive.com>,
	 aou@eecs.berkeley.edu, anup@brainfault.org,
	Atish Patra <atishp@rivosinc.com>,
	jszhang@kernel.org,  vincent.chen@sifive.com,
	sunnanyong@huawei.com, linux-riscv@lists.infradead.org,
	linux-kernel@vger.kernel.org, Atish Patra <atishp@rivosinc.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH] RISC-V: Prevent sbi_ecall() from being inlined
Date: Thu, 27 Jan 2022 15:00:21 -0800 (PST)	[thread overview]
Message-ID: <mhng-a2eafdcc-50bf-4947-a52c-e7c89a0f6c86@palmer-ri-x1c9> (raw)
In-Reply-To: <20220127195554.15705-1-palmer@rivosinc.com>

On Thu, 27 Jan 2022 11:55:55 PST (-0800), Palmer Dabbelt wrote:
> From: Palmer Dabbelt <palmer@rivosinc.com>
>
> The SBI spec defines SBI calls as following the standard calling
> convention, but we don't actually inform GCC of that when making an

As per some discussion on the SBI spec, this definition isn't internally 
consistent.  I've got some WIP to make proper inline SBI macros -- 
that'll be necessary either way the spec goes (ie, to get rid of all the 
zeros), but I'm going to leave this alone until the spec gets sorted 
out.

> ecall.  Unfortunately this does actually manifest for the more complex
> SBI call wrappers, for example sbi_s, for example sbi_send_ipi_v02()
> uses t1.
>
> This patch just marks sbi_ecall() as noinline, which implicitly enforces
> the standard calling convention.
>
> Fixes : b9dcd9e41587 ("RISC-V: Add basic support for SBI v0.2")
> Cc: stable@vger.kernel.org
> Reported-by: Atish Patra <atishp@rivosinc.com>
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> ---
> This is more of a stop-gap fix than anything else, but it's small enough
> that it should be straight-forward to back port to stable.  This bug has
> existed forever, in theory, but none of this was specified in SBI-0.1
> so the backport to the introduction of 0.2 should be sufficient.
> No extant versions OpenSBI or BBL will manifest issues here, as they
> save all registers, but the spec is quite explicit so we're better off
> getting back in line sooner rather than later.
>
> There'll be some marginal performance impact here.  I'll send a
> follow-on to clean up the SBI call wrappers in a way that allows
> inlining without violating the spec, but that'll be a bigger change and
> thus isn't really suitable for stable.
> ---
>  arch/riscv/kernel/sbi.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index f72527fcb347..7be586f5dc69 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -21,6 +21,11 @@ static int (*__sbi_rfence)(int fid, const struct cpumask *cpu_mask,
>  			   unsigned long start, unsigned long size,
>  			   unsigned long arg4, unsigned long arg5) __ro_after_init;
>
> +/*
> + * This ecall stub can't be inlined because we're relying on the presence of a
> + * function call to enforce the calling convention.
> + */
> +noinline
>  struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
>  			unsigned long arg1, unsigned long arg2,
>  			unsigned long arg3, unsigned long arg4,

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

  reply	other threads:[~2022-01-27 23:00 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-27 19:55 [PATCH] RISC-V: Prevent sbi_ecall() from being inlined Palmer Dabbelt
2022-01-27 19:55 ` Palmer Dabbelt
2022-01-27 23:00 ` Palmer Dabbelt [this message]
2022-01-27 23:00   ` Palmer Dabbelt

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=mhng-a2eafdcc-50bf-4947-a52c-e7c89a0f6c86@palmer-ri-x1c9 \
    --to=palmer@rivosinc.com \
    --cc=anup@brainfault.org \
    --cc=aou@eecs.berkeley.edu \
    --cc=atishp@rivosinc.com \
    --cc=jszhang@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=paul.walmsley@sifive.com \
    --cc=stable@vger.kernel.org \
    --cc=sunnanyong@huawei.com \
    --cc=vincent.chen@sifive.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.