| Message ID | 20240220150833.13674-1-philmd@linaro.org |
|---|---|
| Headers | show |
| Series | hw/nmi: Remove @cpu_index argument | expand |
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? Thanks, Thomas > Philippe Mathieu-Daudé (4): > hw/nmi: Use object_child_foreach_recursive() in nmi_children() > hw/s390x/virtio-ccw: Always deliver NMI to first CPU > hw/nmi: Remove @cpu_index argument from NMIClass::nmi_handler() > hw/nmi: Remove @cpu_index argument from nmi_trigger() > > qapi/run-state.json | 5 +++-- > include/hw/nmi.h | 24 ++++++++++++++++++++++-- > hw/core/nmi.c | 24 +++++++++--------------- > hw/hppa/machine.c | 8 +++++--- > hw/i386/x86.c | 7 ++++--- > hw/intc/m68k_irqc.c | 6 ++++-- > hw/ipmi/ipmi.c | 3 +-- > hw/m68k/q800-glue.c | 6 ++++-- > hw/misc/macio/gpio.c | 6 ++++-- > hw/ppc/pnv.c | 6 ++++-- > hw/ppc/spapr.c | 6 ++++-- > hw/s390x/s390-virtio-ccw.c | 8 ++++---- > hw/watchdog/watchdog.c | 2 +- > system/cpus.c | 2 +- > hmp-commands.hx | 2 +- > 15 files changed, 71 insertions(+), 44 deletions(-) >
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? Heterogeneous machines, a NMI have to reach all NMI-aware HW. See also "Remove cpu_interrupt() from hw/": https://lore.kernel.org/qemu-devel/20240220192625.17944-1-philmd@linaro.org/
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". > > Thanks, > Thomas > > >> Philippe Mathieu-Daudé (4): >> hw/nmi: Use object_child_foreach_recursive() in nmi_children() >> hw/s390x/virtio-ccw: Always deliver NMI to first CPU >> hw/nmi: Remove @cpu_index argument from NMIClass::nmi_handler() >> hw/nmi: Remove @cpu_index argument from nmi_trigger()
> On 20 Mar 2024, at 12:19, Philippe Mathieu-Daudé <philmd@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 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 those two things are sort of similar. e.g. we could have a machine in which many of the components all receive a ‘reset’ signal, but that would indeed be wired up explicitly from the thing generating that reset signal and all the components wanting to receive it. And, yes, there may be components that are not reset by that signal…. Cheers Mark. >> >> Thanks, >> Thomas >> >> >>> Philippe Mathieu-Daudé (4): >>> hw/nmi: Use object_child_foreach_recursive() in nmi_children() >>> hw/s390x/virtio-ccw: Always deliver NMI to first CPU >>> hw/nmi: Remove @cpu_index argument from NMIClass::nmi_handler() >>> hw/nmi: Remove @cpu_index argument from nmi_trigger() >
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 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
> 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
On Wed, 20 Mar 2024 at 12:31, Mark Burton <mburton@qti.qualcomm.com> wrote: > > On 20 Mar 2024, at 13:00, Peter Maydell <peter.maydell@linaro.org> wrote: > > 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? The places we want to generate 'NMI' are: * when the user uses the 'nmi' command in the monitor * in the generic "a watchdog timer expired" handling code (in the case where the user used a monitor command to say "trigger an NMI if the watchdog times out") * when the user requested to send the VM an NMI via IPMI In all those cases we don't have a pointer to the board or any kind of idea of what a GPIO wire would be, and because at the moment TYPE_MACHINE is not a subclass of TYPE_DEVICE a machine model can't have external GPIO lines anyway. From a convenience point of view all those callsites simply want to be able to call a function to say "trigger an NMI please" (which is what nmi_monitor_handle() does). Beyond that the implementation of that function is just whatever is convenient. If in your particular machine model it makes sense to have NMI be something you deal with via GPIO wiring, you can implement the TYPE_NMI interface on your machine class to work by raising a GPIO line. thanks -- PMM
> On 20 Mar 2024, at 14:55, 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 12:31, Mark Burton <mburton@qti.qualcomm.com> wrote: >>> On 20 Mar 2024, at 13:00, Peter Maydell <peter.maydell@linaro.org> wrote: >>> 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? > > The places we want to generate 'NMI' are: > * when the user uses the 'nmi' command in the monitor > * in the generic "a watchdog timer expired" handling code (in the > case where the user used a monitor command to say "trigger an NMI > if the watchdog times out") > * when the user requested to send the VM an NMI via IPMI > > In all those cases we don't have a pointer to the board or > any kind of idea of what a GPIO wire would be, and because at > the moment TYPE_MACHINE is not a subclass of TYPE_DEVICE a > machine model can't have external GPIO lines anyway. From > a convenience point of view all those callsites simply want > to be able to call a function to say "trigger an NMI please" > (which is what nmi_monitor_handle() does). Beyond that the > implementation of that function is just whatever is convenient. > > If in your particular machine model it makes sense to have NMI > be something you deal with via GPIO wiring, you can implement the > TYPE_NMI interface on your machine class to work by raising a > GPIO line. I’d broaden this to all ’signals’ (IRQ, Reset etc) - and I guess similar statements apply, with the “bridge” between the function and the GPIO mechanism moved closer or further from the originator(s) of the activity. The issue isn’t my “machine” model, rather the compose-ability of (any) such machine. A-priori, a model writer doesn’t know if they should respond directly to an NMI or not - Hence they dont know if they should implement the TYPE_NMI or not. That’s a decision only the machine composer knows. My suggestion would be to use a GPIO interface to models, which can then be appropriately wired. (And, hence, to have a single place that implements the TYPE_NMI interface and provides the GPIO wire ready for wiring to appropriate devices). Cheers Mark. > > thanks > -- PMM
On Wed, 20 Mar 2024 at 14:10, Mark Burton <mburton@qti.qualcomm.com> wrote: > I’d broaden this to all ’signals’ (IRQ, Reset etc) - and I guess > similar statements apply, with the “bridge” between the function > and the GPIO mechanism moved closer or further from the originator(s) > of the activity. > > The issue isn’t my “machine” model, rather the compose-ability of > (any) such machine. A-priori, a model writer doesn’t know if they > should respond directly to an NMI or not - Hence they dont know if > they should implement the TYPE_NMI or not. That’s a decision only > the machine composer knows. > My suggestion would be to use a GPIO interface to models, which can > then be appropriately wired. (And, hence, to have a single place > that implements the TYPE_NMI interface and provides the GPIO wire > ready for wiring to appropriate devices). I feel like that's a long way in the future, but my back-of-the-envelope design sketch of that is that the TYPE_MACHINE class that's implementing the "I am just a container for all the devices that the user has specified and wired together" machine would itself implement TYPE_NMI and when an NMI came in it would assert a GPIO line that the user could wire up, or not wire up, as they chose. Right now we can't do that though, because, among other reasons, TYPE_MACHINE isn't a TYPE_DEVICE. (I do want to fix that, though: I'm hoping it won't be too difficult.) thanks -- PMM
> On 20 Mar 2024, at 16: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 14:10, Mark Burton <mburton@qti.qualcomm.com> wrote: >> I’d broaden this to all ’signals’ (IRQ, Reset etc) - and I guess >> similar statements apply, with the “bridge” between the function >> and the GPIO mechanism moved closer or further from the originator(s) >> of the activity. >> >> The issue isn’t my “machine” model, rather the compose-ability of >> (any) such machine. A-priori, a model writer doesn’t know if they >> should respond directly to an NMI or not - Hence they dont know if >> they should implement the TYPE_NMI or not. That’s a decision only >> the machine composer knows. >> My suggestion would be to use a GPIO interface to models, which can >> then be appropriately wired. (And, hence, to have a single place >> that implements the TYPE_NMI interface and provides the GPIO wire >> ready for wiring to appropriate devices). > > I feel like that's a long way in the future, but my back-of-the-envelope > design sketch of that is that the TYPE_MACHINE class that's implementing > the "I am just a container for all the devices that the user has > specified and wired together" machine would itself implement TYPE_NMI and > when an NMI came in it would assert a GPIO line that the user could > wire up, or not wire up, as they chose. > Yeah - makes sense. Cheers Mark. > Right now we can't do that though, because, among other reasons, > TYPE_MACHINE isn't a TYPE_DEVICE. (I do want to fix that, though: > I'm hoping it won't be too difficult.) > > thanks > -- PMM
On 3/20/24 16:00, Peter Maydell wrote: > On Wed, 20 Mar 2024 at 14:10, Mark Burton <mburton@qti.qualcomm.com> wrote: >> I’d broaden this to all ’signals’ (IRQ, Reset etc) - and I guess >> similar statements apply, with the “bridge” between the function >> and the GPIO mechanism moved closer or further from the originator(s) >> of the activity. >> >> The issue isn’t my “machine” model, rather the compose-ability of >> (any) such machine. A-priori, a model writer doesn’t know if they >> should respond directly to an NMI or not - Hence they dont know if >> they should implement the TYPE_NMI or not. That’s a decision only >> the machine composer knows. >> My suggestion would be to use a GPIO interface to models, which can >> then be appropriately wired. (And, hence, to have a single place >> that implements the TYPE_NMI interface and provides the GPIO wire >> ready for wiring to appropriate devices). > > I feel like that's a long way in the future, but my back-of-the-envelope > design sketch of that is that the TYPE_MACHINE class that's implementing > the "I am just a container for all the devices that the user has > specified and wired together" machine would itself implement TYPE_NMI and > when an NMI came in it would assert a GPIO line that the user could > wire up, or not wire up, as they chose. > > Right now we can't do that though, because, among other reasons, > TYPE_MACHINE isn't a TYPE_DEVICE. (I do want to fix that, though: > I'm hoping it won't be too difficult.) Oh that's interesting. Will that introduce an extra level of container with multiple machines below ? /qemu /machine[0] ... /peripheral (container) /peripheral-anon (container) /machine[1] ... /peripheral (container) /peripheral-anon (container) /unattached (container) ... /sysbus (System) /system[0] (memory-region) Thanks, C.
On Fri, 22 Mar 2024 at 14:08, Cédric Le Goater <clegoate@redhat.com> wrote: > > On 3/20/24 16:00, Peter Maydell wrote: > > On Wed, 20 Mar 2024 at 14:10, Mark Burton <mburton@qti.qualcomm.com> wrote: > >> I’d broaden this to all ’signals’ (IRQ, Reset etc) - and I guess > >> similar statements apply, with the “bridge” between the function > >> and the GPIO mechanism moved closer or further from the originator(s) > >> of the activity. > >> > >> The issue isn’t my “machine” model, rather the compose-ability of > >> (any) such machine. A-priori, a model writer doesn’t know if they > >> should respond directly to an NMI or not - Hence they dont know if > >> they should implement the TYPE_NMI or not. That’s a decision only > >> the machine composer knows. > >> My suggestion would be to use a GPIO interface to models, which can > >> then be appropriately wired. (And, hence, to have a single place > >> that implements the TYPE_NMI interface and provides the GPIO wire > >> ready for wiring to appropriate devices). > > > > I feel like that's a long way in the future, but my back-of-the-envelope > > design sketch of that is that the TYPE_MACHINE class that's implementing > > the "I am just a container for all the devices that the user has > > specified and wired together" machine would itself implement TYPE_NMI and > > when an NMI came in it would assert a GPIO line that the user could > > wire up, or not wire up, as they chose. > > > > Right now we can't do that though, because, among other reasons, > > TYPE_MACHINE isn't a TYPE_DEVICE. (I do want to fix that, though: > > I'm hoping it won't be too difficult.) > > Oh that's interesting. Will that introduce an extra level of container > with multiple machines below ? No, I don't intend that we should have multiple machines in one simulation, only that the thing which is "container for all the machine's devices" shouldn't be a weirdly distinct type from the SoC "container for devices" devices. What I'm primarily hoping to remedy by making TYPE_MACHINE a subclass of TYPE_DEVICE to deal with inconsistencies like: * reset of machine objects is nonstandard * machine models can't use facilities like having qdev gpio lines, so wind up calling qemu_allocate_irqs() directly None of these are big things, but they're a bit paper-cut-ish. -- PMM