| Message ID | 1443545952-1025-1-git-send-email-dimitri.j.ledkov@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
Hello, A bit of context. I'm from Clear Linux* Project for Intel Architecture and we use lkvm as the hypervisor in Clear Containers for Docker Engine http://blog.surgut.co.uk/2015/09/clear-containers-for-docker-engine.html For us, we really do not want any output coming from the hypervisor, or kernel, or init. We use systemd, and the only unit that has TTY connected for output is the ultimate docker workload user has requested. Thus I am avert --vey-quiet mode for lkvm. I understand that this is outside of the usual intended use-case for kvmtool (i.e. kernel development), but it's really lean and nice to work with. On 30 September 2015 at 17:11, Andre Przywara <andre.przywara@arm.com> wrote: > 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? > I'd be happy to change these to pr_debug() messages. > 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. Sounds good to me. That should work as well. > - We fix the EOL mayhem we have atm while at it. > Not quite sure what you mean by `EOL mayhem' could you please elaborate? > If you don't mind I will give this a try later this week. > Thumbs up! >> >> 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. We do use flat kernel loading, and we have extra patches for that. But I haven't reconciled that with current upstream tree yet. > 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. > These look very interesting. I will definitely look into them. >> #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? I have no idea about this one. Imho, earlyprintk=serial is too much. On the other hand it's inline with the original lkvm usecase. Not at all needed for my usecase. Maybe, I should add --very-quiet option which doesn't enable earlyprintk by default? > Maybe we should clarify the semantics in the documentation? > > Cheers, > Andre. > > [1] http://marc.info/?l=kvm&m=143825354808135 > >> } >> >> /* Architecture-specific KVM init */ >>
On Wed, Sep 30, 2015 at 05:11:15PM +0100, Andre Przywara wrote: > 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. Sounds good to me. Will -- 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
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); + } 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); #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"); + } + } } /* Architecture-specific KVM init */
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(-)