Re: [PATCH v10 02/25] x86/mm: define ARCH_SUPPORTS_SPECULATIVE_PAGE_FAULT

[Punit Agrawal]

Laurent Dufour <ldufour@xxxxxxxxxxxxxxxxxx> writes:

> On 08/05/2018 13:04, Punit Agrawal wrote:
>> Hi Laurent,
>> 
>> Laurent Dufour <ldufour@xxxxxxxxxxxxxxxxxx> writes:
>> 
>>> Set ARCH_SUPPORTS_SPECULATIVE_PAGE_FAULT which turns on the
>>> Speculative Page Fault handler when building for 64bit.
>>>
>>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>>> Signed-off-by: Laurent Dufour <ldufour@xxxxxxxxxxxxxxxxxx>
>>> ---
>>>  arch/x86/Kconfig | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>>> index d8983df5a2bc..ebdeb48e4a4a 100644
>>> --- a/arch/x86/Kconfig
>>> +++ b/arch/x86/Kconfig
>>> @@ -30,6 +30,7 @@ config X86_64
>>>  	select MODULES_USE_ELF_RELA
>>>  	select X86_DEV_DMA_OPS
>>>  	select ARCH_HAS_SYSCALL_WRAPPER
>>> +	select ARCH_SUPPORTS_SPECULATIVE_PAGE_FAULT
>> 
>> I'd suggest merging this patch with the one making changes to the
>> architectural fault handler towards the end of the series.
>> 
>> The Kconfig change is closely tied to the architectural support for SPF
>> and makes sense to be in a single patch.
>> 
>> If there's a good reason to keep them as separate patches, please move
>> the architecture Kconfig changes after the patch adding fault handler
>> changes.
>> 
>> It's better to enable the feature once the core infrastructure is merged
>> rather than at the beginning of the series to avoid potential bad
>> fallout from incomplete functionality during bisection.
>
> Indeed bisection was the reason why Andrew asked me to push the configuration
> enablement on top of the series (https://lkml.org/lkml/2017/10/10/1229).

The config options have gone through another round of splitting (between
core and architecture) since that comment. I agree that it still makes
sense to define the core config - CONFIG_SPECULATIVE_PAGE_FAULT early
on.

Just to clarify, my suggestion was to only move the architecture configs
further down.

>
> I also think it would be better to have the architecture enablement in on patch
> but that would mean that the code will not be build when bisecting without the
> latest patch adding the per architecture code.

I don't see that as a problem. But if I'm in the minority, I am OK with
leaving things as they are as well.

Thanks,
Punit