From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45566) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fXn55-0006kQ-BV for qemu-devel@nongnu.org; Tue, 26 Jun 2018 08:28:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fXn54-000233-6P for qemu-devel@nongnu.org; Tue, 26 Jun 2018 08:28:23 -0400 References: <20180625115352.6889-1-david@redhat.com> <20180625115352.6889-6-david@redhat.com> <70637e9f-6fd6-a92e-0d2e-878f23460ac3@redhat.com> <384da416-9ce3-30aa-7bff-0ff0652cb8f4@redhat.com> <20180626142702.444a3457.cohuck@redhat.com> From: David Hildenbrand Message-ID: <437da9a4-6f38-b51e-a26a-b183609ba081@redhat.com> Date: Tue, 26 Jun 2018 14:28:18 +0200 MIME-Version: 1.0 In-Reply-To: <20180626142702.444a3457.cohuck@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [qemu-s390x] [PATCH v3 5/9] s390x/tcg: properly implement the TOD List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck Cc: Thomas Huth , qemu-s390x@nongnu.org, Alexander Graf , qemu-devel@nongnu.org, Christian Borntraeger , Richard Henderson On 26.06.2018 14:27, Cornelia Huck wrote: > On Tue, 26 Jun 2018 14:06:00 +0200 > David Hildenbrand wrote: > >>>> @@ -34,10 +58,24 @@ static void qemu_s390_tod_class_init(ObjectClass *oc, void *data) >>>> tdc->set = qemu_s390_tod_set; >>>> } >>>> >>>> +static void qemu_s390_tod_init(Object *obj) >>>> +{ >>>> + S390TODState *td = S390_TOD(obj); >>>> + struct tm tm; >>>> + >>>> + qemu_get_timedate(&tm, 0); >>>> + td->base.high = 0; >>>> + td->base.low = TOD_UNIX_EPOCH + (time2tod(mktimegm(&tm)) * 1000000000ULL); >>>> + if (td->base.low < TOD_UNIX_EPOCH) { >>>> + td->base.high += 1; >>>> + } >>>> +} >>> >>> Nit: It would be sufficient to do this in the realize() function instead. >> >> Then I'll have to overwrite the realize function and store the >> parent_realize function - something that I want to avoid if not really >> necessary. > > Agreed. I'd just leave it as it is now. > >> >> (for now it was also done in the cpu initfn, so that should be fine) >> >>> >>>> static TypeInfo qemu_s390_tod_info = { >>>> .name = TYPE_QEMU_S390_TOD, >>>> .parent = TYPE_S390_TOD, >>>> .instance_size = sizeof(S390TODState), >>>> + .instance_init = qemu_s390_tod_init, >>>> .class_init = qemu_s390_tod_class_init, >>>> .class_size = sizeof(S390TODClass), >>>> }; >>> [...] >>>> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c >>>> index dd5273949b..be341b5295 100644 >>>> --- a/target/s390x/misc_helper.c >>>> +++ b/target/s390x/misc_helper.c >>>> @@ -28,6 +28,8 @@ >>>> #include "qemu/timer.h" >>>> #include "exec/exec-all.h" >>>> #include "exec/cpu_ldst.h" >>>> +#include "qapi/error.h" >>>> +#include "tcg_s390x.h" >>>> >>>> #if !defined(CONFIG_USER_ONLY) >>>> #include "sysemu/cpus.h" >>>> @@ -39,6 +41,7 @@ >>>> #include "hw/s390x/ioinst.h" >>>> #include "hw/s390x/s390-pci-inst.h" >>>> #include "hw/boards.h" >>>> +#include "hw/s390x/tod.h" >>>> #endif >>>> >>>> /* #define DEBUG_HELPER */ >>>> @@ -138,25 +141,32 @@ void HELPER(spx)(CPUS390XState *env, uint64_t a1) >>>> /* Store Clock */ >>>> uint64_t HELPER(stck)(CPUS390XState *env) >>>> { >>>> - uint64_t time; >>>> + S390TODState *td = s390_get_todstate(); >>>> + S390TODClass *tdc = S390_TOD_GET_CLASS(td); >>>> + S390TOD tod; >>>> >>>> - time = env->tod_offset + >>>> - time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); >>>> - >>>> - return time; >>>> + tdc->get(td, &tod, &error_abort); >>>> + return tod.low; >>>> } >>>> >>>> /* Set Clock Comparator */ >>>> void HELPER(sckc)(CPUS390XState *env, uint64_t time) >>>> { >>>> + S390TODState *td = s390_get_todstate(); >>>> + S390TODClass *tdc = S390_TOD_GET_CLASS(td); >>>> + S390TOD tod_base; >>>> + >>>> if (time == -1ULL) { >>>> return; >>>> } >>>> >>>> env->ckc = time; >>>> >>>> + tdc->get(td, &tod_base, &error_abort); >>>> + tod_base.low -= time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); >>> >>> So the tdc->get first adds the time2tod, and then you subtract it here >>> again? Can't you simply use td->base.low directly instead? >> >> That might be a good idea, previously I had tdc->get_base(), but dropped >> it because it cannot be implemented for KVM. >> >> Simply accessing the member here should be fine. Thanks! > > So, we're guaranteed to have td->base.low at the correct value? (Sorry, > having a bit of a hard time following around here.) > Yes, just verified, changed and tested. We will have the value at that place. I'll give some more time to review the other parts, then I can resend. -- Thanks, David / dhildenb