On 3/7/22 17:21, Peter Maydell wrote:
On Mon, 7 Mar 2022 at 19:19, Daniel Henrique Barboza
<danielhb...@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
