From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1MnQpP-00025h-Tm
	for qemu-devel@nongnu.org; Tue, 15 Sep 2009 01:43:47 -0400
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1MnQpL-00025J-8I
	for qemu-devel@nongnu.org; Tue, 15 Sep 2009 01:43:47 -0400
Received: from [199.232.76.173] (port=33001 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1MnQpL-00025G-37
	for qemu-devel@nongnu.org; Tue, 15 Sep 2009 01:43:43 -0400
Received: from mx1.redhat.com ([209.132.183.28]:31233)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <gleb@redhat.com>) id 1MnQpK-0006vp-LZ
	for qemu-devel@nongnu.org; Tue, 15 Sep 2009 01:43:42 -0400
Date: Tue, 15 Sep 2009 08:43:39 +0300
From: Gleb Natapov <gleb@redhat.com>
Message-ID: <20090915054339.GD30746@redhat.com>
References: <20090914125141.GB30746@redhat.com>
	<20090915000824.GA16210@morn.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20090915000824.GA16210@morn.localdomain>
Subject: [Qemu-devel] Re: [PATCH][SEABIOS] Move qemu config port access
 functions into separate file.
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Kevin O'Connor <kevin@koconnor.net>
Cc: qemu-devel@nongnu.org

On Mon, Sep 14, 2009 at 08:08:24PM -0400, Kevin O'Connor wrote:
> On Mon, Sep 14, 2009 at 03:51:41PM +0300, Gleb Natapov wrote:
> > Move qemu config code from smbios.c to its own files. Add support for
> > -boot menu=on|off qemu option.
> 
> Hi Gleb,
> 
> A couple of comments:
> 
> >      // Allow user to modify BCV/IPL order.
> > -    interactive_bootmenu();
> > +    if (qemu_cfg_show_boot_menu())
> > +        interactive_bootmenu();
> 
> Can you move this test into interactive_bootmenu()?  (For non-qemu
> users the flow control looks odd otherwise.)
> 
OK. The flow control will not go away though just move to other place so
non-qemu user will see odd flow control anyway :)

> > --- /dev/null
> > +++ b/src/pv.c
> 
> What is "pv"?  How about "qemu-cfg.c"?
> 
pv == ParaVirtualization. I want to put non qemu specific thing there
to. For instance I put kvm_para_available() function there that checks
cpuid signature. I want to use it to replace if(CONFIG_KVM) in
ram_probe().

> > +void qemu_cfg_port_probe(void)
> > +{
> > +    char *sig = "QEMU";
> > +    int i;
> > +
> > +    qemu_cfg_present = 1;
> > +
> > +    qemu_cfg_select(QEMU_CFG_SIGNATURE);
> > +
> > +    for (i = 0; i < 4; i++)
> > +        if (inb(QEMU_CFG_DATA_PORT) != sig[i]) {
> > +            qemu_cfg_present = 0;
> > +            break;
> > +        }
> > +    dprintf(4, "qemu_cfg_present=%d\n", qemu_cfg_present);
> > +}
> 
> This needs to have a "if (! CONFIG_COREBOOT) return;" - as the current
> check for qemu is not safe on real hardware.
> 
Will add.

> Otherwise it looks good to me.
> 
> As an aside, it would be good to have a conversation on general BIOS
> configuration options.  These types of settings are going to be useful
> on real hardware also - it would be nice to come up with a scheme that
> would work on qemu and coreboot.  Maybe something like
> get_config_u32("ShowBootMenu") - where on qemu it would get the info
> from the qemu port but on coreboot it would pull the setting from the
> coreboot flash filesystem.
> 
Lets have conversation now. Sounds useful to me. Do you want to use
strings as option names though? They add to BIOS image size and it is
limited, no?

--
			Gleb.