On Fri, Jan 25, 2019 at 04:44:39PM +0000, Alexandru Elisei wrote:
> On 1/25/19 4:14 PM, Andre Przywara wrote:
> > On Fri, 25 Jan 2019 17:05:45 +0100
> > Andrew Jones <drjones@xxxxxxxxxx> wrote:
> >
> > Hi,
> >
> >> On Fri, Jan 25, 2019 at 04:31:37PM +0100, Andrew Jones wrote:
> >>> On Fri, Jan 25, 2019 at 02:56:30PM +0000, Alexandru Elisei wrote:
> >>>> On 1/24/19 1:35 PM, Andrew Jones wrote:
> >>>>> On Thu, Jan 24, 2019 at 02:00:20PM +0100, Andrew Jones wrote:
> >>>>>> [..]
> >>>>>> chr_testdev_init() ensures vcon is NULL if it fails to
> >>>>>> initialize. chr_testdev_exit() immediately returns if vcon is
> >>>>>> NULL. This was done by design to allow fallback exits to be
> >>>>>> placed below the chr_testdev_exit call, e.g. halt().
> >>>>>>
> >>>>>> We should be able to drop patch 3/7 and change exit() to this
> >>>>>>
> >>>>>> void exit(int code)
> >>>>>> {
> >>>>>> chr_testdev_exit(code);
> >>>>>> psci_system_off();
> >>>>>> halt(code);
> >>>>>> __builtin_unreachable();
> >>>>>> }
> >>>>>>
> >>>>> There's also a framework for exits that can't return status
> >>>>> codes. powerpc uses it. Before exiting with psci_system_off we
> >>>>> need to make this print statement
> >>>>>
> >>>>> printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1);
> >>>>>
> >>>>> And run_qemu in arm/run needs to be changed to run_qemu_status.
> >>>>> It's hacky, but maybe we can live with it until kvmtool offers
> >>>>> some sort of debug exit.
> >>>>>
> >>>>> Thanks,
> >>>>> drew
> >>>> I can make the change, but if I understand the
> >>>> scripts/arch-run.bash code correctly, run_qemu() will check if
> >>>> QEMU was terminated because of a signal and adjust the return
> >>>> code to take that into account. Using run_qemu_status() means
> >>>> that check won't be made when the tests are run under QEMU, is
> >>>> that acceptable?
> >>> run_qemu_status() first calls run_qemu(). If the return value from
> >>> run_qemu() is 1, then it'll change the value to whatever the EXIT:
> >>> status line says it should be. If run_qemu() detected that a signal
> >>> caused the exit, then the return value won't be 1. In that case
> >>> run_qemu_status() will simply propagate the value to the caller.
> >>>
> >>> It might be worth doing a few tests to ensure that all works out as
> >>> designed, but I'm pretty sure it should.
> >>>
> >> It just occurred to me that you must not be using the run scripts
> >> anyway, since they would require further patches to work. In that
> >> case, there's no need to change arm/run unless you also provide those
> >> additional patches.
> >>
> >> BTW, I wouldn't be opposed to a second run script, rather than trying
> >> to make one script work for both qemu and kvmtool. Ideally both
> >> scripts would be driven by the same higher level scripts using the
> >> same unittests.cfg file though. The unittests.cfg extra_params field
> >> will make that a bit challenging, but otherwise I think adding a few
> >> new helper functions to scripts/arch-run.bash may be all that's
> >> necessary.
> > Yeah, I had some patches along those lines: split test parameters
> > from QEMU parameters, abstract common stuff like number of cores and
> > amount of memory, mark tests as QEMU only and so on. And I had a
> > separate run script for kvmtool, IIRC.
> >
> > If there is interest I can try to post them, but I would consider
> > this an additional effort on top of this series.
> >
> > Cheers,
> > Andre.
>
> I don't use the kvm-unit-tests run script, that is correct. I would prefer that
> I don't change arm/run and keep the function exit() like you originally suggested:
>
> void exit(int code)
> {
> chr_testdev_exit(code);
> psci_system_off();
> halt(code);
> __builtin_unreachable();
> }
>
> If anyone is interested, I or Andre can post patches for the run scripts as a separate set. Is that alright with you, Andrew?
>
Yup, that's fine.
