Re: [Qemu-devel] [qemu-s390x] [PULL 2/2] iotests: simple mirror test with kvm on 1G image

[Kevin Wolf]

Am 06.12.2018 um 12:05 hat Christian Borntraeger geschrieben:
> 
> 
> On 05.12.2018 17:09, Vladimir Sementsov-Ogievskiy wrote:
> > 05.12.2018 18:52, Christian Borntraeger wrote:
> >>
> >>
> >> On 05.12.2018 14:39, Vladimir Sementsov-Ogievskiy wrote:
> >>> 05.12.2018 15:35, Christian Borntraeger wrote:
> >>>>
> >>>>
> >>>> On 05.12.2018 13:00, Vladimir Sementsov-Ogievskiy wrote:
> >>>>> 05.12.2018 12:01, Christian Borntraeger wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 05.12.2018 09:46, Kevin Wolf wrote:
> >>>>>>> Am 05.12.2018 um 09:23 hat Christian Borntraeger geschrieben:
> >>>>>>>>>>> +# prepare source image
> >>>>>>>>>>> +qemu_img_create('-f', iotests.imgfmt, '-o', 
> >>>>>>>>>>> 'preallocation=metadata', disk,
> >>>>>>>>>>> +                str(size))
> >>>>>>>>>>> +
> >>>>>>>>>>> +vm = QEMUMachine(iotests.qemu_prog)
> >>>>>>>>>>> +vm.add_args('-machine', 'pc,accel=kvm')
> >>>>>>>>
> >>>>>>>> This (pc) clearly does not work on other architectures.
> >>>>>>>> In addition to that, I also need to add -no-shutdown on s390 (see 
> >>>>>>>> 068 for a similar case)
> >>>>>>>
> >>>>>>> Leaving out pc definitely makes sense, and the bug still reproduces 
> >>>>>>> for
> >>>>>>> me without it.
> >>>>>>>
> >>>>>>> I don't understand the -no-shutdown, though. Already for 068, neither
> >>>>>>> the code nor the commit message when it was added explain why this is
> >>>>>>> needed.
> >>>>>>>
> >>>>>>> Can you turn this into a proper patch and add a comment why 
> >>>>>>> -no-shutdown
> >>>>>>> is needed?
> >>>>>>
> >>>>>> I already sent this patch. The reason is that there is no BIOS in a 
> >>>>>> classical sense
> >>>>>> on s390x. If no bootable image (external kernel or from disk) is 
> >>>>>> found, the small boot
> >>>>>> bios loads a disabled wait PSW. The default action for that is then 
> >>>>>> shutdown.
> >>>>>>
> >>>>>
> >>>>> Is it an option for you just drop the whole line 
> >>>>> "vm.add_args('-machine', 'pc,accel=kvm')"?
> >>>>> The problem without it for me was that gdb failed to produce full and 
> >>>>> nice backtrace, but
> >>>>> test worked anyway
> >>>>
> >>>> In the commid message Vladimir said that kvm is necessary to trigger the 
> >>>> problem.
> >>>>
> >>>
> >>> No, I didn't)
> >>>
> >>> and it's in the comment:
> >>> # 3. drop kvm and use iotests.VM() (maybe, because of qtest) (however, it 
> >>> still
> >>> #    reproduces, if just drop kvm, but gdb failed to produce full 
> >>> backtraces
> >>> #    for me)
> >>
> >> Ok, so I would be fine with completely dropping that line.
> >>
> >> the patch would then be
> >>
> >>
> >>
> >> "-machine pc" will not work all architectures. Lets fall back to the
> >> default machine by not specifying anything for the machine.
> >>
> >> In addition we also need to specify -no-shutdown on s390 as qemu will
> >> exit on guest shutdown. This happens when there is no kernel or bootable
> >> disk on s390.
> >>
> >> Signed-off-by: Christian Borntraeger <address@hidden>
> >> ---
> >>   tests/qemu-iotests/235 | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
> >> index da044ed34e..329da8f0c2 100755
> >> --- a/tests/qemu-iotests/235
> >> +++ b/tests/qemu-iotests/235
> >> @@ -49,7 +49,8 @@ qemu_img_create('-f', iotests.imgfmt, '-o', 
> >> 'preallocation=metadata', disk,
> >>                   str(size))
> >>   
> >>   vm = QEMUMachine(iotests.qemu_prog)
> >> -vm.add_args('-machine', 'pc,accel=kvm')
> >> +if iotests.qemu_default_machine == 's390-ccw-virtio':
> >> +        vm.add_args('-no-shutdown')
> >>   vm.add_args('-drive', 'id=src,file=' + disk)
> >>   vm.launch()
> >>   
> >>
> >>
> >> Shall I resend a v2?
> >>
> > 
> > so, we need -no-shutdown even if we drop kvm? I hoped that not..
> > Hmm. grep points only to one iotest doing the same about no-shutdown
> > - 068..
> 
> Kevin, shall I send the above patch as v2?

Don't bother, I think v1 is fine. By specifying kvm explicitly, it's at
least clear that we're not using qtest like in other tests.

Kevin