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

[Alexandru Elisei <alexandru.elisei@arm.com>]

On 1/25/19 4:14 PM, Andre Przywara wrote:
> 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.

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?