| Message ID | 20221020152404.283980-3-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: > Add a few functions to apic.c to make it easier to enable and disable > the local apic timer. > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > lib/x86/apic.c | 37 +++++++++++++++++++++++++++++++++++++ > lib/x86/apic.h | 6 ++++++ > 2 files changed, 43 insertions(+) > > diff --git a/lib/x86/apic.c b/lib/x86/apic.c > index 5131525a..dc6d3862 100644 > --- a/lib/x86/apic.c > +++ b/lib/x86/apic.c > @@ -256,3 +256,40 @@ void init_apic_map(void) > id_map[j++] = i; > } > } > + > +void apic_setup_timer(int vector, bool periodic) > +{ > + /* APIC runs with 'CPU core clock' divided by value in APIC_TDCR */ > + > + u32 lvtt = vector | > + (periodic ? APIC_LVT_TIMER_PERIODIC : APIC_LVT_TIMER_ONESHOT); Rather than take @periodic, take the mode. That way this funky ternary operator goes away and the callers are self-tdocumenting, e.g. this apic_setup_timer(TIMER_VECTOR, APIC_LVT_TIMER_PERIODIC); is more obvious than apic_setup_timer(TIMER_VECTOR, true); > + > + apic_cleanup_timer(); > + apic_write(APIC_TDCR, APIC_TDR_DIV_1); > + apic_write(APIC_LVTT, lvtt); > +} > + > +void apic_start_timer(u32 counter) > +{ > + apic_write(APIC_TMICT, counter); > +} > + > +void apic_stop_timer(void) > +{ > + apic_write(APIC_TMICT, 0); > +} > + > +void apic_cleanup_timer(void) > +{ > + u32 lvtt = apic_read(APIC_LVTT); > + > + // stop the counter > + apic_stop_timer(); > + > + // mask the timer interrupt > + apic_write(APIC_LVTT, lvtt | APIC_LVT_MASKED); > + > + // ensure that a pending timer is serviced > + irq_enable(); Jumping back to the "nop" patch, I'm reinforcing my vote to add sti_nop(). I actually starting typing a response to say this is broken before remembering that a nop got added to irq_enable(). > + irq_disable(); > +} > diff --git a/lib/x86/apic.h b/lib/x86/apic.h > index 6d27f047..db691e2a 100644 > --- a/lib/x86/apic.h > +++ b/lib/x86/apic.h > @@ -58,6 +58,12 @@ void disable_apic(void); > void reset_apic(void); > void init_apic_map(void); > > +void apic_cleanup_timer(void); > +void apic_setup_timer(int vector, bool periodic); > + > +void apic_start_timer(u32 counter); > +void apic_stop_timer(void); > + > /* Converts byte-addressable APIC register offset to 4-byte offset. */ > static inline u32 apic_reg_index(u32 reg) > { > -- > 2.26.3 >
On Thu, 2022-10-20 at 19:14 +0000, Sean Christopherson wrote: > On Thu, Oct 20, 2022, Maxim Levitsky wrote: > > Add a few functions to apic.c to make it easier to enable and disable > > the local apic timer. > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > lib/x86/apic.c | 37 +++++++++++++++++++++++++++++++++++++ > > lib/x86/apic.h | 6 ++++++ > > 2 files changed, 43 insertions(+) > > > > diff --git a/lib/x86/apic.c b/lib/x86/apic.c > > index 5131525a..dc6d3862 100644 > > --- a/lib/x86/apic.c > > +++ b/lib/x86/apic.c > > @@ -256,3 +256,40 @@ void init_apic_map(void) > > id_map[j++] = i; > > } > > } > > + > > +void apic_setup_timer(int vector, bool periodic) > > +{ > > + /* APIC runs with 'CPU core clock' divided by value in APIC_TDCR */ > > + > > + u32 lvtt = vector | > > + (periodic ? APIC_LVT_TIMER_PERIODIC : APIC_LVT_TIMER_ONESHOT); > > Rather than take @periodic, take the mode. That way this funky ternary operator > goes away and the callers are self-tdocumenting, e.g. this > > apic_setup_timer(TIMER_VECTOR, APIC_LVT_TIMER_PERIODIC); > > is more obvious than > > apic_setup_timer(TIMER_VECTOR, true); Makes sense. I also wanted to pass the divider, but ended up hardcoding it to 1. > > > + > > + apic_cleanup_timer(); > > + apic_write(APIC_TDCR, APIC_TDR_DIV_1); > > + apic_write(APIC_LVTT, lvtt); > > +} > > + > > +void apic_start_timer(u32 counter) > > +{ > > + apic_write(APIC_TMICT, counter); > > +} Makes sense. > > + > > +void apic_stop_timer(void) > > +{ > > + apic_write(APIC_TMICT, 0); > > +} > > + > > +void apic_cleanup_timer(void) > > +{ > > + u32 lvtt = apic_read(APIC_LVTT); > > + > > + // stop the counter > > + apic_stop_timer(); > > + > > + // mask the timer interrupt > > + apic_write(APIC_LVTT, lvtt | APIC_LVT_MASKED); > > + > > + // ensure that a pending timer is serviced > > + irq_enable(); > > Jumping back to the "nop" patch, I'm reinforcing my vote to add sti_nop(). I > actually starting typing a response to say this is broken before remembering that > a nop got added to irq_enable(). OK, although, for someone that doesn't know about the interrupt shadow (I guess most of the people that will look at this code), the above won't confuse them, in fact sti_nop() might confuse someone who doesn't know about why this nop is needed. Just a note. Best regards, Maxim Levitsky > > > + irq_disable(); > > +} > > diff --git a/lib/x86/apic.h b/lib/x86/apic.h > > index 6d27f047..db691e2a 100644 > > --- a/lib/x86/apic.h > > +++ b/lib/x86/apic.h > > @@ -58,6 +58,12 @@ void disable_apic(void); > > void reset_apic(void); > > void init_apic_map(void); > > > > +void apic_cleanup_timer(void); > > +void apic_setup_timer(int vector, bool periodic); > > + > > +void apic_start_timer(u32 counter); > > +void apic_stop_timer(void); > > + > > /* Converts byte-addressable APIC register offset to 4-byte offset. */ > > static inline u32 apic_reg_index(u32 reg) > > { > > -- > > 2.26.3 > > >
On Mon, Oct 24, 2022, Maxim Levitsky wrote: > On Thu, 2022-10-20 at 19:14 +0000, Sean Christopherson wrote: > > On Thu, Oct 20, 2022, Maxim Levitsky wrote: > > > + // ensure that a pending timer is serviced > > > + irq_enable(); > > > > Jumping back to the "nop" patch, I'm reinforcing my vote to add sti_nop(). I > > actually starting typing a response to say this is broken before remembering that > > a nop got added to irq_enable(). > > OK, although, for someone that doesn't know about the interrupt shadow (I > guess most of the people that will look at this code), the above won't > confuse them, in fact sti_nop() might confuse someone who doesn't know about > why this nop is needed. The difference is that sti_nop() might leave unfamiliar readers asking "why", but it won't actively mislead them. And the "why" can be easily answered by a comment above sti_nop() to describe its purpose. A "see also safe_halt()" with a comment there would be extra helpful, as "safe halt" is the main reason the STI shadow is even a thing. On the other hand, shoving a NOP into irq_enable() is pretty much guaranteed to cause problems for readers that do know about STI shadows since there's nothing in the name "irq_enable" that suggests that the helper also intentionally eats the interrupt shadow, and especically because the kernel's local_irq_enable() distills down to a bare STI.
On Mon, 2022-10-24 at 16:10 +0000, Sean Christopherson wrote: > On Mon, Oct 24, 2022, Maxim Levitsky wrote: > > On Thu, 2022-10-20 at 19:14 +0000, Sean Christopherson wrote: > > > On Thu, Oct 20, 2022, Maxim Levitsky wrote: > > > > + // ensure that a pending timer is serviced > > > > + irq_enable(); > > > > > > Jumping back to the "nop" patch, I'm reinforcing my vote to add sti_nop(). I > > > actually starting typing a response to say this is broken before remembering that > > > a nop got added to irq_enable(). > > > > OK, although, for someone that doesn't know about the interrupt shadow (I > > guess most of the people that will look at this code), the above won't > > confuse them, in fact sti_nop() might confuse someone who doesn't know about > > why this nop is needed. > > The difference is that sti_nop() might leave unfamiliar readers asking "why", but > it won't actively mislead them. And the "why" can be easily answered by a comment > above sti_nop() to describe its purpose. A "see also safe_halt()" with a comment > there would be extra helpful, as "safe halt" is the main reason the STI shadow is > even a thing. > > On the other hand, shoving a NOP into irq_enable() is pretty much guaranteed to > cause problems for readers that do know about STI shadows since there's nothing > in the name "irq_enable" that suggests that the helper also intentionally eats the > interrupt shadow, and especically because the kernel's local_irq_enable() distills > down to a bare STI. I still don't agree with you on this at all. I would like to hear what other KVM developers think about it. safe_halt actually is a example for function that abstacts away the nop - just what I want to do. A comment in irq_enable() about that nop also is fine to have. Best regards, Maxim Levitsky >
On Thu, Oct 27, 2022, Maxim Levitsky wrote: > On Mon, 2022-10-24 at 16:10 +0000, Sean Christopherson wrote: > > On Mon, Oct 24, 2022, Maxim Levitsky wrote: > > > On Thu, 2022-10-20 at 19:14 +0000, Sean Christopherson wrote: > > > > On Thu, Oct 20, 2022, Maxim Levitsky wrote: > > > > > + // ensure that a pending timer is serviced > > > > > + irq_enable(); > > > > > > > > Jumping back to the "nop" patch, I'm reinforcing my vote to add sti_nop(). I > > > > actually starting typing a response to say this is broken before remembering that > > > > a nop got added to irq_enable(). > > > > > > OK, although, for someone that doesn't know about the interrupt shadow (I > > > guess most of the people that will look at this code), the above won't > > > confuse them, in fact sti_nop() might confuse someone who doesn't know about > > > why this nop is needed. > > > > The difference is that sti_nop() might leave unfamiliar readers asking "why", but > > it won't actively mislead them. And the "why" can be easily answered by a comment > > above sti_nop() to describe its purpose. A "see also safe_halt()" with a comment > > there would be extra helpful, as "safe halt" is the main reason the STI shadow is > > even a thing. > > > > On the other hand, shoving a NOP into irq_enable() is pretty much guaranteed to > > cause problems for readers that do know about STI shadows since there's nothing > > in the name "irq_enable" that suggests that the helper also intentionally eats the > > interrupt shadow, and especically because the kernel's local_irq_enable() distills > > down to a bare STI. > > I still don't agree with you on this at all. I would like to hear what other > KVM developers think about it. Why not just kill off irq_enable() and irq_disable() and use sti() and cli()? Then we don't have to come to any agreement on whether or not shoving a NOP into irq_enable() is a good idea. > safe_halt actually is a example for function that abstacts away the nop - > just what I want to do. The difference is that "safe halt" is established terminology that specifically means "STI immediately followed by HLT".
On Thu, 2022-10-27 at 15:54 +0000, Sean Christopherson wrote: > On Thu, Oct 27, 2022, Maxim Levitsky wrote: > > On Mon, 2022-10-24 at 16:10 +0000, Sean Christopherson wrote: > > > On Mon, Oct 24, 2022, Maxim Levitsky wrote: > > > > On Thu, 2022-10-20 at 19:14 +0000, Sean Christopherson wrote: > > > > > On Thu, Oct 20, 2022, Maxim Levitsky wrote: > > > > > > + // ensure that a pending timer is serviced > > > > > > + irq_enable(); > > > > > > > > > > Jumping back to the "nop" patch, I'm reinforcing my vote to add sti_nop(). I > > > > > actually starting typing a response to say this is broken before remembering that > > > > > a nop got added to irq_enable(). > > > > > > > > OK, although, for someone that doesn't know about the interrupt shadow (I > > > > guess most of the people that will look at this code), the above won't > > > > confuse them, in fact sti_nop() might confuse someone who doesn't know about > > > > why this nop is needed. > > > > > > The difference is that sti_nop() might leave unfamiliar readers asking "why", but > > > it won't actively mislead them. And the "why" can be easily answered by a comment > > > above sti_nop() to describe its purpose. A "see also safe_halt()" with a comment > > > there would be extra helpful, as "safe halt" is the main reason the STI shadow is > > > even a thing. > > > > > > On the other hand, shoving a NOP into irq_enable() is pretty much guaranteed to > > > cause problems for readers that do know about STI shadows since there's nothing > > > in the name "irq_enable" that suggests that the helper also intentionally eats the > > > interrupt shadow, and especically because the kernel's local_irq_enable() distills > > > down to a bare STI. > > > > I still don't agree with you on this at all. I would like to hear what other > > KVM developers think about it. > > Why not just kill off irq_enable() and irq_disable() and use sti() and cli()? > Then we don't have to come to any agreement on whether or not shoving a NOP into > irq_enable() is a good idea. > > > safe_halt actually is a example for function that abstacts away the nop - > > just what I want to do. > > The difference is that "safe halt" is established terminology that specifically > means "STI immediately followed by HLT". > OK, let it be. Best regards, Maxim Levitsky
diff --git a/lib/x86/apic.c b/lib/x86/apic.c index 5131525a..dc6d3862 100644 --- a/lib/x86/apic.c +++ b/lib/x86/apic.c @@ -256,3 +256,40 @@ void init_apic_map(void) id_map[j++] = i; } } + +void apic_setup_timer(int vector, bool periodic) +{ + /* APIC runs with 'CPU core clock' divided by value in APIC_TDCR */ + + u32 lvtt = vector | + (periodic ? APIC_LVT_TIMER_PERIODIC : APIC_LVT_TIMER_ONESHOT); + + apic_cleanup_timer(); + apic_write(APIC_TDCR, APIC_TDR_DIV_1); + apic_write(APIC_LVTT, lvtt); +} + +void apic_start_timer(u32 counter) +{ + apic_write(APIC_TMICT, counter); +} + +void apic_stop_timer(void) +{ + apic_write(APIC_TMICT, 0); +} + +void apic_cleanup_timer(void) +{ + u32 lvtt = apic_read(APIC_LVTT); + + // stop the counter + apic_stop_timer(); + + // mask the timer interrupt + apic_write(APIC_LVTT, lvtt | APIC_LVT_MASKED); + + // ensure that a pending timer is serviced + irq_enable(); + irq_disable(); +} diff --git a/lib/x86/apic.h b/lib/x86/apic.h index 6d27f047..db691e2a 100644 --- a/lib/x86/apic.h +++ b/lib/x86/apic.h @@ -58,6 +58,12 @@ void disable_apic(void); void reset_apic(void); void init_apic_map(void); +void apic_cleanup_timer(void); +void apic_setup_timer(int vector, bool periodic); + +void apic_start_timer(u32 counter); +void apic_stop_timer(void); + /* Converts byte-addressable APIC register offset to 4-byte offset. */ static inline u32 apic_reg_index(u32 reg) {
Add a few functions to apic.c to make it easier to enable and disable the local apic timer. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- lib/x86/apic.c | 37 +++++++++++++++++++++++++++++++++++++ lib/x86/apic.h | 6 ++++++ 2 files changed, 43 insertions(+)