From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52261) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WumLd-0007Yh-A2 for qemu-devel@nongnu.org; Wed, 11 Jun 2014 13:30:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WumLX-0004te-BH for qemu-devel@nongnu.org; Wed, 11 Jun 2014 13:30:05 -0400 Sender: Paolo Bonzini Message-ID: <53989212.6060603@redhat.com> Date: Wed, 11 Jun 2014 19:29:54 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1402506183-29736-1-git-send-email-aik@ozlabs.ru> <1402506183-29736-3-git-send-email-aik@ozlabs.ru> In-Reply-To: <1402506183-29736-3-git-send-email-aik@ozlabs.ru> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 2/4] target-s390x: Migrate to new NMI interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy , qemu-devel@nongnu.org Cc: Peter Maydell , Alex Bligh , Markus Armbruster , Luiz Capitulino , Alexander Graf , qemu-ppc@nongnu.org, Stefan Hajnoczi , Cornelia Huck , Richard Henderson Il 11/06/2014 19:03, Alexey Kardashevskiy ha scritto: > This implements an NMI interface for s390 machine. > > This removes #ifdef s390 branch in qmp_inject_nmi so new s390's > nmi_monitor_handler() callback is going to be used for NMI. > > Since nmi_monitor_handler()-calling code is platform independent, > CPUState::cpu_index is used instead of S390CPU::env.cpu_num. > There should not be any change in behaviour as both @cpu_index and > @cpu_num are global CPU numbers. > > Also, s390_cpu_restart() takes care of preforming operations in > the specific CPU thread so no extra measure is required here either. > > Since the only error s390_cpu_restart() can return is ENOSYS, convert > it to QERR_UNSUPPORTED. > > Signed-off-by: Alexey Kardashevskiy > --- > Changes: > v6: > * supported NMI interface > > v5: > * added ENOSYS -> QERR_UNSUPPORTED, qapi/qmp/qerror.h was added for this > > v4: > * s/\/nmi_monitor_handler/ > > v3: > * now contains both old code removal and new code insertion, easier to > track changes > > --- > Is there any good reason to have @cpu_num in addition to @cpu_index? > Just asking :) > --- > cpus.c | 14 -------------- > hw/s390x/s390-virtio.c | 31 +++++++++++++++++++++++++++++++ > target-s390x/cpu.c | 1 + > 3 files changed, 32 insertions(+), 14 deletions(-) > > diff --git a/cpus.c b/cpus.c > index da1f6a9..f3756b4 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -1481,20 +1481,6 @@ void qmp_inject_nmi(Error **errp) > apic_deliver_nmi(cpu->apic_state); > } > } > -#elif defined(TARGET_S390X) > - CPUState *cs; > - S390CPU *cpu; > - > - CPU_FOREACH(cs) { > - cpu = S390_CPU(cs); > - if (cpu->env.cpu_num == monitor_get_cpu_index()) { > - if (s390_cpu_restart(S390_CPU(cs)) == -1) { > - error_set(errp, QERR_UNSUPPORTED); > - return; > - } > - break; > - } > - } > #else > nmi(monitor_get_cpu_index(), errp); > #endif > diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c > index 93c7ace..9c5b7b2 100644 > --- a/hw/s390x/s390-virtio.c > +++ b/hw/s390x/s390-virtio.c > @@ -38,6 +38,7 @@ > #include "hw/s390x/sclp.h" > #include "hw/s390x/s390_flic.h" > #include "hw/s390x/s390-virtio.h" > +#include "hw/nmi.h" > > //#define DEBUG_S390 > > @@ -51,6 +52,7 @@ > > #define MAX_BLK_DEVS 10 > #define ZIPL_FILENAME "s390-zipl.rom" > +#define TYPE_NMI_S390 "s390_nmi" > > static VirtIOS390Bus *s390_bus; > static S390CPU **ipi_states; > @@ -277,6 +279,9 @@ static void s390_init(MachineState *machine) > > /* Create VirtIO network adapters */ > s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390"); > + > + object_property_add_child(OBJECT(machine), "nmi", > + object_new(TYPE_NMI_S390), NULL); > } > > static QEMUMachine s390_machine = { > @@ -295,8 +300,34 @@ static QEMUMachine s390_machine = { > .is_default = 1, > }; > > +static void s390_nmi(NMI *n, int cpu_index, Error **errp) > +{ > + CPUState *cs = qemu_get_cpu(cpu_index); > + > + if (s390_cpu_restart(S390_CPU(cs))) { > + error_set(errp, QERR_UNSUPPORTED); > + } > +} > + > +static void s390_nmi_class_init(ObjectClass *oc, void *data) > +{ > + NMIClass *nc = NMI_CLASS(oc); > + nc->nmi_monitor_handler = s390_nmi; > +} > + > +static const TypeInfo s390_nmi_info = { > + .name = TYPE_NMI_S390, > + .parent = TYPE_OBJECT, > + .class_init = s390_nmi_class_init, > + .interfaces = (InterfaceInfo[]) { > + { TYPE_NMI }, > + { } > + }, > +}; > + > static void s390_machine_init(void) > { > + type_register_static(&s390_nmi_info); > qemu_register_machine(&s390_machine); I would use the /machine object itself, not an extra object. Otherwise this looks good. Paolo > } > > diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c > index c3082b7..15142ff 100644 > --- a/target-s390x/cpu.c > +++ b/target-s390x/cpu.c > @@ -27,6 +27,7 @@ > #include "qemu-common.h" > #include "qemu/timer.h" > #include "hw/hw.h" > +#include "qapi/qmp/qerror.h" > #ifndef CONFIG_USER_ONLY > #include "sysemu/arch_init.h" > #endif >