Re: [kvm-unit-tests PATCH 5/7] lib: arm: Fallback to psci_system_off() in exit()

[Andre Przywara <andre.przywara@arm.com>]

On Fri, 25 Jan 2019 17:05:45 +0100
Andrew Jones <drjones@redhat.com> 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.