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