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 <a...@ozlabs.ru>
>>> ---
>>> 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
