From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43052) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wv1je-0005hB-BA for qemu-devel@nongnu.org; Thu, 12 Jun 2014 05:56:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wv1jX-00087z-8z for qemu-devel@nongnu.org; Thu, 12 Jun 2014 05:55:54 -0400 Received: from mail-pa0-f53.google.com ([209.85.220.53]:41414) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wv1jX-00087Y-25 for qemu-devel@nongnu.org; Thu, 12 Jun 2014 05:55:47 -0400 Received: by mail-pa0-f53.google.com with SMTP id kx10so833463pab.26 for ; Thu, 12 Jun 2014 02:55:45 -0700 (PDT) Message-ID: <5399791B.8000500@ozlabs.ru> Date: Thu, 12 Jun 2014 19:55:39 +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> <20140612083113.1e4cd667.cornelia.huck@de.ibm.com> <53997520.8020509@ozlabs.ru> <5399755D.1080803@suse.de> In-Reply-To: <5399755D.1080803@suse.de> 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: Alexander Graf , Cornelia Huck Cc: Peter Maydell , Stefan Hajnoczi , Markus Armbruster , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Alex Bligh , Paolo Bonzini , Luiz Capitulino , Richard Henderson On 06/12/2014 07:39 PM, Alexander Graf wrote: > > On 12.06.14 11:38, Alexey Kardashevskiy wrote: >> On 06/12/2014 04:31 PM, Cornelia Huck wrote: >>> On Thu, 12 Jun 2014 03:03:01 +1000 >>> Alexey Kardashevskiy wrote: >>> >>>> 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/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" >>> I'd prefer "s390-nmi" instead. >>> >>>> 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); >>> This only adds the nmi interface to the old s390-virtio machine; we >>> want this for the s390-virtio-ccw machine as well. >>> >>>> } >>>> >>>> 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); >>> s390-virtio-ccw needs this as well. >>> >>>> qemu_register_machine(&s390_machine); >>>> } >>> The best way would probably be to put all nmi-related things into a new >>> file that registers the nmi type and provides an s390_register_nmi() >>> helper. >> >> I pushed some version to git@github.com:aik/qemu.git , branch nmi-v7 >> Please have a look and give it a go - I do not have s390 kernel/images >> handy. Thanks! >> >> It does not look like we really need a new file for NMI now. >> Also, should I put it under #ifdef CONFIG_USER_ONLY on s390? > > The machine only ever gets compiled on for non-user, so why would you need > any #ifdefs? Oh, right, this is a leftover in my head from when it was a CPU callback :) -- Alexey