From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [PATCHv4 15/15] Pass boot device list to firmware.
Date: Sun, 14 Nov 2010 23:45:40 +0200
Message-ID: <20101114214540.GE14828@redhat.com>
References: <1289749181-12070-1-git-send-email-gleb@redhat.com>
 <1289749181-12070-16-git-send-email-gleb@redhat.com>
 <AANLkTi=1JpWB7GR3s2soWzgMsN3QixyfKF9uA6kCE5K3@mail.gmail.com>
 <20101114205432.GB14828@redhat.com>
 <AANLkTikEoGvCv_3a71ub4P=iAyn+XRDCFTU8+vFTtb+F@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: QUOTED-PRINTABLE
Cc: Gleb Natapov <gleb@redhat.com>, qemu-devel@nongnu.org,
	kvm@vger.kernel.org, armbru@redhat.com, alex.williamson@redhat.com,
	kevin@koconnor.net
To: Blue Swirl <blauwirbel@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:49176 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1755954Ab0KNVp4 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sun, 14 Nov 2010 16:45:56 -0500
Content-Disposition: inline
In-Reply-To: <AANLkTikEoGvCv_3a71ub4P=iAyn+XRDCFTU8+vFTtb+F@mail.gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Sun, Nov 14, 2010 at 09:13:03PM +0000, Blue Swirl wrote:
> On Sun, Nov 14, 2010 at 8:54 PM, Michael S. Tsirkin <mst@redhat.com> =
wrote:
> > On Sun, Nov 14, 2010 at 08:49:59PM +0000, Blue Swirl wrote:
> >> On Sun, Nov 14, 2010 at 3:39 PM, Gleb Natapov <gleb@redhat.com> wr=
ote:
> >> >
> >> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> >> > ---
> >> > =A0hw/fw_cfg.c | =A0 14 ++++++++++++++
> >> > =A0hw/fw_cfg.h | =A0 =A04 +++-
> >> > =A0sysemu.h =A0 =A0| =A0 =A01 +
> >> > =A0vl.c =A0 =A0 =A0 =A0| =A0 51 ++++++++++++++++++++++++++++++++=
+++++++++++++++++++
> >> > =A04 files changed, 69 insertions(+), 1 deletions(-)
> >> >
> >> > diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
> >> > index 7b9434f..f6a67db 100644
> >> > --- a/hw/fw_cfg.c
> >> > +++ b/hw/fw_cfg.c
> >> > @@ -53,6 +53,7 @@ struct FWCfgState {
> >> > =A0 =A0 FWCfgFiles *files;
> >> > =A0 =A0 uint16_t cur_entry;
> >> > =A0 =A0 uint32_t cur_offset;
> >> > + =A0 =A0Notifier machine_ready;
> >> > =A0};
> >> >
> >> > =A0static void fw_cfg_write(FWCfgState *s, uint8_t value)
> >> > @@ -315,6 +316,15 @@ int fw_cfg_add_file(FWCfgState *s, =A0const=
 char *filename, uint8_t *data,
> >> > =A0 =A0 return 1;
> >> > =A0}
> >> >
> >> > +static void fw_cfg_machine_ready(struct Notifier* n)
> >> > +{
> >> > + =A0 =A0uint32_t len;
> >> > + =A0 =A0char *bootindex =3D get_boot_devices_list(&len);
> >> > +
> >> > + =A0 =A0fw_cfg_add_bytes(container_of(n, FWCfgState, machine_re=
ady),
> >> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 FW_CFG_BOOTINDEX, (uin=
t8_t*)bootindex, len);
> >>
> >> I started to implement this to OpenBIOS but I noticed a small issu=
e.
> >> First the first byte must be read to determine length. Then the re=
ad
> >> routine will be called again to read the correct amount of bytes. =
This
> >> would work, but since there is no shortage of IDs, I'd prefer a sy=
stem
> >> where one ID is used to query the length and another ID is used to
> >> read the data, without the length byte. This is similar how comman=
d
> >> line, initrd etc. are handled.
> >>
> >> This would have the advantage that since fw_cfg uses little endian
> >> format, the length value would easily scale to for example 64 bits=
 to
> >> support terabytes of boot device lists. ;-)
> >
> > Yea. Let's just print # of devices as a property, in ASCII.
> > No endian-ness, no nothing.
> > Also - can we just NULL-terminate each ID?
>=20
> No, we should use LE numbers like other IDs. To be more specific, thi=
s
> is what I meant (instead of FW_CFG_BOOTINDEX):
> FW_CFG_BOOTINDEX_LEN: get LE integer length of the boot device data.
> FW_CFG_BOOTINDEX_DATA: get the boot device data as NUL terminated C
> strings, all strings back-to-back. The reader can determine number of
> strings.

Sounds good.