| Message ID | 1433478358-993-6-git-send-email-bharata@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jun 05, 2015 at 09:55:55AM +0530, Bharata B Rao wrote: > Factor out bits of sPAPR specific CPU initialization code into > a separate routine so that it can be called from CPU hotplug > path too. > > While at this, use MSR_EP define instead of using 6 directly. Don't do this please. MSR[EP] is an obsolete flag from 601. The MSR[IP] flag that we're controlling here just happened to re-use the same bit position, so using the existing MSR_EP define is misleading. A symbolic name is good, but you should create a new one for MSR[IP] instead. Or you could just drop this change and do the cleanup of the hardcoded 6 some other time.
On Mon, 15 Jun 2015 16:59:08 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Fri, Jun 05, 2015 at 09:55:55AM +0530, Bharata B Rao wrote: > > Factor out bits of sPAPR specific CPU initialization code into > > a separate routine so that it can be called from CPU hotplug > > path too. > > > > While at this, use MSR_EP define instead of using 6 directly. > > Don't do this please. MSR[EP] is an obsolete flag from 601. The > MSR[IP] flag that we're controlling here just happened to re-use the > same bit position, so using the existing MSR_EP define is misleading. Actually, I had the same discussion with Bharata already some weeks ago: http://lists.gnu.org/archive/html/qemu-ppc/2015-05/msg00133.html > A symbolic name is good, but you should create a new one for MSR[IP] > instead. ... and I had to realize that IP = EP. IP likely stands for "interrupt prefix" (I guess), and EP simply means "exception prefix", so just two words for the same meaning. It's just the "on 601" comment in QEMU that is completely misleading. So IMHO it should be fine to keep the "MSR_EP" here (and maybe update the comment in cpu.h with a separate patch?). Thomas
On Mon, Jun 15, 2015 at 10:15:09AM +0200, Thomas Huth wrote: > On Mon, 15 Jun 2015 16:59:08 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Fri, Jun 05, 2015 at 09:55:55AM +0530, Bharata B Rao wrote: > > > Factor out bits of sPAPR specific CPU initialization code into > > > a separate routine so that it can be called from CPU hotplug > > > path too. > > > > > > While at this, use MSR_EP define instead of using 6 directly. > > > > Don't do this please. MSR[EP] is an obsolete flag from 601. The > > MSR[IP] flag that we're controlling here just happened to re-use the > > same bit position, so using the existing MSR_EP define is misleading. > > Actually, I had the same discussion with Bharata already some weeks ago: > > http://lists.gnu.org/archive/html/qemu-ppc/2015-05/msg00133.html > > > A symbolic name is good, but you should create a new one for MSR[IP] > > instead. > > ... and I had to realize that IP = EP. IP likely stands for "interrupt > prefix" (I guess), and EP simply means "exception prefix", so just two > words for the same meaning. It's just the "on 601" comment in QEMU that > is completely misleading. So IMHO it should be fine to keep the > "MSR_EP" here (and maybe update the comment in cpu.h with a separate > patch?). I don't entirely agree. Yes EP and IP have related functions - it's pretty common in ppc history that when an MSR bit is re-used it's for something similar (for example IS/IR). But MSR[IP] is still a different name from MSR[EP], and I don't know if the semantics are identical, though I'm sure they're similar.
On Tue, 16 Jun 2015 15:40:05 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Mon, Jun 15, 2015 at 10:15:09AM +0200, Thomas Huth wrote: > > On Mon, 15 Jun 2015 16:59:08 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > On Fri, Jun 05, 2015 at 09:55:55AM +0530, Bharata B Rao wrote: > > > > Factor out bits of sPAPR specific CPU initialization code into > > > > a separate routine so that it can be called from CPU hotplug > > > > path too. > > > > > > > > While at this, use MSR_EP define instead of using 6 directly. > > > > > > Don't do this please. MSR[EP] is an obsolete flag from 601. The > > > MSR[IP] flag that we're controlling here just happened to re-use the > > > same bit position, so using the existing MSR_EP define is misleading. > > > > Actually, I had the same discussion with Bharata already some weeks ago: > > > > http://lists.gnu.org/archive/html/qemu-ppc/2015-05/msg00133.html > > > > > A symbolic name is good, but you should create a new one for MSR[IP] > > > instead. > > > > ... and I had to realize that IP = EP. IP likely stands for "interrupt > > prefix" (I guess), and EP simply means "exception prefix", so just two > > words for the same meaning. It's just the "on 601" comment in QEMU that > > is completely misleading. So IMHO it should be fine to keep the > > "MSR_EP" here (and maybe update the comment in cpu.h with a separate > > patch?). > > I don't entirely agree. Yes EP and IP have related functions - it's > pretty common in ppc history that when an MSR bit is re-used it's for > something similar (for example IS/IR). But MSR[IP] is still a > different name from MSR[EP], and I don't know if the semantics are > identical, though I'm sure they're similar. I had a look at the 601 User's Manual, and it says: Bit Name Description 25 EP Exception prefix. The setting of this bit specifies whether an exception vector offset is prepended with Fs or 0s. Then, looking at the 603 User's Manual, it says: Bit Name Description 25 IP Exception prefix. The setting of this bit specifies whether an exception vector offset is prepended with Fs or 0s. So it's the very same bit, just the name has already been changed between the 601 and 603 already. So I think it's either fine to keep the MSR_EP in the patch (and change the comment for the define in a separate patch?), or to introduce another define, MSR_IP, with a comment that it is the same as MSR_EP, just with a different name. However, I still wonder whether this bit applies to the spapr code at all since it is not defined in the modern PowerISA spec anymore, as far as I can see. Thomas
On Tue, Jun 16, 2015 at 08:36:34AM +0200, Thomas Huth wrote: > On Tue, 16 Jun 2015 15:40:05 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Mon, Jun 15, 2015 at 10:15:09AM +0200, Thomas Huth wrote: > > > On Mon, 15 Jun 2015 16:59:08 +1000 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > On Fri, Jun 05, 2015 at 09:55:55AM +0530, Bharata B Rao wrote: > > > > > Factor out bits of sPAPR specific CPU initialization code into > > > > > a separate routine so that it can be called from CPU hotplug > > > > > path too. > > > > > > > > > > While at this, use MSR_EP define instead of using 6 directly. > > > > > > > > Don't do this please. MSR[EP] is an obsolete flag from 601. The > > > > MSR[IP] flag that we're controlling here just happened to re-use the > > > > same bit position, so using the existing MSR_EP define is misleading. > > > > > > Actually, I had the same discussion with Bharata already some weeks ago: > > > > > > http://lists.gnu.org/archive/html/qemu-ppc/2015-05/msg00133.html > > > > > > > A symbolic name is good, but you should create a new one for MSR[IP] > > > > instead. > > > > > > ... and I had to realize that IP = EP. IP likely stands for "interrupt > > > prefix" (I guess), and EP simply means "exception prefix", so just two > > > words for the same meaning. It's just the "on 601" comment in QEMU that > > > is completely misleading. So IMHO it should be fine to keep the > > > "MSR_EP" here (and maybe update the comment in cpu.h with a separate > > > patch?). > > > > I don't entirely agree. Yes EP and IP have related functions - it's > > pretty common in ppc history that when an MSR bit is re-used it's for > > something similar (for example IS/IR). But MSR[IP] is still a > > different name from MSR[EP], and I don't know if the semantics are > > identical, though I'm sure they're similar. > > I had a look at the 601 User's Manual, and it says: > > Bit Name Description > 25 EP Exception prefix. The setting of this bit specifies whether an > exception vector offset is prepended with Fs or 0s. > > Then, looking at the 603 User's Manual, it says: > > Bit Name Description > 25 IP Exception prefix. The setting of this bit specifies whether an > exception vector offset is prepended with Fs or 0s. > > So it's the very same bit, just the name has already been changed > between the 601 and 603 already. > > So I think it's either fine to keep the MSR_EP in the patch (and change > the comment for the define in a separate patch?), or to introduce > another define, MSR_IP, with a comment that it is the same as MSR_EP, > just with a different name. I'd still prefer to see the modern name in the #define. > However, I still wonder whether this bit applies to the spapr code at > all since it is not defined in the modern PowerISA spec anymore, as far > as I can see. Hrm.. yeah. I am wondering if it is not architected, but actually present as an implementation specific MSR bit on the modern CPUs. I see that Bharata has reposted leaving the literal 6 as is, which is probably a fair call until we find a consensus here.
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 65a86eb..a0ecfd1 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1408,6 +1408,34 @@ static void spapr_boot_set(void *opaque, const char *boot_device, machine->boot_order = g_strdup(boot_device); } +static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu) +{ + CPUPPCState *env = &cpu->env; + + /* Set time-base frequency to 512 MHz */ + cpu_ppc_tb_init(env, TIMEBASE_FREQ); + + /* PAPR always has exception vectors in RAM not ROM. To ensure this, + * MSR[IP] should never be set. + */ + env->msr_mask &= ~(1 << MSR_EP); + + /* Tell KVM that we're in PAPR mode */ + if (kvm_enabled()) { + kvmppc_set_papr(cpu); + } + + if (cpu->max_compat) { + if (ppc_set_compat(cpu, cpu->max_compat) < 0) { + exit(1); + } + } + + xics_cpu_setup(spapr->icp, cpu); + + qemu_register_reset(spapr_cpu_reset, cpu); +} + /* pSeries LPAR / sPAPR hardware init */ static void ppc_spapr_init(MachineState *machine) { @@ -1417,7 +1445,6 @@ static void ppc_spapr_init(MachineState *machine) const char *kernel_cmdline = machine->kernel_cmdline; const char *initrd_filename = machine->initrd_filename; PowerPCCPU *cpu; - CPUPPCState *env; PCIHostState *phb; int i; MemoryRegion *sysmem = get_system_memory(); @@ -1501,30 +1528,7 @@ static void ppc_spapr_init(MachineState *machine) fprintf(stderr, "Unable to find PowerPC CPU definition\n"); exit(1); } - env = &cpu->env; - - /* Set time-base frequency to 512 MHz */ - cpu_ppc_tb_init(env, TIMEBASE_FREQ); - - /* PAPR always has exception vectors in RAM not ROM. To ensure this, - * MSR[IP] should never be set. - */ - env->msr_mask &= ~(1 << 6); - - /* Tell KVM that we're in PAPR mode */ - if (kvm_enabled()) { - kvmppc_set_papr(cpu); - } - - if (cpu->max_compat) { - if (ppc_set_compat(cpu, cpu->max_compat) < 0) { - exit(1); - } - } - - xics_cpu_setup(spapr->icp, cpu); - - qemu_register_reset(spapr_cpu_reset, cpu); + spapr_cpu_init(spapr, cpu); } if (kvm_enabled()) {
Factor out bits of sPAPR specific CPU initialization code into a separate routine so that it can be called from CPU hotplug path too. While at this, use MSR_EP define instead of using 6 directly. Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> --- hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++------------------------- 1 file changed, 29 insertions(+), 25 deletions(-)