| Message ID | 20180620100825.22476-5-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: TOD refactoring + TCG CPU hotplug support | expand |
On 20.06.2018 12:08, David Hildenbrand wrote: > Let's stop the timer and delete any pending CKC IRQ before doing > anything else. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > target/s390x/misc_helper.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c > index 6ccbe1fe9a..d5f9f5e1d3 100644 > --- a/target/s390x/misc_helper.c > +++ b/target/s390x/misc_helper.c > @@ -158,6 +158,12 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time) > Error *err = NULL; > S390TOD tod_base; > > + /* stop the timer and remove pending CKC IRQs */ > + timer_del(env->tod_timer); > + qemu_mutex_lock_iothread(); > + env->pending_int &= ~INTERRUPT_EXT_CLOCK_COMPARATOR; > + qemu_mutex_unlock_iothread(); Reviewed-by: Thomas Huth <thuth@redhat.com> > if (time == -1ULL) { I wonder whether that check is still adequate? Is there really a way to disable the clock comparator like this? At least I haven't seen it in the PoP. > return; > } >
On 21.06.2018 14:09, Thomas Huth wrote: > On 20.06.2018 12:08, David Hildenbrand wrote: >> Let's stop the timer and delete any pending CKC IRQ before doing >> anything else. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> target/s390x/misc_helper.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c >> index 6ccbe1fe9a..d5f9f5e1d3 100644 >> --- a/target/s390x/misc_helper.c >> +++ b/target/s390x/misc_helper.c >> @@ -158,6 +158,12 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time) >> Error *err = NULL; >> S390TOD tod_base; >> >> + /* stop the timer and remove pending CKC IRQs */ >> + timer_del(env->tod_timer); >> + qemu_mutex_lock_iothread(); >> + env->pending_int &= ~INTERRUPT_EXT_CLOCK_COMPARATOR; >> + qemu_mutex_unlock_iothread(); > > Reviewed-by: Thomas Huth <thuth@redhat.com> > >> if (time == -1ULL) { > > I wonder whether that check is still adequate? Is there really a way to > disable the clock comparator like this? At least I haven't seen it in > the PoP. e.g. 4-61 (Control) 4. When the clock-comparator sign control is zero, (a) the program can set the clock comparator to all zeros to ensure that an interruption condition is immediately present, and (b) the program can set the clock comparator to a value of all binary ones to ensure that a clock-comparator interrup- tion is never recognized. [...] Rational from 4-59 (Control): The clock comparator provides a means of causing an interruption when the TOD-clock value exceeds a value specified by the program. We can never exceed all binary 1s. So it is really exceeding, not "hitting" > >> return; >> } >> >
On Thu, 21 Jun 2018 14:09:28 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 20.06.2018 12:08, David Hildenbrand wrote: > > Let's stop the timer and delete any pending CKC IRQ before doing > > anything else. > > > > Signed-off-by: David Hildenbrand <david@redhat.com> > > --- > > target/s390x/misc_helper.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c > > index 6ccbe1fe9a..d5f9f5e1d3 100644 > > --- a/target/s390x/misc_helper.c > > +++ b/target/s390x/misc_helper.c > > @@ -158,6 +158,12 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time) > > Error *err = NULL; > > S390TOD tod_base; > > > > + /* stop the timer and remove pending CKC IRQs */ > > + timer_del(env->tod_timer); > > + qemu_mutex_lock_iothread(); > > + env->pending_int &= ~INTERRUPT_EXT_CLOCK_COMPARATOR; > > + qemu_mutex_unlock_iothread(); > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > > if (time == -1ULL) { > > I wonder whether that check is still adequate? Is there really a way to > disable the clock comparator like this? At least I haven't seen it in > the PoP. Me neither. It seems to have been in the code since the beginning... > > > return; > > } > > >
On 21.06.2018 15:54, David Hildenbrand wrote: > On 21.06.2018 14:09, Thomas Huth wrote: >> On 20.06.2018 12:08, David Hildenbrand wrote: [...] >>> if (time == -1ULL) { >> >> I wonder whether that check is still adequate? Is there really a way to >> disable the clock comparator like this? At least I haven't seen it in >> the PoP. > > e.g. 4-61 (Control) > > 4. When the clock-comparator sign control is zero, > (a) the program can set the clock comparator to > all zeros to ensure that an interruption condition > is immediately present, and (b) the program can > set the clock comparator to a value of all binary > ones to ensure that a clock-comparator interrup- > tion is never recognized. [...] > > Rational from 4-59 (Control): > > The clock comparator provides a means of causing > an interruption when the TOD-clock value exceeds a > value specified by the program. > > We can never exceed all binary 1s. So it is really exceeding, not "hitting" Thanks! ... maybe you could add a comment to the code with a pointer to that section in the PoP while you're at it? (just if you agree that it is useful) Thomas
On 21.06.2018 15:58, Cornelia Huck wrote: > On Thu, 21 Jun 2018 14:09:28 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> On 20.06.2018 12:08, David Hildenbrand wrote: >>> Let's stop the timer and delete any pending CKC IRQ before doing >>> anything else. >>> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> --- >>> target/s390x/misc_helper.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c >>> index 6ccbe1fe9a..d5f9f5e1d3 100644 >>> --- a/target/s390x/misc_helper.c >>> +++ b/target/s390x/misc_helper.c >>> @@ -158,6 +158,12 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time) >>> Error *err = NULL; >>> S390TOD tod_base; >>> >>> + /* stop the timer and remove pending CKC IRQs */ >>> + timer_del(env->tod_timer); >>> + qemu_mutex_lock_iothread(); >>> + env->pending_int &= ~INTERRUPT_EXT_CLOCK_COMPARATOR; >>> + qemu_mutex_unlock_iothread(); >> >> Reviewed-by: Thomas Huth <thuth@redhat.com> >> >>> if (time == -1ULL) { >> >> I wonder whether that check is still adequate? Is there really a way to >> disable the clock comparator like this? At least I haven't seen it in >> the PoP. > > Me neither. It seems to have been in the code since the beginning... I agree that this is somewhat buried deep down in the PoP and not obvious when only looking at SET CLOCK COMPARATOR documentation. I'll add a comment like "we can never exceed -1 and therefore never trigger an IRQ"
On Thu, 21 Jun 2018 15:54:36 +0200 David Hildenbrand <david@redhat.com> wrote: > On 21.06.2018 14:09, Thomas Huth wrote: > > On 20.06.2018 12:08, David Hildenbrand wrote: > >> Let's stop the timer and delete any pending CKC IRQ before doing > >> anything else. > >> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- > >> target/s390x/misc_helper.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c > >> index 6ccbe1fe9a..d5f9f5e1d3 100644 > >> --- a/target/s390x/misc_helper.c > >> +++ b/target/s390x/misc_helper.c > >> @@ -158,6 +158,12 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time) > >> Error *err = NULL; > >> S390TOD tod_base; > >> > >> + /* stop the timer and remove pending CKC IRQs */ > >> + timer_del(env->tod_timer); > >> + qemu_mutex_lock_iothread(); > >> + env->pending_int &= ~INTERRUPT_EXT_CLOCK_COMPARATOR; > >> + qemu_mutex_unlock_iothread(); > > > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > > >> if (time == -1ULL) { > > > > I wonder whether that check is still adequate? Is there really a way to > > disable the clock comparator like this? At least I haven't seen it in > > the PoP. > > e.g. 4-61 (Control) > > 4. When the clock-comparator sign control is zero, > (a) the program can set the clock comparator to > all zeros to ensure that an interruption condition > is immediately present, and (b) the program can > set the clock comparator to a value of all binary > ones to ensure that a clock-comparator interrup- > tion is never recognized. [...] > > Rational from 4-59 (Control): > > The clock comparator provides a means of causing > an interruption when the TOD-clock value exceeds a > value specified by the program. > > We can never exceed all binary 1s. So it is really exceeding, not "hitting" OK, so that actually makes sense with your change on top. Before we just jumped out. Agree about adding a comment.
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c index 6ccbe1fe9a..d5f9f5e1d3 100644 --- a/target/s390x/misc_helper.c +++ b/target/s390x/misc_helper.c @@ -158,6 +158,12 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time) Error *err = NULL; S390TOD tod_base; + /* stop the timer and remove pending CKC IRQs */ + timer_del(env->tod_timer); + qemu_mutex_lock_iothread(); + env->pending_int &= ~INTERRUPT_EXT_CLOCK_COMPARATOR; + qemu_mutex_unlock_iothread(); + if (time == -1ULL) { return; }
Let's stop the timer and delete any pending CKC IRQ before doing anything else. Signed-off-by: David Hildenbrand <david@redhat.com> --- target/s390x/misc_helper.c | 6 ++++++ 1 file changed, 6 insertions(+)