Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <17d0646b-bcd5-48ff-a55c-d5cf88cdd015@sifive.com>
Date:   Sat, 25 Nov 2023 18:06:38 -0600
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] riscv: Fix SMP when shadow call stacks are enabled
Content-Language: en-US
To:     Conor Dooley <conor.dooley@microchip.com>
Cc:     Palmer Dabbelt <palmer@dabbelt.com>,
        linux-riscv@lists.infradead.org, Albert Ou <aou@eecs.berkeley.edu>,
        Andy Chiu <andy.chiu@sifive.com>,
        =?UTF-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= <cleger@rivosinc.com>,
        Greentime Hu <greentime.hu@sifive.com>,
        Guo Ren <guoren@kernel.org>, Heiko Stuebner <heiko@sntech.de>,
        Masahiro Yamada <masahiroy@kernel.org>,
        Nam Cao <namcaov@gmail.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Sami Tolvanen <samitolvanen@google.com>,
        linux-kernel@vger.kernel.org
References: <20231121211958.3158576-1-samuel.holland@sifive.com>
 <20231123-vision-nuttiness-b75be703f162@wendy>
From:   Samuel Holland <samuel.holland@sifive.com>
In-Reply-To: <20231123-vision-nuttiness-b75be703f162@wendy>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Conor,

On 2023-11-23 8:06 AM, Conor Dooley wrote:
> On Tue, Nov 21, 2023 at 01:19:29PM -0800, Samuel Holland wrote:
>> This fixes two bugs in SCS initialization for secondary CPUs. First,
>> the SCS was not initialized at all in the spinwait boot path. Second,
>> the code for the SBI HSM path attempted to initialize the SCS before
>> enabling the MMU. However, that involves dereferencing the thread
>> pointer, which requires the MMU to be enabled.
>>
>> Fix both issues by setting up the SCS in the common secondary entry
>> path, after enabling the MMU.
> 
> I'm curious, mostly because I do not know much about the implemtnation
> of the shadow call stack, but does it actually work correctly when the
> kernel is built without mmu support?

I imagine it would work. The SCS implementation is purely software; it stores
the return address in a stack at `gp` instead of with the rest of local
variables at `sp`.

The problem here is that we are passing a pointer between CPUs with different
views of the virtual address space (i.e. the boot CPU sees the kernel at
0xffffffff80000000 while the CPU being brought up sees it at its physical
address), and then dereferencing it. If there is no MMU support, then the
virtual address space is identity mapped on all CPUs, and there is no problem.

Regards,
Samuel

>> Fixes: d1584d791a29 ("riscv: Implement Shadow Call Stack")
>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>> ---
>>
>>  arch/riscv/kernel/head.S | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
>> index b77397432403..76ace1e0b46f 100644
>> --- a/arch/riscv/kernel/head.S
>> +++ b/arch/riscv/kernel/head.S
>> @@ -154,7 +154,6 @@ secondary_start_sbi:
>>  	XIP_FIXUP_OFFSET a3
>>  	add a3, a3, a1
>>  	REG_L sp, (a3)
>> -	scs_load_current
>>  
>>  .Lsecondary_start_common:
>>  
>> @@ -165,6 +164,7 @@ secondary_start_sbi:
>>  	call relocate_enable_mmu
>>  #endif
>>  	call .Lsetup_trap_vector
>> +	scs_load_current
>>  	tail smp_callin
>>  #endif /* CONFIG_SMP */
>>  
>> -- 
>> 2.42.0
>>