Re: [PATCHv4 15/15] Pass boot device list to firmware.

[Gleb Natapov <gleb@redhat.com>]

On Mon, Nov 15, 2010 at 09:53:50AM +0200, Michael S. Tsirkin wrote:
> On Mon, Nov 15, 2010 at 09:40:08AM +0200, Gleb Natapov wrote:
> > On Sun, Nov 14, 2010 at 10:40:33PM -0500, Kevin O'Connor wrote:
> > > On Sun, Nov 14, 2010 at 05:39:41PM +0200, Gleb Natapov wrote:
> > > > +/*
> > > > + * This function returns device list as an array in a below format:
> > > > + * +-----+-----+---------------+-----+---------------+--
> > > > + * |  n  |  l1 |   devpath1    |  l2 |  devpath2     | ...
> > > > + * +-----+-----+---------------+-----+---------------+--
> > > > + * where:
> > > > + *   n - a number of devise pathes (one byte)
> > > > + *   l - length of following device path string (one byte)
> > > > + *   devpath - non-null terminated string of length l representing
> > > > + *             one device path
> > > > + */
> > > 
> > > Why not just return a newline separated list that is null terminated?
> > > 
> > Doing it like this will needlessly complicate firmware side. How do you
> > know how much memory to allocate before reading device list?
> 
> Do a memory scan, count newlines until you reach 0?
> 
To do memory scan you need to read it into memory first. To read it into
memory you need to know how much memory to allocate to know how much
memory to allocate you meed to do memory scan... Notice pattern here :)
Of course you can scan IO space too discarding everything you read first
time, but why introduce broken interface in the first place?

> > Doing it
> > like Blue suggest (have BOOTINDEX_LEN and BOOTINDEX_STRING) solves this.
> > To create nice array from bootindex string you firmware will still have
> > to do additional pass on it though.
> 
> Why is this a problem? Pass over memory is cheap, isn't it?
> 
More code, each line of code potentially introduce bug. But I will go with
Blue suggestion anyway since we already use it for other things.

> > With format like above the code
> > would look like that:
> > 
> > qemu_cfg_read(&n, 1);
> > arr = alloc(n);
> > for (i=0; i<n; i++) {
> >  qemu_cfg_read(&l, 1);
> >  arr[i] = zalloc(l+1);
> >  qemu_cfg_read(arr[i], l);
> > }
> >  
> > 
> > --
> > 			Gleb.
> 
> 
> At this point I don't care about format.
I do.

> But I would like one without 1-byte-length limitations,
> just so we can cover whatever pci can through at us.
> 
I agree. 1-byte for one device string may be to limiting. It is still
more then 15 PCI bridges on a PC and if you have your pci bus go that
deep you are doing something very wrong. But according to spec device
name can be 32 byte long and device address may be 64bit physical
address and that makes length of one device element to be 50 byte.

--
			Gleb.