From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58141) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WusbM-0006ES-H4 for qemu-devel@nongnu.org; Wed, 11 Jun 2014 20:10:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WusbG-0001u8-SL for qemu-devel@nongnu.org; Wed, 11 Jun 2014 20:10:44 -0400 Received: from mail-pa0-f47.google.com ([209.85.220.47]:41686) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WusbG-0001u2-HV for qemu-devel@nongnu.org; Wed, 11 Jun 2014 20:10:38 -0400 Received: by mail-pa0-f47.google.com with SMTP id fa1so348033pad.34 for ; Wed, 11 Jun 2014 17:10:38 -0700 (PDT) Message-ID: <5398EFF6.7060603@ozlabs.ru> Date: Thu, 12 Jun 2014 10:10:30 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1402506183-29736-1-git-send-email-aik@ozlabs.ru> <1402506183-29736-2-git-send-email-aik@ozlabs.ru> <53989009.1000300@redhat.com> In-Reply-To: <53989009.1000300@redhat.com> Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 1/4] cpus: Define callback for QEMU "nmi" command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Peter Maydell , Alex Bligh , Markus Armbruster , Luiz Capitulino , Alexander Graf , qemu-ppc@nongnu.org, Stefan Hajnoczi , Cornelia Huck , Paolo Bonzini , Richard Henderson On 06/12/2014 03:21 AM, Eric Blake wrote: > On 06/11/2014 11:03 AM, Alexey Kardashevskiy wrote: >> This introduces an NMI (Non Maskable Interrupt) interface with >> a single nmi_monitor_handler() method. A machine or a device can >> implement it. This searches for an QOM object which supports the interface >> and if it is implemented , calls it. The callback implements an action >> required to cause debug crash dump on in-kernel debugger invocation. >> The callback returns Error**. >> >> This adds support for it in qmp_inject_nmi(). Since no architecture >> supports it at the moment, there is no change in behaviour. >> >> This changes inject-nmi command description for HMP and QMP. >> >> Signed-off-by: Alexey Kardashevskiy >> --- > >> +++ b/hw/core/nmi.c >> @@ -0,0 +1,81 @@ >> +/* >> + * NMI monitor handler class and helpers. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, >> + * or (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, see . >> + */ > > For the GPL license to work, there has to be a Copyright line. Hm. Ok. include/hw/fw-path-provider.h (where I copied this from) does not have one and it was ok. > >> +static int do_nmi(Object *o, void *opaque) >> +{ >> + struct do_nmi_s *ns = opaque; >> + NMI *n = (NMI *) object_dynamic_cast(o, TYPE_NMI); > > Is the cast to (NMI *) necessary, or does object_dynamic_cast() already > return something that can be assigned to an arbitrary pointer type? All the macros from include/qom/object.h do this cast and so do I. object_dynamic_cast returns Object*. >> +++ b/include/hw/nmi.h >> @@ -0,0 +1,46 @@ >> +/* >> + * NMI monitor handler class and helpers definitions. > > Same comment about Copyright. > > >> +#define TYPE_NMI "nmi" >> + >> +#define NMI_CLASS(klass) \ >> + OBJECT_CLASS_CHECK(NMIClass, (klass), TYPE_NMI) >> +#define NMI_GET_CLASS(obj) \ >> + OBJECT_GET_CLASS(NMIClass, (obj), TYPE_NMI) >> +#define NMI(obj) \ >> + INTERFACE_CHECK(NMI, (obj), TYPE_NMI) >> + >> +typedef struct NMI { >> + Object parent_obj; >> +} NMI; > > Eww. This is declaring both 'NMI' as a type, and 'NMI()' as a macro. > Is that going to trip people up? Is 'NMIState' a better name? My bad, I'll fix. I'll probably get rid of the struct. > > >> +++ b/qapi-schema.json >> @@ -1116,13 +1116,13 @@ >> ## >> # @inject-nmi: >> # >> -# Injects an Non-Maskable Interrupt into all guest's VCPUs. >> +# Injects an Non-Maskable Interrupt into the default CPU (x86/s390) or all CPUs (ppc64). > > s/an Non/a Non/ Yep, here and in other places I read it as "en-em-eye" but it is not correct right here :) I'll fix, thanks. >> +++ b/qmp-commands.hx >> @@ -477,7 +477,7 @@ SQMP >> inject-nmi >> ---------- >> >> -Inject an NMI on guest's CPUs. >> +Inject an NMI on the default CPU (x86/s390) or all CPUs (ppc64). > > Oh, I see the problem. The acronym 'NMI' is pronounced with a leading > vowel (an "en-em-eye"), but the expansion is pronounced with a consonant > (a non-maskable interrupt), so the correct article depends on whether > you are spelling things out. -- Alexey