| Message ID | 151618083506.20461.14178623580944316317.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: fix CPU device tree nodes | expand |
On Wed, Jan 17, 2018 at 10:20:35AM +0100, Greg Kurz wrote: > When QEMU is started, all cold-plugged CPUs are reset twice: first > during initialization and then during machine reset. This is sub- > optimal. > > The first reset is only needed for hot-plugged CPUs because the CPU > hotplug code doesn't reset them. This patch adds the necessary code > to reset hot-plugged CPUs on the CPU core hotplug path, and removes > the now useless initial CPU reset. > > We just need to mark the newly created CPU as halted to prevent it > to run until it is put online later. > > Full CPU reset is now explicitely triggered from the machine code > only, either during system reset or during CPU hotplug. > > Signed-off-by: Greg Kurz <groug@kaod.org> Hrm, this looks ok in outline, but makes me nervous in a couple of ways. > --- > hw/ppc/spapr.c | 8 ++++++++ > hw/ppc/spapr_cpu_core.c | 8 ++++++-- > include/hw/ppc/spapr_cpu_core.h | 2 ++ > 3 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index bca838fce638..a2ff401f738a 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3336,6 +3336,14 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > void *fdt; > int fdt_offset; > > + if (hotplugged) { First, I'm always wary of using the hotplugged parameter, because what qemu means by it often doesn't line up with what PAPR means by it. > + int i; > + > + for (i = 0; i < cc->nr_threads; i++) { > + spapr_cpu_reset(core->threads[i]); > + } > + } > + > fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr); > > spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err); > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > index ac19b2e0b72c..268be7784efb 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -22,7 +22,7 @@ > #include "sysemu/hw_accel.h" > #include "qemu/error-report.h" > > -static void spapr_cpu_reset(void *opaque) > +void spapr_cpu_reset(void *opaque) > { > PowerPCCPU *cpu = opaque; > CPUState *cs = CPU(cpu); > @@ -63,7 +63,11 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, > cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr)); > > qemu_register_reset(spapr_cpu_reset, cpu); > - spapr_cpu_reset(cpu); > + > + /* CPU must not execute anything until explicitely started otherwise the > + * guest will crash. > + */ > + CPU(cpu)->halted = 1; And poking specifics in a CPU that hasn't already been set to a known state by a reset also worries me. > } > > /* > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h > index 1129f344aa0c..763a2168461e 100644 > --- a/include/hw/ppc/spapr_cpu_core.h > +++ b/include/hw/ppc/spapr_cpu_core.h > @@ -38,4 +38,6 @@ typedef struct sPAPRCPUCoreClass { > } sPAPRCPUCoreClass; > > const char *spapr_get_cpu_core_type(const char *cpu_type); > +void spapr_cpu_reset(void *opaque); > + > #endif >
On Thu, 18 Jan 2018 14:43:40 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > On Wed, Jan 17, 2018 at 10:20:35AM +0100, Greg Kurz wrote: > > When QEMU is started, all cold-plugged CPUs are reset twice: first > > during initialization and then during machine reset. This is sub- > > optimal. > > > > The first reset is only needed for hot-plugged CPUs because the CPU > > hotplug code doesn't reset them. This patch adds the necessary code > > to reset hot-plugged CPUs on the CPU core hotplug path, and removes > > the now useless initial CPU reset. > > > > We just need to mark the newly created CPU as halted to prevent it > > to run until it is put online later. > > > > Full CPU reset is now explicitely triggered from the machine code > > only, either during system reset or during CPU hotplug. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > Hrm, this looks ok in outline, but makes me nervous in a couple of > ways. > > > > --- > > hw/ppc/spapr.c | 8 ++++++++ > > hw/ppc/spapr_cpu_core.c | 8 ++++++-- > > include/hw/ppc/spapr_cpu_core.h | 2 ++ > > 3 files changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index bca838fce638..a2ff401f738a 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -3336,6 +3336,14 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > void *fdt; > > int fdt_offset; > > > > + if (hotplugged) { > > First, I'm always wary of using the hotplugged parameter, because what > qemu means by it often doesn't line up with what PAPR means by it. > Hmmm... you're right, hotplugged in QDEV simply means that the device was not created during initial machine startup. ie, any device added with QMP/HMP is always hotplugged. To cope with the DRC state management, commit 94fd9cbaa3190 added the fact that QEMU mustn't be waiting for an incoming migration as well. ie, if QEMU is started with -incoming and CPUs are added before migration starts, like libvirt does, this code wouldn't reset the CPUs... I guess we should check qdev->hotplugged instead. Makes sense ? > > + int i; > > + > > + for (i = 0; i < cc->nr_threads; i++) { > > + spapr_cpu_reset(core->threads[i]); > > + } > > + } > > + > > fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr); > > > > spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err); > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > index ac19b2e0b72c..268be7784efb 100644 > > --- a/hw/ppc/spapr_cpu_core.c > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -22,7 +22,7 @@ > > #include "sysemu/hw_accel.h" > > #include "qemu/error-report.h" > > > > -static void spapr_cpu_reset(void *opaque) > > +void spapr_cpu_reset(void *opaque) > > { > > PowerPCCPU *cpu = opaque; > > CPUState *cs = CPU(cpu); > > @@ -63,7 +63,11 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, > > cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr)); > > > > qemu_register_reset(spapr_cpu_reset, cpu); > > - spapr_cpu_reset(cpu); > > + > > + /* CPU must not execute anything until explicitely started otherwise the > > + * guest will crash. > > + */ > > + CPU(cpu)->halted = 1; > > And poking specifics in a CPU that hasn't already been set to a known > state by a reset also worries me. > IIUC the halted flag doesn't really depend any CPU state. It is only a way to prevent the CPU from executing, which is needed if the CPU wasn't set to a known state. FWIW I've seen other places where it is set before resetting the CPU (eg, s390_cpu_initfn() or cpu_devinit() for Sun4m). I was thinking of another solution: create a DeviceClass reset function that would call spapr_cpu_reset() for all CPUs of a core, and register it in spapr_cpu_core_class_init(). It would be called by QDEV during realization of hot-plugged cores only. Unfortunately, this also happens after the call to spapr_core_plug() (see device_set_realized()). > > } > > > > /* > > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h > > index 1129f344aa0c..763a2168461e 100644 > > --- a/include/hw/ppc/spapr_cpu_core.h > > +++ b/include/hw/ppc/spapr_cpu_core.h > > @@ -38,4 +38,6 @@ typedef struct sPAPRCPUCoreClass { > > } sPAPRCPUCoreClass; > > > > const char *spapr_get_cpu_core_type(const char *cpu_type); > > +void spapr_cpu_reset(void *opaque); > > + > > #endif > > >
On Thu, 18 Jan 2018 16:39:30 +0100 Greg Kurz <groug@kaod.org> wrote: > On Thu, 18 Jan 2018 14:43:40 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Wed, Jan 17, 2018 at 10:20:35AM +0100, Greg Kurz wrote: > > > When QEMU is started, all cold-plugged CPUs are reset twice: first > > > during initialization and then during machine reset. This is sub- > > > optimal. > > > > > > The first reset is only needed for hot-plugged CPUs because the CPU > > > hotplug code doesn't reset them. This patch adds the necessary code > > > to reset hot-plugged CPUs on the CPU core hotplug path, and removes > > > the now useless initial CPU reset. > > > > > > We just need to mark the newly created CPU as halted to prevent it > > > to run until it is put online later. > > > > > > Full CPU reset is now explicitely triggered from the machine code > > > only, either during system reset or during CPU hotplug. > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > Hrm, this looks ok in outline, but makes me nervous in a couple of > > ways. > > > > > > > --- > > > hw/ppc/spapr.c | 8 ++++++++ > > > hw/ppc/spapr_cpu_core.c | 8 ++++++-- > > > include/hw/ppc/spapr_cpu_core.h | 2 ++ > > > 3 files changed, 16 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index bca838fce638..a2ff401f738a 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -3336,6 +3336,14 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > > void *fdt; > > > int fdt_offset; > > > > > > + if (hotplugged) { > > > > First, I'm always wary of using the hotplugged parameter, because what > > qemu means by it often doesn't line up with what PAPR means by it. > > > > Hmmm... you're right, hotplugged in QDEV simply means that the device > was not created during initial machine startup. > > ie, any device added with QMP/HMP is always hotplugged. > > To cope with the DRC state management, commit 94fd9cbaa3190 added the > fact that QEMU mustn't be waiting for an incoming migration as well. > > ie, if QEMU is started with -incoming and CPUs are added before migration > starts, like libvirt does, this code wouldn't reset the CPUs... > > I guess we should check qdev->hotplugged instead. Makes sense ? > Actually, we could even just do this in spapr_cpu_init(): - spapr_cpu_reset(cpu); + if (DEVICE(cpu)->hotplugged) { + spapr_cpu_reset(cpu); + } > > > + int i; > > > + > > > + for (i = 0; i < cc->nr_threads; i++) { > > > + spapr_cpu_reset(core->threads[i]); > > > + } > > > + } > > > + > > > fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr); > > > > > > spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err); > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > > index ac19b2e0b72c..268be7784efb 100644 > > > --- a/hw/ppc/spapr_cpu_core.c > > > +++ b/hw/ppc/spapr_cpu_core.c > > > @@ -22,7 +22,7 @@ > > > #include "sysemu/hw_accel.h" > > > #include "qemu/error-report.h" > > > > > > -static void spapr_cpu_reset(void *opaque) > > > +void spapr_cpu_reset(void *opaque) > > > { > > > PowerPCCPU *cpu = opaque; > > > CPUState *cs = CPU(cpu); > > > @@ -63,7 +63,11 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, > > > cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr)); > > > > > > qemu_register_reset(spapr_cpu_reset, cpu); > > > - spapr_cpu_reset(cpu); > > > + > > > + /* CPU must not execute anything until explicitely started otherwise the > > > + * guest will crash. > > > + */ > > > + CPU(cpu)->halted = 1; > > > > And poking specifics in a CPU that hasn't already been set to a known > > state by a reset also worries me. > > > > IIUC the halted flag doesn't really depend any CPU state. It is > only a way to prevent the CPU from executing, which is needed > if the CPU wasn't set to a known state. > > FWIW I've seen other places where it is set before resetting the > CPU (eg, s390_cpu_initfn() or cpu_devinit() for Sun4m). > > I was thinking of another solution: create a DeviceClass reset function > that would call spapr_cpu_reset() for all CPUs of a core, and register > it in spapr_cpu_core_class_init(). It would be called by QDEV during > realization of hot-plugged cores only. Unfortunately, this also happens > after the call to spapr_core_plug() (see device_set_realized()). > > > > } > > > > > > /* > > > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h > > > index 1129f344aa0c..763a2168461e 100644 > > > --- a/include/hw/ppc/spapr_cpu_core.h > > > +++ b/include/hw/ppc/spapr_cpu_core.h > > > @@ -38,4 +38,6 @@ typedef struct sPAPRCPUCoreClass { > > > } sPAPRCPUCoreClass; > > > > > > const char *spapr_get_cpu_core_type(const char *cpu_type); > > > +void spapr_cpu_reset(void *opaque); > > > + > > > #endif > > > > > >
On Thu, Jan 18, 2018 at 04:39:30PM +0100, Greg Kurz wrote: > On Thu, 18 Jan 2018 14:43:40 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Wed, Jan 17, 2018 at 10:20:35AM +0100, Greg Kurz wrote: > > > When QEMU is started, all cold-plugged CPUs are reset twice: first > > > during initialization and then during machine reset. This is sub- > > > optimal. > > > > > > The first reset is only needed for hot-plugged CPUs because the CPU > > > hotplug code doesn't reset them. This patch adds the necessary code > > > to reset hot-plugged CPUs on the CPU core hotplug path, and removes > > > the now useless initial CPU reset. > > > > > > We just need to mark the newly created CPU as halted to prevent it > > > to run until it is put online later. > > > > > > Full CPU reset is now explicitely triggered from the machine code > > > only, either during system reset or during CPU hotplug. > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > Hrm, this looks ok in outline, but makes me nervous in a couple of > > ways. > > > > > > > --- > > > hw/ppc/spapr.c | 8 ++++++++ > > > hw/ppc/spapr_cpu_core.c | 8 ++++++-- > > > include/hw/ppc/spapr_cpu_core.h | 2 ++ > > > 3 files changed, 16 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index bca838fce638..a2ff401f738a 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -3336,6 +3336,14 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > > void *fdt; > > > int fdt_offset; > > > > > > + if (hotplugged) { > > > > First, I'm always wary of using the hotplugged parameter, because what > > qemu means by it often doesn't line up with what PAPR means by it. > > > > Hmmm... you're right, hotplugged in QDEV simply means that the device > was not created during initial machine startup. > > ie, any device added with QMP/HMP is always hotplugged. > > To cope with the DRC state management, commit 94fd9cbaa3190 added the > fact that QEMU mustn't be waiting for an incoming migration as well. > > ie, if QEMU is started with -incoming and CPUs are added before migration > starts, like libvirt does, this code wouldn't reset the CPUs... > > I guess we should check qdev->hotplugged instead. Makes sense ? Actually I don't think that dev->hotplugged is any better in terms of meaning what you want it to. > > > + int i; > > > + > > > + for (i = 0; i < cc->nr_threads; i++) { > > > + spapr_cpu_reset(core->threads[i]); > > > + } > > > + } > > > + > > > fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr); > > > > > > spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err); > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > > index ac19b2e0b72c..268be7784efb 100644 > > > --- a/hw/ppc/spapr_cpu_core.c > > > +++ b/hw/ppc/spapr_cpu_core.c > > > @@ -22,7 +22,7 @@ > > > #include "sysemu/hw_accel.h" > > > #include "qemu/error-report.h" > > > > > > -static void spapr_cpu_reset(void *opaque) > > > +void spapr_cpu_reset(void *opaque) > > > { > > > PowerPCCPU *cpu = opaque; > > > CPUState *cs = CPU(cpu); > > > @@ -63,7 +63,11 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, > > > cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr)); > > > > > > qemu_register_reset(spapr_cpu_reset, cpu); > > > - spapr_cpu_reset(cpu); > > > + > > > + /* CPU must not execute anything until explicitely started otherwise the > > > + * guest will crash. > > > + */ > > > + CPU(cpu)->halted = 1; > > > > And poking specifics in a CPU that hasn't already been set to a known > > state by a reset also worries me. > > > > IIUC the halted flag doesn't really depend any CPU state. It is > only a way to prevent the CPU from executing, which is needed > if the CPU wasn't set to a known state. > > FWIW I've seen other places where it is set before resetting the > CPU (eg, s390_cpu_initfn() or cpu_devinit() for Sun4m). Its effects do, though - e.g. the recent patches to explicitly disable the decrementer on stopped vcpus, because halted alone wasn't enough to keep them quiescent. > I was thinking of another solution: create a DeviceClass reset function > that would call spapr_cpu_reset() for all CPUs of a core, and register > it in spapr_cpu_core_class_init(). It would be called by QDEV during > realization of hot-plugged cores only. Unfortunately, this also happens > after the call to spapr_core_plug() (see device_set_realized()). Well, feel free to come up with something. AIUI there's no urgency about this, though - I'm not too worried by extraneous resets, as long as we have enough to be correct. > > > > } > > > > > > /* > > > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h > > > index 1129f344aa0c..763a2168461e 100644 > > > --- a/include/hw/ppc/spapr_cpu_core.h > > > +++ b/include/hw/ppc/spapr_cpu_core.h > > > @@ -38,4 +38,6 @@ typedef struct sPAPRCPUCoreClass { > > > } sPAPRCPUCoreClass; > > > > > > const char *spapr_get_cpu_core_type(const char *cpu_type); > > > +void spapr_cpu_reset(void *opaque); > > > + > > > #endif > > > > > >
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index bca838fce638..a2ff401f738a 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3336,6 +3336,14 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, void *fdt; int fdt_offset; + if (hotplugged) { + int i; + + for (i = 0; i < cc->nr_threads; i++) { + spapr_cpu_reset(core->threads[i]); + } + } + fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr); spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err); diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c index ac19b2e0b72c..268be7784efb 100644 --- a/hw/ppc/spapr_cpu_core.c +++ b/hw/ppc/spapr_cpu_core.c @@ -22,7 +22,7 @@ #include "sysemu/hw_accel.h" #include "qemu/error-report.h" -static void spapr_cpu_reset(void *opaque) +void spapr_cpu_reset(void *opaque) { PowerPCCPU *cpu = opaque; CPUState *cs = CPU(cpu); @@ -63,7 +63,11 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr)); qemu_register_reset(spapr_cpu_reset, cpu); - spapr_cpu_reset(cpu); + + /* CPU must not execute anything until explicitely started otherwise the + * guest will crash. + */ + CPU(cpu)->halted = 1; } /* diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h index 1129f344aa0c..763a2168461e 100644 --- a/include/hw/ppc/spapr_cpu_core.h +++ b/include/hw/ppc/spapr_cpu_core.h @@ -38,4 +38,6 @@ typedef struct sPAPRCPUCoreClass { } sPAPRCPUCoreClass; const char *spapr_get_cpu_core_type(const char *cpu_type); +void spapr_cpu_reset(void *opaque); + #endif
When QEMU is started, all cold-plugged CPUs are reset twice: first during initialization and then during machine reset. This is sub- optimal. The first reset is only needed for hot-plugged CPUs because the CPU hotplug code doesn't reset them. This patch adds the necessary code to reset hot-plugged CPUs on the CPU core hotplug path, and removes the now useless initial CPU reset. We just need to mark the newly created CPU as halted to prevent it to run until it is put online later. Full CPU reset is now explicitely triggered from the machine code only, either during system reset or during CPU hotplug. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/spapr.c | 8 ++++++++ hw/ppc/spapr_cpu_core.c | 8 ++++++-- include/hw/ppc/spapr_cpu_core.h | 2 ++ 3 files changed, 16 insertions(+), 2 deletions(-)