Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [RFC PATCH] arm64/fpsimd: Don't disable softirq when touching
 FPSIMD/SVE state
To:     Dave Martin <Dave.Martin@arm.com>
Cc:     linux-arm-kernel@lists.infradead.org, bigeasy@linutronix.de,
        linux-rt-users@vger.kernel.org, catalin.marinas@arm.com,
        will.deacon@arm.com, ard.biesheuvel@linaro.org,
        linux-kernel@vger.kernel.org
References: <20190208165513.8435-1-julien.grall@arm.com>
 <20190404105233.GD3567@e103592.cambridge.arm.com>
 <a46f9fd4-dd31-c892-aec1-ae69072be92d@arm.com>
 <20190405150722.GE3567@e103592.cambridge.arm.com>
 <899713e0-2d32-04a0-b2f2-3493f7299033@arm.com>
 <20190411163423.GJ3567@e103592.cambridge.arm.com>
From:   Julien Grall <julien.grall@arm.com>
Message-ID: <e647de48-fd74-eabc-0a57-c2c519a36012@arm.com>
Date:   Thu, 11 Apr 2019 17:50:39 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <20190411163423.GJ3567@e103592.cambridge.arm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Dave,

On 4/11/19 5:34 PM, Dave Martin wrote:
> On Thu, Apr 11, 2019 at 04:58:41PM +0100, Julien Grall wrote:
>> Hi Dave,
>>
>> On 4/5/19 4:07 PM, Dave Martin wrote:
>>> On Fri, Apr 05, 2019 at 10:02:45AM +0100, Julien Grall wrote:
>>>>>> +#ifdef CONFIG_KERNEL_MODE_NEON
>>>>>> +
>>>>>>   /*
>>>>>>    * may_use_simd - whether it is allowable at this time to issue SIMD
>>>>>>    *                instructions or access the SIMD register file
>>>>>> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
>>>>>> index 5ebe73b69961..b7e5dac26190 100644
>>>>>> --- a/arch/arm64/kernel/fpsimd.c
>>>>>> +++ b/arch/arm64/kernel/fpsimd.c
>>>>>> @@ -90,7 +90,8 @@
>>>>>>    * To prevent this from racing with the manipulation of the task's FPSIMD state
>>>>>>    * from task context and thereby corrupting the state, it is necessary to
>>>>>>    * protect any manipulation of a task's fpsimd_state or TIF_FOREIGN_FPSTATE
>>>>>> - * flag with local_bh_disable() unless softirqs are already masked.
>>>>>> + * flag with kernel_neon_{disable, enable}. This will still allow softirqs to
>>>>>> + * run but prevent them to use FPSIMD.
>>>>>>    *
>>>>>>    * For a certain task, the sequence may look something like this:
>>>>>>    * - the task gets scheduled in; if both the task's fpsimd_cpu field
>>>>>> @@ -142,6 +143,9 @@ extern void __percpu *efi_sve_state;
>>>>>>   #endif /* ! CONFIG_ARM64_SVE */
>>>>>> +static void kernel_neon_disable(void);
>>>>>> +static void kernel_neon_enable(void);
>>>>>
>>>>> I find these names a bit confusing: _disable() actualy enables FPSIMD/SVE
>>>>> context access for the current context (i.e., makes it safe).
>>>>>
>>>>> Since these also disable/enable preemption, perhaps we can align them
>>>>> with the existing get_cpu()/put_cpu(), something like:
>>>>>
>>>>> void get_cpu_fpsimd_context();
>>>>> vold put_cpu_fpsimd_context();
>>>>>
>>>>> If you consider it's worth adding the checking helper I alluded to
>>>>> above, it could then be called something like:
>>>>>
>>>>> bool have_cpu_fpsimd_context();
>>>>
>>>> I am not sure where you suggested a checking helper above. Do you refer to
>>>> exposing kernel_neon_busy even for !CONFIG_KERNEL_MODE_NEON?
>>>
>>> Hmmm, looks like I got my reply out of order here.
>>>
>>> I meant the helper (if any) to check
>>> !preemptible() && !__this_cpu_read(kernel_neon_busy).
>>
>> I guess you are using && instead of || because some of the caller may not
>> call get_cpu_fpsimd_context() before but still disable preemption, right?
>>
>> Wouldn't it be better to have all the user calling get_cpu_fpsimd_context()
>> and put_cpu_fpsimd_context()?
>>
>> This has the advantage to uniformize how the way FPSIMD is protected and
>> also...
> 
> My expectation is that all users would have called
> get_cpu_fpsimd_context().

This is not the case today (see kvm_arch_vcpu_put_fp), I will look at 
protecting it with a call to get_cpu_fpsimd_context().

> 
> The reason for writing the check that way is that we can't meaningfully
> inspect percpu variables unless we are non-preemptible already.  The &&
> means we don't do the percpu read at all is the case where preemptible()
> is true.

I am not sure to understand why it would be necessary. 
this_cpu_read(kernel_neon_busy) should be sufficient here.

If it is set then, preemption is disabled. Or are you worried about user 
directly setting kernel_neon_busy instead of calling get_cpu_fpsimd_context?

> 
> Or do you think my logic is wrong somewhere?  (It's possible...)

I think your logic would not return the correct value. We want 
have_cpu_fpsimd_context() to return true if it is not preemptible and 
kernel_neon_busy is true. So we would want:

!preemptible() && __this_cpu_read(kernel_neon_busy)

If we speak about the implementation of have_cpu_fpsimd_context(), then 
we want:

!preemptible() && __this_cpu_read(kernel_neon_busy)

Cheers,

-- 
Julien Grall