On 26.06.2018 14:27, Cornelia Huck wrote:
> On Tue, 26 Jun 2018 14:06:00 +0200
> David Hildenbrand <da...@redhat.com> 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
