Re: [PATCH] cpu: Add starts_halted() method

[Philippe Mathieu-Daudé]

On 7/9/20 12:55 PM, Greg Kurz wrote:
> On Thu, 9 Jul 2020 12:18:06 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> On 7/9/20 11:54 AM, Greg Kurz wrote:
>>> On Thu, 9 Jul 2020 07:11:09 +0200
>>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>> On 7/8/20 11:39 PM, Eduardo Habkost wrote:
>>>>> On Wed, Jul 08, 2020 at 06:45:57PM +0200, Philippe Mathieu-Daudé wrote:
>>>>>> On 7/8/20 5:25 PM, Eduardo Habkost wrote:
>>>>>>> On Wed, Jul 08, 2020 at 02:14:03PM +0100, Peter Maydell wrote:
>>>>>>>> On Wed, 8 Jul 2020 at 12:12, David Gibson 
>>>>>>>> <david@gibson.dropbear.id.au> wrote:
>>>>>>>>> On Wed, Jul 08, 2020 at 10:38:29AM +0200, Philippe Mathieu-Daudé 
>>>>>>>>> wrote:
>>>>>>>>>> Class boolean field certainly sounds better, but I am not sure this
>>>>>>>>>> is a property of the machine. Rather the arch? So move the field
>>>>>>>>>> to CPUClass? Maybe not, let's discuss :)
>>>>>>>>>
>>>>>>>>> It is absolutely a property of the machine.  e.g. I don't think we
>>>>>>>>> want this for powernv.  pseries is a bit of a special case since it is
>>>>>>>>> explicitly a paravirt platform.  But even for emulated hardware, the
>>>>>>>>> board can absolutely strap things so that cpus do or don't start
>>>>>>>>> immediately.
>>>>>>>>
>>>>>>>> It's a property of the individual CPU, I think. One common setup
>>>>>>>> for Arm systems is that the primary CPU starts powered up but
>>>>>>>> the secondaries all start powered down.
>>>>>>>
>>>>>>> Both statements can be true.  It can be a property of the
>>>>>>> individual CPU (although I'm not convinced it has to), but it
>>>>>>> still needs to be controlled by the machine.
>>>>>>
>>>>>> From what said Peter, I understand this is a property of the
>>>>>> chipset. Chipsets are modelled unevenly.
>>>>>>
>>>>>> IIUC QEMU started with single-core CPUs.
>>>>>> CPUState had same meaning for 'core' or 'cpu', 1-1 mapping.
>>>>>>
>>>>>> Then multicore CPUs could be easily modelled using multiple
>>>>>> single-core CPUs, usually created in the machine code.
>>>>>>
>>>>>> Then we moved to SoC models, creating the cores in the SoC.
>>>>>> Some SoCs have array of cores, eventually heterogeneous
>>>>>> (see the ZynqMP). We have containers of CPUState.
>>>>>>
>>>>>> On an ARM-based SoC, you might have the first core started
>>>>>> (as said Peter) or not.
>>>>>>
>>>>>> BCM2836 / BCM2837 and ZynqMP start will all ARM cores off.
>>>>>> On the BCM chipsets, a DSP core will boot the ARM cores.
>>>>>> On the ZynqMP, a MicroBlaze core boots them.
>>>>>> As QEMU doesn't models heterogeneous architectures, we start
>>>>>> modelling after the unmodelled cores booted us, so either one
>>>>>> or all cores on.
>>>>>>
>>>>>> In this case, we narrowed down the 'start-powered-off' field
>>>>>> to the SoC, which happens to be how ARM SoCs are modelled.
>>>>>
>>>>> I was not aware of the start-powered-off property.  If we make it
>>>>> generic, we can just let spapr use it.
>>>>>
>>>>>>
>>>>>>
>>>>>> Chipsets providing a JTAG interface can have a SRST signal,
>>>>>> the "system reset". When a JTAG probe is attached, it can
>>>>>> keeps the whole chipset in a reset state. This is equivalent
>>>>>> to QEMU '-S' mode (single step mode).
>>>>>>
>>>>>>
>>>>>> I don't know about pseries hardware, but if this is 'explicit
>>>>>> to paravirt platform', then I expect this to be the same with
>>>>>> other accelerators/architectures.
>>>>>>
>>>>>> If paravirtualized -> cores start off by default. Let the
>>>>>> hypervisor start them. So still a property of the CPUState
>>>>>> depending on the accelerator used?
>>>>>
>>>>> I don't understand this part.  Why would this depend on the
>>>>> accelerator?
>>>>
>>>> Because starting a virtualized machine with all cores powered-off
>>>> with TCG accelerator should at least emit a warning? Or change
>>>> the behavior and start them powered-on? This is machine-specific
>>>> although.
>>>>
>>>
>>> FYI, PAPR requires all vCPUs to be "stopped" by default. It is up to the
>>> guest to start them explicitly through an RTAS call. The hypervisor is
>>> only responsible to start a single vCPU (see spapr_cpu_set_entry_state()
>>> called from spapr_machine_reset()) to be able to boot the guest.
>>>
>>> So I'm not sure to see how that would depend on the accelerator...
>>
>> $ qemu-system-ppc64 -M pseries-5.0,accel=tcg -d in_asm
>> qemu-system-ppc64: warning: TCG doesn't support requested feature,
>> cap-cfpc=workaround
>> qemu-system-ppc64: warning: TCG doesn't support requested feature,
>> cap-sbbc=workaround
>> qemu-system-ppc64: warning: TCG doesn't support requested feature,
>> cap-ibs=workaround
>> qemu-system-ppc64: warning: TCG doesn't support requested feature,
>> cap-ccf-assist=on
>> ----------------
>> IN:
>> 0x00000100:  48003f00  b        0x4000
>>
>> ----------------
>> IN:
>> 0x00004000:  7c7f1b78  mr       r31, r3
>> 0x00004004:  7d6000a6  mfmsr    r11
>> 0x00004008:  3980a000  li       r12, 0xa000
>> 0x0000400c:  798c83c6  sldi     r12, r12, 0x30
>> 0x00004010:  7d6b6378  or       r11, r11, r12
>> 0x00004014:  7d600164  mtmsrd   r11
>> ...
>>
>> The vCPU doesn't seem stopped to me...
>>
>> Am I missing something?
>>
> 
> Yeah this is the boot vCPU which is required to be started
> by the platform as explained above, but if you had more
> vCPUs the other ones would be stopped until the guest OS
> asks us to start them.

Ah OK, so we are good :)

The machine simply has to set the 'start-powered-off' flag on
all vCPUS except the 1st one.