From mboxrd@z Thu Jan  1 00:00:00 1970
From: Pekka Enberg <penberg@cs.helsinki.fi>
Subject: Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
Date: Mon, 23 May 2011 14:45:10 +0300
Message-ID: <4DDA48C6.2030103@cs.helsinki.fi>
References: <1306149553-26793-1-git-send-email-levinsasha928@gmail.com> <1306149553-26793-5-git-send-email-levinsasha928@gmail.com> <20110523113824.GE4042@elte.hu>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: Sasha Levin <levinsasha928@gmail.com>, john@jfloren.net,
	kvm@vger.kernel.org, asias.hejun@gmail.com, gorcunov@gmail.com,
	prasadjoshi124@gmail.com
To: Ingo Molnar <mingo@elte.hu>
Return-path: <kvm-owner@vger.kernel.org>
Received: from courier.cs.helsinki.fi ([128.214.9.1]:43531 "EHLO
	mail.cs.helsinki.fi" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1753647Ab1EWLpM (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 23 May 2011 07:45:12 -0400
In-Reply-To: <20110523113824.GE4042@elte.hu>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 5/23/11 2:38 PM, Ingo Molnar wrote:
> * 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 ...

Prasad, didn't you have patches to do exactly that?