[PATCH v2] um: Enable preemption in UML

[Anton Ivanov]

On 21/09/2023 10:42, Anton Ivanov wrote:
> 
> 
> On 21/09/2023 10:35, Johannes Berg wrote:
>> Hi,
>>
>> Haven't chimed in here before, but thanks for looking at this! I
>> actually really wanted at least the things you get for debug from this
>> in the past, so much appreciated.
>>
>> I think I actually tried a simpler approach and it kind of even worked,
>> but my userspace likely has very little FPU usage.
>>
>>
>>> +/* UML knows about 387 features up to and including AVX512, tile, etc are not yet
>>> + * supported.
>>> + */
>>
>> Do you ensure that somehow? Warn if it's there at least? I may have
>> missed that.
> 
> We can if we want. We can just cut-off feature reflection into the kernel in the
> setup. Presently it reflects all features. That can be filtered if we want to.
> For example - x86 has boot flags to disable xrestore and xrestoreopt
> 
>>
>> Or is it just for the saving? But what if you don't save enough and the
>> host CPU has more?
> 
> This is just for the saving. AFAIK no linux module touches _TILE and the other recent
> fancy stuff. The host kernel source looks that way.

This brings back the question of

1. Alternatives (this has become even more nightmarish since I last looked at it)
2. Static branch by key - this actually looks doable and is used by nearly all
crypto, I may look at it when I have some free time.

> 
>>
>>
>>> +void kernel_fpu_begin(void)
>>> +{
>>> +#if defined(PREEMPT) || defined(PREEMPT_VOLUNTRARY)
>>
>> maybe leave it as an inline in case it'd be empty, i.e. neither of these
>> are defined?
>>
>>> +    preempt_disable();
>>> +
>>> +    WARN_ON(this_cpu_read(in_kernel_fpu));
>>> +
>>> +    this_cpu_write(in_kernel_fpu, true);
>>> +
>>> +#ifdef CONFIG_64BIT
>>> +    if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVEOPT)))
>>> +        __builtin_ia32_xsaveopt64(&current->thread.fpu, KNOWN_387_FEATURES);
>>> +    else {
>>> +        if (cpu_has(&boot_cpu_data, X86_FEATURE_XSAVE))
>>> +            __builtin_ia32_xsave64(&current->thread.fpu, KNOWN_387_FEATURES);
>>> +        else
>>> +            __builtin_ia32_fxsave64(&current->thread.fpu);
>>> +    }
>>
>> nit: you could make all of these else if chains, and avoid the braces:
>>
>> if (likely(...))
>>     ...
>> else if (cpu_has(...))
>>     ...
>> else
>>     ...
>>
>>
>>> +void kernel_fpu_end(void)
>>
>> same comments as above apply here.
>>
>>
>>>   void sigio_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs)
>>>   {
>>> +    preempt_disable();
>>>       _sigio_handler(regs, irqs_suspended);
>>> +    preempt_enable();
>>>
>>
>> I may have to look at those time-travel bits here :)
> 
> Yes. I was going to ask you to double check if they all work as intended.
> 
> Basic use seems without tt seems OK.
> 
>>
>> Thanks,
>> johannes
>>
>> _______________________________________________
>> linux-um mailing list
>> linux-um at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-um
>>
> 

-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/