[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/3] cpu-timers, icount: new modules
From: |
Claudio Fontana |
Subject: |
Re: [PATCH 3/3] cpu-timers, icount: new modules |
Date: |
Fri, 10 Jul 2020 21:20:08 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 |
On 7/10/20 8:33 AM, Cornelia Huck wrote:
> On Thu, 9 Jul 2020 20:46:56 +0200
> Claudio Fontana <cfontana@suse.de> wrote:
>
>> On 7/9/20 8:38 PM, Claudio Fontana wrote:
>>> On 7/8/20 5:05 PM, Paolo Bonzini wrote:
>>>> On 08/07/20 17:00, Claudio Fontana wrote:
>>>>>> Bisectable, 100% failure rate, etc. :( Can you split the patch in
>>>>>> multiple parts, specifically separating any rename or introducing of
>>>>>> includes from the final file move?
>>>>> Hi Paolo,
>>>>>
>>>>> will take a look!
>>>>>
>>>>> Is this captured by some travis / cirrus-ci / anything I can easily see
>>>>> the result of?
>>>>>
>>>>>
>>>>
>>>> Nope, unfortunately we don't have an s390 CI. But if you can get your
>>>> hands on one, just "./configure --target-list=s390x-softmmu && make &&
>>>> make check-block" will show it.
>>>
>>> So this is tricky, but I am making some progress after getting my hands on
>>> one.
>>> Maybe if someone understands s390 keys better, I could be clued in.
>>
>>
>> Also adding Cornelia to Cc:.
>>
>> Maybe the savevm_s390_storage_keys SaveVMHandlers etc assume that the icount
>> state part of the vmstate is there?
>
> I don't see anything that would deal with icount here. Adding Jason to
> cc: in case he has an idea. (I assume it would behave the same under
> KVM, as the only thing different are the internal callbacks.)
yes, same between tcg and kvm.
>
>>
>>
>>>
>>> In short this goes away if I again set icount to enabled for qtest,
>>> basically ensuring that --enable-tcg is there and then reenabling icount.
>>>
>>> qtest was forcing icount and shift=0 by creating qemu options, in order to
>>> misuse its counter feature,
>>> instead of using a separate counter.
>>>
>>> Removing that ugliness we end up with different behavior of save/load,
>>> because vmstate will now suddenly not contain icount-related values anymore.
>>> What I do not understand is why this causes a problem because save should
>>> just not store the icount state and load should just not load the icount
>>> state,
>>> and why we die on the load of s390 keys state (it works just fine for other
>>> architectures).
>
> Yes, I don't really see why skeys is so special. No endianness stuff, I
> assume?
No, does not seem to be the issue.
I discovered a way simpler way to "fix" it:
static bool icount_state_needed(void *opaque)
{
return 1;
}
Ie, making sure that the state is always saved/restored, even when unused.
Really weird.
I logged/debugged the vmstate code, and I can see that things seem symmetric
between save and load when it comes to timers.
something puts 0s into the key somehow...
>
>>>
>>> Here is a diff that makes the problem disappear, but needs --enable-tcg:
>>>
>>>
>>> ----------------------------------------------------------------------------------------------------
>>> diff --git a/accel/qtest.c b/accel/qtest.c
>>> index 119d0f16a4..4cb16abc2c 100644
>>> --- a/accel/qtest.c
>>> +++ b/accel/qtest.c
>>> @@ -23,6 +23,12 @@
>>>
>>> static int qtest_init_accel(MachineState *ms)
>>> {
>>> + QemuOpts *opts = qemu_opts_create(qemu_find_opts("icount"), NULL, 0,
>>> + &error_abort);
>>> + qemu_opt_set(opts, "shift", "0", &error_abort);
>>> + icount_configure(opts, &error_abort);
>>> + qemu_opts_del(opts);
>>> +
>>> return 0;
>>> }
>>>
>>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>>> index f39fd5270b..a5e788c86a 100644
>>> --- a/softmmu/vl.c
>>> +++ b/softmmu/vl.c
>>> @@ -2786,10 +2786,12 @@ static void configure_accelerators(const char
>>> *progname)
>>> error_report("falling back to %s", ac->name);
>>> }
>>>
>>> + /*
>>> if (icount_enabled() && !tcg_enabled()) {
>>> error_report("-icount is not allowed with hardware
>>> virtualization");
>>> exit(1);
>>> }
>>> + */
>>> }
>>>
>>> static void create_default_memdev(MachineState *ms, const char *path)
>>> ----------------------------------------------------------------------------------------------------
>>>
>>> Without this patch, here is the full failure, maybe someone has a good
>>> hint, otherwise I'll keep digging from here inside the s390-specific code.
>>>
>>> QA output created by 267
>>>
>>> === No block devices at all ===
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing:
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> Error: No block device can accept snapshots
>>> (qemu) info snapshots
>>> No available block device supports snapshots
>>> (qemu) loadvm snap0
>>> Error: No block device supports snapshots
>>> (qemu) quit
>>>
>>>
>>> === -drive if=none ===
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=none
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> Error: Device 'none0' is writable but does not support snapshots
>>> (qemu) info snapshots
>>> No available block device supports snapshots
>>> (qemu) loadvm snap0
>>> Error: Device 'none0' is writable but does not support snapshots
>>> (qemu) quit
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=none
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID TAG VM SIZE DATE VM CLOCK
>>> -- snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000
>>> (qemu) loadvm snap0
>>> (qemu) quit
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=none -device
>>> virtio-blk,drive=none0
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID TAG VM SIZE DATE VM CLOCK
>>> -- snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000
>>> (qemu) loadvm snap0
>>> (qemu) quit
>>>
>>>
>>> === -drive if=virtio ===
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=virtio
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> Error: Device 'virtio0' is writable but does not support snapshots
>>> (qemu) info snapshots
>>> No available block device supports snapshots
>>> (qemu) loadvm snap0
>>> Error: Device 'virtio0' is writable but does not support snapshots
>>> (qemu) quit
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID TAG VM SIZE DATE VM CLOCK
>>> -- snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000
>>> (qemu) loadvm snap0
>>> (qemu) quit
>>>
>>>
>>> === Simple -blockdev ===
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> Error: Device '' is writable but does not support snapshots
>>> (qemu) info snapshots
>>> No available block device supports snapshots
>>> (qemu) loadvm snap0
>>> Error: Device '' is writable but does not support snapshots
>>> (qemu) quit
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file
>>> -blockdev driver=IMGFMT,file=file,node-name=fmt
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID TAG VM SIZE DATE VM CLOCK
>>> -- snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000
>>> (qemu) loadvm snap0
>>> (qemu) quit
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file
>>> -blockdev driver=raw,file=file,node-name=raw -blockdev
>>> driver=IMGFMT,file=raw,node-name=fmt
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID TAG VM SIZE DATE VM CLOCK
>>> -- snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000
>>> (qemu) loadvm snap0
>>> (qemu) quit
>>>
>>>
>>> === -blockdev with a filter on top ===
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file
>>> -blockdev driver=IMGFMT,file=file,node-name=fmt -blockdev
>>> driver=copy-on-read,file=fmt,node-name=filter
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID TAG VM SIZE DATE VM CLOCK
>>> -- snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000
>>> (qemu) loadvm snap0
>>> (qemu) quit
>>>
>>>
>>> === -blockdev with a backing file ===
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> backing_file=TEST_DIR/t.IMGFMT.base
>>> Testing: -blockdev
>>> driver=file,filename=TEST_DIR/t.IMGFMT.base,node-name=backing-file
>>> -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev
>>> driver=IMGFMT,file=file,backing=backing-file,node-name=fmt
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID TAG VM SIZE DATE VM CLOCK
>>> -- snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000
>>> (qemu) loadvm snap0
>>> Unexpected storage key flag data: 0
>>> error while loading state for instance 0x0 of device 's390-skeys'
>>> Error: Error -22 while loading VM state
>>>
>>>
>>>
>>>
>>>>
>>>>>>
>>>>>> #if defined CONFIG_TCG || !defined NEED_CPU_H
>>>>>> extern bool icount_enabled(void);
>>>>>> #else
>>>>>> #define icount_enabled() 0
>>>>>> #endif
>>>>>>
>>>>>> (This way, more TCG-only code in cpus.c gets elided). You can integrate
>>>>>> this change in the next version.
>>>>>>
>>>>>> Paolo
>>>>>>
>>>>>
>>>>> Weird, I tested with --disable-tcg explicitly (but may be some time ago
>>>>> now, as I constantly rebased).
>>>>>
>>>>> Will take a look at the introduction of this #defines in place of
>>>>> variables,
>>>>> as this mechanisms will not work in the future for target-specific
>>>>> modules.
>>>>
>>>> This is only done for per-target files so it should not be a problem.
>>>>
>>>> Paolo
>>>>
>>>>
>>>
>>>
>>
>
>
- Re: [PATCH 3/3] cpu-timers, icount: new modules, (continued)
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Thomas Huth, 2020/07/08
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Paolo Bonzini, 2020/07/08
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Claudio Fontana, 2020/07/08
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Thomas Huth, 2020/07/08
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Claudio Fontana, 2020/07/09
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Claudio Fontana, 2020/07/09
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Cornelia Huck, 2020/07/10
- Re: [PATCH 3/3] cpu-timers, icount: new modules,
Claudio Fontana <=
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Cornelia Huck, 2020/07/13
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Claudio Fontana, 2020/07/11
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Cornelia Huck, 2020/07/13
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Claudio Fontana, 2020/07/13
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Thomas Huth, 2020/07/10
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Paolo Bonzini, 2020/07/10
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Claudio Fontana, 2020/07/11
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Paolo Bonzini, 2020/07/11
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Claudio Fontana, 2020/07/11
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Paolo Bonzini, 2020/07/11