| Message ID | 20240220150833.13674-4-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/nmi: Remove @cpu_index argument | expand |
On Tue, 20 Feb 2024 at 15:09, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > Only s390x was using the 'cpu_index' argument, but since the > previous commit it isn't anymore (it use the first cpu). > Since this argument is now completely unused, remove it. Have > the callback return a boolean indicating failure. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > include/hw/nmi.h | 11 ++++++++++- > hw/core/nmi.c | 3 +-- > hw/hppa/machine.c | 8 +++++--- > hw/i386/x86.c | 7 ++++--- > hw/intc/m68k_irqc.c | 6 ++++-- > 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 | 6 ++++-- > 10 files changed, 44 insertions(+), 21 deletions(-) > > diff --git a/include/hw/nmi.h b/include/hw/nmi.h > index fff41bebc6..c70db941c9 100644 > --- a/include/hw/nmi.h > +++ b/include/hw/nmi.h > @@ -37,7 +37,16 @@ typedef struct NMIState NMIState; > struct NMIClass { > InterfaceClass parent_class; > > - void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp); > + /** > + * nmi_handler: Callback to handle NMI notifications. > + * > + * @n: Class #NMIState state > + * @errp: pointer to error object > + * > + * On success, return %true. > + * On failure, store an error through @errp and return %false. > + */ > + bool (*nmi_handler)(NMIState *n, Error **errp); Any particular reason to change the method name here? Do we really need to indicate failure both through the bool return and the Error** ? thanks -- PMM
On 20/3/24 14:23, Peter Maydell wrote: > On Tue, 20 Feb 2024 at 15:09, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> Only s390x was using the 'cpu_index' argument, but since the >> previous commit it isn't anymore (it use the first cpu). >> Since this argument is now completely unused, remove it. Have >> the callback return a boolean indicating failure. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> include/hw/nmi.h | 11 ++++++++++- >> hw/core/nmi.c | 3 +-- >> hw/hppa/machine.c | 8 +++++--- >> hw/i386/x86.c | 7 ++++--- >> hw/intc/m68k_irqc.c | 6 ++++-- >> 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 | 6 ++++-- >> 10 files changed, 44 insertions(+), 21 deletions(-) >> >> diff --git a/include/hw/nmi.h b/include/hw/nmi.h >> index fff41bebc6..c70db941c9 100644 >> --- a/include/hw/nmi.h >> +++ b/include/hw/nmi.h >> @@ -37,7 +37,16 @@ typedef struct NMIState NMIState; >> struct NMIClass { >> InterfaceClass parent_class; >> >> - void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp); >> + /** >> + * nmi_handler: Callback to handle NMI notifications. >> + * >> + * @n: Class #NMIState state >> + * @errp: pointer to error object >> + * >> + * On success, return %true. >> + * On failure, store an error through @errp and return %false. >> + */ >> + bool (*nmi_handler)(NMIState *n, Error **errp); > > Any particular reason to change the method name here? > > Do we really need to indicate failure both through the bool return > and the Error** ? No, but this is the style *recommended* by the Error API since commit e3fe3988d7 ("error: Document Error API usage rules"): error: Document Error API usage rules This merely codifies existing practice, with one exception: the rule advising against returning void, where existing practice is mixed. When the Error API was created, we adopted the (unwritten) rule to return void when the function returns no useful value on success, unlike GError, which recommends to return true on success and false on error then. [...] Make the rule advising against returning void official by putting it in writing. This will hopefully reduce confusion. * - Whenever practical, also return a value that indicates success / * failure. This can make the error checking more concise, and can * avoid useless error object creation and destruction. Note that * we still have many functions returning void. We recommend * • bool-valued functions return true on success / false on failure, * • pointer-valued functions return non-null / null pointer, and * • integer-valued functions return non-negative / negative. Anyway I'll respin removing @cpu_index as a single change :)
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > On 20/3/24 14:23, Peter Maydell wrote: >> On Tue, 20 Feb 2024 at 15:09, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >>> >>> Only s390x was using the 'cpu_index' argument, but since the >>> previous commit it isn't anymore (it use the first cpu). >>> Since this argument is now completely unused, remove it. Have >>> the callback return a boolean indicating failure. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> include/hw/nmi.h | 11 ++++++++++- >>> hw/core/nmi.c | 3 +-- >>> hw/hppa/machine.c | 8 +++++--- >>> hw/i386/x86.c | 7 ++++--- >>> hw/intc/m68k_irqc.c | 6 ++++-- >>> 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 | 6 ++++-- >>> 10 files changed, 44 insertions(+), 21 deletions(-) >>> >>> diff --git a/include/hw/nmi.h b/include/hw/nmi.h >>> index fff41bebc6..c70db941c9 100644 >>> --- a/include/hw/nmi.h >>> +++ b/include/hw/nmi.h >>> @@ -37,7 +37,16 @@ typedef struct NMIState NMIState; >>> struct NMIClass { >>> InterfaceClass parent_class; >>> >>> - void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp); >>> + /** >>> + * nmi_handler: Callback to handle NMI notifications. >>> + * >>> + * @n: Class #NMIState state >>> + * @errp: pointer to error object >>> + * >>> + * On success, return %true. >>> + * On failure, store an error through @errp and return %false. >>> + */ >>> + bool (*nmi_handler)(NMIState *n, Error **errp); >> Any particular reason to change the method name here? >> Do we really need to indicate failure both through the bool return >> and the Error** ? > > No, but this is the style *recommended* by the Error API since > commit e3fe3988d7 ("error: Document Error API usage rules"): > > error: Document Error API usage rules > > This merely codifies existing practice, with one exception: the rule > advising against returning void, where existing practice is mixed. > > When the Error API was created, we adopted the (unwritten) rule to > return void when the function returns no useful value on success, > unlike GError, which recommends to return true on success and false > on error then. > > [...] > > Make the rule advising against returning void official by putting it > in writing. This will hopefully reduce confusion. > > * - Whenever practical, also return a value that indicates success / > * failure. This can make the error checking more concise, and can > * avoid useless error object creation and destruction. Note that It's the difference between if (!frobnicate(arg, errp)) { return; } and frobnicate(arg, &err); if (err) { error_propagate(errp, err); return; } Readabilty dies by a thousand cuts. GError got this right. We deviated from it for Error, until we understood why it's right. Another win: &error_abort gives you a backtrace into frobnicate() with the former, and into error_propagate() with the latter. > * we still have many functions returning void. We recommend > * • bool-valued functions return true on success / false on failure, > * • pointer-valued functions return non-null / null pointer, and > * • integer-valued functions return non-negative / negative. > > Anyway I'll respin removing @cpu_index as a single change :)
On Wed, 20 Mar 2024 at 19:05, Markus Armbruster <armbru@redhat.com> wrote: > > Philippe Mathieu-Daudé <philmd@linaro.org> writes: > > > On 20/3/24 14:23, Peter Maydell wrote: > >> On Tue, 20 Feb 2024 at 15:09, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > >>> > >>> Only s390x was using the 'cpu_index' argument, but since the > >>> previous commit it isn't anymore (it use the first cpu). > >>> Since this argument is now completely unused, remove it. Have > >>> the callback return a boolean indicating failure. > >>> > >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > >>> --- > >>> include/hw/nmi.h | 11 ++++++++++- > >>> hw/core/nmi.c | 3 +-- > >>> hw/hppa/machine.c | 8 +++++--- > >>> hw/i386/x86.c | 7 ++++--- > >>> hw/intc/m68k_irqc.c | 6 ++++-- > >>> 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 | 6 ++++-- > >>> 10 files changed, 44 insertions(+), 21 deletions(-) > >>> > >>> diff --git a/include/hw/nmi.h b/include/hw/nmi.h > >>> index fff41bebc6..c70db941c9 100644 > >>> --- a/include/hw/nmi.h > >>> +++ b/include/hw/nmi.h > >>> @@ -37,7 +37,16 @@ typedef struct NMIState NMIState; > >>> struct NMIClass { > >>> InterfaceClass parent_class; > >>> > >>> - void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp); > >>> + /** > >>> + * nmi_handler: Callback to handle NMI notifications. > >>> + * > >>> + * @n: Class #NMIState state > >>> + * @errp: pointer to error object > >>> + * > >>> + * On success, return %true. > >>> + * On failure, store an error through @errp and return %false. > >>> + */ > >>> + bool (*nmi_handler)(NMIState *n, Error **errp); > >> Any particular reason to change the method name here? > >> Do we really need to indicate failure both through the bool return > >> and the Error** ? > > > > No, but this is the style *recommended* by the Error API since > > commit e3fe3988d7 ("error: Document Error API usage rules"): > > > > error: Document Error API usage rules > > > > This merely codifies existing practice, with one exception: the rule > > advising against returning void, where existing practice is mixed. > > > > When the Error API was created, we adopted the (unwritten) rule to > > return void when the function returns no useful value on success, > > unlike GError, which recommends to return true on success and false > > on error then. > > > > [...] > > > > Make the rule advising against returning void official by putting it > > in writing. This will hopefully reduce confusion. > > > > * - Whenever practical, also return a value that indicates success / > > * failure. This can make the error checking more concise, and can > > * avoid useless error object creation and destruction. Note that > > It's the difference between > > if (!frobnicate(arg, errp)) { > return; > } > > and > > frobnicate(arg, &err); > if (err) { > error_propagate(errp, err); > return; > } > > Readabilty dies by a thousand cuts. > > GError got this right. We deviated from it for Error, until we > understood why it's right. > > Another win: &error_abort gives you a backtrace into frobnicate() with > the former, and into error_propagate() with the latter. Fair enough. (When I made the comment I was vaguely wondering if we wanted to keep the return value available to distinguish "this hook has handled the NMI, don't keep iterating" from "no error, but you should keep iterating through other handlers". But I think in the end my feeling is we should always stop after the first NMI handler we find regardless. -- PMM
diff --git a/include/hw/nmi.h b/include/hw/nmi.h index fff41bebc6..c70db941c9 100644 --- a/include/hw/nmi.h +++ b/include/hw/nmi.h @@ -37,7 +37,16 @@ typedef struct NMIState NMIState; struct NMIClass { InterfaceClass parent_class; - void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp); + /** + * nmi_handler: Callback to handle NMI notifications. + * + * @n: Class #NMIState state + * @errp: pointer to error object + * + * On success, return %true. + * On failure, store an error through @errp and return %false. + */ + bool (*nmi_handler)(NMIState *n, Error **errp); }; void nmi_monitor_handle(int cpu_index, Error **errp); diff --git a/hw/core/nmi.c b/hw/core/nmi.c index f5220111c1..409164d445 100644 --- a/hw/core/nmi.c +++ b/hw/core/nmi.c @@ -40,8 +40,7 @@ static int do_nmi(Object *o, void *opaque) NMIClass *nc = NMI_GET_CLASS(n); ns->handled = true; - nc->nmi_monitor_handler(n, ns->cpu_index, &ns->err); - if (ns->err) { + if (!nc->nmi_handler(n, &ns->err)) { return -1; } } diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c index 5fcaf5884b..75b61a0683 100644 --- a/hw/hppa/machine.c +++ b/hw/hppa/machine.c @@ -673,13 +673,15 @@ static void hppa_machine_reset(MachineState *ms, ShutdownCause reason) cpu[0]->env.gr[19] = FW_CFG_IO_BASE; } -static void hppa_nmi(NMIState *n, int cpu_index, Error **errp) +static bool hppa_nmi(NMIState *n, Error **errp) { CPUState *cs; CPU_FOREACH(cs) { cpu_interrupt(cs, CPU_INTERRUPT_NMI); } + + return true; } static void HP_B160L_machine_init_class_init(ObjectClass *oc, void *data) @@ -705,7 +707,7 @@ static void HP_B160L_machine_init_class_init(ObjectClass *oc, void *data) mc->default_ram_id = "ram"; mc->default_nic = "tulip"; - nc->nmi_monitor_handler = hppa_nmi; + nc->nmi_handler = hppa_nmi; } static const TypeInfo HP_B160L_machine_init_typeinfo = { @@ -741,7 +743,7 @@ static void HP_C3700_machine_init_class_init(ObjectClass *oc, void *data) mc->default_ram_id = "ram"; mc->default_nic = "tulip"; - nc->nmi_monitor_handler = hppa_nmi; + nc->nmi_handler = hppa_nmi; } static const TypeInfo HP_C3700_machine_init_typeinfo = { diff --git a/hw/i386/x86.c b/hw/i386/x86.c index 684dce90e9..0d756c4857 100644 --- a/hw/i386/x86.c +++ b/hw/i386/x86.c @@ -512,9 +512,8 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms) return ms->possible_cpus; } -static void x86_nmi(NMIState *n, int cpu_index, Error **errp) +static bool x86_nmi(NMIState *n, Error **errp) { - /* cpu index isn't used */ CPUState *cs; CPU_FOREACH(cs) { @@ -526,6 +525,8 @@ static void x86_nmi(NMIState *n, int cpu_index, Error **errp) cpu_interrupt(cs, CPU_INTERRUPT_NMI); } } + + return true; } static long get_file_size(FILE *f) @@ -1416,7 +1417,7 @@ static void x86_machine_class_init(ObjectClass *oc, void *data) mc->possible_cpu_arch_ids = x86_possible_cpu_arch_ids; x86mc->save_tsc_khz = true; x86mc->fwcfg_dma_enabled = true; - nc->nmi_monitor_handler = x86_nmi; + nc->nmi_handler = x86_nmi; object_class_property_add(oc, X86_MACHINE_SMM, "OnOffAuto", x86_machine_get_smm, x86_machine_set_smm, diff --git a/hw/intc/m68k_irqc.c b/hw/intc/m68k_irqc.c index 4b11fb9f72..acc9348218 100644 --- a/hw/intc/m68k_irqc.c +++ b/hw/intc/m68k_irqc.c @@ -71,9 +71,11 @@ static void m68k_irqc_instance_init(Object *obj) qdev_init_gpio_in(DEVICE(obj), m68k_set_irq, M68K_IRQC_LEVEL_NUM); } -static void m68k_nmi(NMIState *n, int cpu_index, Error **errp) +static bool m68k_nmi(NMIState *n, Error **errp) { m68k_set_irq(n, M68K_IRQC_LEVEL_7, 1); + + return true; } static const VMStateDescription vmstate_m68k_irqc = { @@ -99,7 +101,7 @@ static void m68k_irqc_class_init(ObjectClass *oc, void *data) InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(oc); device_class_set_props(dc, m68k_irqc_properties); - nc->nmi_monitor_handler = m68k_nmi; + nc->nmi_handler = m68k_nmi; dc->reset = m68k_irqc_reset; dc->vmsd = &vmstate_m68k_irqc; ic->get_statistics = m68k_irqc_get_statistics; diff --git a/hw/m68k/q800-glue.c b/hw/m68k/q800-glue.c index b5a7713863..f92bd5064a 100644 --- a/hw/m68k/q800-glue.c +++ b/hw/m68k/q800-glue.c @@ -159,13 +159,15 @@ static void glue_auxmode_set_irq(void *opaque, int irq, int level) s->auxmode = level; } -static void glue_nmi(NMIState *n, int cpu_index, Error **errp) +static bool glue_nmi(NMIState *n, Error **errp) { GLUEState *s = GLUE(n); /* Hold NMI active for 100ms */ GLUE_set_irq(s, GLUE_IRQ_IN_NMI, 1); timer_mod(s->nmi_release, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 100); + + return true; } static void glue_nmi_release(void *opaque) @@ -238,7 +240,7 @@ static void glue_class_init(ObjectClass *klass, void *data) dc->vmsd = &vmstate_glue; device_class_set_props(dc, glue_properties); rc->phases.hold = glue_reset_hold; - nc->nmi_monitor_handler = glue_nmi; + nc->nmi_handler = glue_nmi; } static const TypeInfo glue_info_types[] = { diff --git a/hw/misc/macio/gpio.c b/hw/misc/macio/gpio.c index 549563747d..9ac93d9ed5 100644 --- a/hw/misc/macio/gpio.c +++ b/hw/misc/macio/gpio.c @@ -183,10 +183,12 @@ static void macio_gpio_reset(DeviceState *dev) macio_set_gpio(s, 1, true); } -static void macio_gpio_nmi(NMIState *n, int cpu_index, Error **errp) +static bool macio_gpio_nmi(NMIState *n, Error **errp) { macio_set_gpio(MACIO_GPIO(n), 9, true); macio_set_gpio(MACIO_GPIO(n), 9, false); + + return true; } static void macio_gpio_class_init(ObjectClass *oc, void *data) @@ -196,7 +198,7 @@ static void macio_gpio_class_init(ObjectClass *oc, void *data) dc->reset = macio_gpio_reset; dc->vmsd = &vmstate_macio_gpio; - nc->nmi_monitor_handler = macio_gpio_nmi; + nc->nmi_handler = macio_gpio_nmi; } static const TypeInfo macio_gpio_init_info = { diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index 0297871bdd..f572f8d0ce 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -2321,13 +2321,15 @@ static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, run_on_cpu_data arg) } } -static void pnv_nmi(NMIState *n, int cpu_index, Error **errp) +static bool pnv_nmi(NMIState *n, Error **errp) { CPUState *cs; CPU_FOREACH(cs) { async_run_on_cpu(cs, pnv_cpu_do_nmi_on_cpu, RUN_ON_CPU_NULL); } + + return true; } static void pnv_machine_class_init(ObjectClass *oc, void *data) @@ -2351,7 +2353,7 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data) mc->default_ram_size = 1 * GiB; mc->default_ram_id = "pnv.ram"; ispc->print_info = pnv_pic_print_info; - nc->nmi_monitor_handler = pnv_nmi; + nc->nmi_handler = pnv_nmi; object_class_property_add_bool(oc, "hb-mode", pnv_machine_get_hb, pnv_machine_set_hb); diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 0d72d286d8..7327bf3429 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3505,13 +3505,15 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg) } } -static void spapr_nmi(NMIState *n, int cpu_index, Error **errp) +static bool spapr_nmi(NMIState *n, Error **errp) { CPUState *cs; CPU_FOREACH(cs) { async_run_on_cpu(cs, spapr_do_system_reset_on_cpu, RUN_ON_CPU_NULL); } + + return true; } int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr, @@ -4672,7 +4674,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) mc->nvdimm_supported = true; smc->resize_hpt_default = SPAPR_RESIZE_HPT_ENABLED; fwc->get_dev_path = spapr_get_fw_dev_path; - nc->nmi_monitor_handler = spapr_nmi; + nc->nmi_handler = spapr_nmi; smc->phb_placement = spapr_phb_placement; vhc->cpu_in_nested = spapr_cpu_in_nested; vhc->deliver_hv_excp = spapr_exit_nested; diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index ba1fa6472f..817f414767 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -603,9 +603,11 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine, return NULL; } -static void s390_nmi(NMIState *n, int cpu_index, Error **errp) +static bool s390_nmi(NMIState *n, Error **errp) { s390_cpu_restart(S390_CPU(first_cpu)); + + return true; } static ram_addr_t s390_fixup_ram_size(ram_addr_t sz) @@ -774,7 +776,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data) mc->default_cpu_type = S390_CPU_TYPE_NAME("qemu"); hc->plug = s390_machine_device_plug; hc->unplug_request = s390_machine_device_unplug_request; - nc->nmi_monitor_handler = s390_nmi; + nc->nmi_handler = s390_nmi; mc->default_ram_id = "s390.ram"; mc->default_nic = "virtio-net-ccw";
Only s390x was using the 'cpu_index' argument, but since the previous commit it isn't anymore (it use the first cpu). Since this argument is now completely unused, remove it. Have the callback return a boolean indicating failure. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/hw/nmi.h | 11 ++++++++++- hw/core/nmi.c | 3 +-- hw/hppa/machine.c | 8 +++++--- hw/i386/x86.c | 7 ++++--- hw/intc/m68k_irqc.c | 6 ++++-- 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 | 6 ++++-- 10 files changed, 44 insertions(+), 21 deletions(-)