Re: [for-4.17] xen/arm: domain_build: Do not use dprintk unconditionally

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 16/09/2022 10:08, Julien Grall wrote:
> 
> 
> Hi,
> 
> On 16/09/2022 08:19, Michal Orzel wrote:
>> Using dprintk results in printing additionally file name and line
>> number. This is something we do not want when printing regular
>> information unconditionally as it looks like as if there was some issue.
> I am OK if you want to switch to a printk() but I disagree with this
> argument. dprintk() is not about error, it is about anything that
> doesn't matter in release build.

In the vast majority of cases, dprintk is used conditionally. That is why
in the debug build you cannot spot a single line of log starting with
a file name + line number. That is why I assume this behaviorto be abnormal
compared to all the other logs.

If someone adds a printk starting with e.g. "$$$" this is also not a bad
usage of printk but would result in an inconsistent behavior.

> 
> I don't think we should just switch to printk() because dprintk() add
> the line/file. There are message we don't necessarily want to have in
> release build. So dprintk(XENLOG_INFO, ...) would be right for them.

I think this is a matter of being consistent.
We do not have a helper to add printk only for a debug build but without adding
filename/line number. That is why almost all the dprintks are used 
conditionally.

> 
> Personally, I find them useful as there no grep required and/or
> confusion (but that's a matter of taste). If it were me, I would add the
> line/file everywhere. But I understand this takes space in the binary
> (hence why this is not present in release build).
> 
> A better argument to switch to printk() is this information is useful to
> the user even outside of the debug build.
> 
>>
>> Fix this by switching to printk because this information may also be
>> helpful on the release builds (it would still require setting loglvl to
>> "info" or lower level).
> 
> I think we should drop XENLOG_INFO to be consistent with the other
> printk() in domain_build.c (after all this is a domain information like
> the other) or use XENLOG_INFO everywhere.
> 
> My preference will be the former because otherwise most of the
> information will not printed in release build by default.
> 
>>
>> Fixes: 5597f32f409c ("xen/arm: assign static shared memory to the default 
>> owner dom_io")
> 
> Fixes should only be used for bugs. This is not one.
> 
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>> Rationale for taking this patch for 4.17:
>> Current code results in an abnormal behavior [1] and was introduced by
> 
> It is not abnormal (see above). This is an expected behavior when you
> use dprintk().

I did not mean abnormal behavior of dprintk but abnormal behavior of logging
even on debug builds. As I said before, I could not spot any message like this
booting Xen at all. This is why I took this as a reference for "normal" 
behavior.

> 
>> the 4.17 feature (static shared memory). Even though it can only be seen
>> on a debug build, it should be fixed now so that we have a consistent
>> behavior across all the logs.
> 
> As I wrote above, I agree this should be printed in release build. But I
> disagree with your arguments.
> 
> Cheers,
> 
> --
> Julien Grall

~Michal