Re: [Qemu-ppc] [PATCH] spapr: support CPU hotplug

[Alexey Kardashevskiy]

On 08/27/2013 01:29 AM, David Gibson wrote:
> On Mon, Aug 26, 2013 at 02:24:49PM +0200, Alexander Graf wrote:
>>
>> On 23.08.2013, at 13:30, Alexey Kardashevskiy wrote:
>>
>>> PAPR+ requires two RTAS calls to be supported by the hypervisor in
>>> order to allow hotplugging VCPUs from the guest. The "start-cpu" RTAS
>>> call was already there but "stop-self" was not.
>>>
>>> This adds the "stop-self" RTAS call.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>> ---
>>> hw/ppc/spapr_rtas.c | 14 ++++++++++++++
>>> 1 file changed, 14 insertions(+)
>>>
>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>> index 394ce05..8a4cfa0 100644
>>> --- a/hw/ppc/spapr_rtas.c
>>> +++ b/hw/ppc/spapr_rtas.c
>>> @@ -202,6 +202,19 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, 
>>> sPAPREnvironment *spapr,
>>>     rtas_st(rets, 0, -3);
>>> }
>>>
>>> +static void rtas_stop_self(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>> +                           uint32_t token, uint32_t nargs,
>>> +                           target_ulong args,
>>> +                           uint32_t nret, target_ulong rets)
>>> +{
>>> +    CPUState *cs = CPU(cpu);
>>> +    CPUPPCState *env = &cpu->env;
>>> +
>>> +    cs->halted = 1;
>>> +    env->msr = 0;
>>
>> So this is here to make sure we don't accidentally get out of halted state 
>> by an interrupt on that vcpu. Could you please somehow make that part 
>> obvious? Either by adding a comment or by only explicitly masking DEC and EE 
>> and a comment :).
>>
>>> +    cs->exit_request = 1;
>>
>> This should probably be qemu_cpu_kick_self().
> 
> Uh, no, I don't think so.  This is there purely to make sure we exit
> the inner loop, and actually test cpu_can_run() which will test
> halted.  AFAICT qemu_cpu_kick_self() won't do anything similar.


rtas_stop_self() eventually returns to kvm_cpu_exec() which calls
qemu_cpu_kick_self() and resets cs->exit_request before return so I do not
really see the difference in behaviour. And actually both ways CPU stops in
exactly the same way. What do I miss?



-- 
Alexey