[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kvm
Subject:    Re: [PATCH kvmtool] Skip a few messages by default: command line args; flat binary; earlyprintk.
From:       Andre Przywara <andre.przywara () arm ! com>
Date:       2015-09-30 16:11:15
Message-ID: 560C09A3.3060503 () arm ! com
[Download RAW message or body]

Hi Dimitri,

thanks for sharing your patches.

On 29/09/15 17:59, Dimitri John Ledkov wrote:
> The partial command line args & earlyprintk=serial are still enabled
> in the debug mode. Warning that a flat binary kernel image is attemped
> to be loaded is completely dropped. These are not that informative,
> once one is past intial debugging, and only polute the console.
> 
> Signed-off-by: Dimitri John Ledkov <dimitri.j.ledkov@intel.com>
> ---
>  builtin-run.c | 10 ++++++----
>  kvm.c         |  1 -
>  x86/kvm.c     |  8 ++++++--
>  3 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/builtin-run.c b/builtin-run.c
> index e0c8732..8edbf88 100644
> --- a/builtin-run.c
> +++ b/builtin-run.c
> @@ -613,10 +613,12 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
>  
>  	kvm->cfg.real_cmdline = real_cmdline;
>  
> -	printf("  # %s run -k %s -m %Lu -c %d --name %s\n", KVM_BINARY_NAME,
> -		kvm->cfg.kernel_filename,
> -		(unsigned long long)kvm->cfg.ram_size / 1024 / 1024,
> -		kvm->cfg.nrcpus, kvm->cfg.guest_name);
> +	if (do_debug_print) {
> +		printf("  # %s run -k %s -m %Lu -c %d --name %s\n", KVM_BINARY_NAME,
> +		       kvm->cfg.kernel_filename,
> +		       (unsigned long long)kvm->cfg.ram_size / 1024 / 1024,
> +		       kvm->cfg.nrcpus, kvm->cfg.guest_name);
> +	}

I like the general idea. In fact I have this very patch (among others)
in my tree too. I applied similar guarding to other messages as well
(mostly those that only show up on ARM, but also the "ended normally"
message). Like any good UNIX tool kvmtool should keep quiet if it has
nothing worthwhile to say ;-)
But looking at it more closely, I see that there is pr_debug() defined
doing that "if (do_debug_print)" already. The only issue is that is
prints source line information, which is not really useful here. But
then again there does not seem to be any user of it?

So what about the following:
- We avoid printing pr_info() messages in the default case. Looking at
its current users in the tree this information is not really useful for
normal users. We enable pr_info() output only if do_debug_print is
enabled or introduce another command line flag (--verbose?) for that.
- We check each user of pr_info() to see whether this information is
actually "informational" or whether it should be converted to pr_warn.
- We change the above line to use pr_info instead of printf.
- We fix the EOL mayhem we have atm while at it.

If you don't mind I will give this a try later this week.

>  
>  	if (init_list__init(kvm) < 0)
>  		die ("Initialisation failed");
> diff --git a/kvm.c b/kvm.c
> index 10ed230..1081072 100644
> --- a/kvm.c
> +++ b/kvm.c
> @@ -378,7 +378,6 @@ bool kvm__load_kernel(struct kvm *kvm, const char *kernel_filename,
>  	if (ret)
>  		goto found_kernel;
>  
> -	pr_warning("%s is not a bzImage. Trying to load it as a flat binary...", kernel_filename);

I think on x86 this message is useful to have: to point people to the
fact that they are trying to load a kernel which most probably isn't one.
Do you actually load a "flat binary", so not a Linux bzImage? If yes,
what is it? Does this work for you? I didn't have the impression that
this code was actually used at all.
If you do use it, could you please give my kernel loading series [1] a
try? I touch flat binary loading there, but had no chance to test it.

>  #endif
>  
>  	ret = load_elf_binary(kvm, fd_kernel, fd_initrd, kernel_cmdline);
> diff --git a/x86/kvm.c b/x86/kvm.c
> index 512ad67..4a5fa41 100644
> --- a/x86/kvm.c
> +++ b/x86/kvm.c
> @@ -124,8 +124,12 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
>  				"i8042.dumbkbd=1 i8042.nopnp=1");
>  	if (video)
>  		strcat(cmdline, " video=vesafb console=tty0");
> -	else
> -		strcat(cmdline, " console=ttyS0 earlyprintk=serial i8042.noaux=1");
> +	else {
> +		strcat(cmdline, " console=ttyS0 i8042.noaux=1");
> +		if (do_debug_print) {
> +			strcat(cmdline, " earlyprintk=serial");
> +		}
> +	}

I am not completely convinced of this one. The do_debug_print is meant
to affect kvmtool's own debug output only and should really have no
effect on the guest's kernel output, shouldn't it?
Maybe we should clarify the semantics in the documentation?

Cheers,
Andre.

[1] http://marc.info/?l=kvm&m=143825354808135

>  }
>  
>  /* Architecture-specific KVM init */
> 
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]