From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ingo Molnar <mingo@elte.hu>
Subject: Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
Date: Mon, 23 May 2011 13:38:24 +0200
Message-ID: <20110523113824.GE4042@elte.hu>
References: <1306149553-26793-1-git-send-email-levinsasha928@gmail.com>
 <1306149553-26793-5-git-send-email-levinsasha928@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: penberg@kernel.org, john@jfloren.net, kvm@vger.kernel.org,
	asias.hejun@gmail.com, gorcunov@gmail.com, prasadjoshi124@gmail.com
To: Sasha Levin <levinsasha928@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx3.mail.elte.hu ([157.181.1.138]:46304 "EHLO mx3.mail.elte.hu"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753803Ab1EWLi2 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 23 May 2011 07:38:28 -0400
Content-Disposition: inline
In-Reply-To: <1306149553-26793-5-git-send-email-levinsasha928@gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>


* Sasha Levin <levinsasha928@gmail.com> wrote:

> @@ -511,7 +515,13 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
>  	kvm->nrcpus = nrcpus;
>  
>  	memset(real_cmdline, 0, sizeof(real_cmdline));
> -	strcpy(real_cmdline, "notsc noapic noacpi pci=conf1 console=ttyS0 earlyprintk=serial");
> +	strcpy(real_cmdline, "notsc noapic noacpi pci=conf1");
> +	if (vnc) {
> +		strcat(real_cmdline, " video=vesafb console=tty0");
> +		vidmode = 0x312;
> +	} else {
> +		strcat(real_cmdline, " console=ttyS0 earlyprintk=serial");
> +	}

Hm, i think all the kernel parameter handling code wants to move into driver 
specific routines as well. Something like:

	serial_init(kvm, real_cmdline);

where serial_init() would append to real_cmdline if needed.

This removes a bit of serial-driver specific knowledge from kvm-run.c.

Same goes for the VESA driver and the above video mode flag logic.

> @@ -597,6 +607,9 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
>  
>  	kvm__init_ram(kvm);
>  
> +	if (vnc)
> +		vesa__init(kvm);

Shouldnt vesa__init() itself know about whether it's active (i.e. the 'vnc' 
flag is set) and return early if it's not set?

That way this could become more encapsulated and self-sufficient:

	vesa__init(kvm);

With no VESA driver specific state exposed to the generic kvm_cmd_run() 
function.

Ideally kvm_cmd_run() hould just be a series of:

	serial_init(kvm, real_cmdline);
	vesa_init(kvm, real_cmdline);
	...

initialization routines. Later on even this could be removed: using section 
tricks we can put init functions into a section and drivers could register 
their init function like initcall(func) functions are registered within the 
kernel. kvm_cmd_run() could thus iterate over that (build time constructed) 
section like this:

extern initcall_t __initcall_start[], __initcall_end[], __early_initcall_end[];

static void __init do_initcalls(void)
{
        initcall_t *fn;

        for (fn = __early_initcall_end; fn < __initcall_end; fn++)
                do_one_initcall(*fn);
}

and would not actually have *any* knowledge about what drivers were built in.

Currently it's fine to initialize everything explicitly - but this would be the 
long term model to work towards ...

Thanks,

	Ingo