diff mbox series

[15/16] target/mips/cpu: Do not allow system-mode use without input clock

Message ID 20200928171539.788309-16-f4bug@amsat.org
State New
Headers show
Series hw/mips: Set CPU frequency | expand

Commit Message

Philippe Mathieu-Daudé Sept. 28, 2020, 5:15 p.m. UTC 
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(+)

Comments

Igor Mammedov Sept. 29, 2020, 1:01 p.m. UTC | #1 
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);
>
Philippe Mathieu-Daudé Sept. 29, 2020, 2:40 p.m. UTC | #2 
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);
>>  
>
Philippe Mathieu-Daudé Oct. 5, 2020, 7:39 a.m. UTC | #3 
+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);
>>>  
>>
>
Igor Mammedov Oct. 5, 2020, 4:25 p.m. UTC | #4 
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 mbox series

Patch

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);