Re: [Qemu-ppc] [PATCH for-2.13 03/10] target/ppc: Remove unnecessary initialization of LPCR_UPRT

[Cédric Le Goater]

On 04/26/2018 08:46 AM, David Gibson wrote:
> On Wed, Apr 25, 2018 at 11:52:18AM +0200, Cédric Le Goater wrote:
>> On 04/17/2018 09:17 AM, David Gibson wrote:
>>> In cpu_ppc_set_papr() the UPRT and GTSE bits of the LPCR are initialized
>>> based on on ppc64_radix_guest().  Which seems reasonable, except that
>>> ppc64_radix_guest() is based on spapr->patb_entry which is only set up
>>> in spapr_machine_reset, called much later than cpu_ppc_set_papr().
>>>
>>> So the initialization here is pointless.  The base cpu initialization
>>> already sets a value that's good enough until the guest uses an hcall to
>>> configure it's preferred MMU mode.
>>>
>>> Signed-off-by: David Gibson <address@hidden>
>>> ---
>>>  target/ppc/translate_init.c | 16 ----------------
>>>  1 file changed, 16 deletions(-)
>>>
>>> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
>>> index bb79d23b50..14f346f441 100644
>>> --- a/target/ppc/translate_init.c
>>> +++ b/target/ppc/translate_init.c
>>> @@ -8897,22 +8897,6 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, 
>>> PPCVirtualHypervisor *vhyp)
>>>      lpcr->default_value &= ~LPCR_RMLS;
>>>      lpcr->default_value |= 1ull << LPCR_RMLS_SHIFT;
>>>  
>>> -    if (env->mmu_model == POWERPC_MMU_3_00) {
>>> -        /* By default we choose legacy mode and switch to new hash or radix
>>> -         * when a register process table hcall is made. So disable process
>>> -         * tables and guest translation shootdown by default
>>> -         *
>>> -         * Hot-plugged CPUs inherit from the guest radix setting under
>>> -         * KVM but not under TCG. Update the default LPCR to keep new
>>> -         * CPUs in sync when radix is enabled.
>>> -         */
>>
>>
>> This is breaking CPU hotplug under TCG. Should we reintroduce the same 
>> settings in spapr_cpu_reset() now ?
> 
> Sod.  Yeah, this code is important for the hotplug case.
> 
> But, no, I don't think it should go into reset, I think it should go
> into the rtas start-cpu path; it only makes sense for secondary CPUs.

yes. I will send a fix.

Thanks,

C.