| Message ID | 20221020152404.283980-2-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm-unit-tests: set of fixes and new tests | expand |
On Thu, Oct 20, 2022, Maxim Levitsky wrote: > Tests that need interrupt shadow can't rely on irq_enable function anyway, > as its comment states, and it is useful to know for sure that interrupts > are enabled after the call to this function. > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > lib/x86/processor.h | 9 ++++----- > x86/apic.c | 1 - > x86/ioapic.c | 1 - > x86/svm_tests.c | 9 --------- > x86/tscdeadline_latency.c | 1 - > x86/vmx_tests.c | 7 ------- > 6 files changed, 4 insertions(+), 24 deletions(-) > > diff --git a/lib/x86/processor.h b/lib/x86/processor.h > index 03242206..9db07346 100644 > --- a/lib/x86/processor.h > +++ b/lib/x86/processor.h > @@ -720,13 +720,12 @@ 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. > - */ > static inline void irq_enable(void) > { > - asm volatile("sti"); > + asm volatile( > + "sti \n\t" Formatting is odd. Doesn't really matter, but I think this can simply be: static inline void sti_nop(void) { asm volatile("sti; nop"); } > + "nop\n\t" I like the idea of a helper to enable IRQs and consume pending interrupts, but I think we should add a new helper instead of changing irq_enable(). Hmm, or alternatively, kill off irq_enable() and irq_disable() entirely and instead add sti_nop(). I like this idea even better. The helpers are all x86-specific, so there's no need to add a layer of abstraction, and sti() + sti_nop() has the benefit of making it very clear what code is being emitted without having to come up with clever function names. And I think we should go even further and provide a helper to do the entire sequence of enable->nop->disable, which is a very common pattern. No idea what to call this one, though I suppose sti_nop_cli() would work. My vote is to replace all irq_enable() and irq_disable() usage with sti() and cli(), and then introduce sti_nop() and sti_nop_cli() (or whatever it gets called) and convert users as appropriate. > + ); > } > > static inline void invlpg(volatile void *va) > diff --git a/x86/apic.c b/x86/apic.c > index 23508ad5..a8964d88 100644 > --- a/x86/apic.c > +++ b/x86/apic.c > @@ -36,7 +36,6 @@ static void __test_tsc_deadline_timer(void) > irq_enable(); > > wrmsr(MSR_IA32_TSCDEADLINE, rdmsr(MSR_IA32_TSC)); > - asm volatile ("nop"); I'm not entirely sure the existing nop is necessary here, but it's a functional change since it hoists the nop above the WRMSR. To be safe, probably best to leave this as-is for now. > report(tdt_count == 1, "tsc deadline timer"); > report(rdmsr(MSR_IA32_TSCDEADLINE) == 0, "tsc deadline timer clearing"); > } ... > diff --git a/x86/tscdeadline_latency.c b/x86/tscdeadline_latency.c > index a3bc4ea4..c54530dd 100644 > --- a/x86/tscdeadline_latency.c > +++ b/x86/tscdeadline_latency.c > @@ -73,7 +73,6 @@ static void start_tsc_deadline_timer(void) > irq_enable(); > > wrmsr(MSR_IA32_TSCDEADLINE, rdmsr(MSR_IA32_TSC)+delta); > - asm volatile ("nop"); Another functional change that should be skipped, at least for now.
On Thu, 2022-10-20 at 18:01 +0000, Sean Christopherson wrote: > On Thu, Oct 20, 2022, Maxim Levitsky wrote: > > Tests that need interrupt shadow can't rely on irq_enable function anyway, > > as its comment states, and it is useful to know for sure that interrupts > > are enabled after the call to this function. > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > lib/x86/processor.h | 9 ++++----- > > x86/apic.c | 1 - > > x86/ioapic.c | 1 - > > x86/svm_tests.c | 9 --------- > > x86/tscdeadline_latency.c | 1 - > > x86/vmx_tests.c | 7 ------- > > 6 files changed, 4 insertions(+), 24 deletions(-) > > > > diff --git a/lib/x86/processor.h b/lib/x86/processor.h > > index 03242206..9db07346 100644 > > --- a/lib/x86/processor.h > > +++ b/lib/x86/processor.h > > @@ -720,13 +720,12 @@ 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. > > - */ > > static inline void irq_enable(void) > > { > > - asm volatile("sti"); > > + asm volatile( > > + "sti \n\t" > > Formatting is odd. Doesn't really matter, but I think this can simply be: > > static inline void sti_nop(void) > { > asm volatile("sti; nop"); "\n\t" is what gcc manual recommends for separating the assembly lines as you know from the gcc manual: https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html "You may place multiple assembler instructions together in a single asm string, separated by the characters normally used in assembly code for the system. A combination that works in most places is a newline to break the line, plus a tab character to move to the instruction field (written as ‘\n\t’). Some assemblers allow semicolons as a line separator. However, note that some assembler dialects use semicolons to start a comment" Looks like gnu assembler does use semicolon for new statements and hash for comments but some assemblers do semicolon for comments. I usually use just "\n", but the safest is "\n\t". > } > > > > + "nop\n\t" > > I like the idea of a helper to enable IRQs and consume pending interrupts, but I > think we should add a new helper instead of changing irq_enable(). > > Hmm, or alternatively, kill off irq_enable() and irq_disable() entirely and instead > add sti_nop(). I like this idea even better. The helpers are all x86-specific, > so there's no need to add a layer of abstraction, and sti() + sti_nop() has the > benefit of making it very clear what code is being emitted without having to come > up with clever function names. > > And I think we should go even further and provide a helper to do the entire sequence > of enable->nop->disable, which is a very common pattern. No idea what to call > this one, though I suppose sti_nop_cli() would work. > > My vote is to replace all irq_enable() and irq_disable() usage with sti() and cli(), > and then introduce sti_nop() and sti_nop_cli() (or whatever it gets called) and > convert users as appropriate. OK. > > > + ); > > } > > > > static inline void invlpg(volatile void *va) > > diff --git a/x86/apic.c b/x86/apic.c > > index 23508ad5..a8964d88 100644 > > --- a/x86/apic.c > > +++ b/x86/apic.c > > @@ -36,7 +36,6 @@ static void __test_tsc_deadline_timer(void) > > irq_enable(); > > > > wrmsr(MSR_IA32_TSCDEADLINE, rdmsr(MSR_IA32_TSC)); > > - asm volatile ("nop"); > > I'm not entirely sure the existing nop is necessary here, but it's a functional > change since it hoists the nop above the WRMSR. To be safe, probably best to > leave this as-is for now. I had doubts about this, IMHO both before and after are equally good, but anyway to be safe, I'll revert this change. > > > report(tdt_count == 1, "tsc deadline timer"); > > report(rdmsr(MSR_IA32_TSCDEADLINE) == 0, "tsc deadline timer clearing"); > > } > > ... > > > diff --git a/x86/tscdeadline_latency.c b/x86/tscdeadline_latency.c > > index a3bc4ea4..c54530dd 100644 > > --- a/x86/tscdeadline_latency.c > > +++ b/x86/tscdeadline_latency.c > > @@ -73,7 +73,6 @@ static void start_tsc_deadline_timer(void) > > irq_enable(); > > > > wrmsr(MSR_IA32_TSCDEADLINE, rdmsr(MSR_IA32_TSC)+delta); > > - asm volatile ("nop"); > > Another functional change that should be skipped, at least for now. OK. > Best regards, Maxim Levitsky
On Mon, Oct 24, 2022, Maxim Levitsky wrote: > On Thu, 2022-10-20 at 18:01 +0000, Sean Christopherson wrote: > > On Thu, Oct 20, 2022, Maxim Levitsky wrote: > > > Tests that need interrupt shadow can't rely on irq_enable function anyway, > > > as its comment states, and it is useful to know for sure that interrupts > > > are enabled after the call to this function. > > > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > > --- > > > lib/x86/processor.h | 9 ++++----- > > > x86/apic.c | 1 - > > > x86/ioapic.c | 1 - > > > x86/svm_tests.c | 9 --------- > > > x86/tscdeadline_latency.c | 1 - > > > x86/vmx_tests.c | 7 ------- > > > 6 files changed, 4 insertions(+), 24 deletions(-) > > > > > > diff --git a/lib/x86/processor.h b/lib/x86/processor.h > > > index 03242206..9db07346 100644 > > > --- a/lib/x86/processor.h > > > +++ b/lib/x86/processor.h > > > @@ -720,13 +720,12 @@ 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. > > > - */ > > > static inline void irq_enable(void) > > > { > > > - asm volatile("sti"); > > > + asm volatile( > > > + "sti \n\t" > > > > Formatting is odd. Doesn't really matter, but I think this can simply be: > > > > static inline void sti_nop(void) > > { > > asm volatile("sti; nop"); > > "\n\t" is what gcc manual recommends for separating the assembly lines as you > know from the gcc manual: > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html "You may place multiple > assembler instructions together in a single asm string, separated by the > characters normally used in assembly code for the system. A combination that > works in most places is a newline to break the line, plus a tab character to > move to the instruction field (written as ‘\n\t’). Some assemblers allow > semicolons as a line separator. However, note that some assembler dialects > use semicolons to start a comment" > > Looks like gnu assembler does use semicolon for new statements and hash for comments > but some assemblers do semicolon for comments. > > I usually use just "\n", but the safest is "\n\t". I'm pretty sure we can ignore GCC's warning here and maximize readability. There are already plenty of asm blobs that use a semicolon.
On Mon, 2022-10-24 at 22:49 +0000, Sean Christopherson wrote: > On Mon, Oct 24, 2022, Maxim Levitsky wrote: > > On Thu, 2022-10-20 at 18:01 +0000, Sean Christopherson wrote: > > > On Thu, Oct 20, 2022, Maxim Levitsky wrote: > > > > Tests that need interrupt shadow can't rely on irq_enable function anyway, > > > > as its comment states, and it is useful to know for sure that interrupts > > > > are enabled after the call to this function. > > > > > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > > > --- > > > > lib/x86/processor.h | 9 ++++----- > > > > x86/apic.c | 1 - > > > > x86/ioapic.c | 1 - > > > > x86/svm_tests.c | 9 --------- > > > > x86/tscdeadline_latency.c | 1 - > > > > x86/vmx_tests.c | 7 ------- > > > > 6 files changed, 4 insertions(+), 24 deletions(-) > > > > > > > > diff --git a/lib/x86/processor.h b/lib/x86/processor.h > > > > index 03242206..9db07346 100644 > > > > --- a/lib/x86/processor.h > > > > +++ b/lib/x86/processor.h > > > > @@ -720,13 +720,12 @@ 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. > > > > - */ > > > > static inline void irq_enable(void) > > > > { > > > > - asm volatile("sti"); > > > > + asm volatile( > > > > + "sti \n\t" > > > > > > Formatting is odd. Doesn't really matter, but I think this can simply be: > > > > > > static inline void sti_nop(void) > > > { > > > asm volatile("sti; nop"); > > > > "\n\t" is what gcc manual recommends for separating the assembly lines as you > > know from the gcc manual: > > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html "You may place multiple > > assembler instructions together in a single asm string, separated by the > > characters normally used in assembly code for the system. A combination that > > works in most places is a newline to break the line, plus a tab character to > > move to the instruction field (written as ‘\n\t’). Some assemblers allow > > semicolons as a line separator. However, note that some assembler dialects > > use semicolons to start a comment" > > > > Looks like gnu assembler does use semicolon for new statements and hash for comments > > but some assemblers do semicolon for comments. > > > > I usually use just "\n", but the safest is "\n\t". > > I'm pretty sure we can ignore GCC's warning here and maximize readability. There > are already plenty of asm blobs that use a semicolon. IMHO this is corner cutting and you yourself said that this is wrong. The other instances which use semicolon should be fixed IMHO. Best regards, Maxim Levitsky >
On Thu, Oct 27, 2022, Maxim Levitsky wrote: > On Mon, 2022-10-24 at 22:49 +0000, Sean Christopherson wrote: > > On Mon, Oct 24, 2022, Maxim Levitsky wrote: > > > I usually use just "\n", but the safest is "\n\t". > > > > I'm pretty sure we can ignore GCC's warning here and maximize readability. There > > are already plenty of asm blobs that use a semicolon. > > IMHO this is corner cutting and you yourself said that this is wrong. > > The other instances which use semicolon should be fixed IMHO. The kernel itself has multiple instances of "sti; ..." alone, I'm quite confident this we can prioritize making the code easy to read without risking future breakage. $ git grep -E "\"sti\;" arch/x86/include/asm/irqflags.h: asm volatile("sti; hlt": : :"memory"); arch/x86/include/asm/mwait.h: asm volatile("sti; .byte 0x0f, 0x01, 0xc9;" arch/x86/include/asm/paravirt.h: PVOP_ALT_VCALLEE0(irq.irq_enable, "sti;", ALT_NOT(X86_FEATURE_XENPV)); tools/testing/selftests/kvm/x86_64/xapic_ipi_test.c: asm volatile("sti; hlt; cli"); tools/testing/selftests/x86/iopl.c: asm volatile("sti; pushf; pop %[flags]"
On Thu, 2022-10-27 at 15:50 +0000, Sean Christopherson wrote: > On Thu, Oct 27, 2022, Maxim Levitsky wrote: > > On Mon, 2022-10-24 at 22:49 +0000, Sean Christopherson wrote: > > > On Mon, Oct 24, 2022, Maxim Levitsky wrote: > > > > I usually use just "\n", but the safest is "\n\t". > > > > > > I'm pretty sure we can ignore GCC's warning here and maximize readability. There > > > are already plenty of asm blobs that use a semicolon. > > > > IMHO this is corner cutting and you yourself said that this is wrong. > > > > The other instances which use semicolon should be fixed IMHO. > > The kernel itself has multiple instances of "sti; ..." alone, I'm quite confident > this we can prioritize making the code easy to read without risking future breakage. > > $ git grep -E "\"sti\;" > arch/x86/include/asm/irqflags.h: asm volatile("sti; hlt": : :"memory"); > arch/x86/include/asm/mwait.h: asm volatile("sti; .byte 0x0f, 0x01, 0xc9;" > arch/x86/include/asm/paravirt.h: PVOP_ALT_VCALLEE0(irq.irq_enable, "sti;", ALT_NOT(X86_FEATURE_XENPV)); > tools/testing/selftests/kvm/x86_64/xapic_ipi_test.c: asm volatile("sti; hlt; cli"); > tools/testing/selftests/x86/iopl.c: asm volatile("sti; pushf; pop %[flags]" > All right, let it be, but then lets also replace of '\n\t' with just '\n', just so that we don't pretend that we follow the gcc advice, to at least be consistent. Best regards, Maxim Levitsky
diff --git a/lib/x86/processor.h b/lib/x86/processor.h index 03242206..9db07346 100644 --- a/lib/x86/processor.h +++ b/lib/x86/processor.h @@ -720,13 +720,12 @@ 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. - */ static inline void irq_enable(void) { - asm volatile("sti"); + asm volatile( + "sti \n\t" + "nop\n\t" + ); } static inline void invlpg(volatile void *va) diff --git a/x86/apic.c b/x86/apic.c index 23508ad5..a8964d88 100644 --- a/x86/apic.c +++ b/x86/apic.c @@ -36,7 +36,6 @@ static void __test_tsc_deadline_timer(void) irq_enable(); wrmsr(MSR_IA32_TSCDEADLINE, rdmsr(MSR_IA32_TSC)); - asm volatile ("nop"); report(tdt_count == 1, "tsc deadline timer"); report(rdmsr(MSR_IA32_TSCDEADLINE) == 0, "tsc deadline timer clearing"); } diff --git a/x86/ioapic.c b/x86/ioapic.c index 4f578ce4..2e460a6d 100644 --- a/x86/ioapic.c +++ b/x86/ioapic.c @@ -129,7 +129,6 @@ static void test_ioapic_simultaneous(void) toggle_irq_line(0x0f); toggle_irq_line(0x0e); irq_enable(); - asm volatile ("nop"); report(g_66 && g_78 && g_66_after_78 && g_66_rip == g_78_rip, "ioapic simultaneous edge interrupts"); } diff --git a/x86/svm_tests.c b/x86/svm_tests.c index e2ec9541..a6397821 100644 --- a/x86/svm_tests.c +++ b/x86/svm_tests.c @@ -1000,7 +1000,6 @@ static bool pending_event_finished(struct svm_test *test) } irq_enable(); - asm volatile ("nop"); irq_disable(); if (!pending_event_ipi_fired) { @@ -1056,7 +1055,6 @@ static void pending_event_cli_test(struct svm_test *test) /* VINTR_MASKING is zero. This should cause the IPI to fire. */ irq_enable(); - asm volatile ("nop"); irq_disable(); if (pending_event_ipi_fired != true) { @@ -1072,7 +1070,6 @@ static void pending_event_cli_test(struct svm_test *test) * that L0 did not leave a stale VINTR in the VMCB. */ irq_enable(); - asm volatile ("nop"); irq_disable(); } @@ -1105,7 +1102,6 @@ static bool pending_event_cli_finished(struct svm_test *test) } irq_enable(); - asm volatile ("nop"); irq_disable(); if (pending_event_ipi_fired != true) { @@ -1243,7 +1239,6 @@ static bool interrupt_finished(struct svm_test *test) } irq_enable(); - asm volatile ("nop"); irq_disable(); vmcb->control.intercept &= ~(1ULL << INTERCEPT_INTR); @@ -1540,7 +1535,6 @@ static void virq_inject_test(struct svm_test *test) } irq_enable(); - asm volatile ("nop"); irq_disable(); if (!virq_fired) { @@ -1557,7 +1551,6 @@ static void virq_inject_test(struct svm_test *test) } irq_enable(); - asm volatile ("nop"); irq_disable(); if (!virq_fired) { @@ -1568,7 +1561,6 @@ static void virq_inject_test(struct svm_test *test) vmmcall(); irq_enable(); - asm volatile ("nop"); irq_disable(); if (virq_fired) { @@ -1739,7 +1731,6 @@ static bool reg_corruption_finished(struct svm_test *test) void* guest_rip = (void*)vmcb->save.rip; irq_enable(); - asm volatile ("nop"); irq_disable(); if (guest_rip == insb_instruction_label && io_port_var != 0xAA) { diff --git a/x86/tscdeadline_latency.c b/x86/tscdeadline_latency.c index a3bc4ea4..c54530dd 100644 --- a/x86/tscdeadline_latency.c +++ b/x86/tscdeadline_latency.c @@ -73,7 +73,6 @@ static void start_tsc_deadline_timer(void) irq_enable(); wrmsr(MSR_IA32_TSCDEADLINE, rdmsr(MSR_IA32_TSC)+delta); - asm volatile ("nop"); } static int enable_tsc_deadline_timer(void) diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c index aa2ecbbc..c8e68931 100644 --- a/x86/vmx_tests.c +++ b/x86/vmx_tests.c @@ -1625,7 +1625,6 @@ static void interrupt_main(void) apic_write(APIC_TMICT, 1000000); irq_enable(); - asm volatile ("nop"); vmcall(); report(rdtsc() - start > 10000 && timer_fired, @@ -1639,7 +1638,6 @@ static void interrupt_main(void) apic_write(APIC_TMICT, 1000000); irq_enable(); - asm volatile ("nop"); vmcall(); report(rdtsc() - start > 10000 && timer_fired, @@ -1709,7 +1707,6 @@ static int interrupt_exit_handler(union exit_reason exit_reason) handle_external_interrupt(vector); } else { irq_enable(); - asm volatile ("nop"); irq_disable(); } if (vmx_get_test_stage() >= 2) @@ -6792,7 +6789,6 @@ static void test_x2apic_wr( /* Clear the external interrupt. */ irq_enable(); - asm volatile ("nop"); irq_disable(); report(handle_x2apic_ipi_ran, "Got pending interrupt after IRQ enabled."); @@ -8543,7 +8539,6 @@ static void vmx_pending_event_test_core(bool guest_hlt) "Guest did not run before host received IPI"); irq_enable(); - asm volatile ("nop"); irq_disable(); report(vmx_pending_event_ipi_fired, "Got pending interrupt after IRQ enabled"); @@ -9526,8 +9521,6 @@ static void vmx_hlt_with_rvi_guest(void) handle_irq(HLT_WITH_RVI_VECTOR, vmx_hlt_with_rvi_guest_isr); irq_enable(); - asm volatile ("nop"); - vmcall(); }
Tests that need interrupt shadow can't rely on irq_enable function anyway, as its comment states, and it is useful to know for sure that interrupts are enabled after the call to this function. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- lib/x86/processor.h | 9 ++++----- x86/apic.c | 1 - x86/ioapic.c | 1 - x86/svm_tests.c | 9 --------- x86/tscdeadline_latency.c | 1 - x86/vmx_tests.c | 7 ------- 6 files changed, 4 insertions(+), 24 deletions(-)