[PATCH 0/9] add LOG_UNSUPP log type + mark hcalls as unsupp

[PATCH 0/9] add LOG_UNSUPP log type + mark hcalls as unsupp

[Daniel Henrique Barboza]

Hi,

I got a lot of noise trying to debug an AIX guest in a pseries machine when running with 
'-d unimp'. The reason is that there is no distinction between features
(in my case, hypercalls) that are unimplemented because we never considered,
versus features that we made a design choice not to implement.

This series adds a new log type, LOG_UNSUPP, as a way to filter the
second case. After changing the log level of existing unsupported
pseries hypercalls, -d unimp was reporting just the ones that I need to
worry about and decide whether we should implement it or mark as
unsupported in our model. After this series there's still one hypercall
thgat is being thrown by AIX. We'll deal with that another day.

I am posting this a little too close of the soft freeze. If we manage
to get it reviewed in time, since it's a debug QoL change that doesn't
break anything, I'd say we take it. 

Also, I didn't find the maintainer for the util/log.c file. Let me know
if I need to CC someone in special for this file.

Daniel Henrique Barboza (9):
  util/log.c: add LOG_UNSUPP type
  hw/ppc/spapr_hcall.c: log h_clean_slb() as unsupported
  hw/ppc/spapr_hcall.c: log h_invalidate_pid() as unsupported
  hw/ppc/spapr_hcall.c: log h_copy_tofrom_guest() as unsupported
  hw/ppc/spapr_hcall.c: log H_GET_EM_PARMS as unsupported
  hw/ppc/spapr_hcall.c: log H_BEST_ENERGY as unsupported
  hw/ppc/spapr_hcall.c: log H_QUERY_VAS_CAPABILITIES as unsupported
  hw/ppc/spapr_hcall.c: log H_GET_PPP as unsupported
  hw/ppc/spapr_hcall.c: log H_VIOCTL as unsupported

 hw/ppc/spapr_hcall.c   | 98 +++++++++++++++++++++++++++++++++++++++++-
 include/hw/ppc/spapr.h |  3 ++
 include/qemu/log.h     |  3 +-
 util/log.c             |  2 +
 4 files changed, 103 insertions(+), 3 deletions(-)

-- 
2.35.1

[PATCH 1/9] util/log.c: add LOG_UNSUPP type

[Daniel Henrique Barboza]

The existing log type 'LOG_UNIMP' is being used as a way to indicate
that a certain feature is not implemented, and to indicate that a
feature is unsupported. Most of the time both cases are similar, until
you want to debug a guest that is running a not so common OS (e.g. AIX
guest in a pseries machine).

The result is that you can be overwhelmed with lots of '-d unimp'
messages of hypercalls and have to do code searches to verify  whether a
specific hypercall is something we never considered adding versus
something that we decided to not support. Note that while the first case
is eligible for further investigation the second is already settled. It
would be helpful to able to distinguish between both.

This patch adds a new log type called LOG_UNSUPP to represent this
subset of features that we are aware of and we decided to not support.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 include/qemu/log.h | 3 ++-
 util/log.c         | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/qemu/log.h b/include/qemu/log.h
index 9b80660207..884a81495b 100644
--- a/include/qemu/log.h
+++ b/include/qemu/log.h
@@ -14,7 +14,7 @@ typedef struct QemuLogFile {
 extern QemuLogFile *qemu_logfile;
 
 
-/* 
+/*
  * The new API:
  *
  */
@@ -64,6 +64,7 @@ static inline bool qemu_log_separate(void)
 #define CPU_LOG_PLUGIN     (1 << 18)
 /* LOG_STRACE is used for user-mode strace logging. */
 #define LOG_STRACE         (1 << 19)
+#define LOG_UNSUPP         (1 << 20)
 
 /* Lock output for a series of related logs.  Since this is not needed
  * for a single qemu_log / qemu_log_mask / qemu_log_mask_and_addr, we
diff --git a/util/log.c b/util/log.c
index 2ee1500bee..3e19859ab3 100644
--- a/util/log.c
+++ b/util/log.c
@@ -334,6 +334,8 @@ const QEMULogItem qemu_log_items[] = {
 #endif
     { LOG_STRACE, "strace",
       "log every user-mode syscall, its input, and its result" },
+    { LOG_UNSUPP, "unsupp",
+      "log unsupported functionality" },
     { 0, NULL, NULL },
 };
 
-- 
2.35.1

[PATCH 2/9] hw/ppc/spapr_hcall.c: log h_clean_slb() as unsupported

[Daniel Henrique Barboza]

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_hcall.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index f008290787..f6778d6857 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -865,7 +865,7 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, SpaprMachineState *spapr,
 static target_ulong h_clean_slb(PowerPCCPU *cpu, SpaprMachineState *spapr,
                                 target_ulong opcode, target_ulong *args)
 {
-    qemu_log_mask(LOG_UNIMP, "Unimplemented SPAPR hcall 0x"TARGET_FMT_lx"%s\n",
+    qemu_log_mask(LOG_UNSUPP, "Unsupported SPAPR hcall 0x"TARGET_FMT_lx"%s\n",
                   opcode, " (H_CLEAN_SLB)");
     return H_FUNCTION;
 }
-- 
2.35.1

[PATCH 3/9] hw/ppc/spapr_hcall.c: log h_invalidate_pid() as unsupported

[Daniel Henrique Barboza]

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_hcall.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index f6778d6857..5839b6a749 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -873,7 +873,7 @@ static target_ulong h_clean_slb(PowerPCCPU *cpu, SpaprMachineState *spapr,
 static target_ulong h_invalidate_pid(PowerPCCPU *cpu, SpaprMachineState *spapr,
                                      target_ulong opcode, target_ulong *args)
 {
-    qemu_log_mask(LOG_UNIMP, "Unimplemented SPAPR hcall 0x"TARGET_FMT_lx"%s\n",
+    qemu_log_mask(LOG_UNSUPP, "Unsupported SPAPR hcall 0x"TARGET_FMT_lx"%s\n",
                   opcode, " (H_INVALIDATE_PID)");
     return H_FUNCTION;
 }
-- 
2.35.1

[PATCH 4/9] hw/ppc/spapr_hcall.c: log h_copy_tofrom_guest() as unsupported

[Daniel Henrique Barboza]

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_hcall.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 5839b6a749..7cfb17689b 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1544,6 +1544,8 @@ static target_ulong h_copy_tofrom_guest(PowerPCCPU *cpu,
      * This HCALL is not required, L1 KVM will take a slow path and walk the
      * page tables manually to do the data copy.
      */
+    qemu_log_mask(LOG_UNSUPP, "Unsupported SPAPR hcall 0x"TARGET_FMT_lx"%s\n",
+                  opcode, " (H_COPY_TOFROM_GUEST)");
     return H_FUNCTION;
 }
 
-- 
2.35.1

[PATCH 5/9] hw/ppc/spapr_hcall.c: log H_GET_EM_PARMS as unsupported

[Daniel Henrique Barboza]

This hcall is part of the Partition Energy Management (PEM) option,
described in PAPR+ V2.89 section 14.14.2. We do not support any form of
energy management in QEMU, so instead of logging this hcall as
unimplemented let's log it as unsupported.

This hcall is popular with AIX 7.2. The terminal gets quite
busy with these debug messages using -d unsupp:

Unsupported SPAPR hcall 0x00000000000002b8 (H_GET_EM_PARMS)

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_hcall.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 7cfb17689b..b572ef50bb 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1549,6 +1549,21 @@ static target_ulong h_copy_tofrom_guest(PowerPCCPU *cpu,
     return H_FUNCTION;
 }
 
+static target_ulong h_get_em_parms(PowerPCCPU *cpu,
+                                   SpaprMachineState *spapr,
+                                   target_ulong opcode,
+                                   target_ulong *args)
+{
+    /*
+     * This HCALL returns energy management parameters as part of
+     * the PEM (Partition Energy Management) option. We do not
+     * support it.
+     */
+    qemu_log_mask(LOG_UNSUPP, "Unsupported SPAPR hcall 0x"TARGET_FMT_lx"%s\n",
+                  opcode, " (H_GET_EM_PARMS)");
+    return H_FUNCTION;
+}
+
 /*
  * When this handler returns, the environment is switched to the L2 guest
  * and TCG begins running that. spapr_exit_nested() performs the switch from
@@ -1887,6 +1902,8 @@ static void hypercall_register_types(void)
     spapr_register_hypercall(KVMPPC_H_ENTER_NESTED, h_enter_nested);
     spapr_register_hypercall(KVMPPC_H_TLB_INVALIDATE, h_tlb_invalidate);
     spapr_register_hypercall(KVMPPC_H_COPY_TOFROM_GUEST, h_copy_tofrom_guest);
+
+    spapr_register_hypercall(H_GET_EM_PARMS, h_get_em_parms);
 }
 
 type_init(hypercall_register_types)
-- 
2.35.1

[PATCH 6/9] hw/ppc/spapr_hcall.c: log H_BEST_ENERGY as unsupported

[Daniel Henrique Barboza]

This is another hcall that is part of the Partition Energy Management
(PEM) option. As with h_get_em_parms(), we do not support it.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_hcall.c   | 27 +++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |  1 +
 2 files changed, 28 insertions(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index b572ef50bb..db6cb6bb89 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1564,6 +1564,31 @@ static target_ulong h_get_em_parms(PowerPCCPU *cpu,
     return H_FUNCTION;
 }
 
+static target_ulong h_best_energy(PowerPCCPU *cpu,
+                                  SpaprMachineState *spapr,
+                                  target_ulong opcode,
+                                  target_ulong *args)
+{
+    /*
+     * This HCALL is described as follows in PAPR+ V2.8 section 14.14.2.1:
+     *
+     * "This hcall() returns a hint to the caller as to the probable impact
+     * toward the goal of minimal platform energy consumption for a given
+     * level of computing capacity that would result from releasing or
+     * activating various computing resources."
+     *
+     * In short, this HCALL provides a hint about what would happen with
+     * the energy consumption if the OS tries to increase/decrease
+     * the performance of a given device/CPU/mem.
+     *
+     * This is also part of the PEM option and, as with h_get_em_parms, is
+     * not supported.
+     */
+    qemu_log_mask(LOG_UNSUPP, "Unsupported SPAPR hcall 0x"TARGET_FMT_lx"%s\n",
+                  opcode, " (H_BEST_ENERGY)");
+    return H_FUNCTION;
+}
+
 /*
  * When this handler returns, the environment is switched to the L2 guest
  * and TCG begins running that. spapr_exit_nested() performs the switch from
@@ -1903,7 +1928,9 @@ static void hypercall_register_types(void)
     spapr_register_hypercall(KVMPPC_H_TLB_INVALIDATE, h_tlb_invalidate);
     spapr_register_hypercall(KVMPPC_H_COPY_TOFROM_GUEST, h_copy_tofrom_guest);
 
+    /* Unsupported PEM option h-calls */
     spapr_register_hypercall(H_GET_EM_PARMS, h_get_em_parms);
+    spapr_register_hypercall(H_BEST_ENERGY, h_best_energy);
 }
 
 type_init(hypercall_register_types)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index f5c33dcc86..7995bc0cb6 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -535,6 +535,7 @@ struct SpaprMachineState {
 #define H_SET_MPP               0x2D0
 #define H_GET_MPP               0x2D4
 #define H_HOME_NODE_ASSOCIATIVITY 0x2EC
+#define H_BEST_ENERGY           0x2F4
 #define H_XIRR_X                0x2FC
 #define H_RANDOM                0x300
 #define H_SET_MODE              0x31C
-- 
2.35.1

[PATCH 7/9] hw/ppc/spapr_hcall.c: log H_QUERY_VAS_CAPABILITIES as unsupported

[Daniel Henrique Barboza]

This hcall is VAS (Virtual Accelerator Switchboard) related. VAS is a
hardware feature that allows kernelspace and userspace to communicate
directly with Nested Accelerators (NX). A description of VAS can be
found at [1].

QEMU does not implement neither VAS nor NXs. AIX guests will query VAS
capabilities once during boot.

Log this hcall as unsupported. It is worth noticing that VAS has a
handful of other hcalls (H_ALLOCATE_VAS_WINDOW, H_QUERY_VAS_WINDOW,
H_GET_NX_FAULT and others), but the AIX guest seems to be satisfied
when H_QUERY_VAS_CAPABILITIES returns H_FUNCTION and give up trying
the others. This means that there is no need to log all of them as
unsupported in QEMU.

[1] https://www.kernel.org/doc/html/latest/powerpc/vas-api.html

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_hcall.c   | 18 ++++++++++++++++++
 include/hw/ppc/spapr.h |  1 +
 2 files changed, 19 insertions(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index db6cb6bb89..fdce44daf7 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1589,6 +1589,20 @@ static target_ulong h_best_energy(PowerPCCPU *cpu,
     return H_FUNCTION;
 }
 
+static target_ulong h_query_vas_capabilities(PowerPCCPU *cpu,
+                                             SpaprMachineState *spapr,
+                                             target_ulong opcode,
+                                             target_ulong *args)
+{
+    /*
+     * This HCALL is VAS (Virtual Accelerator Switchboard) related. VAS
+     * is not supported in QEMU.
+     */
+    qemu_log_mask(LOG_UNSUPP, "Unsupported SPAPR hcall 0x"TARGET_FMT_lx"%s\n",
+                  opcode, " (H_QUERY_VAS_CAPABILITIES)");
+    return H_FUNCTION;
+}
+
 /*
  * When this handler returns, the environment is switched to the L2 guest
  * and TCG begins running that. spapr_exit_nested() performs the switch from
@@ -1931,6 +1945,10 @@ static void hypercall_register_types(void)
     /* Unsupported PEM option h-calls */
     spapr_register_hypercall(H_GET_EM_PARMS, h_get_em_parms);
     spapr_register_hypercall(H_BEST_ENERGY, h_best_energy);
+
+    /* Unsupported VAS h-calls */
+    spapr_register_hypercall(H_QUERY_VAS_CAPABILITIES,
+                             h_query_vas_capabilities);
 }
 
 type_init(hypercall_register_types)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 7995bc0cb6..10df519b0a 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -545,6 +545,7 @@ struct SpaprMachineState {
 #define H_INVALIDATE_PID        0x378
 #define H_REGISTER_PROC_TBL     0x37C
 #define H_SIGNAL_SYS_RESET      0x380
+#define H_QUERY_VAS_CAPABILITIES 0x398
 
 #define H_INT_GET_SOURCE_INFO   0x3A8
 #define H_INT_SET_SOURCE_CONFIG 0x3AC
-- 
2.35.1

[PATCH 8/9] hw/ppc/spapr_hcall.c: log H_GET_PPP as unsupported

[Daniel Henrique Barboza]

H_GET_PPP is a SPLPAR hcall that we aren't able to support because, at
least for now, we can't retrieve any performance metrics of the
partition.

Mark it as unsupported.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_hcall.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index fdce44daf7..f7240fbd41 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -638,6 +638,21 @@ static target_ulong h_prod(PowerPCCPU *cpu, SpaprMachineState *spapr,
     return H_SUCCESS;
 }
 
+static target_ulong h_get_ppp(PowerPCCPU *cpu,
+                              SpaprMachineState *spapr,
+                              target_ulong opcode,
+                              target_ulong *args)
+{
+    /*
+     * H_GET_PPP (partition performance parameters) isn't supported
+     * the same way h_get_em_parms or any other performance/metric
+     * related HCALL is not supported in QEMU.
+     */
+    qemu_log_mask(LOG_UNSUPP, "Unsupported SPAPR hcall 0x"TARGET_FMT_lx"%s\n",
+                  opcode, " (H_GET_PPP)");
+    return H_FUNCTION;
+}
+
 static target_ulong h_rtas(PowerPCCPU *cpu, SpaprMachineState *spapr,
                            target_ulong opcode, target_ulong *args)
 {
@@ -1894,6 +1909,7 @@ static void hypercall_register_types(void)
     spapr_register_hypercall(H_CEDE, h_cede);
     spapr_register_hypercall(H_CONFER, h_confer);
     spapr_register_hypercall(H_PROD, h_prod);
+    spapr_register_hypercall(H_GET_PPP, h_get_ppp);
 
     /* hcall-join */
     spapr_register_hypercall(H_JOIN, h_join);
-- 
2.35.1

[PATCH 9/9] hw/ppc/spapr_hcall.c: log H_VIOCTL as unsupported

[Daniel Henrique Barboza]

H_VIOCTL is one of many VIO (Virtualized I/O) hcalls that we do not
support in QEMU. An AIX 7.2 guest will attempt to execute this hcall a
couple of times during boot, receiving H_FUNCTION replies every time,
and eventually give it up.

There are many VIO hcalls and we don't support none. H_VIOCTL seems to
be the one that convinces the guest that we don't have VIO support in
the platform, so let's add this one as unsupported in our model.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_hcall.c   | 14 ++++++++++++++
 include/hw/ppc/spapr.h |  1 +
 2 files changed, 15 insertions(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index f7240fbd41..ddaeea84fc 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1618,6 +1618,17 @@ static target_ulong h_query_vas_capabilities(PowerPCCPU *cpu,
     return H_FUNCTION;
 }
 
+static target_ulong h_vioctl(PowerPCCPU *cpu,
+                             SpaprMachineState *spapr,
+                             target_ulong opcode,
+                             target_ulong *args)
+{
+    /* We do not support any VIO (Virtualized I/O) HCALL */
+    qemu_log_mask(LOG_UNSUPP, "Unsupported SPAPR hcall 0x"TARGET_FMT_lx"%s\n",
+                  opcode, " (H_VIOCTL)");
+    return H_FUNCTION;
+}
+
 /*
  * When this handler returns, the environment is switched to the L2 guest
  * and TCG begins running that. spapr_exit_nested() performs the switch from
@@ -1965,6 +1976,9 @@ static void hypercall_register_types(void)
     /* Unsupported VAS h-calls */
     spapr_register_hypercall(H_QUERY_VAS_CAPABILITIES,
                              h_query_vas_capabilities);
+
+    /* Unsupported Virtualized I/O (VIO) h-calls */
+    spapr_register_hypercall(H_VIOCTL, h_vioctl);
 }
 
 type_init(hypercall_register_types)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 10df519b0a..232ac7650d 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -530,6 +530,7 @@ struct SpaprMachineState {
 #define H_DEL_CONN              0x288
 #define H_JOIN                  0x298
 #define H_VASI_STATE            0x2A4
+#define H_VIOCTL                0x2A8
 #define H_ENABLE_CRQ            0x2B0
 #define H_GET_EM_PARMS          0x2B8
 #define H_SET_MPP               0x2D0
-- 
2.35.1

Re: [PATCH 0/9] add LOG_UNSUPP log type + mark hcalls as unsupp

[Philippe Mathieu-Daudé]

On 7/3/22 20:15, Daniel Henrique Barboza wrote:
> Hi,
> 
> I got a lot of noise trying to debug an AIX guest in a pseries machine when running with
> '-d unimp'. The reason is that there is no distinction between features
> (in my case, hypercalls) that are unimplemented because we never considered,
> versus features that we made a design choice not to implement.
> 
> This series adds a new log type, LOG_UNSUPP, as a way to filter the
> second case. After changing the log level of existing unsupported
> pseries hypercalls, -d unimp was reporting just the ones that I need to
> worry about and decide whether we should implement it or mark as
> unsupported in our model. After this series there's still one hypercall
> thgat is being thrown by AIX. We'll deal with that another day.

Why not simply use a trace_spapr_hcall_unsupported() event?

Then you can filter using '-trace spapr_hcall_unsupported'.

Re: [PATCH 0/9] add LOG_UNSUPP log type + mark hcalls as unsupp

[Peter Maydell]

On Mon, 7 Mar 2022 at 19:19, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
> Hi,
>
> I got a lot of noise trying to debug an AIX guest in a pseries machine when running with
> '-d unimp'. The reason is that there is no distinction between features
> (in my case, hypercalls) that are unimplemented because we never considered,
> versus features that we made a design choice not to implement.
>
> This series adds a new log type, LOG_UNSUPP, as a way to filter the
> second case. After changing the log level of existing unsupported
> pseries hypercalls, -d unimp was reporting just the ones that I need to
> worry about and decide whether we should implement it or mark as
> unsupported in our model. After this series there's still one hypercall
> thgat is being thrown by AIX. We'll deal with that another day.

So the intention of the distinction is:
  LOG_UNIMP: we don't implement this, but we should
  LOG_UNSUPP: we don't implement this, and that's OK because it's optional

?

I think I'd be happier about adding a new log category if we had
some examples of where we should be using it other than just in
the spapr hcall code, to indicate that it's a bit more broadly
useful. If this is a distinction that only makes sense for that
narrow use case, then as Philippe says a tracepoint might be a
better choice.

thanks
-- PMM

Re: [PATCH 0/9] add LOG_UNSUPP log type + mark hcalls as unsupp

[Daniel Henrique Barboza]

On 3/7/22 17:21, Peter Maydell wrote:
> On Mon, 7 Mar 2022 at 19:19, Daniel Henrique Barboza
> <danielhb413@gmail.com> wrote:
>>
>> Hi,
>>
>> I got a lot of noise trying to debug an AIX guest in a pseries machine when running with
>> '-d unimp'. The reason is that there is no distinction between features
>> (in my case, hypercalls) that are unimplemented because we never considered,
>> versus features that we made a design choice not to implement.
>>
>> This series adds a new log type, LOG_UNSUPP, as a way to filter the
>> second case. After changing the log level of existing unsupported
>> pseries hypercalls, -d unimp was reporting just the ones that I need to
>> worry about and decide whether we should implement it or mark as
>> unsupported in our model. After this series there's still one hypercall
>> thgat is being thrown by AIX. We'll deal with that another day.
> 
> So the intention of the distinction is:
>    LOG_UNIMP: we don't implement this, but we should
>    LOG_UNSUPP: we don't implement this, and that's OK because it's optional
> 
> ?

The idea is that LOG_UNIMP is too broad and it's used to indicate features that are
unknown to QEMU and also features that QEMU knows about but does not support it. It's
not necessarily a way of telling "we should implement this" but more like "we know/do
not know what this is".


> 
> I think I'd be happier about adding a new log category if we had
> some examples of where we should be using it other than just in
> the spapr hcall code, to indicate that it's a bit more broadly
> useful. If this is a distinction that only makes sense for that
> narrow use case, then as Philippe says a tracepoint might be a
> better choice.

target/arm/translate.c, do_coproc_insn():


     /* Unknown register; this might be a guest error or a QEMU
      * unimplemented feature.
      */
     if (is64) {
         qemu_log_mask(LOG_UNIMP, "%s access to unsupported AArch32 "
                       "64 bit system register cp:%d opc1: %d crm:%d "
                       "(%s)\n",
                       isread ? "read" : "write", cpnum, opc1, crm,
                       s->ns ? "non-secure" : "secure");
     } else {
         qemu_log_mask(LOG_UNIMP, "%s access to unsupported AArch32 "
                       "system register cp:%d opc1:%d crn:%d crm:%d opc2:%d "
                       "(%s)\n",
                       isread ? "read" : "write", cpnum, opc1, crn, crm, opc2,
                       s->ns ? "non-secure" : "secure");
     }

This use of LOG_UNIMP is logging something that we don't know about, it's unknown.
And hw/arm/smmuv3.c, decode_ste():


     if (STE_CFG_S2_ENABLED(config)) {
         qemu_log_mask(LOG_UNIMP, "SMMUv3 does not support stage 2 yet\n");
         goto bad_ste;
     }

     if (STE_S1CDMAX(ste) != 0) {
         qemu_log_mask(LOG_UNIMP,
                       "SMMUv3 does not support multiple context descriptors yet\n");
         goto bad_ste;
     }

     if (STE_S1STALLD(ste)) {
         qemu_log_mask(LOG_UNIMP,
                       "SMMUv3 S1 stalling fault model not allowed yet\n");
         goto bad_ste;
     }


This is something we know what it is and are deciding not to support it. Both are being
logged as LOG_UNIMP. This is the distinction I was trying to achieve with this new
log type. The example in decode_ste() could be logged as LOG_UNSUPP.


Perhaps an easier sell would be adding a "LOG_UNKNOWN" for the features/regs/etc that
we do not know about. The semantics/intention seems clearer than what I'm trying to
achieve with LOG_UNSUPP.


And yeah, if this is too convoluted to push forward I can always tracepoint like Philippe
suggested as well.



Thanks,


Daniel



> 
> thanks
> -- PMM

Re: [PATCH 0/9] add LOG_UNSUPP log type + mark hcalls as unsupp

[Peter Maydell]

On Mon, 7 Mar 2022 at 22:00, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
>
>
> On 3/7/22 17:21, Peter Maydell wrote:
> > On Mon, 7 Mar 2022 at 19:19, Daniel Henrique Barboza
> > <danielhb413@gmail.com> wrote:
> >>
> >> Hi,
> >>
> >> I got a lot of noise trying to debug an AIX guest in a pseries machine when running with
> >> '-d unimp'. The reason is that there is no distinction between features
> >> (in my case, hypercalls) that are unimplemented because we never considered,
> >> versus features that we made a design choice not to implement.
> >>
> >> This series adds a new log type, LOG_UNSUPP, as a way to filter the
> >> second case. After changing the log level of existing unsupported
> >> pseries hypercalls, -d unimp was reporting just the ones that I need to
> >> worry about and decide whether we should implement it or mark as
> >> unsupported in our model. After this series there's still one hypercall
> >> thgat is being thrown by AIX. We'll deal with that another day.
> >
> > So the intention of the distinction is:
> >    LOG_UNIMP: we don't implement this, but we should
> >    LOG_UNSUPP: we don't implement this, and that's OK because it's optional
> >
> > ?
>
> The idea is that LOG_UNIMP is too broad and it's used to indicate features that are
> unknown to QEMU and also features that QEMU knows about but does not support it. It's
> not necessarily a way of telling "we should implement this" but more like "we know/do
> not know what this is".

From the point of view of debugging the guest, I don't care
whether the QEMU developers know that they've not got round
to something or whether they've just forgotten it. I care
about "is this because I, the guest program, did something wrong,
or is it because QEMU is not completely emulating something
I should really be able to expect to be present". This is why we
distinguish LOG_UNIMP from LOG_GUEST_ERROR.

> > I think I'd be happier about adding a new log category if we had
> > some examples of where we should be using it other than just in
> > the spapr hcall code, to indicate that it's a bit more broadly
> > useful. If this is a distinction that only makes sense for that
> > narrow use case, then as Philippe says a tracepoint might be a
> > better choice.
>
> target/arm/translate.c, do_coproc_insn():

> This use of LOG_UNIMP is logging something that we don't know about, it's unknown.

(Some of the things that get logged here will really be things that
we conceptually "know about" and don't implement -- the logging
is a catch-all for any kind of unimplemented register, whether the
specs define it or not.)

> And hw/arm/smmuv3.c, decode_ste():

> This is something we know what it is and are deciding not to support it. Both are being
> logged as LOG_UNIMP. This is the distinction I was trying to achieve with this new
> log type. The example in decode_ste() could be logged as LOG_UNSUPP.

I don't see much benefit in distinguishing these two cases, to be
honest. You could maybe have sold me on "you're accessing something
that is optional and we happen not to provide it" vs "you're
accessing something that should be there and isn't", because that's
a distinction that guest code authors might plausibly care about.
To the extent that you want to helpfully say "this is because
QEMU doesn't implement an entire feature" you can say that in the
free-form text message.

-- PMM

Re: [PATCH 0/9] add LOG_UNSUPP log type + mark hcalls as unsupp

[Daniel Henrique Barboza]

On 3/8/22 06:18, Peter Maydell wrote:
> On Mon, 7 Mar 2022 at 22:00, Daniel Henrique Barboza
> <danielhb413@gmail.com> wrote:
>>
>>
>>
>> On 3/7/22 17:21, Peter Maydell wrote:
>>> On Mon, 7 Mar 2022 at 19:19, Daniel Henrique Barboza
>>> <danielhb413@gmail.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> I got a lot of noise trying to debug an AIX guest in a pseries machine when running with
>>>> '-d unimp'. The reason is that there is no distinction between features
>>>> (in my case, hypercalls) that are unimplemented because we never considered,
>>>> versus features that we made a design choice not to implement.
>>>>
>>>> This series adds a new log type, LOG_UNSUPP, as a way to filter the
>>>> second case. After changing the log level of existing unsupported
>>>> pseries hypercalls, -d unimp was reporting just the ones that I need to
>>>> worry about and decide whether we should implement it or mark as
>>>> unsupported in our model. After this series there's still one hypercall
>>>> thgat is being thrown by AIX. We'll deal with that another day.
>>>
>>> So the intention of the distinction is:
>>>     LOG_UNIMP: we don't implement this, but we should
>>>     LOG_UNSUPP: we don't implement this, and that's OK because it's optional
>>>
>>> ?
>>
>> The idea is that LOG_UNIMP is too broad and it's used to indicate features that are
>> unknown to QEMU and also features that QEMU knows about but does not support it. It's
>> not necessarily a way of telling "we should implement this" but more like "we know/do
>> not know what this is".
> 
>  From the point of view of debugging the guest, I don't care
> whether the QEMU developers know that they've not got round
> to something or whether they've just forgotten it. I care
> about "is this because I, the guest program, did something wrong,
> or is it because QEMU is not completely emulating something
> I should really be able to expect to be present". This is why we
> distinguish LOG_UNIMP from LOG_GUEST_ERROR.
> 
>>> I think I'd be happier about adding a new log category if we had
>>> some examples of where we should be using it other than just in
>>> the spapr hcall code, to indicate that it's a bit more broadly
>>> useful. If this is a distinction that only makes sense for that
>>> narrow use case, then as Philippe says a tracepoint might be a
>>> better choice.
>>
>> target/arm/translate.c, do_coproc_insn():
> 
>> This use of LOG_UNIMP is logging something that we don't know about, it's unknown.
> 
> (Some of the things that get logged here will really be things that
> we conceptually "know about" and don't implement -- the logging
> is a catch-all for any kind of unimplemented register, whether the
> specs define it or not.)
> 
>> And hw/arm/smmuv3.c, decode_ste():
> 
>> This is something we know what it is and are deciding not to support it. Both are being
>> logged as LOG_UNIMP. This is the distinction I was trying to achieve with this new
>> log type. The example in decode_ste() could be logged as LOG_UNSUPP.
> 
> I don't see much benefit in distinguishing these two cases, to be
> honest. You could maybe have sold me on "you're accessing something
> that is optional and we happen not to provide it" vs "you're
> accessing something that should be there and isn't", because that's
> a distinction that guest code authors might plausibly care about.
> To the extent that you want to helpfully say "this is because
> QEMU doesn't implement an entire feature" you can say that in the
> free-form text message.


Fair enough. I'll rely on tracepointing the cases where I think this distinction
is helpful for debugging.



Thanks,


Daniel

> 
> -- PMM

Re: [PATCH 0/9] add LOG_UNSUPP log type + mark hcalls as unsupp

[Philippe Mathieu-Daudé]

On 8/3/22 10:18, Peter Maydell wrote:
> On Mon, 7 Mar 2022 at 22:00, Daniel Henrique Barboza
> <danielhb413@gmail.com> wrote:
>> On 3/7/22 17:21, Peter Maydell wrote:
>>> On Mon, 7 Mar 2022 at 19:19, Daniel Henrique Barboza
>>> <danielhb413@gmail.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> I got a lot of noise trying to debug an AIX guest in a pseries machine when running with
>>>> '-d unimp'. The reason is that there is no distinction between features
>>>> (in my case, hypercalls) that are unimplemented because we never considered,
>>>> versus features that we made a design choice not to implement.
>>>>
>>>> This series adds a new log type, LOG_UNSUPP, as a way to filter the
>>>> second case. After changing the log level of existing unsupported
>>>> pseries hypercalls, -d unimp was reporting just the ones that I need to
>>>> worry about and decide whether we should implement it or mark as
>>>> unsupported in our model. After this series there's still one hypercall
>>>> thgat is being thrown by AIX. We'll deal with that another day.
>>>
>>> So the intention of the distinction is:
>>>     LOG_UNIMP: we don't implement this, but we should
>>>     LOG_UNSUPP: we don't implement this, and that's OK because it's optional
>>>
>>> ?
>>
>> The idea is that LOG_UNIMP is too broad and it's used to indicate features that are
>> unknown to QEMU and also features that QEMU knows about but does not support it. It's
>> not necessarily a way of telling "we should implement this" but more like "we know/do
>> not know what this is".
> 
>  From the point of view of debugging the guest, I don't care
> whether the QEMU developers know that they've not got round
> to something or whether they've just forgotten it. I care
> about "is this because I, the guest program, did something wrong,
> or is it because QEMU is not completely emulating something
> I should really be able to expect to be present". This is why we
> distinguish LOG_UNIMP from LOG_GUEST_ERROR.

I agree with this view. Another distinctions:

  * tracing API
    - have multiple backends to send the events
    - is optional, might be completely disabled in build
      (which is why we use it to debug or analyze perfs)

  * qemu_log() API
    - logs to stdout
    - is present in all build variants
      (so we can always look at guest misbehavior as
       Peter described).

LOG_UNSUPP doesn't add value wrt guest misbehavior IMO,
which is why I'd stick to trace events for this.

>>> I think I'd be happier about adding a new log category if we had
>>> some examples of where we should be using it other than just in
>>> the spapr hcall code, to indicate that it's a bit more broadly
>>> useful. If this is a distinction that only makes sense for that
>>> narrow use case, then as Philippe says a tracepoint might be a
>>> better choice.
>>
>> target/arm/translate.c, do_coproc_insn():
> 
>> This use of LOG_UNIMP is logging something that we don't know about, it's unknown.
> 
> (Some of the things that get logged here will really be things that
> we conceptually "know about" and don't implement -- the logging
> is a catch-all for any kind of unimplemented register, whether the
> specs define it or not.)
> 
>> And hw/arm/smmuv3.c, decode_ste():
> 
>> This is something we know what it is and are deciding not to support it. Both are being
>> logged as LOG_UNIMP. This is the distinction I was trying to achieve with this new
>> log type. The example in decode_ste() could be logged as LOG_UNSUPP.
> 
> I don't see much benefit in distinguishing these two cases, to be
> honest. You could maybe have sold me on "you're accessing something
> that is optional and we happen not to provide it" vs "you're
> accessing something that should be there and isn't", because that's
> a distinction that guest code authors might plausibly care about.
> To the extent that you want to helpfully say "this is because
> QEMU doesn't implement an entire feature" you can say that in the
> free-form text message.
> 
> -- PMM
>