[PATCH v2] um: Enable preemption in UML

[Anton Ivanov]

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.

> 
> 
>> +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/