Re: [kvm-unit-tests PATCH v3 01/27] x86: replace irq_{enable|disable}() with sti()/cli()

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

Am 06/12/2022 um 14:55 schrieb Maxim Levitsky:
> On Thu, 2022-12-01 at 14:46 +0100, Emanuele Giuseppe Esposito wrote:
>>
>> Am 22/11/2022 um 17:11 schrieb Maxim Levitsky:
>>> This removes a layer of indirection which is strictly
>>> speaking not needed since its x86 code anyway.
>>>
>>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
>>> ---
>>>  lib/x86/processor.h       | 19 +++++-----------
>>>  lib/x86/smp.c             |  2 +-
>>>  x86/apic.c                |  2 +-
>>>  x86/asyncpf.c             |  6 ++---
>>>  x86/eventinj.c            | 22 +++++++++---------
>>>  x86/hyperv_connections.c  |  2 +-
>>>  x86/hyperv_stimer.c       |  4 ++--
>>>  x86/hyperv_synic.c        |  6 ++---
>>>  x86/intel-iommu.c         |  2 +-
>>>  x86/ioapic.c              | 14 ++++++------
>>>  x86/pmu.c                 |  4 ++--
>>>  x86/svm.c                 |  4 ++--
>>>  x86/svm_tests.c           | 48 +++++++++++++++++++--------------------
>>>  x86/taskswitch2.c         |  4 ++--
>>>  x86/tscdeadline_latency.c |  4 ++--
>>>  x86/vmexit.c              | 18 +++++++--------
>>>  x86/vmx_tests.c           | 42 +++++++++++++++++-----------------
>>>  17 files changed, 98 insertions(+), 105 deletions(-)
>>>
>>> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
>>> index 7a9e8c82..b89f6a7c 100644
>>> --- a/lib/x86/processor.h
>>> +++ b/lib/x86/processor.h
>>> @@ -653,11 +653,17 @@ static inline void pause(void)
>>>  	asm volatile ("pause");
>>>  }
>>>  
>>> +/* Disable interrupts as per x86 spec */
>>>  static inline void cli(void)
>>>  {
>>>  	asm volatile ("cli");
>>>  }
>>>  
>>> +/*
>>> + * Enable interrupts.
>>> + * Note that next instruction after sti will not have interrupts
>>> + * evaluated due to concept of 'interrupt shadow'
>>> + */
>>>  static inline void sti(void)
>>>  {
>>>  	asm volatile ("sti");
>>> @@ -732,19 +738,6 @@ static inline void wrtsc(u64 tsc)
>>>  	wrmsr(MSR_IA32_TSC, tsc);
>>>  }
>>>  
>>> -static inline void irq_disable(void)
>>> -{
>>> -	asm volatile("cli");
>>> -}
>>> -
>>> -/* Note that irq_enable() does not ensure an interrupt shadow due
>>> - * to the vagaries of compiler optimizations.  If you need the
>>> - * shadow, use a single asm with "sti" and the instruction after it.
>> Minor nitpick: instead of a new doc comment, why not use this same
>> above? Looks clearer to me.
>>
>> Regardless,
>> Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>
> 
> I am not 100% sure what you mean.
> Note that cli() doesn't have the same interrupt window thing as sti().
> 

I mean replacing

>>> +/*
>>> + * Enable interrupts.
>>> + * Note that next instruction after sti will not have interrupts
>>> + * evaluated due to concept of 'interrupt shadow'
>>> + */

with

>>> -/* Note that irq_enable() does not ensure an interrupt shadow due
>>> - * to the vagaries of compiler optimizations.  If you need the
>>> - * shadow, use a single asm with "sti" and the instruction after it.

Emanuele