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