* [PATCH qemu v2 0/2] ppc/spapr: Implement H_WATCHDOG
@ 2022-06-17 6:07 Alexey Kardashevskiy
2022-06-17 6:07 ` [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target Alexey Kardashevskiy
` (2 more replies)
0 siblings, 3 replies; 21+ messages in thread
From: Alexey Kardashevskiy @ 2022-06-17 6:07 UTC (permalink / raw)
To: qemu-ppc
Cc: qemu-devel, Daniel Henrique Barboza, Cédric Le Goater,
Alexey Kardashevskiy
This implements H_WATCHDOG. More detailed comments are in the patches.
This is based on sha1
96c343cc774b Joel Stanley "linux-user: Add PowerPC ISA 3.1 and MMA to hwcap".
Please comment. Thanks.
Alexey Kardashevskiy (2):
ppc: Define SETFIELD for the ppc target
ppc/spapr: Implement H_WATCHDOG
include/hw/pci-host/pnv_phb3_regs.h | 16 --
include/hw/ppc/spapr.h | 29 +++-
target/ppc/cpu.h | 5 +
hw/intc/pnv_xive.c | 20 ---
hw/intc/pnv_xive2.c | 20 ---
hw/pci-host/pnv_phb4.c | 16 --
hw/ppc/spapr.c | 4 +
hw/watchdog/spapr_watchdog.c | 248 ++++++++++++++++++++++++++++
hw/watchdog/meson.build | 1 +
hw/watchdog/trace-events | 7 +
10 files changed, 293 insertions(+), 73 deletions(-)
create mode 100644 hw/watchdog/spapr_watchdog.c
--
2.30.2
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target
2022-06-17 6:07 [PATCH qemu v2 0/2] ppc/spapr: Implement H_WATCHDOG Alexey Kardashevskiy
@ 2022-06-17 6:07 ` Alexey Kardashevskiy
2022-06-17 16:50 ` Daniel Henrique Barboza
` (3 more replies)
2022-06-17 6:07 ` [PATCH qemu v2 2/2] ppc/spapr: Implement H_WATCHDOG Alexey Kardashevskiy
2022-06-17 16:51 ` [PATCH qemu v2 0/2] " Daniel Henrique Barboza
2 siblings, 4 replies; 21+ messages in thread
From: Alexey Kardashevskiy @ 2022-06-17 6:07 UTC (permalink / raw)
To: qemu-ppc
Cc: qemu-devel, Daniel Henrique Barboza, Cédric Le Goater,
Alexey Kardashevskiy
It keeps repeating, move it to the header. This uses __builtin_ctzl() to
allow using the macros in #define.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
target/ppc/cpu.h | 5 +++++
hw/intc/pnv_xive.c | 20 --------------------
hw/intc/pnv_xive2.c | 20 --------------------
hw/pci-host/pnv_phb4.c | 16 ----------------
5 files changed, 5 insertions(+), 72 deletions(-)
diff --git a/include/hw/pci-host/pnv_phb3_regs.h b/include/hw/pci-host/pnv_phb3_regs.h
index a174ef1f7045..38f8ce9d7406 100644
--- a/include/hw/pci-host/pnv_phb3_regs.h
+++ b/include/hw/pci-host/pnv_phb3_regs.h
@@ -12,22 +12,6 @@
#include "qemu/host-utils.h"
-/*
- * QEMU version of the GETFIELD/SETFIELD macros
- *
- * These are common with the PnvXive model.
- */
-static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
-{
- return (word & mask) >> ctz64(mask);
-}
-
-static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
- uint64_t value)
-{
- return (word & ~mask) | ((value << ctz64(mask)) & mask);
-}
-
/*
* PBCQ XSCOM registers
*/
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 6d78078f379d..9a1f1e9999a3 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -47,6 +47,11 @@
PPC_BIT32(bs))
#define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs))
+#define GETFIELD(mask, word) \
+ (((word) & (mask)) >> __builtin_ctzl(mask))
+#define SETFIELD(mask, word, val) \
+ (((word) & ~(mask)) | (((uint64_t)(val) << __builtin_ctzl(mask)) & (mask)))
+
/*****************************************************************************/
/* Exception vectors definitions */
enum {
diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
index 1ce1d7b07d63..c7b75ed12ee0 100644
--- a/hw/intc/pnv_xive.c
+++ b/hw/intc/pnv_xive.c
@@ -66,26 +66,6 @@ static const XiveVstInfo vst_infos[] = {
qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n", \
(xive)->chip->chip_id, ## __VA_ARGS__);
-/*
- * QEMU version of the GETFIELD/SETFIELD macros
- *
- * TODO: It might be better to use the existing extract64() and
- * deposit64() but this means that all the register definitions will
- * change and become incompatible with the ones found in skiboot.
- *
- * Keep it as it is for now until we find a common ground.
- */
-static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
-{
- return (word & mask) >> ctz64(mask);
-}
-
-static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
- uint64_t value)
-{
- return (word & ~mask) | ((value << ctz64(mask)) & mask);
-}
-
/*
* When PC_TCTXT_CHIPID_OVERRIDE is configured, the PC_TCTXT_CHIPID
* field overrides the hardwired chip ID in the Powerbus operations
diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
index a39e070e82d2..3fe349749384 100644
--- a/hw/intc/pnv_xive2.c
+++ b/hw/intc/pnv_xive2.c
@@ -75,26 +75,6 @@ static const XiveVstInfo vst_infos[] = {
qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n", \
(xive)->chip->chip_id, ## __VA_ARGS__);
-/*
- * QEMU version of the GETFIELD/SETFIELD macros
- *
- * TODO: It might be better to use the existing extract64() and
- * deposit64() but this means that all the register definitions will
- * change and become incompatible with the ones found in skiboot.
- *
- * Keep it as it is for now until we find a common ground.
- */
-static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
-{
- return (word & mask) >> ctz64(mask);
-}
-
-static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
- uint64_t value)
-{
- return (word & ~mask) | ((value << ctz64(mask)) & mask);
-}
-
/*
* TODO: Document block id override
*/
diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 13ba9e45d8b6..0913e7c8f015 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -31,22 +31,6 @@
qemu_log_mask(LOG_GUEST_ERROR, "phb4_pec[%d:%d]: " fmt "\n", \
(pec)->chip_id, (pec)->index, ## __VA_ARGS__)
-/*
- * QEMU version of the GETFIELD/SETFIELD macros
- *
- * These are common with the PnvXive model.
- */
-static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
-{
- return (word & mask) >> ctz64(mask);
-}
-
-static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
- uint64_t value)
-{
- return (word & ~mask) | ((value << ctz64(mask)) & mask);
-}
-
static PCIDevice *pnv_phb4_find_cfg_dev(PnvPHB4 *phb)
{
PCIHostState *pci = PCI_HOST_BRIDGE(phb);
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH qemu v2 2/2] ppc/spapr: Implement H_WATCHDOG
2022-06-17 6:07 [PATCH qemu v2 0/2] ppc/spapr: Implement H_WATCHDOG Alexey Kardashevskiy
2022-06-17 6:07 ` [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target Alexey Kardashevskiy
@ 2022-06-17 6:07 ` Alexey Kardashevskiy
2022-06-17 16:49 ` Daniel Henrique Barboza
2022-06-18 11:01 ` Cédric Le Goater
2022-06-17 16:51 ` [PATCH qemu v2 0/2] " Daniel Henrique Barboza
2 siblings, 2 replies; 21+ messages in thread
From: Alexey Kardashevskiy @ 2022-06-17 6:07 UTC (permalink / raw)
To: qemu-ppc
Cc: qemu-devel, Daniel Henrique Barboza, Cédric Le Goater,
Alexey Kardashevskiy
The new PAPR 2.12 defines a watchdog facility managed via the new
H_WATCHDOG hypercall.
This adds H_WATCHDOG support which a proposed driver for pseries uses:
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=303120
This was tested by running QEMU with a debug kernel and command line:
-append \
"pseries-wdt.timeout=60 pseries-wdt.nowayout=1 pseries-wdt.action=2"
and running "echo V > /dev/watchdog0" inside the VM.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v2:
* QOM'ed timers, "action" and "expire" are available via QMP
* removed @timeout from SpaprWatchdog
* moved the driver to hw/watchdog
* fixed error handling in the hcall handler
* used new SETFIELD/GETFIELD
---
include/hw/ppc/spapr.h | 29 +++-
hw/ppc/spapr.c | 4 +
hw/watchdog/spapr_watchdog.c | 248 +++++++++++++++++++++++++++++++++++
hw/watchdog/meson.build | 1 +
hw/watchdog/trace-events | 7 +
5 files changed, 288 insertions(+), 1 deletion(-)
create mode 100644 hw/watchdog/spapr_watchdog.c
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 072dda2c7265..ef1e38abd5c7 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -164,6 +164,25 @@ struct SpaprMachineClass {
SpaprIrq *irq;
};
+#define WDT_MAX_WATCHDOGS 4 /* Maximum number of watchdog devices */
+
+#define WDT_HARD_POWER_OFF 0
+#define WDT_HARD_RESTART 1
+#define WDT_DUMP_RESTART 2
+
+#define TYPE_SPAPR_WDT "spapr-wdt"
+OBJECT_DECLARE_SIMPLE_TYPE(SpaprWatchdog, SPAPR_WDT)
+
+typedef struct SpaprWatchdog {
+ /*< private >*/
+ DeviceState parent_obj;
+ /*< public >*/
+
+ unsigned num;
+ QEMUTimer timer;
+ uint8_t action;
+} SpaprWatchdog;
+
/**
* SpaprMachineState:
*/
@@ -264,6 +283,8 @@ struct SpaprMachineState {
uint32_t FORM2_assoc_array[NUMA_NODES_MAX_NUM][FORM2_NUMA_ASSOC_SIZE];
Error *fwnmi_migration_blocker;
+
+ SpaprWatchdog wds[WDT_MAX_WATCHDOGS];
};
#define H_SUCCESS 0
@@ -344,6 +365,7 @@ struct SpaprMachineState {
#define H_P7 -60
#define H_P8 -61
#define H_P9 -62
+#define H_NOOP -63
#define H_UNSUPPORTED -67
#define H_OVERLAP -68
#define H_UNSUPPORTED_FLAG -256
@@ -564,8 +586,9 @@ struct SpaprMachineState {
#define H_SCM_HEALTH 0x400
#define H_RPT_INVALIDATE 0x448
#define H_SCM_FLUSH 0x44C
+#define H_WATCHDOG 0x45C
-#define MAX_HCALL_OPCODE H_SCM_FLUSH
+#define MAX_HCALL_OPCODE H_WATCHDOG
/* The hcalls above are standardized in PAPR and implemented by pHyp
* as well.
@@ -1027,6 +1050,7 @@ extern const VMStateDescription vmstate_spapr_cap_large_decr;
extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
extern const VMStateDescription vmstate_spapr_cap_fwnmi;
extern const VMStateDescription vmstate_spapr_cap_rpt_invalidate;
+extern const VMStateDescription vmstate_spapr_wdt;
static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
{
@@ -1063,4 +1087,7 @@ target_ulong spapr_vof_client_architecture_support(MachineState *ms,
target_ulong ovec_addr);
void spapr_vof_client_dt_finalize(SpaprMachineState *spapr, void *fdt);
+/* H_WATCHDOG */
+void spapr_watchdog_init(SpaprMachineState *spapr);
+
#endif /* HW_SPAPR_H */
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index fd4942e8813c..9a5382d5270f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -898,6 +898,8 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
add_str(hypertas, "hcall-hpt-resize");
}
+ add_str(hypertas, "hcall-watchdog");
+
_FDT(fdt_setprop(fdt, rtas, "ibm,hypertas-functions",
hypertas->str, hypertas->len));
g_string_free(hypertas, TRUE);
@@ -3051,6 +3053,8 @@ static void spapr_machine_init(MachineState *machine)
spapr->vof->fw_size = fw_size; /* for claim() on itself */
spapr_register_hypercall(KVMPPC_H_VOF_CLIENT, spapr_h_vof_client);
}
+
+ spapr_watchdog_init(spapr);
}
#define DEFAULT_KVM_TYPE "auto"
diff --git a/hw/watchdog/spapr_watchdog.c b/hw/watchdog/spapr_watchdog.c
new file mode 100644
index 000000000000..aeaf7c52cbad
--- /dev/null
+++ b/hw/watchdog/spapr_watchdog.c
@@ -0,0 +1,248 @@
+/*
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "target/ppc/cpu.h"
+#include "migration/vmstate.h"
+#include "trace.h"
+
+#include "hw/ppc/spapr.h"
+
+/*
+ * Bits 47: "leaveOtherWatchdogsRunningOnTimeout", specified on
+ * the "Start watchdog" operation,
+ * 0 - stop out-standing watchdogs on timeout,
+ * 1 - leave outstanding watchdogs running on timeout
+ */
+#define PSERIES_WDTF_LEAVE_OTHER PPC_BIT(47)
+
+/* Bits 48-55: "operation" */
+#define PSERIES_WDTF_OP(op) SETFIELD(PPC_BITMASK(48, 55), 0, (op))
+#define PSERIES_WDTF_OP_START PSERIES_WDTF_OP(0x1)
+#define PSERIES_WDTF_OP_STOP PSERIES_WDTF_OP(0x2)
+#define PSERIES_WDTF_OP_QUERY PSERIES_WDTF_OP(0x3)
+#define PSERIES_WDTF_OP_QUERY_LPM PSERIES_WDTF_OP(0x4)
+
+/* Bits 56-63: "timeoutAction" */
+#define PSERIES_WDTF_ACTION(ac) SETFIELD(PPC_BITMASK(56, 63), 0, (ac))
+#define PSERIES_WDTF_ACTION_HARD_POWER_OFF PSERIES_WDTF_ACTION(0x1)
+#define PSERIES_WDTF_ACTION_HARD_RESTART PSERIES_WDTF_ACTION(0x2)
+#define PSERIES_WDTF_ACTION_DUMP_RESTART PSERIES_WDTF_ACTION(0x3)
+#define PSERIES_WDTF_RESERVED PPC_BITMASK(0, 46)
+
+/*
+ * For the "Query watchdog capabilities" operation, a uint64 structure
+ * defined as:
+ * Bits 0-15: The minimum supported timeout in milliseconds
+ * Bits 16-31: The number of watchdogs supported
+ * Bits 32-63: Reserved
+ */
+#define PSERIES_WDTQ_MIN_TIMEOUT(ms) SETFIELD(PPC_BITMASK(0, 15), 0, (ms))
+#define PSERIES_WDTQ_NUM(n) SETFIELD(PPC_BITMASK(16, 31), 0, (n))
+
+/*
+ * For the "Query watchdog LPM requirement" operation:
+ * 1 = The given "watchdogNumber" must be stopped prior to suspending
+ * 2 = The given "watchdogNumber" does not have to be stopped prior to
+ * suspending
+ */
+#define PSERIES_WDTQL_STOPPED 1
+#define PSERIES_WDTQL_QUERY_NOT_STOPPED 2
+
+#define WDT_MIN_TIMEOUT 1 /* 1ms */
+
+static void watchdog_expired(void *pw)
+{
+ struct SpaprWatchdog *w = pw;
+ CPUState *cs;
+
+ trace_spapr_watchdog_expired(w->num, w->action);
+ switch (w->action) {
+ case WDT_HARD_POWER_OFF:
+ qemu_system_vmstop_request(RUN_STATE_SHUTDOWN);
+ break;
+ case WDT_HARD_RESTART:
+ qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+ break;
+ case WDT_DUMP_RESTART:
+ CPU_FOREACH(cs) {
+ async_run_on_cpu(cs, spapr_do_system_reset_on_cpu, RUN_ON_CPU_NULL);
+ }
+ break;
+ }
+}
+
+static target_ulong watchdog_stop(unsigned watchdogNumber,
+ struct SpaprWatchdog *w)
+{
+ target_ulong ret = H_NOOP;
+
+ if (timer_pending(&w->timer)) {
+ timer_del(&w->timer);
+ ret = H_SUCCESS;
+ }
+ trace_spapr_watchdog_stop(watchdogNumber, ret);
+
+ return ret;
+}
+
+static target_ulong h_watchdog(PowerPCCPU *cpu,
+ SpaprMachineState *spapr,
+ target_ulong opcode, target_ulong *args)
+{
+ target_ulong flags = args[0];
+ target_ulong watchdogNumber = args[1];
+ target_ulong timeoutInMs = args[2];
+ unsigned operation = flags & PSERIES_WDTF_OP(~0);
+ unsigned timeoutAction = flags & PSERIES_WDTF_ACTION(~0);
+ struct SpaprWatchdog *w;
+
+ if (flags & PSERIES_WDTF_RESERVED) {
+ return H_PARAMETER;
+ }
+
+ switch (operation) {
+ case PSERIES_WDTF_OP_START:
+ if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
+ return H_P2;
+ }
+ if (timeoutInMs <= WDT_MIN_TIMEOUT) {
+ return H_P3;
+ }
+
+ w = &spapr->wds[watchdogNumber - 1];
+ switch (timeoutAction) {
+ case PSERIES_WDTF_ACTION_HARD_POWER_OFF:
+ w->action = WDT_HARD_POWER_OFF;
+ break;
+ case PSERIES_WDTF_ACTION_HARD_RESTART:
+ w->action = WDT_HARD_RESTART;
+ break;
+ case PSERIES_WDTF_ACTION_DUMP_RESTART:
+ w->action = WDT_DUMP_RESTART;
+ break;
+ default:
+ return H_PARAMETER;
+ }
+ timer_mod(&w->timer,
+ qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + timeoutInMs);
+ trace_spapr_watchdog_start(flags, watchdogNumber, timeoutInMs);
+ break;
+ case PSERIES_WDTF_OP_STOP:
+ if (watchdogNumber == (uint64_t) ~0) {
+ int i;
+
+ for (i = 1; i <= ARRAY_SIZE(spapr->wds); ++i) {
+ watchdog_stop(i, &spapr->wds[i - 1]);
+ }
+ } else if (watchdogNumber <= ARRAY_SIZE(spapr->wds)) {
+ watchdog_stop(watchdogNumber, &spapr->wds[watchdogNumber - 1]);
+ } else {
+ return H_P2;
+ }
+ break;
+ case PSERIES_WDTF_OP_QUERY:
+ args[0] = PSERIES_WDTQ_MIN_TIMEOUT(WDT_MIN_TIMEOUT) |
+ PSERIES_WDTQ_NUM(ARRAY_SIZE(spapr->wds));
+ trace_spapr_watchdog_query(args[0]);
+ break;
+ case PSERIES_WDTF_OP_QUERY_LPM:
+ if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
+ return H_P2;
+ }
+ args[0] = PSERIES_WDTQL_QUERY_NOT_STOPPED;
+ trace_spapr_watchdog_query_lpm(args[0]);
+ break;
+ default:
+ return H_PARAMETER;
+ }
+
+ return H_SUCCESS;
+}
+
+void spapr_watchdog_init(SpaprMachineState *spapr)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(spapr->wds); ++i) {
+ char name[16];
+ SpaprWatchdog *w = &spapr->wds[i];
+
+ w->num = i + 1;
+ snprintf(name, sizeof(name) - 1, "wdt%d", i + 1);
+ object_initialize_child_with_props(OBJECT(spapr), name, w,
+ sizeof(SpaprWatchdog),
+ TYPE_SPAPR_WDT,
+ &error_fatal, NULL);
+ qdev_realize(DEVICE(w), NULL, &error_fatal);
+ }
+}
+
+static bool watchdog_needed(void *opaque)
+{
+ SpaprWatchdog *w = opaque;
+
+ return timer_pending(&w->timer);
+}
+
+static const VMStateDescription vmstate_wdt = {
+ .name = "spapr_watchdog",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = watchdog_needed,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8(action, SpaprWatchdog),
+ VMSTATE_TIMER(timer, SpaprWatchdog),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static void spapr_wdt_realize(DeviceState *dev, Error **errp)
+{
+ SpaprWatchdog *w = SPAPR_WDT(dev);
+
+ timer_init_ms(&w->timer, QEMU_CLOCK_VIRTUAL, watchdog_expired, w);
+
+ object_property_add_uint64_ptr(OBJECT(dev), "expire",
+ (uint64_t *)&w->timer.expire_time,
+ OBJ_PROP_FLAG_READ);
+ object_property_add_uint8_ptr(OBJECT(dev), "action", &w->action,
+ OBJ_PROP_FLAG_READ);
+}
+
+static void spapr_wdt_class_init(ObjectClass *oc, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(oc);
+
+ dc->realize = spapr_wdt_realize;
+ dc->vmsd = &vmstate_wdt;
+ dc->user_creatable = false;
+}
+
+static const TypeInfo spapr_wdt_info = {
+ .name = TYPE_SPAPR_WDT,
+ .parent = TYPE_DEVICE,
+ .instance_size = sizeof(SpaprWatchdog),
+ .class_init = spapr_wdt_class_init,
+};
+
+static void spapr_watchdog_register_types(void)
+{
+ spapr_register_hypercall(H_WATCHDOG, h_watchdog);
+ type_register_static(&spapr_wdt_info);
+}
+
+type_init(spapr_watchdog_register_types)
diff --git a/hw/watchdog/meson.build b/hw/watchdog/meson.build
index 054c403dea7c..8974b5cf4c8a 100644
--- a/hw/watchdog/meson.build
+++ b/hw/watchdog/meson.build
@@ -6,3 +6,4 @@ softmmu_ss.add(when: 'CONFIG_WDT_DIAG288', if_true: files('wdt_diag288.c'))
softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('wdt_aspeed.c'))
softmmu_ss.add(when: 'CONFIG_WDT_IMX2', if_true: files('wdt_imx2.c'))
softmmu_ss.add(when: 'CONFIG_WDT_SBSA', if_true: files('sbsa_gwdt.c'))
+specific_ss.add(when: 'CONFIG_PSERIES', if_true: files('spapr_watchdog.c'))
diff --git a/hw/watchdog/trace-events b/hw/watchdog/trace-events
index e7523e22aaf2..89ccbcfdfd20 100644
--- a/hw/watchdog/trace-events
+++ b/hw/watchdog/trace-events
@@ -9,3 +9,10 @@ cmsdk_apb_watchdog_lock(uint32_t lock) "CMSDK APB watchdog: lock %" PRIu32
# wdt-aspeed.c
aspeed_wdt_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
aspeed_wdt_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
+
+# spapr_watchdog.c
+spapr_watchdog_start(uint64_t flags, uint64_t num, uint64_t timeout) "Flags 0x%" PRIx64 " num=%" PRId64 " %" PRIu64 "ms"
+spapr_watchdog_stop(uint64_t num, uint64_t ret) "num=%" PRIu64 " ret=%" PRId64
+spapr_watchdog_query(uint64_t caps) "caps=0x%" PRIx64
+spapr_watchdog_query_lpm(uint64_t caps) "caps=0x%" PRIx64
+spapr_watchdog_expired(uint64_t num, unsigned action) "num=%" PRIu64 " action=%u"
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH qemu v2 2/2] ppc/spapr: Implement H_WATCHDOG
2022-06-17 6:07 ` [PATCH qemu v2 2/2] ppc/spapr: Implement H_WATCHDOG Alexey Kardashevskiy
@ 2022-06-17 16:49 ` Daniel Henrique Barboza
2022-06-18 11:01 ` Cédric Le Goater
1 sibling, 0 replies; 21+ messages in thread
From: Daniel Henrique Barboza @ 2022-06-17 16:49 UTC (permalink / raw)
To: Alexey Kardashevskiy, qemu-ppc; +Cc: qemu-devel, Cédric Le Goater
On 6/17/22 03:07, Alexey Kardashevskiy wrote:
> The new PAPR 2.12 defines a watchdog facility managed via the new
> H_WATCHDOG hypercall.
>
> This adds H_WATCHDOG support which a proposed driver for pseries uses:
> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=303120
>
> This was tested by running QEMU with a debug kernel and command line:
> -append \
> "pseries-wdt.timeout=60 pseries-wdt.nowayout=1 pseries-wdt.action=2"
>
> and running "echo V > /dev/watchdog0" inside the VM.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
LGTM. The watchdogs can be found under /machines/wdtN:
(qemu) info qom-tree /machine/wdt1
/wdt1 (spapr-wdt)
(qemu) info qom-tree /machine/wdt2
/wdt2 (spapr-wdt)
(qemu) info qom-tree /machine/wdt3
/wdt3 (spapr-wdt)
(qemu) info qom-tree /machine/wdt4
/wdt4 (spapr-wdt)
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Changes:
> v2:
> * QOM'ed timers, "action" and "expire" are available via QMP
> * removed @timeout from SpaprWatchdog
> * moved the driver to hw/watchdog
> * fixed error handling in the hcall handler
> * used new SETFIELD/GETFIELD
> ---
> include/hw/ppc/spapr.h | 29 +++-
> hw/ppc/spapr.c | 4 +
> hw/watchdog/spapr_watchdog.c | 248 +++++++++++++++++++++++++++++++++++
> hw/watchdog/meson.build | 1 +
> hw/watchdog/trace-events | 7 +
> 5 files changed, 288 insertions(+), 1 deletion(-)
> create mode 100644 hw/watchdog/spapr_watchdog.c
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 072dda2c7265..ef1e38abd5c7 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -164,6 +164,25 @@ struct SpaprMachineClass {
> SpaprIrq *irq;
> };
>
> +#define WDT_MAX_WATCHDOGS 4 /* Maximum number of watchdog devices */
> +
> +#define WDT_HARD_POWER_OFF 0
> +#define WDT_HARD_RESTART 1
> +#define WDT_DUMP_RESTART 2
> +
> +#define TYPE_SPAPR_WDT "spapr-wdt"
> +OBJECT_DECLARE_SIMPLE_TYPE(SpaprWatchdog, SPAPR_WDT)
> +
> +typedef struct SpaprWatchdog {
> + /*< private >*/
> + DeviceState parent_obj;
> + /*< public >*/
> +
> + unsigned num;
> + QEMUTimer timer;
> + uint8_t action;
> +} SpaprWatchdog;
> +
> /**
> * SpaprMachineState:
> */
> @@ -264,6 +283,8 @@ struct SpaprMachineState {
> uint32_t FORM2_assoc_array[NUMA_NODES_MAX_NUM][FORM2_NUMA_ASSOC_SIZE];
>
> Error *fwnmi_migration_blocker;
> +
> + SpaprWatchdog wds[WDT_MAX_WATCHDOGS];
> };
>
> #define H_SUCCESS 0
> @@ -344,6 +365,7 @@ struct SpaprMachineState {
> #define H_P7 -60
> #define H_P8 -61
> #define H_P9 -62
> +#define H_NOOP -63
> #define H_UNSUPPORTED -67
> #define H_OVERLAP -68
> #define H_UNSUPPORTED_FLAG -256
> @@ -564,8 +586,9 @@ struct SpaprMachineState {
> #define H_SCM_HEALTH 0x400
> #define H_RPT_INVALIDATE 0x448
> #define H_SCM_FLUSH 0x44C
> +#define H_WATCHDOG 0x45C
>
> -#define MAX_HCALL_OPCODE H_SCM_FLUSH
> +#define MAX_HCALL_OPCODE H_WATCHDOG
>
> /* The hcalls above are standardized in PAPR and implemented by pHyp
> * as well.
> @@ -1027,6 +1050,7 @@ extern const VMStateDescription vmstate_spapr_cap_large_decr;
> extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
> extern const VMStateDescription vmstate_spapr_cap_fwnmi;
> extern const VMStateDescription vmstate_spapr_cap_rpt_invalidate;
> +extern const VMStateDescription vmstate_spapr_wdt;
>
> static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
> {
> @@ -1063,4 +1087,7 @@ target_ulong spapr_vof_client_architecture_support(MachineState *ms,
> target_ulong ovec_addr);
> void spapr_vof_client_dt_finalize(SpaprMachineState *spapr, void *fdt);
>
> +/* H_WATCHDOG */
> +void spapr_watchdog_init(SpaprMachineState *spapr);
> +
> #endif /* HW_SPAPR_H */
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fd4942e8813c..9a5382d5270f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -898,6 +898,8 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
> add_str(hypertas, "hcall-hpt-resize");
> }
>
> + add_str(hypertas, "hcall-watchdog");
> +
> _FDT(fdt_setprop(fdt, rtas, "ibm,hypertas-functions",
> hypertas->str, hypertas->len));
> g_string_free(hypertas, TRUE);
> @@ -3051,6 +3053,8 @@ static void spapr_machine_init(MachineState *machine)
> spapr->vof->fw_size = fw_size; /* for claim() on itself */
> spapr_register_hypercall(KVMPPC_H_VOF_CLIENT, spapr_h_vof_client);
> }
> +
> + spapr_watchdog_init(spapr);
> }
>
> #define DEFAULT_KVM_TYPE "auto"
> diff --git a/hw/watchdog/spapr_watchdog.c b/hw/watchdog/spapr_watchdog.c
> new file mode 100644
> index 000000000000..aeaf7c52cbad
> --- /dev/null
> +++ b/hw/watchdog/spapr_watchdog.c
> @@ -0,0 +1,248 @@
> +/*
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library 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
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "target/ppc/cpu.h"
> +#include "migration/vmstate.h"
> +#include "trace.h"
> +
> +#include "hw/ppc/spapr.h"
> +
> +/*
> + * Bits 47: "leaveOtherWatchdogsRunningOnTimeout", specified on
> + * the "Start watchdog" operation,
> + * 0 - stop out-standing watchdogs on timeout,
> + * 1 - leave outstanding watchdogs running on timeout
> + */
> +#define PSERIES_WDTF_LEAVE_OTHER PPC_BIT(47)
> +
> +/* Bits 48-55: "operation" */
> +#define PSERIES_WDTF_OP(op) SETFIELD(PPC_BITMASK(48, 55), 0, (op))
> +#define PSERIES_WDTF_OP_START PSERIES_WDTF_OP(0x1)
> +#define PSERIES_WDTF_OP_STOP PSERIES_WDTF_OP(0x2)
> +#define PSERIES_WDTF_OP_QUERY PSERIES_WDTF_OP(0x3)
> +#define PSERIES_WDTF_OP_QUERY_LPM PSERIES_WDTF_OP(0x4)
> +
> +/* Bits 56-63: "timeoutAction" */
> +#define PSERIES_WDTF_ACTION(ac) SETFIELD(PPC_BITMASK(56, 63), 0, (ac))
> +#define PSERIES_WDTF_ACTION_HARD_POWER_OFF PSERIES_WDTF_ACTION(0x1)
> +#define PSERIES_WDTF_ACTION_HARD_RESTART PSERIES_WDTF_ACTION(0x2)
> +#define PSERIES_WDTF_ACTION_DUMP_RESTART PSERIES_WDTF_ACTION(0x3)
> +#define PSERIES_WDTF_RESERVED PPC_BITMASK(0, 46)
> +
> +/*
> + * For the "Query watchdog capabilities" operation, a uint64 structure
> + * defined as:
> + * Bits 0-15: The minimum supported timeout in milliseconds
> + * Bits 16-31: The number of watchdogs supported
> + * Bits 32-63: Reserved
> + */
> +#define PSERIES_WDTQ_MIN_TIMEOUT(ms) SETFIELD(PPC_BITMASK(0, 15), 0, (ms))
> +#define PSERIES_WDTQ_NUM(n) SETFIELD(PPC_BITMASK(16, 31), 0, (n))
> +
> +/*
> + * For the "Query watchdog LPM requirement" operation:
> + * 1 = The given "watchdogNumber" must be stopped prior to suspending
> + * 2 = The given "watchdogNumber" does not have to be stopped prior to
> + * suspending
> + */
> +#define PSERIES_WDTQL_STOPPED 1
> +#define PSERIES_WDTQL_QUERY_NOT_STOPPED 2
> +
> +#define WDT_MIN_TIMEOUT 1 /* 1ms */
> +
> +static void watchdog_expired(void *pw)
> +{
> + struct SpaprWatchdog *w = pw;
> + CPUState *cs;
> +
> + trace_spapr_watchdog_expired(w->num, w->action);
> + switch (w->action) {
> + case WDT_HARD_POWER_OFF:
> + qemu_system_vmstop_request(RUN_STATE_SHUTDOWN);
> + break;
> + case WDT_HARD_RESTART:
> + qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> + break;
> + case WDT_DUMP_RESTART:
> + CPU_FOREACH(cs) {
> + async_run_on_cpu(cs, spapr_do_system_reset_on_cpu, RUN_ON_CPU_NULL);
> + }
> + break;
> + }
> +}
> +
> +static target_ulong watchdog_stop(unsigned watchdogNumber,
> + struct SpaprWatchdog *w)
> +{
> + target_ulong ret = H_NOOP;
> +
> + if (timer_pending(&w->timer)) {
> + timer_del(&w->timer);
> + ret = H_SUCCESS;
> + }
> + trace_spapr_watchdog_stop(watchdogNumber, ret);
> +
> + return ret;
> +}
> +
> +static target_ulong h_watchdog(PowerPCCPU *cpu,
> + SpaprMachineState *spapr,
> + target_ulong opcode, target_ulong *args)
> +{
> + target_ulong flags = args[0];
> + target_ulong watchdogNumber = args[1];
> + target_ulong timeoutInMs = args[2];
> + unsigned operation = flags & PSERIES_WDTF_OP(~0);
> + unsigned timeoutAction = flags & PSERIES_WDTF_ACTION(~0);
> + struct SpaprWatchdog *w;
> +
> + if (flags & PSERIES_WDTF_RESERVED) {
> + return H_PARAMETER;
> + }
> +
> + switch (operation) {
> + case PSERIES_WDTF_OP_START:
> + if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
> + return H_P2;
> + }
> + if (timeoutInMs <= WDT_MIN_TIMEOUT) {
> + return H_P3;
> + }
> +
> + w = &spapr->wds[watchdogNumber - 1];
> + switch (timeoutAction) {
> + case PSERIES_WDTF_ACTION_HARD_POWER_OFF:
> + w->action = WDT_HARD_POWER_OFF;
> + break;
> + case PSERIES_WDTF_ACTION_HARD_RESTART:
> + w->action = WDT_HARD_RESTART;
> + break;
> + case PSERIES_WDTF_ACTION_DUMP_RESTART:
> + w->action = WDT_DUMP_RESTART;
> + break;
> + default:
> + return H_PARAMETER;
> + }
> + timer_mod(&w->timer,
> + qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + timeoutInMs);
> + trace_spapr_watchdog_start(flags, watchdogNumber, timeoutInMs);
> + break;
> + case PSERIES_WDTF_OP_STOP:
> + if (watchdogNumber == (uint64_t) ~0) {
> + int i;
> +
> + for (i = 1; i <= ARRAY_SIZE(spapr->wds); ++i) {
> + watchdog_stop(i, &spapr->wds[i - 1]);
> + }
> + } else if (watchdogNumber <= ARRAY_SIZE(spapr->wds)) {
> + watchdog_stop(watchdogNumber, &spapr->wds[watchdogNumber - 1]);
> + } else {
> + return H_P2;
> + }
> + break;
> + case PSERIES_WDTF_OP_QUERY:
> + args[0] = PSERIES_WDTQ_MIN_TIMEOUT(WDT_MIN_TIMEOUT) |
> + PSERIES_WDTQ_NUM(ARRAY_SIZE(spapr->wds));
> + trace_spapr_watchdog_query(args[0]);
> + break;
> + case PSERIES_WDTF_OP_QUERY_LPM:
> + if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
> + return H_P2;
> + }
> + args[0] = PSERIES_WDTQL_QUERY_NOT_STOPPED;
> + trace_spapr_watchdog_query_lpm(args[0]);
> + break;
> + default:
> + return H_PARAMETER;
> + }
> +
> + return H_SUCCESS;
> +}
> +
> +void spapr_watchdog_init(SpaprMachineState *spapr)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(spapr->wds); ++i) {
> + char name[16];
> + SpaprWatchdog *w = &spapr->wds[i];
> +
> + w->num = i + 1;
> + snprintf(name, sizeof(name) - 1, "wdt%d", i + 1);
> + object_initialize_child_with_props(OBJECT(spapr), name, w,
> + sizeof(SpaprWatchdog),
> + TYPE_SPAPR_WDT,
> + &error_fatal, NULL);
> + qdev_realize(DEVICE(w), NULL, &error_fatal);
> + }
> +}
> +
> +static bool watchdog_needed(void *opaque)
> +{
> + SpaprWatchdog *w = opaque;
> +
> + return timer_pending(&w->timer);
> +}
> +
> +static const VMStateDescription vmstate_wdt = {
> + .name = "spapr_watchdog",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = watchdog_needed,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT8(action, SpaprWatchdog),
> + VMSTATE_TIMER(timer, SpaprWatchdog),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static void spapr_wdt_realize(DeviceState *dev, Error **errp)
> +{
> + SpaprWatchdog *w = SPAPR_WDT(dev);
> +
> + timer_init_ms(&w->timer, QEMU_CLOCK_VIRTUAL, watchdog_expired, w);
> +
> + object_property_add_uint64_ptr(OBJECT(dev), "expire",
> + (uint64_t *)&w->timer.expire_time,
> + OBJ_PROP_FLAG_READ);
> + object_property_add_uint8_ptr(OBJECT(dev), "action", &w->action,
> + OBJ_PROP_FLAG_READ);
> +}
> +
> +static void spapr_wdt_class_init(ObjectClass *oc, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(oc);
> +
> + dc->realize = spapr_wdt_realize;
> + dc->vmsd = &vmstate_wdt;
> + dc->user_creatable = false;
> +}
> +
> +static const TypeInfo spapr_wdt_info = {
> + .name = TYPE_SPAPR_WDT,
> + .parent = TYPE_DEVICE,
> + .instance_size = sizeof(SpaprWatchdog),
> + .class_init = spapr_wdt_class_init,
> +};
> +
> +static void spapr_watchdog_register_types(void)
> +{
> + spapr_register_hypercall(H_WATCHDOG, h_watchdog);
> + type_register_static(&spapr_wdt_info);
> +}
> +
> +type_init(spapr_watchdog_register_types)
> diff --git a/hw/watchdog/meson.build b/hw/watchdog/meson.build
> index 054c403dea7c..8974b5cf4c8a 100644
> --- a/hw/watchdog/meson.build
> +++ b/hw/watchdog/meson.build
> @@ -6,3 +6,4 @@ softmmu_ss.add(when: 'CONFIG_WDT_DIAG288', if_true: files('wdt_diag288.c'))
> softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('wdt_aspeed.c'))
> softmmu_ss.add(when: 'CONFIG_WDT_IMX2', if_true: files('wdt_imx2.c'))
> softmmu_ss.add(when: 'CONFIG_WDT_SBSA', if_true: files('sbsa_gwdt.c'))
> +specific_ss.add(when: 'CONFIG_PSERIES', if_true: files('spapr_watchdog.c'))
> diff --git a/hw/watchdog/trace-events b/hw/watchdog/trace-events
> index e7523e22aaf2..89ccbcfdfd20 100644
> --- a/hw/watchdog/trace-events
> +++ b/hw/watchdog/trace-events
> @@ -9,3 +9,10 @@ cmsdk_apb_watchdog_lock(uint32_t lock) "CMSDK APB watchdog: lock %" PRIu32
> # wdt-aspeed.c
> aspeed_wdt_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> aspeed_wdt_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
> +
> +# spapr_watchdog.c
> +spapr_watchdog_start(uint64_t flags, uint64_t num, uint64_t timeout) "Flags 0x%" PRIx64 " num=%" PRId64 " %" PRIu64 "ms"
> +spapr_watchdog_stop(uint64_t num, uint64_t ret) "num=%" PRIu64 " ret=%" PRId64
> +spapr_watchdog_query(uint64_t caps) "caps=0x%" PRIx64
> +spapr_watchdog_query_lpm(uint64_t caps) "caps=0x%" PRIx64
> +spapr_watchdog_expired(uint64_t num, unsigned action) "num=%" PRIu64 " action=%u"
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target
2022-06-17 6:07 ` [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target Alexey Kardashevskiy
@ 2022-06-17 16:50 ` Daniel Henrique Barboza
2022-06-20 3:37 ` Alexey Kardashevskiy
2022-06-18 10:36 ` Cédric Le Goater
` (2 subsequent siblings)
3 siblings, 1 reply; 21+ messages in thread
From: Daniel Henrique Barboza @ 2022-06-17 16:50 UTC (permalink / raw)
To: Alexey Kardashevskiy, qemu-ppc; +Cc: qemu-devel, Cédric Le Goater
On 6/17/22 03:07, Alexey Kardashevskiy wrote:
> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
> allow using the macros in #define.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
> target/ppc/cpu.h | 5 +++++
> hw/intc/pnv_xive.c | 20 --------------------
> hw/intc/pnv_xive2.c | 20 --------------------
> hw/pci-host/pnv_phb4.c | 16 ----------------
> 5 files changed, 5 insertions(+), 72 deletions(-)
>
> diff --git a/include/hw/pci-host/pnv_phb3_regs.h b/include/hw/pci-host/pnv_phb3_regs.h
> index a174ef1f7045..38f8ce9d7406 100644
> --- a/include/hw/pci-host/pnv_phb3_regs.h
> +++ b/include/hw/pci-host/pnv_phb3_regs.h
> @@ -12,22 +12,6 @@
>
> #include "qemu/host-utils.h"
>
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * These are common with the PnvXive model.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> - return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> - uint64_t value)
> -{
> - return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
> /*
> * PBCQ XSCOM registers
> */
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 6d78078f379d..9a1f1e9999a3 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -47,6 +47,11 @@
> PPC_BIT32(bs))
> #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs))
>
> +#define GETFIELD(mask, word) \
> + (((word) & (mask)) >> __builtin_ctzl(mask))
> +#define SETFIELD(mask, word, val) \
> + (((word) & ~(mask)) | (((uint64_t)(val) << __builtin_ctzl(mask)) & (mask)))
> +
> /*****************************************************************************/
> /* Exception vectors definitions */
> enum {
> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> index 1ce1d7b07d63..c7b75ed12ee0 100644
> --- a/hw/intc/pnv_xive.c
> +++ b/hw/intc/pnv_xive.c
> @@ -66,26 +66,6 @@ static const XiveVstInfo vst_infos[] = {
> qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n", \
> (xive)->chip->chip_id, ## __VA_ARGS__);
>
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * TODO: It might be better to use the existing extract64() and
> - * deposit64() but this means that all the register definitions will
> - * change and become incompatible with the ones found in skiboot.
> - *
> - * Keep it as it is for now until we find a common ground.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> - return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> - uint64_t value)
> -{
> - return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
> /*
> * When PC_TCTXT_CHIPID_OVERRIDE is configured, the PC_TCTXT_CHIPID
> * field overrides the hardwired chip ID in the Powerbus operations
> diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
> index a39e070e82d2..3fe349749384 100644
> --- a/hw/intc/pnv_xive2.c
> +++ b/hw/intc/pnv_xive2.c
> @@ -75,26 +75,6 @@ static const XiveVstInfo vst_infos[] = {
> qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n", \
> (xive)->chip->chip_id, ## __VA_ARGS__);
>
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * TODO: It might be better to use the existing extract64() and
> - * deposit64() but this means that all the register definitions will
> - * change and become incompatible with the ones found in skiboot.
> - *
> - * Keep it as it is for now until we find a common ground.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> - return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> - uint64_t value)
> -{
> - return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
> /*
> * TODO: Document block id override
> */
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 13ba9e45d8b6..0913e7c8f015 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -31,22 +31,6 @@
> qemu_log_mask(LOG_GUEST_ERROR, "phb4_pec[%d:%d]: " fmt "\n", \
> (pec)->chip_id, (pec)->index, ## __VA_ARGS__)
>
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * These are common with the PnvXive model.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> - return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> - uint64_t value)
> -{
> - return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
> static PCIDevice *pnv_phb4_find_cfg_dev(PnvPHB4 *phb)
> {
> PCIHostState *pci = PCI_HOST_BRIDGE(phb);
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH qemu v2 0/2] ppc/spapr: Implement H_WATCHDOG
2022-06-17 6:07 [PATCH qemu v2 0/2] ppc/spapr: Implement H_WATCHDOG Alexey Kardashevskiy
2022-06-17 6:07 ` [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target Alexey Kardashevskiy
2022-06-17 6:07 ` [PATCH qemu v2 2/2] ppc/spapr: Implement H_WATCHDOG Alexey Kardashevskiy
@ 2022-06-17 16:51 ` Daniel Henrique Barboza
2 siblings, 0 replies; 21+ messages in thread
From: Daniel Henrique Barboza @ 2022-06-17 16:51 UTC (permalink / raw)
To: Alexey Kardashevskiy, qemu-ppc; +Cc: qemu-devel, Cédric Le Goater
On 6/17/22 03:07, Alexey Kardashevskiy wrote:
> This implements H_WATCHDOG. More detailed comments are in the patches.
>
> This is based on sha1
> 96c343cc774b Joel Stanley "linux-user: Add PowerPC ISA 3.1 and MMA to hwcap".
>
> Please comment. Thanks.
This version worked with the kernel side patches you mentioned in patch 2/2,
thanks. Also tested migrating the guest with the WDT active and the guest
rebooted in the destination.
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>
>
>
> Alexey Kardashevskiy (2):
> ppc: Define SETFIELD for the ppc target
> ppc/spapr: Implement H_WATCHDOG
>
> include/hw/pci-host/pnv_phb3_regs.h | 16 --
> include/hw/ppc/spapr.h | 29 +++-
> target/ppc/cpu.h | 5 +
> hw/intc/pnv_xive.c | 20 ---
> hw/intc/pnv_xive2.c | 20 ---
> hw/pci-host/pnv_phb4.c | 16 --
> hw/ppc/spapr.c | 4 +
> hw/watchdog/spapr_watchdog.c | 248 ++++++++++++++++++++++++++++
> hw/watchdog/meson.build | 1 +
> hw/watchdog/trace-events | 7 +
> 10 files changed, 293 insertions(+), 73 deletions(-)
> create mode 100644 hw/watchdog/spapr_watchdog.c
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target
2022-06-17 6:07 ` [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target Alexey Kardashevskiy
2022-06-17 16:50 ` Daniel Henrique Barboza
@ 2022-06-18 10:36 ` Cédric Le Goater
2022-06-21 12:59 ` Peter Maydell
2022-06-24 20:12 ` Daniel Henrique Barboza
3 siblings, 0 replies; 21+ messages in thread
From: Cédric Le Goater @ 2022-06-18 10:36 UTC (permalink / raw)
To: Alexey Kardashevskiy, qemu-ppc; +Cc: qemu-devel, Daniel Henrique Barboza
On 6/17/22 08:07, Alexey Kardashevskiy wrote:
> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
> allow using the macros in #define.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks Alexey,
C.
> ---
> include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
> target/ppc/cpu.h | 5 +++++
> hw/intc/pnv_xive.c | 20 --------------------
> hw/intc/pnv_xive2.c | 20 --------------------
> hw/pci-host/pnv_phb4.c | 16 ----------------
> 5 files changed, 5 insertions(+), 72 deletions(-)
>
> diff --git a/include/hw/pci-host/pnv_phb3_regs.h b/include/hw/pci-host/pnv_phb3_regs.h
> index a174ef1f7045..38f8ce9d7406 100644
> --- a/include/hw/pci-host/pnv_phb3_regs.h
> +++ b/include/hw/pci-host/pnv_phb3_regs.h
> @@ -12,22 +12,6 @@
>
> #include "qemu/host-utils.h"
>
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * These are common with the PnvXive model.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> - return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> - uint64_t value)
> -{
> - return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
> /*
> * PBCQ XSCOM registers
> */
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 6d78078f379d..9a1f1e9999a3 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -47,6 +47,11 @@
> PPC_BIT32(bs))
> #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs))
>
> +#define GETFIELD(mask, word) \
> + (((word) & (mask)) >> __builtin_ctzl(mask))
> +#define SETFIELD(mask, word, val) \
> + (((word) & ~(mask)) | (((uint64_t)(val) << __builtin_ctzl(mask)) & (mask)))
> +
> /*****************************************************************************/
> /* Exception vectors definitions */
> enum {
> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> index 1ce1d7b07d63..c7b75ed12ee0 100644
> --- a/hw/intc/pnv_xive.c
> +++ b/hw/intc/pnv_xive.c
> @@ -66,26 +66,6 @@ static const XiveVstInfo vst_infos[] = {
> qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n", \
> (xive)->chip->chip_id, ## __VA_ARGS__);
>
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * TODO: It might be better to use the existing extract64() and
> - * deposit64() but this means that all the register definitions will
> - * change and become incompatible with the ones found in skiboot.
> - *
> - * Keep it as it is for now until we find a common ground.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> - return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> - uint64_t value)
> -{
> - return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
> /*
> * When PC_TCTXT_CHIPID_OVERRIDE is configured, the PC_TCTXT_CHIPID
> * field overrides the hardwired chip ID in the Powerbus operations
> diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
> index a39e070e82d2..3fe349749384 100644
> --- a/hw/intc/pnv_xive2.c
> +++ b/hw/intc/pnv_xive2.c
> @@ -75,26 +75,6 @@ static const XiveVstInfo vst_infos[] = {
> qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n", \
> (xive)->chip->chip_id, ## __VA_ARGS__);
>
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * TODO: It might be better to use the existing extract64() and
> - * deposit64() but this means that all the register definitions will
> - * change and become incompatible with the ones found in skiboot.
> - *
> - * Keep it as it is for now until we find a common ground.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> - return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> - uint64_t value)
> -{
> - return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
> /*
> * TODO: Document block id override
> */
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 13ba9e45d8b6..0913e7c8f015 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -31,22 +31,6 @@
> qemu_log_mask(LOG_GUEST_ERROR, "phb4_pec[%d:%d]: " fmt "\n", \
> (pec)->chip_id, (pec)->index, ## __VA_ARGS__)
>
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * These are common with the PnvXive model.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> - return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> - uint64_t value)
> -{
> - return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
> static PCIDevice *pnv_phb4_find_cfg_dev(PnvPHB4 *phb)
> {
> PCIHostState *pci = PCI_HOST_BRIDGE(phb);
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH qemu v2 2/2] ppc/spapr: Implement H_WATCHDOG
2022-06-17 6:07 ` [PATCH qemu v2 2/2] ppc/spapr: Implement H_WATCHDOG Alexey Kardashevskiy
2022-06-17 16:49 ` Daniel Henrique Barboza
@ 2022-06-18 11:01 ` Cédric Le Goater
2022-06-20 3:13 ` Alexey Kardashevskiy
1 sibling, 1 reply; 21+ messages in thread
From: Cédric Le Goater @ 2022-06-18 11:01 UTC (permalink / raw)
To: Alexey Kardashevskiy, qemu-ppc; +Cc: qemu-devel, Daniel Henrique Barboza
On 6/17/22 08:07, Alexey Kardashevskiy wrote:
> The new PAPR 2.12 defines a watchdog facility managed via the new
> H_WATCHDOG hypercall.
>
> This adds H_WATCHDOG support which a proposed driver for pseries uses:
> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=303120
>
> This was tested by running QEMU with a debug kernel and command line:
> -append \
> "pseries-wdt.timeout=60 pseries-wdt.nowayout=1 pseries-wdt.action=2"
>
> and running "echo V > /dev/watchdog0" inside the VM.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v2:
> * QOM'ed timers, "action" and "expire" are available via QMP
> * removed @timeout from SpaprWatchdog
> * moved the driver to hw/watchdog
> * fixed error handling in the hcall handler
> * used new SETFIELD/GETFIELD
> ---
> include/hw/ppc/spapr.h | 29 +++-
> hw/ppc/spapr.c | 4 +
> hw/watchdog/spapr_watchdog.c | 248 +++++++++++++++++++++++++++++++++++
> hw/watchdog/meson.build | 1 +
> hw/watchdog/trace-events | 7 +
> 5 files changed, 288 insertions(+), 1 deletion(-)
> create mode 100644 hw/watchdog/spapr_watchdog.c
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 072dda2c7265..ef1e38abd5c7 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -164,6 +164,25 @@ struct SpaprMachineClass {
> SpaprIrq *irq;
> };
>
> +#define WDT_MAX_WATCHDOGS 4 /* Maximum number of watchdog devices */
> +
> +#define WDT_HARD_POWER_OFF 0
> +#define WDT_HARD_RESTART 1
> +#define WDT_DUMP_RESTART 2
> +
> +#define TYPE_SPAPR_WDT "spapr-wdt"
> +OBJECT_DECLARE_SIMPLE_TYPE(SpaprWatchdog, SPAPR_WDT)
> +
> +typedef struct SpaprWatchdog {
> + /*< private >*/
> + DeviceState parent_obj;
> + /*< public >*/
> +
> + unsigned num;
uint8_t should be enough no ? I see num is only used for trace events.
> + QEMUTimer timer;
> + uint8_t action;
> +} SpaprWatchdog;
> +
> /**
> * SpaprMachineState:
> */
> @@ -264,6 +283,8 @@ struct SpaprMachineState {
> uint32_t FORM2_assoc_array[NUMA_NODES_MAX_NUM][FORM2_NUMA_ASSOC_SIZE];
>
> Error *fwnmi_migration_blocker;
> +
> + SpaprWatchdog wds[WDT_MAX_WATCHDOGS];
> };
>
> #define H_SUCCESS 0
> @@ -344,6 +365,7 @@ struct SpaprMachineState {
> #define H_P7 -60
> #define H_P8 -61
> #define H_P9 -62
> +#define H_NOOP -63
> #define H_UNSUPPORTED -67
> #define H_OVERLAP -68
> #define H_UNSUPPORTED_FLAG -256
> @@ -564,8 +586,9 @@ struct SpaprMachineState {
> #define H_SCM_HEALTH 0x400
> #define H_RPT_INVALIDATE 0x448
> #define H_SCM_FLUSH 0x44C
> +#define H_WATCHDOG 0x45C
>
> -#define MAX_HCALL_OPCODE H_SCM_FLUSH
> +#define MAX_HCALL_OPCODE H_WATCHDOG
>
> /* The hcalls above are standardized in PAPR and implemented by pHyp
> * as well.
> @@ -1027,6 +1050,7 @@ extern const VMStateDescription vmstate_spapr_cap_large_decr;
> extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
> extern const VMStateDescription vmstate_spapr_cap_fwnmi;
> extern const VMStateDescription vmstate_spapr_cap_rpt_invalidate;
> +extern const VMStateDescription vmstate_spapr_wdt;
>
> static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
> {
> @@ -1063,4 +1087,7 @@ target_ulong spapr_vof_client_architecture_support(MachineState *ms,
> target_ulong ovec_addr);
> void spapr_vof_client_dt_finalize(SpaprMachineState *spapr, void *fdt);
>
> +/* H_WATCHDOG */
> +void spapr_watchdog_init(SpaprMachineState *spapr);
> +
> #endif /* HW_SPAPR_H */
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fd4942e8813c..9a5382d5270f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -898,6 +898,8 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
> add_str(hypertas, "hcall-hpt-resize");
> }
>
> + add_str(hypertas, "hcall-watchdog");
> +
> _FDT(fdt_setprop(fdt, rtas, "ibm,hypertas-functions",
> hypertas->str, hypertas->len));
> g_string_free(hypertas, TRUE);
> @@ -3051,6 +3053,8 @@ static void spapr_machine_init(MachineState *machine)
> spapr->vof->fw_size = fw_size; /* for claim() on itself */
> spapr_register_hypercall(KVMPPC_H_VOF_CLIENT, spapr_h_vof_client);
> }
> +
> + spapr_watchdog_init(spapr);
> }
>
> #define DEFAULT_KVM_TYPE "auto"
> diff --git a/hw/watchdog/spapr_watchdog.c b/hw/watchdog/spapr_watchdog.c
> new file mode 100644
> index 000000000000..aeaf7c52cbad
> --- /dev/null
> +++ b/hw/watchdog/spapr_watchdog.c
> @@ -0,0 +1,248 @@
> +/*
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library 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
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "target/ppc/cpu.h"
> +#include "migration/vmstate.h"
> +#include "trace.h"
> +
> +#include "hw/ppc/spapr.h"
> +
> +/*
> + * Bits 47: "leaveOtherWatchdogsRunningOnTimeout", specified on
> + * the "Start watchdog" operation,
> + * 0 - stop out-standing watchdogs on timeout,
> + * 1 - leave outstanding watchdogs running on timeout
> + */
> +#define PSERIES_WDTF_LEAVE_OTHER PPC_BIT(47)
> +
> +/* Bits 48-55: "operation" */
> +#define PSERIES_WDTF_OP(op) SETFIELD(PPC_BITMASK(48, 55), 0, (op))
> +#define PSERIES_WDTF_OP_START PSERIES_WDTF_OP(0x1)
> +#define PSERIES_WDTF_OP_STOP PSERIES_WDTF_OP(0x2)
> +#define PSERIES_WDTF_OP_QUERY PSERIES_WDTF_OP(0x3)
> +#define PSERIES_WDTF_OP_QUERY_LPM PSERIES_WDTF_OP(0x4)
> +
> +/* Bits 56-63: "timeoutAction" */
> +#define PSERIES_WDTF_ACTION(ac) SETFIELD(PPC_BITMASK(56, 63), 0, (ac))
> +#define PSERIES_WDTF_ACTION_HARD_POWER_OFF PSERIES_WDTF_ACTION(0x1)
> +#define PSERIES_WDTF_ACTION_HARD_RESTART PSERIES_WDTF_ACTION(0x2)
> +#define PSERIES_WDTF_ACTION_DUMP_RESTART PSERIES_WDTF_ACTION(0x3)
> +#define PSERIES_WDTF_RESERVED PPC_BITMASK(0, 46)
> +
> +/*
> + * For the "Query watchdog capabilities" operation, a uint64 structure
> + * defined as:
> + * Bits 0-15: The minimum supported timeout in milliseconds
> + * Bits 16-31: The number of watchdogs supported
> + * Bits 32-63: Reserved
> + */
> +#define PSERIES_WDTQ_MIN_TIMEOUT(ms) SETFIELD(PPC_BITMASK(0, 15), 0, (ms))
> +#define PSERIES_WDTQ_NUM(n) SETFIELD(PPC_BITMASK(16, 31), 0, (n))
> +
> +/*
> + * For the "Query watchdog LPM requirement" operation:
> + * 1 = The given "watchdogNumber" must be stopped prior to suspending
> + * 2 = The given "watchdogNumber" does not have to be stopped prior to
> + * suspending
> + */
> +#define PSERIES_WDTQL_STOPPED 1
> +#define PSERIES_WDTQL_QUERY_NOT_STOPPED 2
> +
> +#define WDT_MIN_TIMEOUT 1 /* 1ms */
> +
> +static void watchdog_expired(void *pw)
> +{
> + struct SpaprWatchdog *w = pw;
s/struct//
> + CPUState *cs;
> +
> + trace_spapr_watchdog_expired(w->num, w->action);
> + switch (w->action) {
> + case WDT_HARD_POWER_OFF:
> + qemu_system_vmstop_request(RUN_STATE_SHUTDOWN);
> + break;
> + case WDT_HARD_RESTART:
> + qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> + break;
> + case WDT_DUMP_RESTART:
> + CPU_FOREACH(cs) {
> + async_run_on_cpu(cs, spapr_do_system_reset_on_cpu, RUN_ON_CPU_NULL);
> + }
> + break;
> + }
> +}
> +
> +static target_ulong watchdog_stop(unsigned watchdogNumber,
> + struct SpaprWatchdog *w)
> +{
> + target_ulong ret = H_NOOP;
> +
> + if (timer_pending(&w->timer)) {
> + timer_del(&w->timer);
> + ret = H_SUCCESS;
> + }
> + trace_spapr_watchdog_stop(watchdogNumber, ret);
> +
> + return ret;
> +}
> +
> +static target_ulong h_watchdog(PowerPCCPU *cpu,
> + SpaprMachineState *spapr,
> + target_ulong opcode, target_ulong *args)
> +{
> + target_ulong flags = args[0];
> + target_ulong watchdogNumber = args[1];
> + target_ulong timeoutInMs = args[2];
> + unsigned operation = flags & PSERIES_WDTF_OP(~0);
> + unsigned timeoutAction = flags & PSERIES_WDTF_ACTION(~0);
> + struct SpaprWatchdog *w;
> +
> + if (flags & PSERIES_WDTF_RESERVED) {
> + return H_PARAMETER;
> + }
> +
> + switch (operation) {
> + case PSERIES_WDTF_OP_START:
> + if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
> + return H_P2;
> + }
> + if (timeoutInMs <= WDT_MIN_TIMEOUT) {
> + return H_P3;
> + }
> +
> + w = &spapr->wds[watchdogNumber - 1];
So first index is 1 in PAPR ...
> + switch (timeoutAction) {
> + case PSERIES_WDTF_ACTION_HARD_POWER_OFF:
> + w->action = WDT_HARD_POWER_OFF;
> + break;
> + case PSERIES_WDTF_ACTION_HARD_RESTART:
> + w->action = WDT_HARD_RESTART;
> + break;
> + case PSERIES_WDTF_ACTION_DUMP_RESTART:
> + w->action = WDT_DUMP_RESTART;
> + break;
> + default:
> + return H_PARAMETER;
> + }
> + timer_mod(&w->timer,
> + qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + timeoutInMs);
> + trace_spapr_watchdog_start(flags, watchdogNumber, timeoutInMs);
> + break;
> + case PSERIES_WDTF_OP_STOP:
> + if (watchdogNumber == (uint64_t) ~0) {
May be add a define for this special value. It's better for readability.
> + int i;
> +
> + for (i = 1; i <= ARRAY_SIZE(spapr->wds); ++i) {
> + watchdog_stop(i, &spapr->wds[i - 1]);
> + }
> + } else if (watchdogNumber <= ARRAY_SIZE(spapr->wds)) {
> + watchdog_stop(watchdogNumber, &spapr->wds[watchdogNumber - 1]);
> + } else {
> + return H_P2;
> + }
> + break;
> + case PSERIES_WDTF_OP_QUERY:
> + args[0] = PSERIES_WDTQ_MIN_TIMEOUT(WDT_MIN_TIMEOUT) |
> + PSERIES_WDTQ_NUM(ARRAY_SIZE(spapr->wds));
> + trace_spapr_watchdog_query(args[0]);
> + break;
> + case PSERIES_WDTF_OP_QUERY_LPM:
> + if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
> + return H_P2;
> + }
> + args[0] = PSERIES_WDTQL_QUERY_NOT_STOPPED;
> + trace_spapr_watchdog_query_lpm(args[0]);
> + break;
> + default:
> + return H_PARAMETER;
> + }
> +
> + return H_SUCCESS;
> +}
> +
> +void spapr_watchdog_init(SpaprMachineState *spapr)
This could have a 'Error **errp' parameter.
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(spapr->wds); ++i) {
> + char name[16];g_autofree char *name = g_strdup_printf("wdt%d", i + 1);
> + SpaprWatchdog *w = &spapr->wds[i];
> +
> + w->num = i + 1;
it should be a property.
Thanks,
C.
> + snprintf(name, sizeof(name) - 1, "wdt%d", i + 1);
> + object_initialize_child_with_props(OBJECT(spapr), name, w,
> + sizeof(SpaprWatchdog),
> + TYPE_SPAPR_WDT,
> + &error_fatal, NULL);
> + qdev_realize(DEVICE(w), NULL, &error_fatal);
> + }
> +}
> +
> +static bool watchdog_needed(void *opaque)
> +{
> + SpaprWatchdog *w = opaque;
> +
> + return timer_pending(&w->timer);
> +}
> +
> +static const VMStateDescription vmstate_wdt = {
> + .name = "spapr_watchdog",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = watchdog_needed,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT8(action, SpaprWatchdog),
> + VMSTATE_TIMER(timer, SpaprWatchdog),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static void spapr_wdt_realize(DeviceState *dev, Error **errp)
> +{
> + SpaprWatchdog *w = SPAPR_WDT(dev);
> +
> + timer_init_ms(&w->timer, QEMU_CLOCK_VIRTUAL, watchdog_expired, w);
> +
> + object_property_add_uint64_ptr(OBJECT(dev), "expire",
> + (uint64_t *)&w->timer.expire_time,
> + OBJ_PROP_FLAG_READ);
> + object_property_add_uint8_ptr(OBJECT(dev), "action", &w->action,
> + OBJ_PROP_FLAG_READ);
> +}
> +
> +static void spapr_wdt_class_init(ObjectClass *oc, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(oc);
> +
> + dc->renualize = spapr_wdt_realize;
> + dc->vmsd = &vmstate_wdt;
> + dc->user_creatable = false;
> +}
> +
> +static const TypeInfo spapr_wdt_info = {
> + .name = TYPE_SPAPR_WDT,
> + .parent = TYPE_DEVICE,
> + .instance_size = sizeof(SpaprWatchdog),
> + .class_init = spapr_wdt_class_init,
> +};
> +
> +static void spapr_watchdog_register_types(void)
> +{
> + spapr_register_hypercall(H_WATCHDOG, h_watchdog);
> + type_register_static(&spapr_wdt_info);
> +}
> +
> +type_init(spapr_watchdog_register_types)
> diff --git a/hw/watchdog/meson.build b/hw/watchdog/meson.build
> index 054c403dea7c..8974b5cf4c8a 100644
> --- a/hw/watchdog/meson.build
> +++ b/hw/watchdog/meson.build
> @@ -6,3 +6,4 @@ softmmu_ss.add(when: 'CONFIG_WDT_DIAG288', if_true: files('wdt_diag288.c'))
> softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('wdt_aspeed.c'))
> softmmu_ss.add(when: 'CONFIG_WDT_IMX2', if_true: files('wdt_imx2.c'))
> softmmu_ss.add(when: 'CONFIG_WDT_SBSA', if_true: files('sbsa_gwdt.c'))
> +specific_ss.add(when: 'CONFIG_PSERIES', if_true: files('spapr_watchdog.c'))
> diff --git a/hw/watchdog/trace-events b/hw/watchdog/trace-events
> index e7523e22aaf2..89ccbcfdfd20 100644
> --- a/hw/watchdog/trace-events
> +++ b/hw/watchdog/trace-events
> @@ -9,3 +9,10 @@ cmsdk_apb_watchdog_lock(uint32_t lock) "CMSDK APB watchdog: lock %" PRIu32
> # wdt-aspeed.c
> aspeed_wdt_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> aspeed_wdt_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
> +
> +# spapr_watchdog.c
> +spapr_watchdog_start(uint64_t flags, uint64_t num, uint64_t timeout) "Flags 0x%" PRIx64 " num=%" PRId64 " %" PRIu64 "ms"
> +spapr_watchdog_stop(uint64_t num, uint64_t ret) "num=%" PRIu64 " ret=%" PRId64
> +spapr_watchdog_query(uint64_t caps) "caps=0x%" PRIx64
> +spapr_watchdog_query_lpm(uint64_t caps) "caps=0x%" PRIx64
> +spapr_watchdog_expired(uint64_t num, unsigned action) "num=%" PRIu64 " action=%u"
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH qemu v2 2/2] ppc/spapr: Implement H_WATCHDOG
2022-06-18 11:01 ` Cédric Le Goater
@ 2022-06-20 3:13 ` Alexey Kardashevskiy
2022-06-20 6:23 ` Cédric Le Goater
0 siblings, 1 reply; 21+ messages in thread
From: Alexey Kardashevskiy @ 2022-06-20 3:13 UTC (permalink / raw)
To: Cédric Le Goater, qemu-ppc; +Cc: qemu-devel, Daniel Henrique Barboza
On 6/18/22 21:01, Cédric Le Goater wrote:
> On 6/17/22 08:07, Alexey Kardashevskiy wrote:
>> The new PAPR 2.12 defines a watchdog facility managed via the new
>> H_WATCHDOG hypercall.
>>
>> This adds H_WATCHDOG support which a proposed driver for pseries uses:
>> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=303120
>>
>> This was tested by running QEMU with a debug kernel and command line:
>> -append \
>> "pseries-wdt.timeout=60 pseries-wdt.nowayout=1 pseries-wdt.action=2"
>>
>> and running "echo V > /dev/watchdog0" inside the VM.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v2:
>> * QOM'ed timers, "action" and "expire" are available via QMP
>> * removed @timeout from SpaprWatchdog
>> * moved the driver to hw/watchdog
>> * fixed error handling in the hcall handler
>> * used new SETFIELD/GETFIELD
>> ---
>> include/hw/ppc/spapr.h | 29 +++-
>> hw/ppc/spapr.c | 4 +
>> hw/watchdog/spapr_watchdog.c | 248 +++++++++++++++++++++++++++++++++++
>> hw/watchdog/meson.build | 1 +
>> hw/watchdog/trace-events | 7 +
>> 5 files changed, 288 insertions(+), 1 deletion(-)
>> create mode 100644 hw/watchdog/spapr_watchdog.c
>>
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 072dda2c7265..ef1e38abd5c7 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -164,6 +164,25 @@ struct SpaprMachineClass {
>> SpaprIrq *irq;
>> };
>> +#define WDT_MAX_WATCHDOGS 4 /* Maximum number of watchdog
>> devices */
>> +
>> +#define WDT_HARD_POWER_OFF 0
>> +#define WDT_HARD_RESTART 1
>> +#define WDT_DUMP_RESTART 2
>> +
>> +#define TYPE_SPAPR_WDT "spapr-wdt"
>> +OBJECT_DECLARE_SIMPLE_TYPE(SpaprWatchdog, SPAPR_WDT)
>> +
>> +typedef struct SpaprWatchdog {
>> + /*< private >*/
>> + DeviceState parent_obj;
>> + /*< public >*/
>> +
>> + unsigned num;
>
> uint8_t should be enough no ? I see num is only used for trace events.
It should but why? It is not migrating, and using uint8_t creates
alignment gap here, and no benefit :) And I am removing it anyway, see
below.
>
>> + QEMUTimer timer;
>> + uint8_t action;
>> +} SpaprWatchdog;
>> +
>> /**
>> * SpaprMachineState:
>> */
>> @@ -264,6 +283,8 @@ struct SpaprMachineState {
>> uint32_t
>> FORM2_assoc_array[NUMA_NODES_MAX_NUM][FORM2_NUMA_ASSOC_SIZE];
>> Error *fwnmi_migration_blocker;
>> +
>> + SpaprWatchdog wds[WDT_MAX_WATCHDOGS];
>> };
>> #define H_SUCCESS 0
>> @@ -344,6 +365,7 @@ struct SpaprMachineState {
>> #define H_P7 -60
>> #define H_P8 -61
>> #define H_P9 -62
>> +#define H_NOOP -63
>> #define H_UNSUPPORTED -67
>> #define H_OVERLAP -68
>> #define H_UNSUPPORTED_FLAG -256
>> @@ -564,8 +586,9 @@ struct SpaprMachineState {
>> #define H_SCM_HEALTH 0x400
>> #define H_RPT_INVALIDATE 0x448
>> #define H_SCM_FLUSH 0x44C
>> +#define H_WATCHDOG 0x45C
>> -#define MAX_HCALL_OPCODE H_SCM_FLUSH
>> +#define MAX_HCALL_OPCODE H_WATCHDOG
>> /* The hcalls above are standardized in PAPR and implemented by pHyp
>> * as well.
>> @@ -1027,6 +1050,7 @@ extern const VMStateDescription
>> vmstate_spapr_cap_large_decr;
>> extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>> extern const VMStateDescription vmstate_spapr_cap_fwnmi;
>> extern const VMStateDescription vmstate_spapr_cap_rpt_invalidate;
>> +extern const VMStateDescription vmstate_spapr_wdt;
>> static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
>> {
>> @@ -1063,4 +1087,7 @@ target_ulong
>> spapr_vof_client_architecture_support(MachineState *ms,
>> target_ulong
>> ovec_addr);
>> void spapr_vof_client_dt_finalize(SpaprMachineState *spapr, void *fdt);
>> +/* H_WATCHDOG */
>> +void spapr_watchdog_init(SpaprMachineState *spapr);
>> +
>> #endif /* HW_SPAPR_H */
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index fd4942e8813c..9a5382d5270f 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -898,6 +898,8 @@ static void spapr_dt_rtas(SpaprMachineState
>> *spapr, void *fdt)
>> add_str(hypertas, "hcall-hpt-resize");
>> }
>> + add_str(hypertas, "hcall-watchdog");
>> +
>> _FDT(fdt_setprop(fdt, rtas, "ibm,hypertas-functions",
>> hypertas->str, hypertas->len));
>> g_string_free(hypertas, TRUE);
>> @@ -3051,6 +3053,8 @@ static void spapr_machine_init(MachineState
>> *machine)
>> spapr->vof->fw_size = fw_size; /* for claim() on itself */
>> spapr_register_hypercall(KVMPPC_H_VOF_CLIENT,
>> spapr_h_vof_client);
>> }
>> +
>> + spapr_watchdog_init(spapr);
>> }
>> #define DEFAULT_KVM_TYPE "auto"
>> diff --git a/hw/watchdog/spapr_watchdog.c b/hw/watchdog/spapr_watchdog.c
>> new file mode 100644
>> index 000000000000..aeaf7c52cbad
>> --- /dev/null
>> +++ b/hw/watchdog/spapr_watchdog.c
>> @@ -0,0 +1,248 @@
>> +/*
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * This library 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
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, see
>> <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "target/ppc/cpu.h"
>> +#include "migration/vmstate.h"
>> +#include "trace.h"
>> +
>> +#include "hw/ppc/spapr.h"
>> +
>> +/*
>> + * Bits 47: "leaveOtherWatchdogsRunningOnTimeout", specified on
>> + * the "Start watchdog" operation,
>> + * 0 - stop out-standing watchdogs on timeout,
>> + * 1 - leave outstanding watchdogs running on timeout
>> + */
>> +#define PSERIES_WDTF_LEAVE_OTHER PPC_BIT(47)
>> +
>> +/* Bits 48-55: "operation" */
>> +#define PSERIES_WDTF_OP(op) SETFIELD(PPC_BITMASK(48, 55),
>> 0, (op))
>> +#define PSERIES_WDTF_OP_START PSERIES_WDTF_OP(0x1)
>> +#define PSERIES_WDTF_OP_STOP PSERIES_WDTF_OP(0x2)
>> +#define PSERIES_WDTF_OP_QUERY PSERIES_WDTF_OP(0x3)
>> +#define PSERIES_WDTF_OP_QUERY_LPM PSERIES_WDTF_OP(0x4)
>> +
>> +/* Bits 56-63: "timeoutAction" */
>> +#define PSERIES_WDTF_ACTION(ac) SETFIELD(PPC_BITMASK(56, 63),
>> 0, (ac))
>> +#define PSERIES_WDTF_ACTION_HARD_POWER_OFF PSERIES_WDTF_ACTION(0x1)
>> +#define PSERIES_WDTF_ACTION_HARD_RESTART PSERIES_WDTF_ACTION(0x2)
>> +#define PSERIES_WDTF_ACTION_DUMP_RESTART PSERIES_WDTF_ACTION(0x3)
>> +#define PSERIES_WDTF_RESERVED PPC_BITMASK(0, 46)
>> +
>> +/*
>> + * For the "Query watchdog capabilities" operation, a uint64 structure
>> + * defined as:
>> + * Bits 0-15: The minimum supported timeout in milliseconds
>> + * Bits 16-31: The number of watchdogs supported
>> + * Bits 32-63: Reserved
>> + */
>> +#define PSERIES_WDTQ_MIN_TIMEOUT(ms) SETFIELD(PPC_BITMASK(0, 15),
>> 0, (ms))
>> +#define PSERIES_WDTQ_NUM(n) SETFIELD(PPC_BITMASK(16, 31),
>> 0, (n))
>> +
>> +/*
>> + * For the "Query watchdog LPM requirement" operation:
>> + * 1 = The given "watchdogNumber" must be stopped prior to suspending
>> + * 2 = The given "watchdogNumber" does not have to be stopped prior to
>> + * suspending
>> + */
>> +#define PSERIES_WDTQL_STOPPED 1
>> +#define PSERIES_WDTQL_QUERY_NOT_STOPPED 2
>> +
>> +#define WDT_MIN_TIMEOUT 1 /* 1ms */
>> +
>> +static void watchdog_expired(void *pw)
>> +{
>> + struct SpaprWatchdog *w = pw;
>
> s/struct//
>
>> + CPUState *cs;
>> +
>> + trace_spapr_watchdog_expired(w->num, w->action);
>> + switch (w->action) {
>> + case WDT_HARD_POWER_OFF:
>> + qemu_system_vmstop_request(RUN_STATE_SHUTDOWN);
>> + break;
>> + case WDT_HARD_RESTART:
>> + qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>> + break;
>> + case WDT_DUMP_RESTART:
>> + CPU_FOREACH(cs) {
>> + async_run_on_cpu(cs, spapr_do_system_reset_on_cpu,
>> RUN_ON_CPU_NULL);
>> + }
>> + break;
>> + }
>> +}
>> +
>> +static target_ulong watchdog_stop(unsigned watchdogNumber,
>> + struct SpaprWatchdog *w)
>> +{
>> + target_ulong ret = H_NOOP;
>> +
>> + if (timer_pending(&w->timer)) {
>> + timer_del(&w->timer);
>> + ret = H_SUCCESS;
>> + }
>> + trace_spapr_watchdog_stop(watchdogNumber, ret);
>> +
>> + return ret;
>> +}
>> +
>> +static target_ulong h_watchdog(PowerPCCPU *cpu,
>> + SpaprMachineState *spapr,
>> + target_ulong opcode, target_ulong *args)
>> +{
>> + target_ulong flags = args[0];
>> + target_ulong watchdogNumber = args[1];
>> + target_ulong timeoutInMs = args[2];
>> + unsigned operation = flags & PSERIES_WDTF_OP(~0);
>> + unsigned timeoutAction = flags & PSERIES_WDTF_ACTION(~0);
>> + struct SpaprWatchdog *w;
>> +
>> + if (flags & PSERIES_WDTF_RESERVED) {
>> + return H_PARAMETER;
>> + }
>> +
>> + switch (operation) {
>> + case PSERIES_WDTF_OP_START:
>> + if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
>> + return H_P2;
>> + }
>> + if (timeoutInMs <= WDT_MIN_TIMEOUT) {
>> + return H_P3;
>> + }
>> +
>> + w = &spapr->wds[watchdogNumber - 1];
>
> So first index is 1 in PAPR ...
Yes, I thought I commented on this somewhere but lost in rebases.
>
>> + switch (timeoutAction) {
>> + case PSERIES_WDTF_ACTION_HARD_POWER_OFF:
>> + w->action = WDT_HARD_POWER_OFF;
>> + break;
>> + case PSERIES_WDTF_ACTION_HARD_RESTART:
>> + w->action = WDT_HARD_RESTART;
>> + break;
>> + case PSERIES_WDTF_ACTION_DUMP_RESTART:
>> + w->action = WDT_DUMP_RESTART;
>> + break;
>> + default:
>> + return H_PARAMETER;
>> + }
>> + timer_mod(&w->timer,
>> + qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + timeoutInMs);
>> + trace_spapr_watchdog_start(flags, watchdogNumber, timeoutInMs);
>> + break;
>> + case PSERIES_WDTF_OP_STOP:
>> + if (watchdogNumber == (uint64_t) ~0) {
>
> May be add a define for this special value. It's better for readability.
Will do.
>> + int i;
>> +
>> + for (i = 1; i <= ARRAY_SIZE(spapr->wds); ++i) {
>> + watchdog_stop(i, &spapr->wds[i - 1]);
>> + }
>> + } else if (watchdogNumber <= ARRAY_SIZE(spapr->wds)) {
>> + watchdog_stop(watchdogNumber, &spapr->wds[watchdogNumber
>> - 1]);
>> + } else {
>> + return H_P2;
>> + }
>> + break;
>> + case PSERIES_WDTF_OP_QUERY:
>> + args[0] = PSERIES_WDTQ_MIN_TIMEOUT(WDT_MIN_TIMEOUT) |
>> + PSERIES_WDTQ_NUM(ARRAY_SIZE(spapr->wds));
>> + trace_spapr_watchdog_query(args[0]);
>> + break;
>> + case PSERIES_WDTF_OP_QUERY_LPM:
>> + if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
>> + return H_P2;
>> + }
>> + args[0] = PSERIES_WDTQL_QUERY_NOT_STOPPED;
>> + trace_spapr_watchdog_query_lpm(args[0]);
>> + break;
>> + default:
>> + return H_PARAMETER;
>> + }
>> +
>> + return H_SUCCESS;
>> +}
>> +
>> +void spapr_watchdog_init(SpaprMachineState *spapr)
>
> This could have a 'Error **errp' parameter.
I was repeating somewhat similar spapr_rtc_create(), and the called -
spapr_machine_init() - does not have *errp. Seems pointless as it fails
- something is horrendously broken.
>
>> +{
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(spapr->wds); ++i) {
>> + char name[16];g_autofree char *name =
>> g_strdup_printf("wdt%d", i + 1);
>
>> + SpaprWatchdog *w = &spapr->wds[i];
>> +
>> + w->num = i + 1;
>
> it should be a property.
This cannot change and used only for tracing, and the QOM name has the
number as well. I am replacing it with
SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
unsigned num = w - spapr->wds;
and removing the @num from the struct.
Thanks,
>
> Thanks,
>
> C.
>
>> + snprintf(name, sizeof(name) - 1, "wdt%d", i + 1);
>> + object_initialize_child_with_props(OBJECT(spapr), name, w,
>> + sizeof(SpaprWatchdog),
>> + TYPE_SPAPR_WDT,
>> + &error_fatal, NULL);
>> + qdev_realize(DEVICE(w), NULL, &error_fatal);
>> + }
>> +}
>> +
>> +static bool watchdog_needed(void *opaque)
>> +{
>> + SpaprWatchdog *w = opaque;
>> +
>> + return timer_pending(&w->timer);
>> +}
>> +
>> +static const VMStateDescription vmstate_wdt = {
>> + .name = "spapr_watchdog",
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .needed = watchdog_needed,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_UINT8(action, SpaprWatchdog),
>> + VMSTATE_TIMER(timer, SpaprWatchdog),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>> +
>> +static void spapr_wdt_realize(DeviceState *dev, Error **errp)
>> +{
>> + SpaprWatchdog *w = SPAPR_WDT(dev);
>> +
>> + timer_init_ms(&w->timer, QEMU_CLOCK_VIRTUAL, watchdog_expired, w);
>> +
>> + object_property_add_uint64_ptr(OBJECT(dev), "expire",
>> + (uint64_t *)&w->timer.expire_time,
>> + OBJ_PROP_FLAG_READ);
>> + object_property_add_uint8_ptr(OBJECT(dev), "action", &w->action,
>> + OBJ_PROP_FLAG_READ);
>> +}
>> +
>> +static void spapr_wdt_class_init(ObjectClass *oc, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(oc);
>> +
>> + dc->renualize = spapr_wdt_realize;
>> + dc->vmsd = &vmstate_wdt;
>> + dc->user_creatable = false;
>> +}
>> +
>> +static const TypeInfo spapr_wdt_info = {
>> + .name = TYPE_SPAPR_WDT,
>> + .parent = TYPE_DEVICE,
>> + .instance_size = sizeof(SpaprWatchdog),
>> + .class_init = spapr_wdt_class_init,
>> +};
>> +
>> +static void spapr_watchdog_register_types(void)
>> +{
>> + spapr_register_hypercall(H_WATCHDOG, h_watchdog);
>> + type_register_static(&spapr_wdt_info);
>> +}
>> +
>> +type_init(spapr_watchdog_register_types)
>> diff --git a/hw/watchdog/meson.build b/hw/watchdog/meson.build
>> index 054c403dea7c..8974b5cf4c8a 100644
>> --- a/hw/watchdog/meson.build
>> +++ b/hw/watchdog/meson.build
>> @@ -6,3 +6,4 @@ softmmu_ss.add(when: 'CONFIG_WDT_DIAG288', if_true:
>> files('wdt_diag288.c'))
>> softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true:
>> files('wdt_aspeed.c'))
>> softmmu_ss.add(when: 'CONFIG_WDT_IMX2', if_true: files('wdt_imx2.c'))
>> softmmu_ss.add(when: 'CONFIG_WDT_SBSA', if_true: files('sbsa_gwdt.c'))
>> +specific_ss.add(when: 'CONFIG_PSERIES', if_true:
>> files('spapr_watchdog.c'))
>> diff --git a/hw/watchdog/trace-events b/hw/watchdog/trace-events
>> index e7523e22aaf2..89ccbcfdfd20 100644
>> --- a/hw/watchdog/trace-events
>> +++ b/hw/watchdog/trace-events
>> @@ -9,3 +9,10 @@ cmsdk_apb_watchdog_lock(uint32_t lock) "CMSDK APB
>> watchdog: lock %" PRIu32
>> # wdt-aspeed.c
>> aspeed_wdt_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
>> aspeed_wdt_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%"
>> PRIx64 " size=%d value=0x%"PRIx64
>> +
>> +# spapr_watchdog.c
>> +spapr_watchdog_start(uint64_t flags, uint64_t num, uint64_t timeout)
>> "Flags 0x%" PRIx64 " num=%" PRId64 " %" PRIu64 "ms"
>> +spapr_watchdog_stop(uint64_t num, uint64_t ret) "num=%" PRIu64 "
>> ret=%" PRId64
>> +spapr_watchdog_query(uint64_t caps) "caps=0x%" PRIx64
>> +spapr_watchdog_query_lpm(uint64_t caps) "caps=0x%" PRIx64
>> +spapr_watchdog_expired(uint64_t num, unsigned action) "num=%" PRIu64
>> " action=%u"
--
Alexey
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target
2022-06-17 16:50 ` Daniel Henrique Barboza
@ 2022-06-20 3:37 ` Alexey Kardashevskiy
2022-06-20 6:17 ` Cédric Le Goater
0 siblings, 1 reply; 21+ messages in thread
From: Alexey Kardashevskiy @ 2022-06-20 3:37 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-ppc; +Cc: qemu-devel, Cédric Le Goater
On 6/18/22 02:50, Daniel Henrique Barboza wrote:
>
>
> On 6/17/22 03:07, Alexey Kardashevskiy wrote:
>> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
>> allow using the macros in #define.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
soooo I do not need to repost it, right?
--
Alexey
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target
2022-06-20 3:37 ` Alexey Kardashevskiy
@ 2022-06-20 6:17 ` Cédric Le Goater
2022-06-20 8:10 ` Alexey Kardashevskiy
0 siblings, 1 reply; 21+ messages in thread
From: Cédric Le Goater @ 2022-06-20 6:17 UTC (permalink / raw)
To: Alexey Kardashevskiy, Daniel Henrique Barboza, qemu-ppc; +Cc: qemu-devel
On 6/20/22 05:37, Alexey Kardashevskiy wrote:
>
>
> On 6/18/22 02:50, Daniel Henrique Barboza wrote:
>>
>>
>> On 6/17/22 03:07, Alexey Kardashevskiy wrote:
>>> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
>>> allow using the macros in #define.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>
>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>
>
>
> soooo I do not need to repost it, right?
>
>
The watchdog patch depends on the availability of these macros.
Doesn't it ?
C.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH qemu v2 2/2] ppc/spapr: Implement H_WATCHDOG
2022-06-20 3:13 ` Alexey Kardashevskiy
@ 2022-06-20 6:23 ` Cédric Le Goater
2022-06-20 8:28 ` Alexey Kardashevskiy
0 siblings, 1 reply; 21+ messages in thread
From: Cédric Le Goater @ 2022-06-20 6:23 UTC (permalink / raw)
To: Alexey Kardashevskiy, qemu-ppc; +Cc: qemu-devel, Daniel Henrique Barboza
On 6/20/22 05:13, Alexey Kardashevskiy wrote:
>
>
> On 6/18/22 21:01, Cédric Le Goater wrote:
>> On 6/17/22 08:07, Alexey Kardashevskiy wrote:
>>> The new PAPR 2.12 defines a watchdog facility managed via the new
>>> H_WATCHDOG hypercall.
>>>
>>> This adds H_WATCHDOG support which a proposed driver for pseries uses:
>>> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=303120
>>>
>>> This was tested by running QEMU with a debug kernel and command line:
>>> -append \
>>> "pseries-wdt.timeout=60 pseries-wdt.nowayout=1 pseries-wdt.action=2"
>>>
>>> and running "echo V > /dev/watchdog0" inside the VM.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>> Changes:
>>> v2:
>>> * QOM'ed timers, "action" and "expire" are available via QMP
>>> * removed @timeout from SpaprWatchdog
>>> * moved the driver to hw/watchdog
>>> * fixed error handling in the hcall handler
>>> * used new SETFIELD/GETFIELD
>>> ---
>>> include/hw/ppc/spapr.h | 29 +++-
>>> hw/ppc/spapr.c | 4 +
>>> hw/watchdog/spapr_watchdog.c | 248 +++++++++++++++++++++++++++++++++++
>>> hw/watchdog/meson.build | 1 +
>>> hw/watchdog/trace-events | 7 +
>>> 5 files changed, 288 insertions(+), 1 deletion(-)
>>> create mode 100644 hw/watchdog/spapr_watchdog.c
>>>
>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>> index 072dda2c7265..ef1e38abd5c7 100644
>>> --- a/include/hw/ppc/spapr.h
>>> +++ b/include/hw/ppc/spapr.h
>>> @@ -164,6 +164,25 @@ struct SpaprMachineClass {
>>> SpaprIrq *irq;
>>> };
>>> +#define WDT_MAX_WATCHDOGS 4 /* Maximum number of watchdog devices */
>>> +
>>> +#define WDT_HARD_POWER_OFF 0
>>> +#define WDT_HARD_RESTART 1
>>> +#define WDT_DUMP_RESTART 2
>>> +
>>> +#define TYPE_SPAPR_WDT "spapr-wdt"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(SpaprWatchdog, SPAPR_WDT)
>>> +
>>> +typedef struct SpaprWatchdog {
>>> + /*< private >*/
>>> + DeviceState parent_obj;
>>> + /*< public >*/
>>> +
>>> + unsigned num;
>>
>> uint8_t should be enough no ? I see num is only used for trace events.
>
>
> It should but why? It is not migrating, and using uint8_t creates alignment gap here, and no benefit :) And I am removing it anyway, see below.
>
>>
>>> + QEMUTimer timer;
>>> + uint8_t action;
>>> +} SpaprWatchdog;
>>> +
>>> /**
>>> * SpaprMachineState:
>>> */
>>> @@ -264,6 +283,8 @@ struct SpaprMachineState {
>>> uint32_t FORM2_assoc_array[NUMA_NODES_MAX_NUM][FORM2_NUMA_ASSOC_SIZE];
>>> Error *fwnmi_migration_blocker;
>>> +
>>> + SpaprWatchdog wds[WDT_MAX_WATCHDOGS];
>>> };
>>> #define H_SUCCESS 0
>>> @@ -344,6 +365,7 @@ struct SpaprMachineState {
>>> #define H_P7 -60
>>> #define H_P8 -61
>>> #define H_P9 -62
>>> +#define H_NOOP -63
>>> #define H_UNSUPPORTED -67
>>> #define H_OVERLAP -68
>>> #define H_UNSUPPORTED_FLAG -256
>>> @@ -564,8 +586,9 @@ struct SpaprMachineState {
>>> #define H_SCM_HEALTH 0x400
>>> #define H_RPT_INVALIDATE 0x448
>>> #define H_SCM_FLUSH 0x44C
>>> +#define H_WATCHDOG 0x45C
>>> -#define MAX_HCALL_OPCODE H_SCM_FLUSH
>>> +#define MAX_HCALL_OPCODE H_WATCHDOG
>>> /* The hcalls above are standardized in PAPR and implemented by pHyp
>>> * as well.
>>> @@ -1027,6 +1050,7 @@ extern const VMStateDescription vmstate_spapr_cap_large_decr;
>>> extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>>> extern const VMStateDescription vmstate_spapr_cap_fwnmi;
>>> extern const VMStateDescription vmstate_spapr_cap_rpt_invalidate;
>>> +extern const VMStateDescription vmstate_spapr_wdt;
>>> static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
>>> {
>>> @@ -1063,4 +1087,7 @@ target_ulong spapr_vof_client_architecture_support(MachineState *ms,
>>> target_ulong ovec_addr);
>>> void spapr_vof_client_dt_finalize(SpaprMachineState *spapr, void *fdt);
>>> +/* H_WATCHDOG */
>>> +void spapr_watchdog_init(SpaprMachineState *spapr);
>>> +
>>> #endif /* HW_SPAPR_H */
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index fd4942e8813c..9a5382d5270f 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -898,6 +898,8 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
>>> add_str(hypertas, "hcall-hpt-resize");
>>> }
>>> + add_str(hypertas, "hcall-watchdog");
>>> +
>>> _FDT(fdt_setprop(fdt, rtas, "ibm,hypertas-functions",
>>> hypertas->str, hypertas->len));
>>> g_string_free(hypertas, TRUE);
>>> @@ -3051,6 +3053,8 @@ static void spapr_machine_init(MachineState *machine)
>>> spapr->vof->fw_size = fw_size; /* for claim() on itself */
>>> spapr_register_hypercall(KVMPPC_H_VOF_CLIENT, spapr_h_vof_client);
>>> }
>>> +
>>> + spapr_watchdog_init(spapr);
>>> }
>>> #define DEFAULT_KVM_TYPE "auto"
>>> diff --git a/hw/watchdog/spapr_watchdog.c b/hw/watchdog/spapr_watchdog.c
>>> new file mode 100644
>>> index 000000000000..aeaf7c52cbad
>>> --- /dev/null
>>> +++ b/hw/watchdog/spapr_watchdog.c
>>> @@ -0,0 +1,248 @@
>>> +/*
>>> + * This library is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2.1 of the License, or (at your option) any later version.
>>> + *
>>> + * This library 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
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public
>>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qapi/error.h"
>>> +#include "target/ppc/cpu.h"
>>> +#include "migration/vmstate.h"
>>> +#include "trace.h"
>>> +
>>> +#include "hw/ppc/spapr.h"
>>> +
>>> +/*
>>> + * Bits 47: "leaveOtherWatchdogsRunningOnTimeout", specified on
>>> + * the "Start watchdog" operation,
>>> + * 0 - stop out-standing watchdogs on timeout,
>>> + * 1 - leave outstanding watchdogs running on timeout
>>> + */
>>> +#define PSERIES_WDTF_LEAVE_OTHER PPC_BIT(47)
>>> +
>>> +/* Bits 48-55: "operation" */
>>> +#define PSERIES_WDTF_OP(op) SETFIELD(PPC_BITMASK(48, 55), 0, (op))
>>> +#define PSERIES_WDTF_OP_START PSERIES_WDTF_OP(0x1)
>>> +#define PSERIES_WDTF_OP_STOP PSERIES_WDTF_OP(0x2)
>>> +#define PSERIES_WDTF_OP_QUERY PSERIES_WDTF_OP(0x3)
>>> +#define PSERIES_WDTF_OP_QUERY_LPM PSERIES_WDTF_OP(0x4)
>>> +
>>> +/* Bits 56-63: "timeoutAction" */
>>> +#define PSERIES_WDTF_ACTION(ac) SETFIELD(PPC_BITMASK(56, 63), 0, (ac))
>>> +#define PSERIES_WDTF_ACTION_HARD_POWER_OFF PSERIES_WDTF_ACTION(0x1)
>>> +#define PSERIES_WDTF_ACTION_HARD_RESTART PSERIES_WDTF_ACTION(0x2)
>>> +#define PSERIES_WDTF_ACTION_DUMP_RESTART PSERIES_WDTF_ACTION(0x3)
>>> +#define PSERIES_WDTF_RESERVED PPC_BITMASK(0, 46)
>>> +
>>> +/*
>>> + * For the "Query watchdog capabilities" operation, a uint64 structure
>>> + * defined as:
>>> + * Bits 0-15: The minimum supported timeout in milliseconds
>>> + * Bits 16-31: The number of watchdogs supported
>>> + * Bits 32-63: Reserved
>>> + */
>>> +#define PSERIES_WDTQ_MIN_TIMEOUT(ms) SETFIELD(PPC_BITMASK(0, 15), 0, (ms))
>>> +#define PSERIES_WDTQ_NUM(n) SETFIELD(PPC_BITMASK(16, 31), 0, (n))
>>> +
>>> +/*
>>> + * For the "Query watchdog LPM requirement" operation:
>>> + * 1 = The given "watchdogNumber" must be stopped prior to suspending
>>> + * 2 = The given "watchdogNumber" does not have to be stopped prior to
>>> + * suspending
>>> + */
>>> +#define PSERIES_WDTQL_STOPPED 1
>>> +#define PSERIES_WDTQL_QUERY_NOT_STOPPED 2
>>> +
>>> +#define WDT_MIN_TIMEOUT 1 /* 1ms */
>>> +
>>> +static void watchdog_expired(void *pw)
>>> +{
>>> + struct SpaprWatchdog *w = pw;
>>
>> s/struct//
>>
>>> + CPUState *cs;
>>> +
>>> + trace_spapr_watchdog_expired(w->num, w->action);
>>> + switch (w->action) {
>>> + case WDT_HARD_POWER_OFF:
>>> + qemu_system_vmstop_request(RUN_STATE_SHUTDOWN);
>>> + break;
>>> + case WDT_HARD_RESTART:
>>> + qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>>> + break;
>>> + case WDT_DUMP_RESTART:
>>> + CPU_FOREACH(cs) {
>>> + async_run_on_cpu(cs, spapr_do_system_reset_on_cpu, RUN_ON_CPU_NULL);
>>> + }
>>> + break;
>>> + }
>>> +}
>>> +
>>> +static target_ulong watchdog_stop(unsigned watchdogNumber,
>>> + struct SpaprWatchdog *w)
>>> +{
>>> + target_ulong ret = H_NOOP;
>>> +
>>> + if (timer_pending(&w->timer)) {
>>> + timer_del(&w->timer);
>>> + ret = H_SUCCESS;
>>> + }
>>> + trace_spapr_watchdog_stop(watchdogNumber, ret);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static target_ulong h_watchdog(PowerPCCPU *cpu,
>>> + SpaprMachineState *spapr,
>>> + target_ulong opcode, target_ulong *args)
>>> +{
>>> + target_ulong flags = args[0];
>>> + target_ulong watchdogNumber = args[1];
>>> + target_ulong timeoutInMs = args[2];
>>> + unsigned operation = flags & PSERIES_WDTF_OP(~0);
>>> + unsigned timeoutAction = flags & PSERIES_WDTF_ACTION(~0);
>>> + struct SpaprWatchdog *w;
>>> +
>>> + if (flags & PSERIES_WDTF_RESERVED) {
>>> + return H_PARAMETER;
>>> + }
>>> +
>>> + switch (operation) {
>>> + case PSERIES_WDTF_OP_START:
>>> + if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
>>> + return H_P2;
>>> + }
>>> + if (timeoutInMs <= WDT_MIN_TIMEOUT) {
>>> + return H_P3;
>>> + }
>>> +
>>> + w = &spapr->wds[watchdogNumber - 1];
>>
>> So first index is 1 in PAPR ...
>
>
> Yes, I thought I commented on this somewhere but lost in rebases.
>
>
>>
>>> + switch (timeoutAction) {
>>> + case PSERIES_WDTF_ACTION_HARD_POWER_OFF:
>>> + w->action = WDT_HARD_POWER_OFF;
>>> + break;
>>> + case PSERIES_WDTF_ACTION_HARD_RESTART:
>>> + w->action = WDT_HARD_RESTART;
>>> + break;
>>> + case PSERIES_WDTF_ACTION_DUMP_RESTART:
>>> + w->action = WDT_DUMP_RESTART;
>>> + break;
>>> + default:
>>> + return H_PARAMETER;
>>> + }
>>> + timer_mod(&w->timer,
>>> + qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + timeoutInMs);
>>> + trace_spapr_watchdog_start(flags, watchdogNumber, timeoutInMs);
>>> + break;
>>> + case PSERIES_WDTF_OP_STOP:
>>> + if (watchdogNumber == (uint64_t) ~0) {
>>
>> May be add a define for this special value. It's better for readability.
>
> Will do.
>
>
>>> + int i;
>>> +
>>> + for (i = 1; i <= ARRAY_SIZE(spapr->wds); ++i) {
>>> + watchdog_stop(i, &spapr->wds[i - 1]);
>>> + }
>>> + } else if (watchdogNumber <= ARRAY_SIZE(spapr->wds)) {
>>> + watchdog_stop(watchdogNumber, &spapr->wds[watchdogNumber - 1]);
>>> + } else {
>>> + return H_P2;
>>> + }
>>> + break;
>>> + case PSERIES_WDTF_OP_QUERY:
>>> + args[0] = PSERIES_WDTQ_MIN_TIMEOUT(WDT_MIN_TIMEOUT) |
>>> + PSERIES_WDTQ_NUM(ARRAY_SIZE(spapr->wds));
>>> + trace_spapr_watchdog_query(args[0]);
>>> + break;
>>> + case PSERIES_WDTF_OP_QUERY_LPM:
>>> + if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
>>> + return H_P2;
>>> + }
>>> + args[0] = PSERIES_WDTQL_QUERY_NOT_STOPPED;
>>> + trace_spapr_watchdog_query_lpm(args[0]);
>>> + break;
>>> + default:
>>> + return H_PARAMETER;
>>> + }
>>> +
>>> + return H_SUCCESS;
>>> +}
>>> +
>>> +void spapr_watchdog_init(SpaprMachineState *spapr)
>>
>> This could have a 'Error **errp' parameter.
>
>
> I was repeating somewhat similar spapr_rtc_create(), and the called - spapr_machine_init() - does not have *errp. Seems pointless as it fails - something is horrendously broken.
Well, it's up to the caller to decide what to do in case of
errors. If in this case, it is "log an error and exit",
I would simply :
spapr_watchdog_init(spapr, &error_fatal);
But it is not necessarily fatal to fail to initialize some
device.
>
>
>>
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(spapr->wds); ++i) {
>>> + char name[16];g_autofree char *name = g_strdup_printf("wdt%d", i + 1);
>>
>>> + SpaprWatchdog *w = &spapr->wds[i];
>>> +
>>> + w->num = i + 1;
>>
>> it should be a property.
>
> This cannot change and used only for tracing, and the QOM name has the number as well. I am replacing it with
>
> SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> unsigned num = w - spapr->wds;
>
> and removing the @num from the struct.
Nice !
Thanks
C.
>
> Thanks,
>
>>
>> Thanks,
>>
>> C.
>>
>>> + snprintf(name, sizeof(name) - 1, "wdt%d", i + 1);
>>> + object_initialize_child_with_props(OBJECT(spapr), name, w,
>>> + sizeof(SpaprWatchdog),
>>> + TYPE_SPAPR_WDT,
>>> + &error_fatal, NULL);
>>> + qdev_realize(DEVICE(w), NULL, &error_fatal);
>>> + }
>>> +}
>>> +
>>> +static bool watchdog_needed(void *opaque)
>>> +{
>>> + SpaprWatchdog *w = opaque;
>>> +
>>> + return timer_pending(&w->timer);
>>> +}
>>> +
>>> +static const VMStateDescription vmstate_wdt = {
>>> + .name = "spapr_watchdog",
>>> + .version_id = 1,
>>> + .minimum_version_id = 1,
>>> + .needed = watchdog_needed,
>>> + .fields = (VMStateField[]) {
>>> + VMSTATE_UINT8(action, SpaprWatchdog),
>>> + VMSTATE_TIMER(timer, SpaprWatchdog),
>>> + VMSTATE_END_OF_LIST()
>>> + }
>>> +};
>>> +
>>> +static void spapr_wdt_realize(DeviceState *dev, Error **errp)
>>> +{
>>> + SpaprWatchdog *w = SPAPR_WDT(dev);
>>> +
>>> + timer_init_ms(&w->timer, QEMU_CLOCK_VIRTUAL, watchdog_expired, w);
>>> +
>>> + object_property_add_uint64_ptr(OBJECT(dev), "expire",
>>> + (uint64_t *)&w->timer.expire_time,
>>> + OBJ_PROP_FLAG_READ);
>>> + object_property_add_uint8_ptr(OBJECT(dev), "action", &w->action,
>>> + OBJ_PROP_FLAG_READ);
>>> +}
>>> +
>>> +static void spapr_wdt_class_init(ObjectClass *oc, void *data)
>>> +{
>>> + DeviceClass *dc = DEVICE_CLASS(oc);
>>> +
>>> + dc->renualize = spapr_wdt_realize;
>>> + dc->vmsd = &vmstate_wdt;
>>> + dc->user_creatable = false;
>>> +}
>>> +
>>> +static const TypeInfo spapr_wdt_info = {
>>> + .name = TYPE_SPAPR_WDT,
>>> + .parent = TYPE_DEVICE,
>>> + .instance_size = sizeof(SpaprWatchdog),
>>> + .class_init = spapr_wdt_class_init,
>>> +};
>>> +
>>> +static void spapr_watchdog_register_types(void)
>>> +{
>>> + spapr_register_hypercall(H_WATCHDOG, h_watchdog);
>>> + type_register_static(&spapr_wdt_info);
>>> +}
>>> +
>>> +type_init(spapr_watchdog_register_types)
>>> diff --git a/hw/watchdog/meson.build b/hw/watchdog/meson.build
>>> index 054c403dea7c..8974b5cf4c8a 100644
>>> --- a/hw/watchdog/meson.build
>>> +++ b/hw/watchdog/meson.build
>>> @@ -6,3 +6,4 @@ softmmu_ss.add(when: 'CONFIG_WDT_DIAG288', if_true: files('wdt_diag288.c'))
>>> softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('wdt_aspeed.c'))
>>> softmmu_ss.add(when: 'CONFIG_WDT_IMX2', if_true: files('wdt_imx2.c'))
>>> softmmu_ss.add(when: 'CONFIG_WDT_SBSA', if_true: files('sbsa_gwdt.c'))
>>> +specific_ss.add(when: 'CONFIG_PSERIES', if_true: files('spapr_watchdog.c'))
>>> diff --git a/hw/watchdog/trace-events b/hw/watchdog/trace-events
>>> index e7523e22aaf2..89ccbcfdfd20 100644
>>> --- a/hw/watchdog/trace-events
>>> +++ b/hw/watchdog/trace-events
>>> @@ -9,3 +9,10 @@ cmsdk_apb_watchdog_lock(uint32_t lock) "CMSDK APB watchdog: lock %" PRIu32
>>> # wdt-aspeed.c
>>> aspeed_wdt_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
>>> aspeed_wdt_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
>>> +
>>> +# spapr_watchdog.c
>>> +spapr_watchdog_start(uint64_t flags, uint64_t num, uint64_t timeout) "Flags 0x%" PRIx64 " num=%" PRId64 " %" PRIu64 "ms"
>>> +spapr_watchdog_stop(uint64_t num, uint64_t ret) "num=%" PRIu64 " ret=%" PRId64
>>> +spapr_watchdog_query(uint64_t caps) "caps=0x%" PRIx64
>>> +spapr_watchdog_query_lpm(uint64_t caps) "caps=0x%" PRIx64
>>> +spapr_watchdog_expired(uint64_t num, unsigned action) "num=%" PRIu64 " action=%u"
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target
2022-06-20 6:17 ` Cédric Le Goater
@ 2022-06-20 8:10 ` Alexey Kardashevskiy
2022-06-21 12:55 ` Daniel Henrique Barboza
0 siblings, 1 reply; 21+ messages in thread
From: Alexey Kardashevskiy @ 2022-06-20 8:10 UTC (permalink / raw)
To: Cédric Le Goater, Daniel Henrique Barboza, qemu-ppc; +Cc: qemu-devel
On 6/20/22 16:17, Cédric Le Goater wrote:
> On 6/20/22 05:37, Alexey Kardashevskiy wrote:
>>
>>
>> On 6/18/22 02:50, Daniel Henrique Barboza wrote:
>>>
>>>
>>> On 6/17/22 03:07, Alexey Kardashevskiy wrote:
>>>> It keeps repeating, move it to the header. This uses
>>>> __builtin_ctzl() to
>>>> allow using the macros in #define.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>
>>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>
>>
>>
>> soooo I do not need to repost it, right?
>>
>>
>
> The watchdog patch depends on the availability of these macros.
> Doesn't it ?
yes but this one could go in now and I will repost 2/2 a little later on
top.
--
Alexey
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH qemu v2 2/2] ppc/spapr: Implement H_WATCHDOG
2022-06-20 6:23 ` Cédric Le Goater
@ 2022-06-20 8:28 ` Alexey Kardashevskiy
0 siblings, 0 replies; 21+ messages in thread
From: Alexey Kardashevskiy @ 2022-06-20 8:28 UTC (permalink / raw)
To: Cédric Le Goater, qemu-ppc; +Cc: qemu-devel, Daniel Henrique Barboza
On 6/20/22 16:23, Cédric Le Goater wrote:
> On 6/20/22 05:13, Alexey Kardashevskiy wrote:
>>
>>
>> On 6/18/22 21:01, Cédric Le Goater wrote:
>>> On 6/17/22 08:07, Alexey Kardashevskiy wrote:
>>>> The new PAPR 2.12 defines a watchdog facility managed via the new
>>>> H_WATCHDOG hypercall.
>>>>
>>>> This adds H_WATCHDOG support which a proposed driver for pseries uses:
>>>> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=303120
>>>>
>>>> This was tested by running QEMU with a debug kernel and command line:
>>>> -append \
>>>> "pseries-wdt.timeout=60 pseries-wdt.nowayout=1 pseries-wdt.action=2"
>>>>
>>>> and running "echo V > /dev/watchdog0" inside the VM.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>> Changes:
>>>> v2:
>>>> * QOM'ed timers, "action" and "expire" are available via QMP
>>>> * removed @timeout from SpaprWatchdog
>>>> * moved the driver to hw/watchdog
>>>> * fixed error handling in the hcall handler
>>>> * used new SETFIELD/GETFIELD
>>>> ---
>>>> include/hw/ppc/spapr.h | 29 +++-
>>>> hw/ppc/spapr.c | 4 +
>>>> hw/watchdog/spapr_watchdog.c | 248
>>>> +++++++++++++++++++++++++++++++++++
>>>> hw/watchdog/meson.build | 1 +
>>>> hw/watchdog/trace-events | 7 +
>>>> 5 files changed, 288 insertions(+), 1 deletion(-)
>>>> create mode 100644 hw/watchdog/spapr_watchdog.c
>>>>
>>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>>> index 072dda2c7265..ef1e38abd5c7 100644
>>>> --- a/include/hw/ppc/spapr.h
>>>> +++ b/include/hw/ppc/spapr.h
>>>> @@ -164,6 +164,25 @@ struct SpaprMachineClass {
>>>> SpaprIrq *irq;
>>>> };
>>>> +#define WDT_MAX_WATCHDOGS 4 /* Maximum number of
>>>> watchdog devices */
>>>> +
>>>> +#define WDT_HARD_POWER_OFF 0
>>>> +#define WDT_HARD_RESTART 1
>>>> +#define WDT_DUMP_RESTART 2
>>>> +
>>>> +#define TYPE_SPAPR_WDT "spapr-wdt"
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(SpaprWatchdog, SPAPR_WDT)
>>>> +
>>>> +typedef struct SpaprWatchdog {
>>>> + /*< private >*/
>>>> + DeviceState parent_obj;
>>>> + /*< public >*/
>>>> +
>>>> + unsigned num;
>>>
>>> uint8_t should be enough no ? I see num is only used for trace events.
>>
>>
>> It should but why? It is not migrating, and using uint8_t creates
>> alignment gap here, and no benefit :) And I am removing it anyway, see
>> below.
>>
>>>
>>>> + QEMUTimer timer;
>>>> + uint8_t action;
>>>> +} SpaprWatchdog;
>>>> +
>>>> /**
>>>> * SpaprMachineState:
>>>> */
>>>> @@ -264,6 +283,8 @@ struct SpaprMachineState {
>>>> uint32_t
>>>> FORM2_assoc_array[NUMA_NODES_MAX_NUM][FORM2_NUMA_ASSOC_SIZE];
>>>> Error *fwnmi_migration_blocker;
>>>> +
>>>> + SpaprWatchdog wds[WDT_MAX_WATCHDOGS];
>>>> };
>>>> #define H_SUCCESS 0
>>>> @@ -344,6 +365,7 @@ struct SpaprMachineState {
>>>> #define H_P7 -60
>>>> #define H_P8 -61
>>>> #define H_P9 -62
>>>> +#define H_NOOP -63
>>>> #define H_UNSUPPORTED -67
>>>> #define H_OVERLAP -68
>>>> #define H_UNSUPPORTED_FLAG -256
>>>> @@ -564,8 +586,9 @@ struct SpaprMachineState {
>>>> #define H_SCM_HEALTH 0x400
>>>> #define H_RPT_INVALIDATE 0x448
>>>> #define H_SCM_FLUSH 0x44C
>>>> +#define H_WATCHDOG 0x45C
>>>> -#define MAX_HCALL_OPCODE H_SCM_FLUSH
>>>> +#define MAX_HCALL_OPCODE H_WATCHDOG
>>>> /* The hcalls above are standardized in PAPR and implemented by pHyp
>>>> * as well.
>>>> @@ -1027,6 +1050,7 @@ extern const VMStateDescription
>>>> vmstate_spapr_cap_large_decr;
>>>> extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>>>> extern const VMStateDescription vmstate_spapr_cap_fwnmi;
>>>> extern const VMStateDescription vmstate_spapr_cap_rpt_invalidate;
>>>> +extern const VMStateDescription vmstate_spapr_wdt;
>>>> static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int
>>>> cap)
>>>> {
>>>> @@ -1063,4 +1087,7 @@ target_ulong
>>>> spapr_vof_client_architecture_support(MachineState *ms,
>>>> target_ulong
>>>> ovec_addr);
>>>> void spapr_vof_client_dt_finalize(SpaprMachineState *spapr, void
>>>> *fdt);
>>>> +/* H_WATCHDOG */
>>>> +void spapr_watchdog_init(SpaprMachineState *spapr);
>>>> +
>>>> #endif /* HW_SPAPR_H */
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index fd4942e8813c..9a5382d5270f 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -898,6 +898,8 @@ static void spapr_dt_rtas(SpaprMachineState
>>>> *spapr, void *fdt)
>>>> add_str(hypertas, "hcall-hpt-resize");
>>>> }
>>>> + add_str(hypertas, "hcall-watchdog");
>>>> +
>>>> _FDT(fdt_setprop(fdt, rtas, "ibm,hypertas-functions",
>>>> hypertas->str, hypertas->len));
>>>> g_string_free(hypertas, TRUE);
>>>> @@ -3051,6 +3053,8 @@ static void spapr_machine_init(MachineState
>>>> *machine)
>>>> spapr->vof->fw_size = fw_size; /* for claim() on itself */
>>>> spapr_register_hypercall(KVMPPC_H_VOF_CLIENT,
>>>> spapr_h_vof_client);
>>>> }
>>>> +
>>>> + spapr_watchdog_init(spapr);
>>>> }
>>>> #define DEFAULT_KVM_TYPE "auto"
>>>> diff --git a/hw/watchdog/spapr_watchdog.c
>>>> b/hw/watchdog/spapr_watchdog.c
>>>> new file mode 100644
>>>> index 000000000000..aeaf7c52cbad
>>>> --- /dev/null
>>>> +++ b/hw/watchdog/spapr_watchdog.c
>>>> @@ -0,0 +1,248 @@
>>>> +/*
>>>> + * This library is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU Lesser General Public
>>>> + * License as published by the Free Software Foundation; either
>>>> + * version 2.1 of the License, or (at your option) any later version.
>>>> + *
>>>> + * This library 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
>>>> + * Lesser General Public License for more details.
>>>> + *
>>>> + * You should have received a copy of the GNU Lesser General Public
>>>> + * License along with this library; if not, see
>>>> <http://www.gnu.org/licenses/>.
>>>> + */
>>>> +
>>>> +#include "qemu/osdep.h"
>>>> +#include "qapi/error.h"
>>>> +#include "target/ppc/cpu.h"
>>>> +#include "migration/vmstate.h"
>>>> +#include "trace.h"
>>>> +
>>>> +#include "hw/ppc/spapr.h"
>>>> +
>>>> +/*
>>>> + * Bits 47: "leaveOtherWatchdogsRunningOnTimeout", specified on
>>>> + * the "Start watchdog" operation,
>>>> + * 0 - stop out-standing watchdogs on timeout,
>>>> + * 1 - leave outstanding watchdogs running on timeout
>>>> + */
>>>> +#define PSERIES_WDTF_LEAVE_OTHER PPC_BIT(47)
>>>> +
>>>> +/* Bits 48-55: "operation" */
>>>> +#define PSERIES_WDTF_OP(op) SETFIELD(PPC_BITMASK(48,
>>>> 55), 0, (op))
>>>> +#define PSERIES_WDTF_OP_START PSERIES_WDTF_OP(0x1)
>>>> +#define PSERIES_WDTF_OP_STOP PSERIES_WDTF_OP(0x2)
>>>> +#define PSERIES_WDTF_OP_QUERY PSERIES_WDTF_OP(0x3)
>>>> +#define PSERIES_WDTF_OP_QUERY_LPM PSERIES_WDTF_OP(0x4)
>>>> +
>>>> +/* Bits 56-63: "timeoutAction" */
>>>> +#define PSERIES_WDTF_ACTION(ac) SETFIELD(PPC_BITMASK(56,
>>>> 63), 0, (ac))
>>>> +#define PSERIES_WDTF_ACTION_HARD_POWER_OFF PSERIES_WDTF_ACTION(0x1)
>>>> +#define PSERIES_WDTF_ACTION_HARD_RESTART PSERIES_WDTF_ACTION(0x2)
>>>> +#define PSERIES_WDTF_ACTION_DUMP_RESTART PSERIES_WDTF_ACTION(0x3)
>>>> +#define PSERIES_WDTF_RESERVED PPC_BITMASK(0, 46)
>>>> +
>>>> +/*
>>>> + * For the "Query watchdog capabilities" operation, a uint64 structure
>>>> + * defined as:
>>>> + * Bits 0-15: The minimum supported timeout in milliseconds
>>>> + * Bits 16-31: The number of watchdogs supported
>>>> + * Bits 32-63: Reserved
>>>> + */
>>>> +#define PSERIES_WDTQ_MIN_TIMEOUT(ms) SETFIELD(PPC_BITMASK(0,
>>>> 15), 0, (ms))
>>>> +#define PSERIES_WDTQ_NUM(n) SETFIELD(PPC_BITMASK(16,
>>>> 31), 0, (n))
>>>> +
>>>> +/*
>>>> + * For the "Query watchdog LPM requirement" operation:
>>>> + * 1 = The given "watchdogNumber" must be stopped prior to suspending
>>>> + * 2 = The given "watchdogNumber" does not have to be stopped prior to
>>>> + * suspending
>>>> + */
>>>> +#define PSERIES_WDTQL_STOPPED 1
>>>> +#define PSERIES_WDTQL_QUERY_NOT_STOPPED 2
>>>> +
>>>> +#define WDT_MIN_TIMEOUT 1 /* 1ms */
>>>> +
>>>> +static void watchdog_expired(void *pw)
>>>> +{
>>>> + struct SpaprWatchdog *w = pw;
>>>
>>> s/struct//
>>>
>>>> + CPUState *cs;
>>>> +
>>>> + trace_spapr_watchdog_expired(w->num, w->action);
>>>> + switch (w->action) {
>>>> + case WDT_HARD_POWER_OFF:
>>>> + qemu_system_vmstop_request(RUN_STATE_SHUTDOWN);
>>>> + break;
>>>> + case WDT_HARD_RESTART:
>>>> + qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>>>> + break;
>>>> + case WDT_DUMP_RESTART:
>>>> + CPU_FOREACH(cs) {
>>>> + async_run_on_cpu(cs, spapr_do_system_reset_on_cpu,
>>>> RUN_ON_CPU_NULL);
>>>> + }
>>>> + break;
>>>> + }
>>>> +}
>>>> +
>>>> +static target_ulong watchdog_stop(unsigned watchdogNumber,
>>>> + struct SpaprWatchdog *w)
>>>> +{
>>>> + target_ulong ret = H_NOOP;
>>>> +
>>>> + if (timer_pending(&w->timer)) {
>>>> + timer_del(&w->timer);
>>>> + ret = H_SUCCESS;
>>>> + }
>>>> + trace_spapr_watchdog_stop(watchdogNumber, ret);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static target_ulong h_watchdog(PowerPCCPU *cpu,
>>>> + SpaprMachineState *spapr,
>>>> + target_ulong opcode, target_ulong
>>>> *args)
>>>> +{
>>>> + target_ulong flags = args[0];
>>>> + target_ulong watchdogNumber = args[1];
>>>> + target_ulong timeoutInMs = args[2];
>>>> + unsigned operation = flags & PSERIES_WDTF_OP(~0);
>>>> + unsigned timeoutAction = flags & PSERIES_WDTF_ACTION(~0);
>>>> + struct SpaprWatchdog *w;
>>>> +
>>>> + if (flags & PSERIES_WDTF_RESERVED) {
>>>> + return H_PARAMETER;
>>>> + }
>>>> +
>>>> + switch (operation) {
>>>> + case PSERIES_WDTF_OP_START:
>>>> + if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
>>>> + return H_P2;
>>>> + }
>>>> + if (timeoutInMs <= WDT_MIN_TIMEOUT) {
>>>> + return H_P3;
>>>> + }
>>>> +
>>>> + w = &spapr->wds[watchdogNumber - 1];
>>>
>>> So first index is 1 in PAPR ...
>>
>>
>> Yes, I thought I commented on this somewhere but lost in rebases.
>>
>>
>>>
>>>> + switch (timeoutAction) {
>>>> + case PSERIES_WDTF_ACTION_HARD_POWER_OFF:
>>>> + w->action = WDT_HARD_POWER_OFF;
>>>> + break;
>>>> + case PSERIES_WDTF_ACTION_HARD_RESTART:
>>>> + w->action = WDT_HARD_RESTART;
>>>> + break;
>>>> + case PSERIES_WDTF_ACTION_DUMP_RESTART:
>>>> + w->action = WDT_DUMP_RESTART;
>>>> + break;
>>>> + default:
>>>> + return H_PARAMETER;
>>>> + }
>>>> + timer_mod(&w->timer,
>>>> + qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
>>>> timeoutInMs);
>>>> + trace_spapr_watchdog_start(flags, watchdogNumber,
>>>> timeoutInMs);
>>>> + break;
>>>> + case PSERIES_WDTF_OP_STOP:
>>>> + if (watchdogNumber == (uint64_t) ~0) {
>>>
>>> May be add a define for this special value. It's better for readability.
>>
>> Will do.
>>
>>
>>>> + int i;
>>>> +
>>>> + for (i = 1; i <= ARRAY_SIZE(spapr->wds); ++i) {
>>>> + watchdog_stop(i, &spapr->wds[i - 1]);
>>>> + }
>>>> + } else if (watchdogNumber <= ARRAY_SIZE(spapr->wds)) {
>>>> + watchdog_stop(watchdogNumber,
>>>> &spapr->wds[watchdogNumber - 1]);
>>>> + } else {
>>>> + return H_P2;
>>>> + }
>>>> + break;
>>>> + case PSERIES_WDTF_OP_QUERY:
>>>> + args[0] = PSERIES_WDTQ_MIN_TIMEOUT(WDT_MIN_TIMEOUT) |
>>>> + PSERIES_WDTQ_NUM(ARRAY_SIZE(spapr->wds));
>>>> + trace_spapr_watchdog_query(args[0]);
>>>> + break;
>>>> + case PSERIES_WDTF_OP_QUERY_LPM:
>>>> + if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
>>>> + return H_P2;
>>>> + }
>>>> + args[0] = PSERIES_WDTQL_QUERY_NOT_STOPPED;
>>>> + trace_spapr_watchdog_query_lpm(args[0]);
>>>> + break;
>>>> + default:
>>>> + return H_PARAMETER;
>>>> + }
>>>> +
>>>> + return H_SUCCESS;
>>>> +}
>>>> +
>>>> +void spapr_watchdog_init(SpaprMachineState *spapr)
>>>
>>> This could have a 'Error **errp' parameter.
>>
>>
>> I was repeating somewhat similar spapr_rtc_create(), and the called -
>> spapr_machine_init() - does not have *errp. Seems pointless as it
>> fails - something is horrendously broken.
>
>
> Well, it's up to the caller to decide what to do in case of
> errors. If in this case, it is "log an error and exit",
> I would simply :
>
> spapr_watchdog_init(spapr, &error_fatal);
>
> But it is not necessarily fatal to fail to initialize some
> device.
The device is so simple and not configurable via the command line that
it is fatal. And it does not touch *errp anyway.
Passing errp also means spapr_watchdog_init() should do clean up of
whatever it did in a loop before hitting the error, for example in this
case - unrealize() previously realized watchdogs. This adds useless code
as it is going to be fatal anyway. Thanks,
>
>>
>>
>>>
>>>> +{
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < ARRAY_SIZE(spapr->wds); ++i) {
>>>> + char name[16];g_autofree char *name =
>>>> g_strdup_printf("wdt%d", i + 1);
>>>
>>>> + SpaprWatchdog *w = &spapr->wds[i];
>>>> +
>>>> + w->num = i + 1;
>>>
>>> it should be a property.
>>
>> This cannot change and used only for tracing, and the QOM name has the
>> number as well. I am replacing it with
>>
>> SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>> unsigned num = w - spapr->wds;
>>
>> and removing the @num from the struct.
>
> Nice !
>
> Thanks
>
> C.
>
>
>>
>> Thanks,
>>
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>>> + snprintf(name, sizeof(name) - 1, "wdt%d", i + 1);
>>>> + object_initialize_child_with_props(OBJECT(spapr), name, w,
>>>> + sizeof(SpaprWatchdog),
>>>> + TYPE_SPAPR_WDT,
>>>> + &error_fatal, NULL);
>>>> + qdev_realize(DEVICE(w), NULL, &error_fatal);
>>>> + }
>>>> +}
>>>> +
>>>> +static bool watchdog_needed(void *opaque)
>>>> +{
>>>> + SpaprWatchdog *w = opaque;
>>>> +
>>>> + return timer_pending(&w->timer);
>>>> +}
>>>> +
>>>> +static const VMStateDescription vmstate_wdt = {
>>>> + .name = "spapr_watchdog",
>>>> + .version_id = 1,
>>>> + .minimum_version_id = 1,
>>>> + .needed = watchdog_needed,
>>>> + .fields = (VMStateField[]) {
>>>> + VMSTATE_UINT8(action, SpaprWatchdog),
>>>> + VMSTATE_TIMER(timer, SpaprWatchdog),
>>>> + VMSTATE_END_OF_LIST()
>>>> + }
>>>> +};
>>>> +
>>>> +static void spapr_wdt_realize(DeviceState *dev, Error **errp)
>>>> +{
>>>> + SpaprWatchdog *w = SPAPR_WDT(dev);
>>>> +
>>>> + timer_init_ms(&w->timer, QEMU_CLOCK_VIRTUAL, watchdog_expired, w);
>>>> +
>>>> + object_property_add_uint64_ptr(OBJECT(dev), "expire",
>>>> + (uint64_t *)&w->timer.expire_time,
>>>> + OBJ_PROP_FLAG_READ);
>>>> + object_property_add_uint8_ptr(OBJECT(dev), "action", &w->action,
>>>> + OBJ_PROP_FLAG_READ);
>>>> +}
>>>> +
>>>> +static void spapr_wdt_class_init(ObjectClass *oc, void *data)
>>>> +{
>>>> + DeviceClass *dc = DEVICE_CLASS(oc);
>>>> +
>>>> + dc->renualize = spapr_wdt_realize;
>>>> + dc->vmsd = &vmstate_wdt;
>>>> + dc->user_creatable = false;
>>>> +}
>>>> +
>>>> +static const TypeInfo spapr_wdt_info = {
>>>> + .name = TYPE_SPAPR_WDT,
>>>> + .parent = TYPE_DEVICE,
>>>> + .instance_size = sizeof(SpaprWatchdog),
>>>> + .class_init = spapr_wdt_class_init,
>>>> +};
>>>> +
>>>> +static void spapr_watchdog_register_types(void)
>>>> +{
>>>> + spapr_register_hypercall(H_WATCHDOG, h_watchdog);
>>>> + type_register_static(&spapr_wdt_info);
>>>> +}
>>>> +
>>>> +type_init(spapr_watchdog_register_types)
>>>> diff --git a/hw/watchdog/meson.build b/hw/watchdog/meson.build
>>>> index 054c403dea7c..8974b5cf4c8a 100644
>>>> --- a/hw/watchdog/meson.build
>>>> +++ b/hw/watchdog/meson.build
>>>> @@ -6,3 +6,4 @@ softmmu_ss.add(when: 'CONFIG_WDT_DIAG288', if_true:
>>>> files('wdt_diag288.c'))
>>>> softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true:
>>>> files('wdt_aspeed.c'))
>>>> softmmu_ss.add(when: 'CONFIG_WDT_IMX2', if_true: files('wdt_imx2.c'))
>>>> softmmu_ss.add(when: 'CONFIG_WDT_SBSA', if_true:
>>>> files('sbsa_gwdt.c'))
>>>> +specific_ss.add(when: 'CONFIG_PSERIES', if_true:
>>>> files('spapr_watchdog.c'))
>>>> diff --git a/hw/watchdog/trace-events b/hw/watchdog/trace-events
>>>> index e7523e22aaf2..89ccbcfdfd20 100644
>>>> --- a/hw/watchdog/trace-events
>>>> +++ b/hw/watchdog/trace-events
>>>> @@ -9,3 +9,10 @@ cmsdk_apb_watchdog_lock(uint32_t lock) "CMSDK APB
>>>> watchdog: lock %" PRIu32
>>>> # wdt-aspeed.c
>>>> aspeed_wdt_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 "
>>>> size=%d"
>>>> aspeed_wdt_write(uint64_t addr, uint32_t size, uint64_t data)
>>>> "@0x%" PRIx64 " size=%d value=0x%"PRIx64
>>>> +
>>>> +# spapr_watchdog.c
>>>> +spapr_watchdog_start(uint64_t flags, uint64_t num, uint64_t
>>>> timeout) "Flags 0x%" PRIx64 " num=%" PRId64 " %" PRIu64 "ms"
>>>> +spapr_watchdog_stop(uint64_t num, uint64_t ret) "num=%" PRIu64 "
>>>> ret=%" PRId64
>>>> +spapr_watchdog_query(uint64_t caps) "caps=0x%" PRIx64
>>>> +spapr_watchdog_query_lpm(uint64_t caps) "caps=0x%" PRIx64
>>>> +spapr_watchdog_expired(uint64_t num, unsigned action) "num=%"
>>>> PRIu64 " action=%u"
>>
>
--
Alexey
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target
2022-06-20 8:10 ` Alexey Kardashevskiy
@ 2022-06-21 12:55 ` Daniel Henrique Barboza
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Henrique Barboza @ 2022-06-21 12:55 UTC (permalink / raw)
To: Alexey Kardashevskiy, Cédric Le Goater, qemu-ppc; +Cc: qemu-devel
On 6/20/22 05:10, Alexey Kardashevskiy wrote:
>
>
> On 6/20/22 16:17, Cédric Le Goater wrote:
>> On 6/20/22 05:37, Alexey Kardashevskiy wrote:
>>>
>>>
>>> On 6/18/22 02:50, Daniel Henrique Barboza wrote:
>>>>
>>>>
>>>> On 6/17/22 03:07, Alexey Kardashevskiy wrote:
>>>>> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
>>>>> allow using the macros in #define.
>>>>>
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>> ---
>>>>
>>>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>
>>>
>>>
>>> soooo I do not need to repost it, right?
>>>
>>>
>>
>> The watchdog patch depends on the availability of these macros.
>> Doesn't it ?
>
> yes but this one could go in now and I will repost 2/2 a little later on top.
As soon as the current ppc PR is merged I can take this patch. If the 2/2 repost
happens earlier than that you can just re-submit it with 1/2 again.
Thanks,
Daniel
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target
2022-06-17 6:07 ` [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target Alexey Kardashevskiy
2022-06-17 16:50 ` Daniel Henrique Barboza
2022-06-18 10:36 ` Cédric Le Goater
@ 2022-06-21 12:59 ` Peter Maydell
2022-06-24 20:12 ` Daniel Henrique Barboza
3 siblings, 0 replies; 21+ messages in thread
From: Peter Maydell @ 2022-06-21 12:59 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: qemu-ppc, qemu-devel, Daniel Henrique Barboza, Cédric Le Goater
On Fri, 17 Jun 2022 at 07:20, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
> allow using the macros in #define.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
> target/ppc/cpu.h | 5 +++++
> hw/intc/pnv_xive.c | 20 --------------------
> hw/intc/pnv_xive2.c | 20 --------------------
> hw/pci-host/pnv_phb4.c | 16 ----------------
> 5 files changed, 5 insertions(+), 72 deletions(-)
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 6d78078f379d..9a1f1e9999a3 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -47,6 +47,11 @@
> PPC_BIT32(bs))
> #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs))
>
> +#define GETFIELD(mask, word) \
> + (((word) & (mask)) >> __builtin_ctzl(mask))
> +#define SETFIELD(mask, word, val) \
> + (((word) & ~(mask)) | (((uint64_t)(val) << __builtin_ctzl(mask)) & (mask)))
Can we retain the explanatory comment that says why we don't
use the standard QEMU mechanism for field extraction
(ie the FIELD_EX*/FIELD_DP* macros and the extract64()/deposit64()
functions) ?
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * TODO: It might be better to use the existing extract64() and
> - * deposit64() but this means that all the register definitions will
> - * change and become incompatible with the ones found in skiboot.
> - *
> - * Keep it as it is for now until we find a common ground.
> - */
thanks
-- PMM
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target
2022-06-17 6:07 ` [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target Alexey Kardashevskiy
` (2 preceding siblings ...)
2022-06-21 12:59 ` Peter Maydell
@ 2022-06-24 20:12 ` Daniel Henrique Barboza
2022-06-27 4:54 ` Alexey Kardashevskiy
3 siblings, 1 reply; 21+ messages in thread
From: Daniel Henrique Barboza @ 2022-06-24 20:12 UTC (permalink / raw)
To: Alexey Kardashevskiy, qemu-ppc; +Cc: qemu-devel, Cédric Le Goater
Alexey,
The newer version of this patch is having trouble with Gitlab runners, as
you can read in my feedback there.
I've tested this one just in case. The same problems happen. E.g. for the
cross-armel-system runner:
In file included from ../hw/intc/pnv_xive.c:14:
../hw/intc/pnv_xive.c: In function ‘pnv_xive_block_id’:
/builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘4222124650659840’ to ‘0’ [-Werror=overflow]
45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of macro ‘GETFIELD’
51 | (((word) & (mask)) >> __builtin_ctzl(mask))
| ^~~~
../hw/intc/pnv_xive_regs.h:77:41: note: in expansion of macro ‘PPC_BITMASK’
77 | #define PC_TCTXT_CHIPID PPC_BITMASK(12, 15)
| ^~~~~~~~~~~
../hw/intc/pnv_xive.c:80:24: note: in expansion of macro ‘PC_TCTXT_CHIPID’
80 | blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val);
| ^~~~~~~~~~~~~~~
../hw/intc/pnv_xive.c: In function ‘pnv_xive_vst_addr’:
/builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘13835058055282163712’ to ‘0’ [-Werror=overflow]
45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of macro ‘GETFIELD’
51 | (((word) & (mask)) >> __builtin_ctzl(mask))
| ^~~~
../hw/intc/pnv_xive_regs.h:230:33: note: in expansion of macro ‘PPC_BITMASK’
230 | #define VSD_MODE PPC_BITMASK(0, 1)
| ^~~~~~~~~~~
../hw/intc/pnv_xive.c:226:18: note: in expansion of macro ‘VSD_MODE’
226 | if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
| ^~~~~~~~
../hw/intc/pnv_xive.c: In function ‘pnv_xive_end_update’:
Link:
https://gitlab.com/danielhb/qemu/-/jobs/2637716673
I don´t know how to deal with that.
For the record: if this is too troublesome to fix, I am ok with just consolidating
the GETFIELD and SETFIELD inlines we already have, under cpu.h, keeping them exactly
as they are today (functions, not macros).
Thanks,
Daniel
On 6/17/22 03:07, Alexey Kardashevskiy wrote:
> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
> allow using the macros in #define.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
> target/ppc/cpu.h | 5 +++++
> hw/intc/pnv_xive.c | 20 --------------------
> hw/intc/pnv_xive2.c | 20 --------------------
> hw/pci-host/pnv_phb4.c | 16 ----------------
> 5 files changed, 5 insertions(+), 72 deletions(-)
>
> diff --git a/include/hw/pci-host/pnv_phb3_regs.h b/include/hw/pci-host/pnv_phb3_regs.h
> index a174ef1f7045..38f8ce9d7406 100644
> --- a/include/hw/pci-host/pnv_phb3_regs.h
> +++ b/include/hw/pci-host/pnv_phb3_regs.h
> @@ -12,22 +12,6 @@
>
> #include "qemu/host-utils.h"
>
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * These are common with the PnvXive model.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> - return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> - uint64_t value)
> -{
> - return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
> /*
> * PBCQ XSCOM registers
> */
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 6d78078f379d..9a1f1e9999a3 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -47,6 +47,11 @@
> PPC_BIT32(bs))
> #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs))
>
> +#define GETFIELD(mask, word) \
> + (((word) & (mask)) >> __builtin_ctzl(mask))
> +#define SETFIELD(mask, word, val) \
> + (((word) & ~(mask)) | (((uint64_t)(val) << __builtin_ctzl(mask)) & (mask)))
> +
> /*****************************************************************************/
> /* Exception vectors definitions */
> enum {
> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> index 1ce1d7b07d63..c7b75ed12ee0 100644
> --- a/hw/intc/pnv_xive.c
> +++ b/hw/intc/pnv_xive.c
> @@ -66,26 +66,6 @@ static const XiveVstInfo vst_infos[] = {
> qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n", \
> (xive)->chip->chip_id, ## __VA_ARGS__);
>
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * TODO: It might be better to use the existing extract64() and
> - * deposit64() but this means that all the register definitions will
> - * change and become incompatible with the ones found in skiboot.
> - *
> - * Keep it as it is for now until we find a common ground.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> - return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> - uint64_t value)
> -{
> - return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
> /*
> * When PC_TCTXT_CHIPID_OVERRIDE is configured, the PC_TCTXT_CHIPID
> * field overrides the hardwired chip ID in the Powerbus operations
> diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
> index a39e070e82d2..3fe349749384 100644
> --- a/hw/intc/pnv_xive2.c
> +++ b/hw/intc/pnv_xive2.c
> @@ -75,26 +75,6 @@ static const XiveVstInfo vst_infos[] = {
> qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n", \
> (xive)->chip->chip_id, ## __VA_ARGS__);
>
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * TODO: It might be better to use the existing extract64() and
> - * deposit64() but this means that all the register definitions will
> - * change and become incompatible with the ones found in skiboot.
> - *
> - * Keep it as it is for now until we find a common ground.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> - return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> - uint64_t value)
> -{
> - return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
> /*
> * TODO: Document block id override
> */
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 13ba9e45d8b6..0913e7c8f015 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -31,22 +31,6 @@
> qemu_log_mask(LOG_GUEST_ERROR, "phb4_pec[%d:%d]: " fmt "\n", \
> (pec)->chip_id, (pec)->index, ## __VA_ARGS__)
>
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * These are common with the PnvXive model.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> - return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> - uint64_t value)
> -{
> - return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
> static PCIDevice *pnv_phb4_find_cfg_dev(PnvPHB4 *phb)
> {
> PCIHostState *pci = PCI_HOST_BRIDGE(phb);
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target
2022-06-24 20:12 ` Daniel Henrique Barboza
@ 2022-06-27 4:54 ` Alexey Kardashevskiy
2022-06-27 17:37 ` Daniel Henrique Barboza
2022-06-27 18:04 ` Daniel Henrique Barboza
0 siblings, 2 replies; 21+ messages in thread
From: Alexey Kardashevskiy @ 2022-06-27 4:54 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-ppc; +Cc: qemu-devel, Cédric Le Goater
On 6/25/22 06:12, Daniel Henrique Barboza wrote:
> Alexey,
>
> The newer version of this patch is having trouble with Gitlab runners, as
> you can read in my feedback there.
>
> I've tested this one just in case. The same problems happen. E.g. for the
> cross-armel-system runner:
>
>
> In file included from ../hw/intc/pnv_xive.c:14:
> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_block_id’:
> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from
> ‘long long unsigned int’ to ‘long unsigned int’ changes value from
> ‘4222124650659840’ to ‘0’ [-Werror=overflow]
> 45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) |
> PPC_BIT(bs))
> |
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of
> macro ‘GETFIELD’
> 51 | (((word) & (mask)) >> __builtin_ctzl(mask))
> | ^~~~
> ../hw/intc/pnv_xive_regs.h:77:41: note: in expansion of macro ‘PPC_BITMASK’
> 77 | #define PC_TCTXT_CHIPID PPC_BITMASK(12, 15)
> | ^~~~~~~~~~~
> ../hw/intc/pnv_xive.c:80:24: note: in expansion of macro ‘PC_TCTXT_CHIPID’
> 80 | blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val);
> | ^~~~~~~~~~~~~~~
> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_vst_addr’:
> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from
> ‘long long unsigned int’ to ‘long unsigned int’ changes value from
> ‘13835058055282163712’ to ‘0’ [-Werror=overflow]
> 45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) |
> PPC_BIT(bs))
> |
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of
> macro ‘GETFIELD’
> 51 | (((word) & (mask)) >> __builtin_ctzl(mask))
> | ^~~~
> ../hw/intc/pnv_xive_regs.h:230:33: note: in expansion of macro
> ‘PPC_BITMASK’
> 230 | #define VSD_MODE PPC_BITMASK(0, 1)
> | ^~~~~~~~~~~
> ../hw/intc/pnv_xive.c:226:18: note: in expansion of macro ‘VSD_MODE’
> 226 | if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
> | ^~~~~~~~
> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_end_update’:
>
>
> Link:
>
> https://gitlab.com/danielhb/qemu/-/jobs/2637716673
>
>
> I don´t know how to deal with that.
>
>
> For the record: if this is too troublesome to fix, I am ok with just
> consolidating
> the GETFIELD and SETFIELD inlines we already have, under cpu.h, keeping
> them exactly
> as they are today (functions, not macros).
>
>
> Thanks,
>
>
> Daniel
>
>
>
> On 6/17/22 03:07, Alexey Kardashevskiy wrote:
>> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
>> allow using the macros in #define.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
>> target/ppc/cpu.h | 5 +++++
>> hw/intc/pnv_xive.c | 20 --------------------
>> hw/intc/pnv_xive2.c | 20 --------------------
>> hw/pci-host/pnv_phb4.c | 16 ----------------
>> 5 files changed, 5 insertions(+), 72 deletions(-)
>>
>> diff --git a/include/hw/pci-host/pnv_phb3_regs.h
>> b/include/hw/pci-host/pnv_phb3_regs.h
>> index a174ef1f7045..38f8ce9d7406 100644
>> --- a/include/hw/pci-host/pnv_phb3_regs.h
>> +++ b/include/hw/pci-host/pnv_phb3_regs.h
>> @@ -12,22 +12,6 @@
>> #include "qemu/host-utils.h"
>> -/*
>> - * QEMU version of the GETFIELD/SETFIELD macros
>> - *
>> - * These are common with the PnvXive model.
>> - */
>> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
>> -{
>> - return (word & mask) >> ctz64(mask);
>> -}
>> -
>> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
>> - uint64_t value)
>> -{
>> - return (word & ~mask) | ((value << ctz64(mask)) & mask);
>> -}
>> -
>> /*
>> * PBCQ XSCOM registers
>> */
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 6d78078f379d..9a1f1e9999a3 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -47,6 +47,11 @@
>> PPC_BIT32(bs))
>> #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) |
>> PPC_BIT8(bs))
>> +#define GETFIELD(mask, word) \
>> + (((word) & (mask)) >> __builtin_ctzl(mask))
Replacing __builtin_ctzl with __builtin_ctzll seems fixing it though, do
you have a quick way to test this? Gitlab's CI takes time :)
https://gitlab.com/aik1/qemu/-/pipelines/573497191 is the current run.
Thanks,
--
Alexey
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target
2022-06-27 4:54 ` Alexey Kardashevskiy
@ 2022-06-27 17:37 ` Daniel Henrique Barboza
2022-06-27 18:04 ` Daniel Henrique Barboza
1 sibling, 0 replies; 21+ messages in thread
From: Daniel Henrique Barboza @ 2022-06-27 17:37 UTC (permalink / raw)
To: Alexey Kardashevskiy, qemu-ppc; +Cc: qemu-devel, Cédric Le Goater
On 6/27/22 01:54, Alexey Kardashevskiy wrote:
>
>
> On 6/25/22 06:12, Daniel Henrique Barboza wrote:
>> Alexey,
>>
>> The newer version of this patch is having trouble with Gitlab runners, as
>> you can read in my feedback there.
>>
>> I've tested this one just in case. The same problems happen. E.g. for the
>> cross-armel-system runner:
>>
>>
>> In file included from ../hw/intc/pnv_xive.c:14:
>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_block_id’:
>> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘4222124650659840’ to ‘0’ [-Werror=overflow]
>> 45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of macro ‘GETFIELD’
>> 51 | (((word) & (mask)) >> __builtin_ctzl(mask))
>> | ^~~~
>> ../hw/intc/pnv_xive_regs.h:77:41: note: in expansion of macro ‘PPC_BITMASK’
>> 77 | #define PC_TCTXT_CHIPID PPC_BITMASK(12, 15)
>> | ^~~~~~~~~~~
>> ../hw/intc/pnv_xive.c:80:24: note: in expansion of macro ‘PC_TCTXT_CHIPID’
>> 80 | blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val);
>> | ^~~~~~~~~~~~~~~
>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_vst_addr’:
>> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘13835058055282163712’ to ‘0’ [-Werror=overflow]
>> 45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of macro ‘GETFIELD’
>> 51 | (((word) & (mask)) >> __builtin_ctzl(mask))
>> | ^~~~
>> ../hw/intc/pnv_xive_regs.h:230:33: note: in expansion of macro ‘PPC_BITMASK’
>> 230 | #define VSD_MODE PPC_BITMASK(0, 1)
>> | ^~~~~~~~~~~
>> ../hw/intc/pnv_xive.c:226:18: note: in expansion of macro ‘VSD_MODE’
>> 226 | if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
>> | ^~~~~~~~
>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_end_update’:
>>
>>
>> Link:
>>
>> https://gitlab.com/danielhb/qemu/-/jobs/2637716673
>>
>>
>> I don´t know how to deal with that.
>>
>>
>> For the record: if this is too troublesome to fix, I am ok with just consolidating
>> the GETFIELD and SETFIELD inlines we already have, under cpu.h, keeping them exactly
>> as they are today (functions, not macros).
>>
>>
>> Thanks,
>>
>>
>> Daniel
>>
>>
>>
>> On 6/17/22 03:07, Alexey Kardashevskiy wrote:
>>> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
>>> allow using the macros in #define.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>> include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
>>> target/ppc/cpu.h | 5 +++++
>>> hw/intc/pnv_xive.c | 20 --------------------
>>> hw/intc/pnv_xive2.c | 20 --------------------
>>> hw/pci-host/pnv_phb4.c | 16 ----------------
>>> 5 files changed, 5 insertions(+), 72 deletions(-)
>>>
>>> diff --git a/include/hw/pci-host/pnv_phb3_regs.h b/include/hw/pci-host/pnv_phb3_regs.h
>>> index a174ef1f7045..38f8ce9d7406 100644
>>> --- a/include/hw/pci-host/pnv_phb3_regs.h
>>> +++ b/include/hw/pci-host/pnv_phb3_regs.h
>>> @@ -12,22 +12,6 @@
>>> #include "qemu/host-utils.h"
>>> -/*
>>> - * QEMU version of the GETFIELD/SETFIELD macros
>>> - *
>>> - * These are common with the PnvXive model.
>>> - */
>>> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
>>> -{
>>> - return (word & mask) >> ctz64(mask);
>>> -}
>>> -
>>> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
>>> - uint64_t value)
>>> -{
>>> - return (word & ~mask) | ((value << ctz64(mask)) & mask);
>>> -}
>>> -
>>> /*
>>> * PBCQ XSCOM registers
>>> */
>>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>>> index 6d78078f379d..9a1f1e9999a3 100644
>>> --- a/target/ppc/cpu.h
>>> +++ b/target/ppc/cpu.h
>>> @@ -47,6 +47,11 @@
>>> PPC_BIT32(bs))
>>> #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs))
>>> +#define GETFIELD(mask, word) \
>>> + (((word) & (mask)) >> __builtin_ctzl(mask))
>
>
> Replacing __builtin_ctzl with __builtin_ctzll seems fixing it though, do you have a quick way to test this? Gitlab's CI takes time :)
> https://gitlab.com/aik1/qemu/-/pipelines/573497191 is the current run.
I'll take a look, but apparently it fixed the problem I reported up above.
Also, there's an error in the msys2-64bit runner that I keep seeing from time to
time. It goes away eventually if you keep retrying it:
[301/1664] Generating input-keymap-qcode-to-qnum.c.inc with a custom command (wrapped by meson to capture output)
[302/1664] Generating input-keymap-qcode-to-sun.c.inc with a custom command (wrapped by meson to capture output)
[303/1664] Generating input-keymap-qnum-to-qcode.c.inc with a custom command (wrapped by meson to capture output)
[304/1664] Generating input-keymap-usb-to-qcode.c.inc with a custom command (wrapped by meson to capture output)
FAILED: ui/input-keymap-usb-to-qcode.c.inc
"C:/GitLab-Runner/builds/aik1/qemu/msys64/mingw64/bin/python3.exe" "C:/GitLab-Runner/builds/aik1/qemu/meson/meson.py" "--internal" "exe" "--capture" "ui/input-keymap-usb-to-qcode.c.inc" "--" "C:/GitLab-Runner/builds/aik1/qemu/msys64/mingw64/bin/python3.exe" "../ui/keycodemapdb/tools/keymap-gen" "code-map" "--lang" "glib2" "--varname" "qemu_input_map_usb_to_qcode" "../ui/keycodemapdb/data/keymaps.csv" "usb" "qcode"
[305/1664] Generating input-keymap-win32-to-qcode.c.inc with a custom command (wrapped by meson to capture output)
ninja: build stopped: subcommand failed.
make[1]: Leaving directory '/c/GitLab-Runner/builds/aik1/qemu/build'
make[1]: *** [Makefile:162: run-ninja] Error 1
make: *** [GNUmakefile:11: all] Error 2
It's a strange one because it's an error triggered by an ui/keymap file which you're
not changing.
Richard already created a thread about it in the QEMU ML, so I'll assume that
this has nothing to do with powerpc code.
Thanks,
Daniel
> Thanks,
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target
2022-06-27 4:54 ` Alexey Kardashevskiy
2022-06-27 17:37 ` Daniel Henrique Barboza
@ 2022-06-27 18:04 ` Daniel Henrique Barboza
2022-06-28 2:57 ` Alexey Kardashevskiy
1 sibling, 1 reply; 21+ messages in thread
From: Daniel Henrique Barboza @ 2022-06-27 18:04 UTC (permalink / raw)
To: Alexey Kardashevskiy, qemu-ppc; +Cc: qemu-devel, Cédric Le Goater
On 6/27/22 01:54, Alexey Kardashevskiy wrote:
>
>
> On 6/25/22 06:12, Daniel Henrique Barboza wrote:
>> Alexey,
>>
>> The newer version of this patch is having trouble with Gitlab runners, as
>> you can read in my feedback there.
>>
>> I've tested this one just in case. The same problems happen. E.g. for the
>> cross-armel-system runner:
>>
>>
>> In file included from ../hw/intc/pnv_xive.c:14:
>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_block_id’:
>> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘4222124650659840’ to ‘0’ [-Werror=overflow]
>> 45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of macro ‘GETFIELD’
>> 51 | (((word) & (mask)) >> __builtin_ctzl(mask))
>> | ^~~~
>> ../hw/intc/pnv_xive_regs.h:77:41: note: in expansion of macro ‘PPC_BITMASK’
>> 77 | #define PC_TCTXT_CHIPID PPC_BITMASK(12, 15)
>> | ^~~~~~~~~~~
>> ../hw/intc/pnv_xive.c:80:24: note: in expansion of macro ‘PC_TCTXT_CHIPID’
>> 80 | blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val);
>> | ^~~~~~~~~~~~~~~
>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_vst_addr’:
>> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘13835058055282163712’ to ‘0’ [-Werror=overflow]
>> 45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of macro ‘GETFIELD’
>> 51 | (((word) & (mask)) >> __builtin_ctzl(mask))
>> | ^~~~
>> ../hw/intc/pnv_xive_regs.h:230:33: note: in expansion of macro ‘PPC_BITMASK’
>> 230 | #define VSD_MODE PPC_BITMASK(0, 1)
>> | ^~~~~~~~~~~
>> ../hw/intc/pnv_xive.c:226:18: note: in expansion of macro ‘VSD_MODE’
>> 226 | if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
>> | ^~~~~~~~
>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_end_update’:
>>
>>
>> Link:
>>
>> https://gitlab.com/danielhb/qemu/-/jobs/2637716673
>>
>>
>> I don´t know how to deal with that.
>>
>>
>> For the record: if this is too troublesome to fix, I am ok with just consolidating
>> the GETFIELD and SETFIELD inlines we already have, under cpu.h, keeping them exactly
>> as they are today (functions, not macros).
>>
>>
>> Thanks,
>>
>>
>> Daniel
>>
>>
>>
>> On 6/17/22 03:07, Alexey Kardashevskiy wrote:
>>> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
>>> allow using the macros in #define.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>> include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
>>> target/ppc/cpu.h | 5 +++++
>>> hw/intc/pnv_xive.c | 20 --------------------
>>> hw/intc/pnv_xive2.c | 20 --------------------
>>> hw/pci-host/pnv_phb4.c | 16 ----------------
>>> 5 files changed, 5 insertions(+), 72 deletions(-)
>>>
>>> diff --git a/include/hw/pci-host/pnv_phb3_regs.h b/include/hw/pci-host/pnv_phb3_regs.h
>>> index a174ef1f7045..38f8ce9d7406 100644
>>> --- a/include/hw/pci-host/pnv_phb3_regs.h
>>> +++ b/include/hw/pci-host/pnv_phb3_regs.h
>>> @@ -12,22 +12,6 @@
>>> #include "qemu/host-utils.h"
>>> -/*
>>> - * QEMU version of the GETFIELD/SETFIELD macros
>>> - *
>>> - * These are common with the PnvXive model.
>>> - */
>>> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
>>> -{
>>> - return (word & mask) >> ctz64(mask);
>>> -}
>>> -
>>> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
>>> - uint64_t value)
>>> -{
>>> - return (word & ~mask) | ((value << ctz64(mask)) & mask);
>>> -}
>>> -
>>> /*
>>> * PBCQ XSCOM registers
>>> */
>>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>>> index 6d78078f379d..9a1f1e9999a3 100644
>>> --- a/target/ppc/cpu.h
>>> +++ b/target/ppc/cpu.h
>>> @@ -47,6 +47,11 @@
>>> PPC_BIT32(bs))
>>> #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs))
>>> +#define GETFIELD(mask, word) \
>>> + (((word) & (mask)) >> __builtin_ctzl(mask))
>
>
> Replacing __builtin_ctzl with __builtin_ctzll seems fixing it though, do you have a quick way to test this? Gitlab's CI takes time :)
> https://gitlab.com/aik1/qemu/-/pipelines/573497191 is the current run.
> Thanks,
This worked for me as well.
Can you re-send this patch with this fix plus the extra comment Peter mentioned
in his review?
----
Can we retain the explanatory comment that says why we don't
use the standard QEMU mechanism for field extraction
(ie the FIELD_EX*/FIELD_DP* macros and the extract64()/deposit64()
functions) ?
------
You can re-send this as v4 (I'm assuming that we're giving up trying to copy
the Skiboot macros) and then I'll take the patch together with the watchdog
v3 implementation.
Thanks,
Daniel
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target
2022-06-27 18:04 ` Daniel Henrique Barboza
@ 2022-06-28 2:57 ` Alexey Kardashevskiy
0 siblings, 0 replies; 21+ messages in thread
From: Alexey Kardashevskiy @ 2022-06-28 2:57 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-ppc; +Cc: qemu-devel, Cédric Le Goater
On 6/28/22 04:04, Daniel Henrique Barboza wrote:
>
>
> On 6/27/22 01:54, Alexey Kardashevskiy wrote:
>>
>>
>> On 6/25/22 06:12, Daniel Henrique Barboza wrote:
>>> Alexey,
>>>
>>> The newer version of this patch is having trouble with Gitlab
>>> runners, as
>>> you can read in my feedback there.
>>>
>>> I've tested this one just in case. The same problems happen. E.g. for
>>> the
>>> cross-armel-system runner:
>>>
>>>
>>> In file included from ../hw/intc/pnv_xive.c:14:
>>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_block_id’:
>>> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from
>>> ‘long long unsigned int’ to ‘long unsigned int’ changes value from
>>> ‘4222124650659840’ to ‘0’ [-Werror=overflow]
>>> 45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be))
>>> | PPC_BIT(bs))
>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of
>>> macro ‘GETFIELD’
>>> 51 | (((word) & (mask)) >> __builtin_ctzl(mask))
>>> | ^~~~
>>> ../hw/intc/pnv_xive_regs.h:77:41: note: in expansion of macro
>>> ‘PPC_BITMASK’
>>> 77 | #define PC_TCTXT_CHIPID PPC_BITMASK(12, 15)
>>> | ^~~~~~~~~~~
>>> ../hw/intc/pnv_xive.c:80:24: note: in expansion of macro
>>> ‘PC_TCTXT_CHIPID’
>>> 80 | blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val);
>>> | ^~~~~~~~~~~~~~~
>>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_vst_addr’:
>>> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from
>>> ‘long long unsigned int’ to ‘long unsigned int’ changes value from
>>> ‘13835058055282163712’ to ‘0’ [-Werror=overflow]
>>> 45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be))
>>> | PPC_BIT(bs))
>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of
>>> macro ‘GETFIELD’
>>> 51 | (((word) & (mask)) >> __builtin_ctzl(mask))
>>> | ^~~~
>>> ../hw/intc/pnv_xive_regs.h:230:33: note: in expansion of macro
>>> ‘PPC_BITMASK’
>>> 230 | #define VSD_MODE PPC_BITMASK(0, 1)
>>> | ^~~~~~~~~~~
>>> ../hw/intc/pnv_xive.c:226:18: note: in expansion of macro ‘VSD_MODE’
>>> 226 | if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
>>> | ^~~~~~~~
>>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_end_update’:
>>>
>>>
>>> Link:
>>>
>>> https://gitlab.com/danielhb/qemu/-/jobs/2637716673
>>>
>>>
>>> I don´t know how to deal with that.
>>>
>>>
>>> For the record: if this is too troublesome to fix, I am ok with just
>>> consolidating
>>> the GETFIELD and SETFIELD inlines we already have, under cpu.h,
>>> keeping them exactly
>>> as they are today (functions, not macros).
>>>
>>>
>>> Thanks,
>>>
>>>
>>> Daniel
>>>
>>>
>>>
>>> On 6/17/22 03:07, Alexey Kardashevskiy wrote:
>>>> It keeps repeating, move it to the header. This uses
>>>> __builtin_ctzl() to
>>>> allow using the macros in #define.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>> include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
>>>> target/ppc/cpu.h | 5 +++++
>>>> hw/intc/pnv_xive.c | 20 --------------------
>>>> hw/intc/pnv_xive2.c | 20 --------------------
>>>> hw/pci-host/pnv_phb4.c | 16 ----------------
>>>> 5 files changed, 5 insertions(+), 72 deletions(-)
>>>>
>>>> diff --git a/include/hw/pci-host/pnv_phb3_regs.h
>>>> b/include/hw/pci-host/pnv_phb3_regs.h
>>>> index a174ef1f7045..38f8ce9d7406 100644
>>>> --- a/include/hw/pci-host/pnv_phb3_regs.h
>>>> +++ b/include/hw/pci-host/pnv_phb3_regs.h
>>>> @@ -12,22 +12,6 @@
>>>> #include "qemu/host-utils.h"
>>>> -/*
>>>> - * QEMU version of the GETFIELD/SETFIELD macros
>>>> - *
>>>> - * These are common with the PnvXive model.
>>>> - */
>>>> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
>>>> -{
>>>> - return (word & mask) >> ctz64(mask);
>>>> -}
>>>> -
>>>> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
>>>> - uint64_t value)
>>>> -{
>>>> - return (word & ~mask) | ((value << ctz64(mask)) & mask);
>>>> -}
>>>> -
>>>> /*
>>>> * PBCQ XSCOM registers
>>>> */
>>>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>>>> index 6d78078f379d..9a1f1e9999a3 100644
>>>> --- a/target/ppc/cpu.h
>>>> +++ b/target/ppc/cpu.h
>>>> @@ -47,6 +47,11 @@
>>>> PPC_BIT32(bs))
>>>> #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) |
>>>> PPC_BIT8(bs))
>>>> +#define GETFIELD(mask, word) \
>>>> + (((word) & (mask)) >> __builtin_ctzl(mask))
>>
>>
>> Replacing __builtin_ctzl with __builtin_ctzll seems fixing it though,
>> do you have a quick way to test this? Gitlab's CI takes time :)
>> https://gitlab.com/aik1/qemu/-/pipelines/573497191 is the current run.
>> Thanks,
>
> This worked for me as well.
>
> Can you re-send this patch with this fix plus the extra comment Peter
> mentioned
> in his review?
>
> ----
> Can we retain the explanatory comment that says why we don't
> use the standard QEMU mechanism for field extraction
> (ie the FIELD_EX*/FIELD_DP* macros and the extract64()/deposit64()
> functions) ?
> ------
>
>
> You can re-send this as v4 (I'm assuming that we're giving up trying to
> copy
> the Skiboot macros) and
Sure, will do.
> then I'll take the patch together with the watchdog
> v3 implementation.
The watchdog patch is not using these macros anymore, I moved to QEMU's
macros, this is why separate patches. Thanks,
>
>
> Thanks,
>
> Daniel
>
>>
>>
--
Alexey
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2022-06-28 2:58 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-17 6:07 [PATCH qemu v2 0/2] ppc/spapr: Implement H_WATCHDOG Alexey Kardashevskiy
2022-06-17 6:07 ` [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target Alexey Kardashevskiy
2022-06-17 16:50 ` Daniel Henrique Barboza
2022-06-20 3:37 ` Alexey Kardashevskiy
2022-06-20 6:17 ` Cédric Le Goater
2022-06-20 8:10 ` Alexey Kardashevskiy
2022-06-21 12:55 ` Daniel Henrique Barboza
2022-06-18 10:36 ` Cédric Le Goater
2022-06-21 12:59 ` Peter Maydell
2022-06-24 20:12 ` Daniel Henrique Barboza
2022-06-27 4:54 ` Alexey Kardashevskiy
2022-06-27 17:37 ` Daniel Henrique Barboza
2022-06-27 18:04 ` Daniel Henrique Barboza
2022-06-28 2:57 ` Alexey Kardashevskiy
2022-06-17 6:07 ` [PATCH qemu v2 2/2] ppc/spapr: Implement H_WATCHDOG Alexey Kardashevskiy
2022-06-17 16:49 ` Daniel Henrique Barboza
2022-06-18 11:01 ` Cédric Le Goater
2022-06-20 3:13 ` Alexey Kardashevskiy
2022-06-20 6:23 ` Cédric Le Goater
2022-06-20 8:28 ` Alexey Kardashevskiy
2022-06-17 16:51 ` [PATCH qemu v2 0/2] " Daniel Henrique Barboza
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.