| Message ID | b6cbf871-53a6-15ee-99d5-0ad2ab9c8b80@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/vPIT: account for "counter stopped" time | expand |
On Tue, May 30, 2023 at 05:30:02PM +0200, Jan Beulich wrote: > To avoid the need for a forward declaration of pit_load_count() in a > subsequent change, move it earlier in the file (along with its helper > callback). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Just a couple of nits, which you might also noticed but decided to not fix given this is just code movement. > > --- a/xen/arch/x86/emul-i8254.c > +++ b/xen/arch/x86/emul-i8254.c > @@ -87,6 +87,57 @@ static int pit_get_count(PITState *pit, > return counter; > } > > +static void cf_check pit_time_fired(struct vcpu *v, void *priv) Seems like v could be constified? > +{ > + uint64_t *count_load_time = priv; > + TRACE_0D(TRC_HVM_EMUL_PIT_TIMER_CB); > + *count_load_time = get_guest_time(v); > +} > + > +static void pit_load_count(PITState *pit, int channel, int val) > +{ > + u32 period; uint32_t Thanks, Roger.
On 01.06.2023 11:17, Roger Pau Monné wrote: > On Tue, May 30, 2023 at 05:30:02PM +0200, Jan Beulich wrote: >> To avoid the need for a forward declaration of pit_load_count() in a >> subsequent change, move it earlier in the file (along with its helper >> callback). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. > Just a couple of nits, which you might also noticed but decided to not > fix given this is just code movement. Indeed, I meant this to be pure code movement. Nevertheless I'd be happy to take care of style issues, if that's deemed okay in a "pure code movement" patch. However, ... >> --- a/xen/arch/x86/emul-i8254.c >> +++ b/xen/arch/x86/emul-i8254.c >> @@ -87,6 +87,57 @@ static int pit_get_count(PITState *pit, >> return counter; >> } >> >> +static void cf_check pit_time_fired(struct vcpu *v, void *priv) > > Seems like v could be constified? ... the function being used as a callback, I doubt adding const would be possible. Otoh ... >> +{ >> + uint64_t *count_load_time = priv; ... there's a blank line missing here, if I was to go for style adjustments. Jan >> + TRACE_0D(TRC_HVM_EMUL_PIT_TIMER_CB); >> + *count_load_time = get_guest_time(v); >> +} >> + >> +static void pit_load_count(PITState *pit, int channel, int val) >> +{ >> + u32 period; > > uint32_t > > Thanks, Roger.
On Thu, Jun 01, 2023 at 11:56:12AM +0200, Jan Beulich wrote: > On 01.06.2023 11:17, Roger Pau Monné wrote: > > On Tue, May 30, 2023 at 05:30:02PM +0200, Jan Beulich wrote: > >> To avoid the need for a forward declaration of pit_load_count() in a > >> subsequent change, move it earlier in the file (along with its helper > >> callback). > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > Thanks. > > > Just a couple of nits, which you might also noticed but decided to not > > fix given this is just code movement. > > Indeed, I meant this to be pure code movement. Nevertheless I'd be happy > to take care of style issues, if that's deemed okay in a "pure code > movement" patch. However, ... It's just small style issues, so it would be OK for me. > >> --- a/xen/arch/x86/emul-i8254.c > >> +++ b/xen/arch/x86/emul-i8254.c > >> @@ -87,6 +87,57 @@ static int pit_get_count(PITState *pit, > >> return counter; > >> } > >> > >> +static void cf_check pit_time_fired(struct vcpu *v, void *priv) > > > > Seems like v could be constified? > > ... the function being used as a callback, I doubt adding const would > be possible. Otoh ... Oh, I see. > >> +{ > >> + uint64_t *count_load_time = priv; > > ... there's a blank line missing here, if I was to go for style > adjustments. Sure. Thanks, Roger.
On 01.06.2023 13:50, Roger Pau Monné wrote: > On Thu, Jun 01, 2023 at 11:56:12AM +0200, Jan Beulich wrote: >> On 01.06.2023 11:17, Roger Pau Monné wrote: >>> On Tue, May 30, 2023 at 05:30:02PM +0200, Jan Beulich wrote: >>>> To avoid the need for a forward declaration of pit_load_count() in a >>>> subsequent change, move it earlier in the file (along with its helper >>>> callback). >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> >>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> >> >> Thanks. >> >>> Just a couple of nits, which you might also noticed but decided to not >>> fix given this is just code movement. >> >> Indeed, I meant this to be pure code movement. Nevertheless I'd be happy >> to take care of style issues, if that's deemed okay in a "pure code >> movement" patch. However, ... > > It's just small style issues, so it would be OK for me. So I've done the obvious ones. There's a further signed/unsigned issue which isn't quite as clear whether to take care of "on the fly": The function's 2nd and 3rd parameters both ought to be unsigned, yet throughout the full file the same issue exists many more times. So I guess I'll leave those untouched for now. Jan
On Thu, Jun 01, 2023 at 04:06:53PM +0200, Jan Beulich wrote: > On 01.06.2023 13:50, Roger Pau Monné wrote: > > On Thu, Jun 01, 2023 at 11:56:12AM +0200, Jan Beulich wrote: > >> On 01.06.2023 11:17, Roger Pau Monné wrote: > >>> On Tue, May 30, 2023 at 05:30:02PM +0200, Jan Beulich wrote: > >>>> To avoid the need for a forward declaration of pit_load_count() in a > >>>> subsequent change, move it earlier in the file (along with its helper > >>>> callback). > >>>> > >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >>> > >>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > >> > >> Thanks. > >> > >>> Just a couple of nits, which you might also noticed but decided to not > >>> fix given this is just code movement. > >> > >> Indeed, I meant this to be pure code movement. Nevertheless I'd be happy > >> to take care of style issues, if that's deemed okay in a "pure code > >> movement" patch. However, ... > > > > It's just small style issues, so it would be OK for me. > > So I've done the obvious ones. There's a further signed/unsigned issue > which isn't quite as clear whether to take care of "on the fly": The > function's 2nd and 3rd parameters both ought to be unsigned, yet > throughout the full file the same issue exists many more times. So I > guess I'll leave those untouched for now. No strong opinion regarding those, I'm fine if you want to adjust also to unsigned. Thanks, Roger.
--- a/xen/arch/x86/emul-i8254.c +++ b/xen/arch/x86/emul-i8254.c @@ -87,6 +87,57 @@ static int pit_get_count(PITState *pit, return counter; } +static void cf_check pit_time_fired(struct vcpu *v, void *priv) +{ + uint64_t *count_load_time = priv; + TRACE_0D(TRC_HVM_EMUL_PIT_TIMER_CB); + *count_load_time = get_guest_time(v); +} + +static void pit_load_count(PITState *pit, int channel, int val) +{ + u32 period; + struct hvm_hw_pit_channel *s = &pit->hw.channels[channel]; + struct vcpu *v = vpit_vcpu(pit); + + ASSERT(spin_is_locked(&pit->lock)); + + if ( val == 0 ) + val = 0x10000; + + if ( v == NULL ) + pit->count_load_time[channel] = 0; + else + pit->count_load_time[channel] = get_guest_time(v); + s->count = val; + period = DIV_ROUND(val * SYSTEM_TIME_HZ, PIT_FREQ); + + if ( (v == NULL) || !is_hvm_vcpu(v) || (channel != 0) ) + return; + + switch ( s->mode ) + { + case 2: + case 3: + /* Periodic timer. */ + TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, period); + create_periodic_time(v, &pit->pt0, period, period, 0, pit_time_fired, + &pit->count_load_time[channel], false); + break; + case 1: + case 4: + /* One-shot timer. */ + TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, 0); + create_periodic_time(v, &pit->pt0, period, 0, 0, pit_time_fired, + &pit->count_load_time[channel], false); + break; + default: + TRACE_0D(TRC_HVM_EMUL_PIT_STOP_TIMER); + destroy_periodic_time(&pit->pt0); + break; + } +} + static int pit_get_out(PITState *pit, int channel) { struct hvm_hw_pit_channel *s = &pit->hw.channels[channel]; @@ -156,57 +207,6 @@ static int pit_get_gate(PITState *pit, i return pit->hw.channels[channel].gate; } -static void cf_check pit_time_fired(struct vcpu *v, void *priv) -{ - uint64_t *count_load_time = priv; - TRACE_0D(TRC_HVM_EMUL_PIT_TIMER_CB); - *count_load_time = get_guest_time(v); -} - -static void pit_load_count(PITState *pit, int channel, int val) -{ - u32 period; - struct hvm_hw_pit_channel *s = &pit->hw.channels[channel]; - struct vcpu *v = vpit_vcpu(pit); - - ASSERT(spin_is_locked(&pit->lock)); - - if ( val == 0 ) - val = 0x10000; - - if ( v == NULL ) - pit->count_load_time[channel] = 0; - else - pit->count_load_time[channel] = get_guest_time(v); - s->count = val; - period = DIV_ROUND(val * SYSTEM_TIME_HZ, PIT_FREQ); - - if ( (v == NULL) || !is_hvm_vcpu(v) || (channel != 0) ) - return; - - switch ( s->mode ) - { - case 2: - case 3: - /* Periodic timer. */ - TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, period); - create_periodic_time(v, &pit->pt0, period, period, 0, pit_time_fired, - &pit->count_load_time[channel], false); - break; - case 1: - case 4: - /* One-shot timer. */ - TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, 0); - create_periodic_time(v, &pit->pt0, period, 0, 0, pit_time_fired, - &pit->count_load_time[channel], false); - break; - default: - TRACE_0D(TRC_HVM_EMUL_PIT_STOP_TIMER); - destroy_periodic_time(&pit->pt0); - break; - } -} - static void pit_latch_count(PITState *pit, int channel) { struct hvm_hw_pit_channel *c = &pit->hw.channels[channel];
To avoid the need for a forward declaration of pit_load_count() in a subsequent change, move it earlier in the file (along with its helper callback). Signed-off-by: Jan Beulich <jbeulich@suse.com>