| Message ID | 20200928171539.788309-16-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/mips: Set CPU frequency | expand |
On Mon, 28 Sep 2020 19:15:38 +0200 Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Now than all QOM users provides the input clock, do not allow > using a CPU core without its input clock connected on system-mode > emulation. For user-mode, keep providing a fixed 200 MHz clock, > as it used by the RDHWR instruction (see commit cdfcad788394). > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > Cc: Igor Mammedov <imammedo@redhat.com> > > We need the qtest check for tests/qtest/machine-none-test.c > which instanciate a CPU with the none machine. Igor, is it > better to remove the MIPS targets from the test cpus_map[]? I don't get idea, could you rephrase/elaborate? > --- > target/mips/cpu.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/target/mips/cpu.c b/target/mips/cpu.c > index 2f75216c324..cc4ee75af30 100644 > --- a/target/mips/cpu.c > +++ b/target/mips/cpu.c > @@ -25,6 +25,7 @@ > #include "kvm_mips.h" > #include "qemu/module.h" > #include "sysemu/kvm.h" > +#include "sysemu/qtest.h" > #include "exec/exec-all.h" > #include "hw/qdev-clock.h" > #include "hw/qdev-properties.h" > @@ -159,11 +160,18 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp) > Error *local_err = NULL; > > if (!clock_get(cs->clock)) { > +#ifdef CONFIG_USER_ONLY > /* > * Initialize the frequency to 200MHz in case > * the clock remains unconnected. > */ > clock_set_hz(cs->clock, 200000000); > +#else > + if (!qtest_enabled()) { > + error_setg(errp, "CPU clock must be connected to a clock source"); > + return; > + } > +#endif > } > mips_cpu_clk_update(cs); >
On 9/29/20 3:01 PM, Igor Mammedov wrote: > On Mon, 28 Sep 2020 19:15:38 +0200 > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > >> Now than all QOM users provides the input clock, do not allow >> using a CPU core without its input clock connected on system-mode >> emulation. For user-mode, keep providing a fixed 200 MHz clock, >> as it used by the RDHWR instruction (see commit cdfcad788394). >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> Cc: Igor Mammedov <imammedo@redhat.com> >> >> We need the qtest check for tests/qtest/machine-none-test.c >> which instanciate a CPU with the none machine. Igor, is it >> better to remove the MIPS targets from the test cpus_map[]? > > I don't get idea, could you rephrase/elaborate? cpu_class_init sets: /* * Reason: CPUs still need special care by board code: wiring up * IRQs, adding reset handlers, halting non-first CPUs, ... */ dc->user_creatable = false; but the CPUs are created via another path in vl.c: current_machine->cpu_type = parse_cpu_option(cpu_option); The machine-none-test assumes CPU objects are user-creatable. With this patch I enforce MIPS CPU to have an input clock connected, so they are not user-creatable anymore. I tried this code ...: --- a/target/mips/cpu.c +++ b/target/mips/cpu.c @@ -229,7 +229,11 @@ static const TypeInfo mips_cpu_type_info = { static void mips_cpu_cpudef_class_init(ObjectClass *oc, void *data) { MIPSCPUClass *mcc = MIPS_CPU_CLASS(oc); + DeviceClass *dc = DEVICE_CLASS(oc); + mcc->cpu_def = data; + /* Reason: clock need to be wired up */ + dc->user_creatable = false; } ... but it is ignored, the CPU can still be created. Not sure how to handle this. > >> --- >> target/mips/cpu.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/target/mips/cpu.c b/target/mips/cpu.c >> index 2f75216c324..cc4ee75af30 100644 >> --- a/target/mips/cpu.c >> +++ b/target/mips/cpu.c >> @@ -25,6 +25,7 @@ >> #include "kvm_mips.h" >> #include "qemu/module.h" >> #include "sysemu/kvm.h" >> +#include "sysemu/qtest.h" >> #include "exec/exec-all.h" >> #include "hw/qdev-clock.h" >> #include "hw/qdev-properties.h" >> @@ -159,11 +160,18 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp) >> Error *local_err = NULL; >> >> if (!clock_get(cs->clock)) { >> +#ifdef CONFIG_USER_ONLY >> /* >> * Initialize the frequency to 200MHz in case >> * the clock remains unconnected. >> */ >> clock_set_hz(cs->clock, 200000000); >> +#else >> + if (!qtest_enabled()) { >> + error_setg(errp, "CPU clock must be connected to a clock source"); >> + return; >> + } >> +#endif >> } >> mips_cpu_clk_update(cs); >> >
+Thomas for qtest On 9/29/20 4:40 PM, Philippe Mathieu-Daudé wrote: > On 9/29/20 3:01 PM, Igor Mammedov wrote: >> On Mon, 28 Sep 2020 19:15:38 +0200 >> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >>> Now than all QOM users provides the input clock, do not allow >>> using a CPU core without its input clock connected on system-mode >>> emulation. For user-mode, keep providing a fixed 200 MHz clock, >>> as it used by the RDHWR instruction (see commit cdfcad788394). >>> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> --- >>> Cc: Igor Mammedov <imammedo@redhat.com> >>> >>> We need the qtest check for tests/qtest/machine-none-test.c >>> which instanciate a CPU with the none machine. Igor, is it >>> better to remove the MIPS targets from the test cpus_map[]? >> >> I don't get idea, could you rephrase/elaborate? > > cpu_class_init sets: > > /* > * Reason: CPUs still need special care by board code: wiring up > * IRQs, adding reset handlers, halting non-first CPUs, ... > */ > dc->user_creatable = false; > > but the CPUs are created via another path in vl.c: > > current_machine->cpu_type = parse_cpu_option(cpu_option); > > The machine-none-test assumes CPU objects are user-creatable. > > With this patch I enforce MIPS CPU to have an input clock > connected, so they are not user-creatable anymore. > > I tried this code ...: > > --- a/target/mips/cpu.c > +++ b/target/mips/cpu.c > @@ -229,7 +229,11 @@ static const TypeInfo mips_cpu_type_info = { > static void mips_cpu_cpudef_class_init(ObjectClass *oc, void *data) > { > MIPSCPUClass *mcc = MIPS_CPU_CLASS(oc); > + DeviceClass *dc = DEVICE_CLASS(oc); > + > mcc->cpu_def = data; > + /* Reason: clock need to be wired up */ > + dc->user_creatable = false; > } > > ... but it is ignored, the CPU can still be created. > > Not sure how to handle this. > >> >>> --- >>> target/mips/cpu.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/target/mips/cpu.c b/target/mips/cpu.c >>> index 2f75216c324..cc4ee75af30 100644 >>> --- a/target/mips/cpu.c >>> +++ b/target/mips/cpu.c >>> @@ -25,6 +25,7 @@ >>> #include "kvm_mips.h" >>> #include "qemu/module.h" >>> #include "sysemu/kvm.h" >>> +#include "sysemu/qtest.h" >>> #include "exec/exec-all.h" >>> #include "hw/qdev-clock.h" >>> #include "hw/qdev-properties.h" >>> @@ -159,11 +160,18 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp) >>> Error *local_err = NULL; >>> >>> if (!clock_get(cs->clock)) { >>> +#ifdef CONFIG_USER_ONLY >>> /* >>> * Initialize the frequency to 200MHz in case >>> * the clock remains unconnected. >>> */ >>> clock_set_hz(cs->clock, 200000000); >>> +#else >>> + if (!qtest_enabled()) { >>> + error_setg(errp, "CPU clock must be connected to a clock source"); >>> + return; >>> + } >>> +#endif >>> } >>> mips_cpu_clk_update(cs); >>> >> >
On Tue, 29 Sep 2020 16:40:24 +0200 Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > On 9/29/20 3:01 PM, Igor Mammedov wrote: > > On Mon, 28 Sep 2020 19:15:38 +0200 > > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > > >> Now than all QOM users provides the input clock, do not allow > >> using a CPU core without its input clock connected on system-mode > >> emulation. For user-mode, keep providing a fixed 200 MHz clock, > >> as it used by the RDHWR instruction (see commit cdfcad788394). > >> > >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > >> --- > >> Cc: Igor Mammedov <imammedo@redhat.com> > >> > >> We need the qtest check for tests/qtest/machine-none-test.c > >> which instanciate a CPU with the none machine. Igor, is it > >> better to remove the MIPS targets from the test cpus_map[]? it will help to pass test but, > > > > I don't get idea, could you rephrase/elaborate? > > cpu_class_init sets: > > /* > * Reason: CPUs still need special care by board code: wiring up > * IRQs, adding reset handlers, halting non-first CPUs, ... > */ > dc->user_creatable = false; > > but the CPUs are created via another path in vl.c: > > current_machine->cpu_type = parse_cpu_option(cpu_option); > > The machine-none-test assumes CPU objects are user-creatable. user_creatable -> able to create using -device, so test doesn't make this assumption. problem is that enforcing would make machine-none not usable for probing CPU features which is used by mgmt, so that would break. > > With this patch I enforce MIPS CPU to have an input clock > connected, so they are not user-creatable anymore. > > I tried this code ...: > > --- a/target/mips/cpu.c > +++ b/target/mips/cpu.c > @@ -229,7 +229,11 @@ static const TypeInfo mips_cpu_type_info = { > static void mips_cpu_cpudef_class_init(ObjectClass *oc, void *data) > { > MIPSCPUClass *mcc = MIPS_CPU_CLASS(oc); > + DeviceClass *dc = DEVICE_CLASS(oc); > + > mcc->cpu_def = data; > + /* Reason: clock need to be wired up */ > + dc->user_creatable = false; > } > > ... but it is ignored, the CPU can still be created. > > Not sure how to handle this. > > > > >> --- > >> target/mips/cpu.c | 8 ++++++++ > >> 1 file changed, 8 insertions(+) > >> > >> diff --git a/target/mips/cpu.c b/target/mips/cpu.c > >> index 2f75216c324..cc4ee75af30 100644 > >> --- a/target/mips/cpu.c > >> +++ b/target/mips/cpu.c > >> @@ -25,6 +25,7 @@ > >> #include "kvm_mips.h" > >> #include "qemu/module.h" > >> #include "sysemu/kvm.h" > >> +#include "sysemu/qtest.h" > >> #include "exec/exec-all.h" > >> #include "hw/qdev-clock.h" > >> #include "hw/qdev-properties.h" > >> @@ -159,11 +160,18 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp) > >> Error *local_err = NULL; > >> > >> if (!clock_get(cs->clock)) { > >> +#ifdef CONFIG_USER_ONLY > >> /* > >> * Initialize the frequency to 200MHz in case > >> * the clock remains unconnected. > >> */ > >> clock_set_hz(cs->clock, 200000000); > >> +#else > >> + if (!qtest_enabled()) { > >> + error_setg(errp, "CPU clock must be connected to a clock source"); > >> + return; > >> + } > >> +#endif > >> } > >> mips_cpu_clk_update(cs); > >> > > >
diff --git a/target/mips/cpu.c b/target/mips/cpu.c index 2f75216c324..cc4ee75af30 100644 --- a/target/mips/cpu.c +++ b/target/mips/cpu.c @@ -25,6 +25,7 @@ #include "kvm_mips.h" #include "qemu/module.h" #include "sysemu/kvm.h" +#include "sysemu/qtest.h" #include "exec/exec-all.h" #include "hw/qdev-clock.h" #include "hw/qdev-properties.h" @@ -159,11 +160,18 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp) Error *local_err = NULL; if (!clock_get(cs->clock)) { +#ifdef CONFIG_USER_ONLY /* * Initialize the frequency to 200MHz in case * the clock remains unconnected. */ clock_set_hz(cs->clock, 200000000); +#else + if (!qtest_enabled()) { + error_setg(errp, "CPU clock must be connected to a clock source"); + return; + } +#endif } mips_cpu_clk_update(cs);
Now than all QOM users provides the input clock, do not allow using a CPU core without its input clock connected on system-mode emulation. For user-mode, keep providing a fixed 200 MHz clock, as it used by the RDHWR instruction (see commit cdfcad788394). Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- Cc: Igor Mammedov <imammedo@redhat.com> We need the qtest check for tests/qtest/machine-none-test.c which instanciate a CPU with the none machine. Igor, is it better to remove the MIPS targets from the test cpus_map[]? --- target/mips/cpu.c | 8 ++++++++ 1 file changed, 8 insertions(+)