Re: [PATCH 0/4] hw/nmi: Remove @cpu_index argument

[Mark Burton]

> On 20 Mar 2024, at 13:00, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of 
> any links or attachments, and do not enable macros.
> 
> On Wed, 20 Mar 2024 at 11:20, Philippe Mathieu-Daudé <philmd@linaro.org> 
> wrote:
>> 
>> On 20/2/24 16:19, Thomas Huth wrote:
>>> On 20/02/2024 16.08, Philippe Mathieu-Daudé wrote:
>>>> Have s390x always deliver NMI to the first CPU,
>>>> remove the @cpu_index argument from handler,
>>>> rename API as nmi_trigger() (not monitor specific).
>>> 
>>> Could you please add some rationale here why this is needed / desired?
>> 
>> I'm not sure it is desired... I'm trying to get the NMI delivery
>> working in heterogeneous machine, but now I'm wondering whether
>> hw/core/nmi.c was designed with that in mind or likely not.
>> 
>> I suppose in a complex machine you explicitly wire IRQ lines such
>> NMI, so they are delivered to a particular INTC or CPU core, and
>> there is no "broadcast this signal to all listeners registered
>> for NMI events".
> 
> I think in a complex heterogenous machine you do want the
> monitor NMI command to do something sensible, but the
> definition of "sensible" is going to be machine-specific:
> probably it will be "raise NMI in some way on some core in
> the main application processor cluster", and it's the machine
> model that's going to know what "sensible" is for that machine.
> 
> The current hw/core/nmi.c code is a bit odd because it's partly
> working with a cpu_index and partly not: the code passes cpu_index
> around, but in practice for the QMP command the user can't set
> which CPU to operate on, and for everything except s390 the
> implementation doesn't care anyway. My impression from the IRC
> discussion is that it's not really necessary for the S390 that
> the monitor user be able to specify which CPU to NMI (and in any
> case you can only do that from the HMP command, not the QMP
> command, AIUI), so getting rid of that weird inconsistency makes
> sense to me: and that's what this patchset is doing.
> 
> What NMI probably ought to be is board-specific: so it's like
> having some notional front panel switch labeled "NMI", and the

Do the youngsters of today know what one of those is ?
        :-)


Is there a reason for not using a GPIO interface for ’NMI’ - wiring it up like 
any other wire?

Cheers
Mark.

> board gets to decide what that means (which is usually going to be
> "send some NMI like interrupt to the first CPU in the main cluster",
> but could be something else). It doesn't need to be like a
> front panel switch with a rotary-selector for 'pick a CPU'
> plus a button for "send NMI to that CPU". In fact we're quite
> close to "it's a board thing" already, because almost every
> implementation of the TYPE_NMI interface is actually a machine
> model. (The exceptions are hw/intc/m68k_irqc.c,
> hw/m68k/q800-glue.c and hw/misc/macio/gpio.c.)
> 
> So I think that:
> * we should indeed drop the cpu_index stuff, per this patch:
>   it's unnecessary cruft we don't really use
> * we should look at whether the three classes listed above
>   which implement TYPE_NMI on a non-machine-model are really
>   the right way to do that, i.e. whether it would be a lot of
>   effort to effectively switch to having nmi_monitor_handler
>   be a simple method on MachineClass. Not walking the QOM
>   tree would make the NMI infrastructure rather simpler.
>   (But I just looked at the macio case, and it's inside a
>   PCI device, so at best that's a bunch of clunky plumbing.)
> * failing that, we should look at whether we should really
>   continue to walk the whole QOM tree calling methods on every
>   TYPE_NMI object, or whether we can say "once we've found one
>   implementation we're done". This also depends on those three
>   non-MachineClass implementations, because obviously there's
>   only ever one MachineClass object in the system. This is
>   kind of useful for heterogenous boards which use the m68k
>   or ppc devices listed above (seems highly unlikely), but it
>   would mean you can override the default "those objects handle
>   NMI" by having your heterogenous board implement TYPE_NMI,
>   and then since it's earlier in the QOM tree that will be
>   the method called, not the ones on specific devices.
>   (This one I think we can easily do -- my quick check suggests
>   that TYPE_M68K_IRQ is only used in the m68k virt board,
>   TYPE_GLUE is only used in the m68k q800 board, and
>   TYPE_MACIO_GPIO is only used in the ppc mac99 board. So in
>   fact in all cases there's only ever one TYPE_NMI interface
>   present in the system.)
> 
> The last two aren't blockers for heterogenous-system work,
> though: they just seem to me like nice cleanup of this interface.
> 
> thanks
> -- PMM