From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57892) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WusZp-00059i-Rl for qemu-devel@nongnu.org; Wed, 11 Jun 2014 20:09:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WusZg-0001Ud-Ri for qemu-devel@nongnu.org; Wed, 11 Jun 2014 20:09:09 -0400 Received: from mail-pa0-f51.google.com ([209.85.220.51]:43467) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WusZg-0001US-K1 for qemu-devel@nongnu.org; Wed, 11 Jun 2014 20:09:00 -0400 Received: by mail-pa0-f51.google.com with SMTP id ey11so349784pad.38 for ; Wed, 11 Jun 2014 17:08:59 -0700 (PDT) Message-ID: <5398EF94.5050407@ozlabs.ru> Date: Thu, 12 Jun 2014 10:08:52 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1402506183-29736-1-git-send-email-aik@ozlabs.ru> <1402506183-29736-3-git-send-email-aik@ozlabs.ru> <53989212.6060603@redhat.com> In-Reply-To: <53989212.6060603@redhat.com> Content-Type: text/plain; charset=KOI8-R 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: Paolo Bonzini , 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 On 06/12/2014 03:29 AM, Paolo Bonzini wrote: > 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. "/machine" object is "machine" parameter from s390_init()? How do I add an interface to it? Create a machine class as it is done by spapr_machine_info? I am not that familiar with QOM and definitely miss something. > > 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 >> > -- Alexey