Re: [PATCH 15/16] target/mips/cpu: Do not allow system-mode use without input clock - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: "Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	"Igor Mammedov" <imammedo@redhat.com>,
	"Thomas Huth" <thuth@redhat.com>
Cc: "Damien Hedde" <damien.hedde@greensocs.com>,
	"Huacai Chen" <zltjiangshi@gmail.com>,
	"Aleksandar Rikalo" <aleksandar.rikalo@syrmia.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	"Paul Burton" <paulburton@kernel.org>,
	"Edgar E . Iglesias" <edgar.iglesias@gmail.com>,
	qemu-devel@nongnu.org,
	"Wainer dos Santos Moschetta" <wainersm@redhat.com>,
	"Aleksandar Markovic" <aleksandar.qemu.devel@gmail.com>,
	"Hervé Poussineau" <hpoussin@reactos.org>,
	"Jiaxun Yang" <jiaxun.yang@flygoat.com>,
	"Cleber Rosa" <crosa@redhat.com>,
	"Huacai Chen" <chenhc@lemote.com>,
	"Aurelien Jarno" <aurelien@aurel32.net>,
	"Richard Henderson" <rth@twiddle.net>
Subject: Re: [PATCH 15/16] target/mips/cpu: Do not allow system-mode use without input clock
Date: Mon, 5 Oct 2020 09:39:27 +0200	[thread overview]
Message-ID: <2e1ef61c-688d-0a1d-ae0a-1cc11d555cd4@amsat.org> (raw)
In-Reply-To: <35db3210-8165-6fe8-c835-22970fb31c93@amsat.org>

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