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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]

On Wed, Jul 08, 2020 at 10:38:29AM +0200, Philippe Mathieu-Daudé wrote:
> Hi Thiago,
> 
> On 7/8/20 1:28 AM, Thiago Jung Bauermann wrote:
> > 
> > Hello Eduardo,
> > 
> > Eduardo Habkost <ehabkost@redhat.com> writes:
> > 
> >> On Tue, Jul 07, 2020 at 05:43:33PM -0300, Thiago Jung Bauermann wrote:
> >>> PowerPC sPAPRs CPUs start in the halted state, but generic QEMU code
> >>> assumes that CPUs start in the non-halted state. spapr_reset_vcpu()
> >>> attempts to rectify this by setting CPUState::halted to 1. But that's too
> >>> late for hotplugged CPUs in a machine configured with 2 or mor threads per
> >>> core.
> >>>
> >>> By then, other parts of QEMU have already caused the vCPU to run in an
> >>> unitialized state a couple of times. For example, ppc_cpu_reset() calls
> >>> ppc_tlb_invalidate_all(), which ends up calling async_run_on_cpu(). This
> >>> kicks the new vCPU while it has CPUState::halted = 0, causing QEMU to issue
> >>> a KVM_RUN ioctl on the new vCPU before the guest is able to make the
> >>> start-cpu RTAS call to initialize its register state.
> >>>
> >>> This doesn't seem to cause visible issues for regular guests, but on a
> >>> secure guest running under the Ultravisor it does. The Ultravisor relies on
> >>> being able to snoop on the start-cpu RTAS call to map vCPUs to guests, and
> >>> this issue causes it to see a stray vCPU that doesn't belong to any guest.
> >>>
> >>> Fix by adding a starts_halted() method to the CPUState class, and making it
> >>> return 1 if the machine is an sPAPR guest.
> >>>
> >>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> >> [...]
> >>> +static uint32_t ppc_cpu_starts_halted(void)
> >>> +{
> >>> +    SpaprMachineState *spapr =
> >>> +        (SpaprMachineState *) object_dynamic_cast(qdev_get_machine(),
> >>> +                                                  TYPE_SPAPR_MACHINE);
> >>
> >> Wouldn't it be simpler to just implement this as a MachineClass
> >> boolean field?  e.g.:
> 
> 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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]