| Message ID | 20240610062105.49848-4-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/ppc: Prefer HumanReadableText over Monitor | expand |
Hi Phillipe, On 6/10/24 11:50, Philippe Mathieu-Daudé wrote: > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index fa23b27a2b..5854358f65 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -1223,7 +1223,13 @@ static void pnv_chip_power9_intc_destroy(PnvChip *chip, PowerPCCPU *cpu) > static void pnv_chip_power9_intc_print_info(PnvChip *chip, PowerPCCPU *cpu, > Monitor *mon) > { > - xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), mon); > + g_autoptr(GString) buf = g_string_new(""); > + g_autoptr(HumanReadableText) info = NULL; > + > + xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), buf); > + > + info = human_readable_text_from_str(buf); > + monitor_puts(mon, info->human_readable_text); > } > > static void pnv_chip_power10_intc_create(PnvChip *chip, PowerPCCPU *cpu, > @@ -1267,7 +1273,13 @@ static void pnv_chip_power10_intc_destroy(PnvChip *chip, PowerPCCPU *cpu) > static void pnv_chip_power10_intc_print_info(PnvChip *chip, PowerPCCPU *cpu, > Monitor *mon) > { > - xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), mon); > + g_autoptr(GString) buf = g_string_new(""); > + g_autoptr(HumanReadableText) info = NULL; > + > + xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), buf); > + > + info = human_readable_text_from_str(buf); > + monitor_puts(mon, info->human_readable_text); > } > We have an existing code duplication in above two routines which is getting worse with these multi-lines getting duplicated. Could _power9_ be changed to inline and called from _power10_ as well? Thanks Harsh
Hi Harsh, On 17/6/24 12:19, Harsh Prateek Bora wrote: > On 6/10/24 11:50, Philippe Mathieu-Daudé wrote: >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c >> index fa23b27a2b..5854358f65 100644 >> --- a/hw/ppc/pnv.c >> +++ b/hw/ppc/pnv.c >> @@ -1223,7 +1223,13 @@ static void >> pnv_chip_power9_intc_destroy(PnvChip *chip, PowerPCCPU *cpu) >> static void pnv_chip_power9_intc_print_info(PnvChip *chip, >> PowerPCCPU *cpu, >> Monitor *mon) >> { >> - xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), mon); >> + g_autoptr(GString) buf = g_string_new(""); >> + g_autoptr(HumanReadableText) info = NULL; >> + >> + xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), buf); >> + >> + info = human_readable_text_from_str(buf); >> + monitor_puts(mon, info->human_readable_text); >> } >> static void pnv_chip_power10_intc_create(PnvChip *chip, PowerPCCPU >> *cpu, >> @@ -1267,7 +1273,13 @@ static void >> pnv_chip_power10_intc_destroy(PnvChip *chip, PowerPCCPU *cpu) >> static void pnv_chip_power10_intc_print_info(PnvChip *chip, >> PowerPCCPU *cpu, >> Monitor *mon) >> { >> - xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), mon); >> + g_autoptr(GString) buf = g_string_new(""); >> + g_autoptr(HumanReadableText) info = NULL; >> + >> + xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), buf); >> + >> + info = human_readable_text_from_str(buf); >> + monitor_puts(mon, info->human_readable_text); >> } > > We have an existing code duplication in above two routines which is > getting worse with these multi-lines getting duplicated. Could _power9_ > be changed to inline and called from _power10_ as well? The duplicated lines get removed later. This series is a mere API upgrade, the final diff is smaller (although harder to review, which is why I did it with many trivial patches). _power9_ / _power10_ simplification could be done but as a separate patch from this series, since different matter IMHO. Thanks for your reviews! Phil.
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h index f120874e0f..bc1cbad8a8 100644 --- a/include/hw/ppc/xive.h +++ b/include/hw/ppc/xive.h @@ -528,7 +528,7 @@ void xive_tctx_tm_write(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset, uint64_t xive_tctx_tm_read(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset, unsigned size); -void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon); +void xive_tctx_pic_print_info(XiveTCTX *tctx, GString *buf); Object *xive_tctx_create(Object *cpu, XivePresenter *xptr, Error **errp); void xive_tctx_reset(XiveTCTX *tctx); void xive_tctx_destroy(XiveTCTX *tctx); diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c index d7e56bfb20..b7c12aa432 100644 --- a/hw/intc/spapr_xive.c +++ b/hw/intc/spapr_xive.c @@ -11,6 +11,7 @@ #include "qemu/log.h" #include "qemu/module.h" #include "qapi/error.h" +#include "qapi/type-helpers.h" #include "qemu/error-report.h" #include "target/ppc/cpu.h" #include "sysemu/cpus.h" @@ -703,12 +704,16 @@ static void spapr_xive_print_info(SpaprInterruptController *intc, Monitor *mon) { SpaprXive *xive = SPAPR_XIVE(intc); CPUState *cs; + g_autoptr(GString) buf = g_string_new(""); + g_autoptr(HumanReadableText) info = NULL; CPU_FOREACH(cs) { PowerPCCPU *cpu = POWERPC_CPU(cs); - xive_tctx_pic_print_info(spapr_cpu_state(cpu)->tctx, mon); + xive_tctx_pic_print_info(spapr_cpu_state(cpu)->tctx, buf); } + info = human_readable_text_from_str(buf); + monitor_puts(mon, info->human_readable_text); spapr_xive_pic_print_info(xive, mon); } diff --git a/hw/intc/xive.c b/hw/intc/xive.c index 057b308ae9..a0d7e7ca67 100644 --- a/hw/intc/xive.c +++ b/hw/intc/xive.c @@ -669,7 +669,7 @@ static const char * const xive_tctx_ring_names[] = { xpc->in_kernel ? xpc->in_kernel(xptr) : false; \ })) -void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon) +void xive_tctx_pic_print_info(XiveTCTX *tctx, GString *buf) { int cpu_index; int i; @@ -693,13 +693,14 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon) } } - monitor_printf(mon, "CPU[%04x]: QW NSR CPPR IPB LSMFB ACK# INC AGE PIPR" - " W2\n", cpu_index); + g_string_append_printf(buf, "CPU[%04x]: " + "QW NSR CPPR IPB LSMFB ACK# INC AGE PIPR W2\n", + cpu_index); for (i = 0; i < XIVE_TM_RING_COUNT; i++) { char *s = xive_tctx_ring_print(&tctx->regs[i * XIVE_TM_RING_SIZE]); - monitor_printf(mon, "CPU[%04x]: %4s %s\n", cpu_index, - xive_tctx_ring_names[i], s); + g_string_append_printf(buf, "CPU[%04x]: %4s %s\n", + cpu_index, xive_tctx_ring_names[i], s); g_free(s); } } diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index fa23b27a2b..5854358f65 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -1223,7 +1223,13 @@ static void pnv_chip_power9_intc_destroy(PnvChip *chip, PowerPCCPU *cpu) static void pnv_chip_power9_intc_print_info(PnvChip *chip, PowerPCCPU *cpu, Monitor *mon) { - xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), mon); + g_autoptr(GString) buf = g_string_new(""); + g_autoptr(HumanReadableText) info = NULL; + + xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), buf); + + info = human_readable_text_from_str(buf); + monitor_puts(mon, info->human_readable_text); } static void pnv_chip_power10_intc_create(PnvChip *chip, PowerPCCPU *cpu, @@ -1267,7 +1273,13 @@ static void pnv_chip_power10_intc_destroy(PnvChip *chip, PowerPCCPU *cpu) static void pnv_chip_power10_intc_print_info(PnvChip *chip, PowerPCCPU *cpu, Monitor *mon) { - xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), mon); + g_autoptr(GString) buf = g_string_new(""); + g_autoptr(HumanReadableText) info = NULL; + + xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), buf); + + info = human_readable_text_from_str(buf); + monitor_puts(mon, info->human_readable_text); } /*
Replace Monitor API by HumanReadableText one. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/hw/ppc/xive.h | 2 +- hw/intc/spapr_xive.c | 7 ++++++- hw/intc/xive.c | 11 ++++++----- hw/ppc/pnv.c | 16 ++++++++++++++-- 4 files changed, 27 insertions(+), 9 deletions(-)