Re: [PATCH V5 01/12] cpus: refactor vm_stop

[Steven Sistare]

On 11/20/2023 2:46 PM, Peter Xu wrote:
> On Mon, Nov 20, 2023 at 02:09:31PM -0500, Steven Sistare wrote:
>> On 11/20/2023 8:22 AM, Fabiano Rosas wrote:
>>> Steve Sistare <steven.sistare@oracle.com> writes:
>>>> Refactor the vm_stop functions into one common subroutine do_vm_stop called
>>>> with options.  No functional change.
>>>>
>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>> ---
>>>>  system/cpus.c | 44 +++++++++++++++++---------------------------
>>>>  1 file changed, 17 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/system/cpus.c b/system/cpus.c
>>>> index 0848e0d..f72c4be 100644
>>>> --- a/system/cpus.c
>>>> +++ b/system/cpus.c
>>>> @@ -252,10 +252,21 @@ void cpu_interrupt(CPUState *cpu, int mask)
>>>>      }
>>>>  }
>>>>  
>>>> -static int do_vm_stop(RunState state, bool send_stop)
>>>> +static int do_vm_stop(RunState state, bool send_stop, bool force)
>>>>  {
>>>>      int ret = 0;
>>>>  
>>>> +    if (qemu_in_vcpu_thread()) {
>>>> +        qemu_system_vmstop_request_prepare();
>>>> +        qemu_system_vmstop_request(state);
>>>> +        /*
>>>> +         * FIXME: should not return to device code in case
>>>> +         * vm_stop() has been requested.
>>>> +         */
>>>> +        cpu_stop_current();
>>>> +        return 0;
>>>> +    }
>>>
>>> At vm_stop_force_state(), this block used to be under
>>> runstate_is_running(), but now it runs unconditionally.
>>
>> vm_stop_force_state callers should never be called in a vcpu thread, so this 
>> block
>> is never executed for them.  I could assert that.
>>
>>> At vm_shutdown(), this block was not executed at all, but now it is.
>>
>> vm_shutdown should never be called from a vcpu thread.
>> I could assert that.
> 
> Would you split the patch into two?  Moving qemu_in_vcpu_thread() is one,
> the rest can be put into another, IMHO.  That may also help to make the
> review easier.  OTOH the code changes look all correct here.  Thanks,

Will do, thanks - steve