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

["Philippe Mathieu-Daudé" <philippe.mathieu.daude@gmail.com>]

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
>