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 <ehabk...@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 <bauer...@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
signature.asc
Description: PGP signature
