| Message ID | 20180417071722.9399-4-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 17 Apr 2018 17:17:15 +1000 David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > 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. > - */ > - if (ppc64_radix_guest(cpu)) { > - lpcr->default_value |= LPCR_UPRT | LPCR_GTSE; > - } else { > - lpcr->default_value &= ~(LPCR_UPRT | LPCR_GTSE); > - } > - } > - > /* Only enable Power-saving mode Exit Cause exceptions on the boot > * CPU. The RTAS command start-cpu will enable them on secondaries. > */
On Fri, Apr 20, 2018 at 01:34:14PM +0200, Greg Kurz wrote: > On Tue, 17 Apr 2018 17:17:15 +1000 > David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> > > --- > > Reviewed-by: Greg Kurz <groug@kaod.org> Merged to ppc-for-2.13.
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 <david@gibson.dropbear.id.au> > --- > 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 ? Thanks, C. > - if (ppc64_radix_guest(cpu)) { > - lpcr->default_value |= LPCR_UPRT | LPCR_GTSE; > - } else { > - lpcr->default_value &= ~(LPCR_UPRT | LPCR_GTSE); > - } > - } > - > /* Only enable Power-saving mode Exit Cause exceptions on the boot > * CPU. The RTAS command start-cpu will enable them on secondaries. > */ >
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 <david@gibson.dropbear.id.au> > > --- > > 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.
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 <david@gibson.dropbear.id.au> >>> --- >>> 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.
On Thu, Apr 26, 2018 at 09:20:11AM +0200, Cédric Le Goater wrote: > 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 <david@gibson.dropbear.id.au> > >>> --- > >>> 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. Not sure if this is still on your radar, but if it is, don't bother. I've already made a fix (along with various others) and will be posting soonish.
On 05/01/2018 08:39 AM, David Gibson wrote: > On Thu, Apr 26, 2018 at 09:20:11AM +0200, Cédric Le Goater wrote: >> 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 <david@gibson.dropbear.id.au> >>>>> --- >>>>> 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. > > Not sure if this is still on your radar, but if it is, don't bother. > I've already made a fix (along with various others) and will be > posting soonish. As you reverted the changes in the ppc-2.13 branch, I postponed the changes. Go ahead. Thanks, C.
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. - */ - if (ppc64_radix_guest(cpu)) { - lpcr->default_value |= LPCR_UPRT | LPCR_GTSE; - } else { - lpcr->default_value &= ~(LPCR_UPRT | LPCR_GTSE); - } - } - /* Only enable Power-saving mode Exit Cause exceptions on the boot * CPU. The RTAS command start-cpu will enable them on secondaries. */
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 <david@gibson.dropbear.id.au> --- target/ppc/translate_init.c | 16 ---------------- 1 file changed, 16 deletions(-)