Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Kevin O'Connor]

On Tue, May 07, 2013 at 07:33:13PM +0300, Michael S. Tsirkin wrote:
> > If you really feel QEMU should direct the pointer and checksum
> > updates, you might want to consider something like:
> > 
> > struct tabledeploy_s {
> > u32 command;
> > union {
> > // COMMAND_ALLOC - allocate a table from the given "file"
> > struct {
> > char alloc_file[FILESZ];
> > u32 alloc_align;
> > u8 alloc_zone;
> > };
> > 
> > // COMMAND_PATCH - patch the table (originating from
> > // "dest_file") to a pointer to the table originating from
> > // "src_file".
> > struct {
> > char patch_dest_file[FILESZ];
> > char patch_src_file[FILESZ];
> > u32 patch_offset;
> > u8 patch_size;
> > };
> > 
> 
> I think ADD is better since this way we can use not the start of table,
> but something in the middle. For example, a single fw_cfg can give us
> multiple tables, it should be up to QEMU.
> 
> It's equivalent to PATCH if the original data is 0.
> 
> > // COMMAND_CHECKSUM - Update a checksum in a table
> > struct {
> > char cksum_file[FILESZ];
> > u32 cksum_offset;
> > u32 cksum_start;
> > u32 cksum_length;
> > };
> > 
> 
> Same here. So I would do:
> 
>   COMMAND_ADD_POINTER
> 
>   COMMAND_ADD_CHECKSUM

That's fine.

-Kevin

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Gleb Natapov]

On Tue, May 07, 2013 at 10:49:14PM +0300, Michael S. Tsirkin wrote:
> On Tue, May 07, 2013 at 10:37:28PM +0300, Gleb Natapov wrote:
> > On Tue, May 07, 2013 at 10:33:17PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, May 07, 2013 at 10:26:34PM +0300, Gleb Natapov wrote:
> > > > On Tue, May 07, 2013 at 09:54:33PM +0300, Michael S. Tsirkin wrote:
> > > > > On Tue, May 07, 2013 at 09:07:04PM +0300, Gleb Natapov wrote:
> > > > > > On Wed, Apr 24, 2013 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
> > > > > > > On Tue, Apr 23, 2013 at 08:23:44PM +0300, Michael S. Tsirkin 
> > > > > > > wrote:
> > > > > > > > On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote:
> > > > > > > > > On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin 
> > > > > > > > > wrote:
> > > > > > > > > > On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor 
> > > > > > > > > > wrote:
> > > > > > > > > > > On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. 
> > > > > > > > > > > Tsirkin wrote:
> > > > > > > > > > > > Okay I'm pretty close to posting some patches
> > > > > > > > > > > > that advance this project further, but wanted to
> > > > > > > > > > > > check something beforehand: there are several tables
> > > > > > > > > > > > that point to other tables (for example: FADT points
> > > > > > > > > > > > to DSDT). What I did is provide a list of fixups
> > > > > > > > > > > > such that bios can patch in pointers without
> > > > > > > > > > > > any need to understand what's what.
> > > > > > > > > > > > Thoughts?
> > > > > > > > > > > 
> > > > > > > > > > > For the RSDP, RSDT, and FADT I think SeaBIOS should just 
> > > > > > > > > > > update those
> > > > > > > > > > > tables to set the pointers within them and then 
> > > > > > > > > > > recalculate the
> > > > > > > > > > > checksum.  I don't think anything complex is needed - 
> > > > > > > > > > > it's easy for
> > > > > > > > > > > SeaBIOS to recognize those special tables and modify them.
> > > > > > > > > > 
> > > > > > > > > > True, that's simple enough.  My worry is we can add more 
> > > > > > > > > > such tables.
> > > > > > > > > > For example, we can decide to switch to XSDT in the future.
> > > > > > > > > 
> > > > > > > > > I know of the following quirks that would have to be handled:
> > > > > > > > > 
> > > > > > > > > 1 - the RSDP must be in the f-segment (where as all other 
> > > > > > > > > tables can
> > > > > > > > > go into "high" memory).
> > > > > > > > > 
> > > > > > > > > 2 - the RSDP has a checksum in a different location from the 
> > > > > > > > > other
> > > > > > > > > tables and (with an XSDT) it can have two checksums.
> > > > > > > > > 
> > > > > > > > > 3 - the RSDP has a pointer to the RSDT (and to the XSDT if 
> > > > > > > > > present).
> > > > > > > > > 
> > > > > > > > > 4 - the RSDT (and XSDT if present) has pointers to all the 
> > > > > > > > > other
> > > > > > > > > tables (except RSDP, RSDT, DSDT, and FACS).  The FADT pointer 
> > > > > > > > > must be
> > > > > > > > > first in the list.
> > > > > > > > > 
> > > > > > > > > 5 - the FADT table has pointers to DSDT and FACS.
> > > > > > > > > 
> > > > > > > > > 6 - the FACS table must be 64 byte aligned.
> > > > > > > > > 
> > > > > > > > > So, will a generic scheme really be able to handle all of the 
> > > > > > > > > above
> > > > > > > > > quirks, or will we just be mixing some hardcoded quirks with 
> > > > > > > > > some
> > > > > > > > > generic quirks?  And, will the code to handle the above 
> > > > > > > > > quirks in a
> > > > > > > > > generic fashion be of a higher complexity than simply 
> > > > > > > > > hard-coding it?
> > > > > > > > > 
> > > > > > > > > -Kevin
> > > > > > > > 
> > > > > > > > -->
> > > > > > > > 
> > > > > > > > So here's an implementation for align and FSEG.
> > > > > > > > Not a big deal as you see.
> > > > > > > > 
> > > > > > > > I really have doubts about it however: BIOS still must be able 
> > > > > > > > to parse
> > > > > > > > get the resume vector in FACS in order to support wakeup, 
> > > > > > > > right? So this
> > > > > > > > means that we need to be able to parse RSDP and FACT.  These 
> > > > > > > > happen to
> > > > > > > > be the only things that need anything not addressed by ADD and 
> > > > > > > > SUB so
> > > > > > > > ... maybe a couple of hardcoded quirks just to allocate these 
> > > > > > > > correctly
> > > > > > > > is cleaner.
> > > > > > > 
> > > > > > > Heh, it's actually pretty easy: let's just ask qemu
> > > > > > > to give us the address of the resume vector
> > > > > > > in a file with a pre-defined name.
> > > > > > > Linker can patch table offset there in the
> > > > > > > regular way.
> > > > > > > 
> > > > > > Seabios can find resume vector address the same way OSPM does: by 
> > > > > > following
> > > > > > pointers in memory.
> > > > > 
> > > > > Yes, that's what we do now.
> > > > > 
> > > > Good.
> > > > 
> > > > > > What QEMU has to do with it?
> > > > > 
> > > > > The paragraphs abo

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Tue, May 07, 2013 at 10:37:28PM +0300, Gleb Natapov wrote:
> On Tue, May 07, 2013 at 10:33:17PM +0300, Michael S. Tsirkin wrote:
> > On Tue, May 07, 2013 at 10:26:34PM +0300, Gleb Natapov wrote:
> > > On Tue, May 07, 2013 at 09:54:33PM +0300, Michael S. Tsirkin wrote:
> > > > On Tue, May 07, 2013 at 09:07:04PM +0300, Gleb Natapov wrote:
> > > > > On Wed, Apr 24, 2013 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
> > > > > > On Tue, Apr 23, 2013 at 08:23:44PM +0300, Michael S. Tsirkin wrote:
> > > > > > > On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote:
> > > > > > > > On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin 
> > > > > > > > wrote:
> > > > > > > > > On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor 
> > > > > > > > > wrote:
> > > > > > > > > > On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. 
> > > > > > > > > > Tsirkin wrote:
> > > > > > > > > > > Okay I'm pretty close to posting some patches
> > > > > > > > > > > that advance this project further, but wanted to
> > > > > > > > > > > check something beforehand: there are several tables
> > > > > > > > > > > that point to other tables (for example: FADT points
> > > > > > > > > > > to DSDT). What I did is provide a list of fixups
> > > > > > > > > > > such that bios can patch in pointers without
> > > > > > > > > > > any need to understand what's what.
> > > > > > > > > > > Thoughts?
> > > > > > > > > > 
> > > > > > > > > > For the RSDP, RSDT, and FADT I think SeaBIOS should just 
> > > > > > > > > > update those
> > > > > > > > > > tables to set the pointers within them and then recalculate 
> > > > > > > > > > the
> > > > > > > > > > checksum.  I don't think anything complex is needed - it's 
> > > > > > > > > > easy for
> > > > > > > > > > SeaBIOS to recognize those special tables and modify them.
> > > > > > > > > 
> > > > > > > > > True, that's simple enough.  My worry is we can add more such 
> > > > > > > > > tables.
> > > > > > > > > For example, we can decide to switch to XSDT in the future.
> > > > > > > > 
> > > > > > > > I know of the following quirks that would have to be handled:
> > > > > > > > 
> > > > > > > > 1 - the RSDP must be in the f-segment (where as all other 
> > > > > > > > tables can
> > > > > > > > go into "high" memory).
> > > > > > > > 
> > > > > > > > 2 - the RSDP has a checksum in a different location from the 
> > > > > > > > other
> > > > > > > > tables and (with an XSDT) it can have two checksums.
> > > > > > > > 
> > > > > > > > 3 - the RSDP has a pointer to the RSDT (and to the XSDT if 
> > > > > > > > present).
> > > > > > > > 
> > > > > > > > 4 - the RSDT (and XSDT if present) has pointers to all the other
> > > > > > > > tables (except RSDP, RSDT, DSDT, and FACS).  The FADT pointer 
> > > > > > > > must be
> > > > > > > > first in the list.
> > > > > > > > 
> > > > > > > > 5 - the FADT table has pointers to DSDT and FACS.
> > > > > > > > 
> > > > > > > > 6 - the FACS table must be 64 byte aligned.
> > > > > > > > 
> > > > > > > > So, will a generic scheme really be able to handle all of the 
> > > > > > > > above
> > > > > > > > quirks, or will we just be mixing some hardcoded quirks with 
> > > > > > > > some
> > > > > > > > generic quirks?  And, will the code to handle the above quirks 
> > > > > > > > in a
> > > > > > > > generic fashion be of a higher complexity than simply 
> > > > > > > > hard-coding it?
> > > > > > > > 
> > > > > > > > -Kevin
> > > > > > > 
> > > > > > > -->
> > > > > > > 
> > > > > > > So here's an implementation for align and FSEG.
> > > > > > > Not a big deal as you see.
> > > > > > > 
> > > > > > > I really have doubts about it however: BIOS still must be able to 
> > > > > > > parse
> > > > > > > get the resume vector in FACS in order to support wakeup, right? 
> > > > > > > So this
> > > > > > > means that we need to be able to parse RSDP and FACT.  These 
> > > > > > > happen to
> > > > > > > be the only things that need anything not addressed by ADD and 
> > > > > > > SUB so
> > > > > > > ... maybe a couple of hardcoded quirks just to allocate these 
> > > > > > > correctly
> > > > > > > is cleaner.
> > > > > > 
> > > > > > Heh, it's actually pretty easy: let's just ask qemu
> > > > > > to give us the address of the resume vector
> > > > > > in a file with a pre-defined name.
> > > > > > Linker can patch table offset there in the
> > > > > > regular way.
> > > > > > 
> > > > > Seabios can find resume vector address the same way OSPM does: by 
> > > > > following
> > > > > pointers in memory.
> > > > 
> > > > Yes, that's what we do now.
> > > > 
> > > Good.
> > > 
> > > > > What QEMU has to do with it?
> > > > 
> > > > The paragraphs above explain the connection.
> > > > 
> > > Do not see any explanation anywhere.
> > 
> > Maybe I don't understand your question then.
> > What exactly would you like to know?
> > 
> My question is why would you "ask qemu to give us the address of the
> resume vector in a file with a pre-defined name". But 

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Gleb Natapov]

On Tue, May 07, 2013 at 10:33:17PM +0300, Michael S. Tsirkin wrote:
> On Tue, May 07, 2013 at 10:26:34PM +0300, Gleb Natapov wrote:
> > On Tue, May 07, 2013 at 09:54:33PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, May 07, 2013 at 09:07:04PM +0300, Gleb Natapov wrote:
> > > > On Wed, Apr 24, 2013 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
> > > > > On Tue, Apr 23, 2013 at 08:23:44PM +0300, Michael S. Tsirkin wrote:
> > > > > > On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote:
> > > > > > > On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin 
> > > > > > > wrote:
> > > > > > > > On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
> > > > > > > > > On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin 
> > > > > > > > > wrote:
> > > > > > > > > > Okay I'm pretty close to posting some patches
> > > > > > > > > > that advance this project further, but wanted to
> > > > > > > > > > check something beforehand: there are several tables
> > > > > > > > > > that point to other tables (for example: FADT points
> > > > > > > > > > to DSDT). What I did is provide a list of fixups
> > > > > > > > > > such that bios can patch in pointers without
> > > > > > > > > > any need to understand what's what.
> > > > > > > > > > Thoughts?
> > > > > > > > > 
> > > > > > > > > For the RSDP, RSDT, and FADT I think SeaBIOS should just 
> > > > > > > > > update those
> > > > > > > > > tables to set the pointers within them and then recalculate 
> > > > > > > > > the
> > > > > > > > > checksum.  I don't think anything complex is needed - it's 
> > > > > > > > > easy for
> > > > > > > > > SeaBIOS to recognize those special tables and modify them.
> > > > > > > > 
> > > > > > > > True, that's simple enough.  My worry is we can add more such 
> > > > > > > > tables.
> > > > > > > > For example, we can decide to switch to XSDT in the future.
> > > > > > > 
> > > > > > > I know of the following quirks that would have to be handled:
> > > > > > > 
> > > > > > > 1 - the RSDP must be in the f-segment (where as all other tables 
> > > > > > > can
> > > > > > > go into "high" memory).
> > > > > > > 
> > > > > > > 2 - the RSDP has a checksum in a different location from the other
> > > > > > > tables and (with an XSDT) it can have two checksums.
> > > > > > > 
> > > > > > > 3 - the RSDP has a pointer to the RSDT (and to the XSDT if 
> > > > > > > present).
> > > > > > > 
> > > > > > > 4 - the RSDT (and XSDT if present) has pointers to all the other
> > > > > > > tables (except RSDP, RSDT, DSDT, and FACS).  The FADT pointer 
> > > > > > > must be
> > > > > > > first in the list.
> > > > > > > 
> > > > > > > 5 - the FADT table has pointers to DSDT and FACS.
> > > > > > > 
> > > > > > > 6 - the FACS table must be 64 byte aligned.
> > > > > > > 
> > > > > > > So, will a generic scheme really be able to handle all of the 
> > > > > > > above
> > > > > > > quirks, or will we just be mixing some hardcoded quirks with some
> > > > > > > generic quirks?  And, will the code to handle the above quirks in 
> > > > > > > a
> > > > > > > generic fashion be of a higher complexity than simply hard-coding 
> > > > > > > it?
> > > > > > > 
> > > > > > > -Kevin
> > > > > > 
> > > > > > -->
> > > > > > 
> > > > > > So here's an implementation for align and FSEG.
> > > > > > Not a big deal as you see.
> > > > > > 
> > > > > > I really have doubts about it however: BIOS still must be able to 
> > > > > > parse
> > > > > > get the resume vector in FACS in order to support wakeup, right? So 
> > > > > > this
> > > > > > means that we need to be able to parse RSDP and FACT.  These happen 
> > > > > > to
> > > > > > be the only things that need anything not addressed by ADD and SUB 
> > > > > > so
> > > > > > ... maybe a couple of hardcoded quirks just to allocate these 
> > > > > > correctly
> > > > > > is cleaner.
> > > > > 
> > > > > Heh, it's actually pretty easy: let's just ask qemu
> > > > > to give us the address of the resume vector
> > > > > in a file with a pre-defined name.
> > > > > Linker can patch table offset there in the
> > > > > regular way.
> > > > > 
> > > > Seabios can find resume vector address the same way OSPM does: by 
> > > > following
> > > > pointers in memory.
> > > 
> > > Yes, that's what we do now.
> > > 
> > Good.
> > 
> > > > What QEMU has to do with it?
> > > 
> > > The paragraphs above explain the connection.
> > > 
> > Do not see any explanation anywhere.
> 
> Maybe I don't understand your question then.
> What exactly would you like to know?
> 
My question is why would you "ask qemu to give us the address of the
resume vector in a file with a pre-defined name". But since you do not
do that in your patches I think we are in agreement that this is not
needed.

--
Gleb.

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Tue, May 07, 2013 at 10:26:34PM +0300, Gleb Natapov wrote:
> On Tue, May 07, 2013 at 09:54:33PM +0300, Michael S. Tsirkin wrote:
> > On Tue, May 07, 2013 at 09:07:04PM +0300, Gleb Natapov wrote:
> > > On Wed, Apr 24, 2013 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
> > > > On Tue, Apr 23, 2013 at 08:23:44PM +0300, Michael S. Tsirkin wrote:
> > > > > On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote:
> > > > > > On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
> > > > > > > On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
> > > > > > > > On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin 
> > > > > > > > wrote:
> > > > > > > > > Okay I'm pretty close to posting some patches
> > > > > > > > > that advance this project further, but wanted to
> > > > > > > > > check something beforehand: there are several tables
> > > > > > > > > that point to other tables (for example: FADT points
> > > > > > > > > to DSDT). What I did is provide a list of fixups
> > > > > > > > > such that bios can patch in pointers without
> > > > > > > > > any need to understand what's what.
> > > > > > > > > Thoughts?
> > > > > > > > 
> > > > > > > > For the RSDP, RSDT, and FADT I think SeaBIOS should just update 
> > > > > > > > those
> > > > > > > > tables to set the pointers within them and then recalculate the
> > > > > > > > checksum.  I don't think anything complex is needed - it's easy 
> > > > > > > > for
> > > > > > > > SeaBIOS to recognize those special tables and modify them.
> > > > > > > 
> > > > > > > True, that's simple enough.  My worry is we can add more such 
> > > > > > > tables.
> > > > > > > For example, we can decide to switch to XSDT in the future.
> > > > > > 
> > > > > > I know of the following quirks that would have to be handled:
> > > > > > 
> > > > > > 1 - the RSDP must be in the f-segment (where as all other tables can
> > > > > > go into "high" memory).
> > > > > > 
> > > > > > 2 - the RSDP has a checksum in a different location from the other
> > > > > > tables and (with an XSDT) it can have two checksums.
> > > > > > 
> > > > > > 3 - the RSDP has a pointer to the RSDT (and to the XSDT if present).
> > > > > > 
> > > > > > 4 - the RSDT (and XSDT if present) has pointers to all the other
> > > > > > tables (except RSDP, RSDT, DSDT, and FACS).  The FADT pointer must 
> > > > > > be
> > > > > > first in the list.
> > > > > > 
> > > > > > 5 - the FADT table has pointers to DSDT and FACS.
> > > > > > 
> > > > > > 6 - the FACS table must be 64 byte aligned.
> > > > > > 
> > > > > > So, will a generic scheme really be able to handle all of the above
> > > > > > quirks, or will we just be mixing some hardcoded quirks with some
> > > > > > generic quirks?  And, will the code to handle the above quirks in a
> > > > > > generic fashion be of a higher complexity than simply hard-coding 
> > > > > > it?
> > > > > > 
> > > > > > -Kevin
> > > > > 
> > > > > -->
> > > > > 
> > > > > So here's an implementation for align and FSEG.
> > > > > Not a big deal as you see.
> > > > > 
> > > > > I really have doubts about it however: BIOS still must be able to 
> > > > > parse
> > > > > get the resume vector in FACS in order to support wakeup, right? So 
> > > > > this
> > > > > means that we need to be able to parse RSDP and FACT.  These happen to
> > > > > be the only things that need anything not addressed by ADD and SUB so
> > > > > ... maybe a couple of hardcoded quirks just to allocate these 
> > > > > correctly
> > > > > is cleaner.
> > > > 
> > > > Heh, it's actually pretty easy: let's just ask qemu
> > > > to give us the address of the resume vector
> > > > in a file with a pre-defined name.
> > > > Linker can patch table offset there in the
> > > > regular way.
> > > > 
> > > Seabios can find resume vector address the same way OSPM does: by 
> > > following
> > > pointers in memory.
> > 
> > Yes, that's what we do now.
> > 
> Good.
> 
> > > What QEMU has to do with it?
> > 
> > The paragraphs above explain the connection.
> > 
> Do not see any explanation anywhere.

Maybe I don't understand your question then.
What exactly would you like to know?

> --
>   Gleb.

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Gleb Natapov]

On Tue, May 07, 2013 at 09:54:33PM +0300, Michael S. Tsirkin wrote:
> On Tue, May 07, 2013 at 09:07:04PM +0300, Gleb Natapov wrote:
> > On Wed, Apr 24, 2013 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
> > > On Tue, Apr 23, 2013 at 08:23:44PM +0300, Michael S. Tsirkin wrote:
> > > > On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote:
> > > > > On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
> > > > > > On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
> > > > > > > On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin 
> > > > > > > wrote:
> > > > > > > > Okay I'm pretty close to posting some patches
> > > > > > > > that advance this project further, but wanted to
> > > > > > > > check something beforehand: there are several tables
> > > > > > > > that point to other tables (for example: FADT points
> > > > > > > > to DSDT). What I did is provide a list of fixups
> > > > > > > > such that bios can patch in pointers without
> > > > > > > > any need to understand what's what.
> > > > > > > > Thoughts?
> > > > > > > 
> > > > > > > For the RSDP, RSDT, and FADT I think SeaBIOS should just update 
> > > > > > > those
> > > > > > > tables to set the pointers within them and then recalculate the
> > > > > > > checksum.  I don't think anything complex is needed - it's easy 
> > > > > > > for
> > > > > > > SeaBIOS to recognize those special tables and modify them.
> > > > > > 
> > > > > > True, that's simple enough.  My worry is we can add more such 
> > > > > > tables.
> > > > > > For example, we can decide to switch to XSDT in the future.
> > > > > 
> > > > > I know of the following quirks that would have to be handled:
> > > > > 
> > > > > 1 - the RSDP must be in the f-segment (where as all other tables can
> > > > > go into "high" memory).
> > > > > 
> > > > > 2 - the RSDP has a checksum in a different location from the other
> > > > > tables and (with an XSDT) it can have two checksums.
> > > > > 
> > > > > 3 - the RSDP has a pointer to the RSDT (and to the XSDT if present).
> > > > > 
> > > > > 4 - the RSDT (and XSDT if present) has pointers to all the other
> > > > > tables (except RSDP, RSDT, DSDT, and FACS).  The FADT pointer must be
> > > > > first in the list.
> > > > > 
> > > > > 5 - the FADT table has pointers to DSDT and FACS.
> > > > > 
> > > > > 6 - the FACS table must be 64 byte aligned.
> > > > > 
> > > > > So, will a generic scheme really be able to handle all of the above
> > > > > quirks, or will we just be mixing some hardcoded quirks with some
> > > > > generic quirks?  And, will the code to handle the above quirks in a
> > > > > generic fashion be of a higher complexity than simply hard-coding it?
> > > > > 
> > > > > -Kevin
> > > > 
> > > > -->
> > > > 
> > > > So here's an implementation for align and FSEG.
> > > > Not a big deal as you see.
> > > > 
> > > > I really have doubts about it however: BIOS still must be able to parse
> > > > get the resume vector in FACS in order to support wakeup, right? So this
> > > > means that we need to be able to parse RSDP and FACT.  These happen to
> > > > be the only things that need anything not addressed by ADD and SUB so
> > > > ... maybe a couple of hardcoded quirks just to allocate these correctly
> > > > is cleaner.
> > > 
> > > Heh, it's actually pretty easy: let's just ask qemu
> > > to give us the address of the resume vector
> > > in a file with a pre-defined name.
> > > Linker can patch table offset there in the
> > > regular way.
> > > 
> > Seabios can find resume vector address the same way OSPM does: by following
> > pointers in memory.
> 
> Yes, that's what we do now.
> 
Good.

> > What QEMU has to do with it?
> 
> The paragraphs above explain the connection.
> 
Do not see any explanation anywhere.

--
Gleb.

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Tue, May 07, 2013 at 09:07:04PM +0300, Gleb Natapov wrote:
> On Wed, Apr 24, 2013 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
> > On Tue, Apr 23, 2013 at 08:23:44PM +0300, Michael S. Tsirkin wrote:
> > > On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote:
> > > > On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
> > > > > On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
> > > > > > On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
> > > > > > > Okay I'm pretty close to posting some patches
> > > > > > > that advance this project further, but wanted to
> > > > > > > check something beforehand: there are several tables
> > > > > > > that point to other tables (for example: FADT points
> > > > > > > to DSDT). What I did is provide a list of fixups
> > > > > > > such that bios can patch in pointers without
> > > > > > > any need to understand what's what.
> > > > > > > Thoughts?
> > > > > > 
> > > > > > For the RSDP, RSDT, and FADT I think SeaBIOS should just update 
> > > > > > those
> > > > > > tables to set the pointers within them and then recalculate the
> > > > > > checksum.  I don't think anything complex is needed - it's easy for
> > > > > > SeaBIOS to recognize those special tables and modify them.
> > > > > 
> > > > > True, that's simple enough.  My worry is we can add more such tables.
> > > > > For example, we can decide to switch to XSDT in the future.
> > > > 
> > > > I know of the following quirks that would have to be handled:
> > > > 
> > > > 1 - the RSDP must be in the f-segment (where as all other tables can
> > > > go into "high" memory).
> > > > 
> > > > 2 - the RSDP has a checksum in a different location from the other
> > > > tables and (with an XSDT) it can have two checksums.
> > > > 
> > > > 3 - the RSDP has a pointer to the RSDT (and to the XSDT if present).
> > > > 
> > > > 4 - the RSDT (and XSDT if present) has pointers to all the other
> > > > tables (except RSDP, RSDT, DSDT, and FACS).  The FADT pointer must be
> > > > first in the list.
> > > > 
> > > > 5 - the FADT table has pointers to DSDT and FACS.
> > > > 
> > > > 6 - the FACS table must be 64 byte aligned.
> > > > 
> > > > So, will a generic scheme really be able to handle all of the above
> > > > quirks, or will we just be mixing some hardcoded quirks with some
> > > > generic quirks?  And, will the code to handle the above quirks in a
> > > > generic fashion be of a higher complexity than simply hard-coding it?
> > > > 
> > > > -Kevin
> > > 
> > > -->
> > > 
> > > So here's an implementation for align and FSEG.
> > > Not a big deal as you see.
> > > 
> > > I really have doubts about it however: BIOS still must be able to parse
> > > get the resume vector in FACS in order to support wakeup, right? So this
> > > means that we need to be able to parse RSDP and FACT.  These happen to
> > > be the only things that need anything not addressed by ADD and SUB so
> > > ... maybe a couple of hardcoded quirks just to allocate these correctly
> > > is cleaner.
> > 
> > Heh, it's actually pretty easy: let's just ask qemu
> > to give us the address of the resume vector
> > in a file with a pre-defined name.
> > Linker can patch table offset there in the
> > regular way.
> > 
> Seabios can find resume vector address the same way OSPM does: by following
> pointers in memory.

Yes, that's what we do now.

> What QEMU has to do with it?

The paragraphs above explain the connection.

> 
> --
>   Gleb.

-- 
MST

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Gleb Natapov]

On Wed, Apr 24, 2013 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
> On Tue, Apr 23, 2013 at 08:23:44PM +0300, Michael S. Tsirkin wrote:
> > On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote:
> > > On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
> > > > On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
> > > > > On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
> > > > > > Okay I'm pretty close to posting some patches
> > > > > > that advance this project further, but wanted to
> > > > > > check something beforehand: there are several tables
> > > > > > that point to other tables (for example: FADT points
> > > > > > to DSDT). What I did is provide a list of fixups
> > > > > > such that bios can patch in pointers without
> > > > > > any need to understand what's what.
> > > > > > Thoughts?
> > > > > 
> > > > > For the RSDP, RSDT, and FADT I think SeaBIOS should just update those
> > > > > tables to set the pointers within them and then recalculate the
> > > > > checksum.  I don't think anything complex is needed - it's easy for
> > > > > SeaBIOS to recognize those special tables and modify them.
> > > > 
> > > > True, that's simple enough.  My worry is we can add more such tables.
> > > > For example, we can decide to switch to XSDT in the future.
> > > 
> > > I know of the following quirks that would have to be handled:
> > > 
> > > 1 - the RSDP must be in the f-segment (where as all other tables can
> > > go into "high" memory).
> > > 
> > > 2 - the RSDP has a checksum in a different location from the other
> > > tables and (with an XSDT) it can have two checksums.
> > > 
> > > 3 - the RSDP has a pointer to the RSDT (and to the XSDT if present).
> > > 
> > > 4 - the RSDT (and XSDT if present) has pointers to all the other
> > > tables (except RSDP, RSDT, DSDT, and FACS).  The FADT pointer must be
> > > first in the list.
> > > 
> > > 5 - the FADT table has pointers to DSDT and FACS.
> > > 
> > > 6 - the FACS table must be 64 byte aligned.
> > > 
> > > So, will a generic scheme really be able to handle all of the above
> > > quirks, or will we just be mixing some hardcoded quirks with some
> > > generic quirks?  And, will the code to handle the above quirks in a
> > > generic fashion be of a higher complexity than simply hard-coding it?
> > > 
> > > -Kevin
> > 
> > -->
> > 
> > So here's an implementation for align and FSEG.
> > Not a big deal as you see.
> > 
> > I really have doubts about it however: BIOS still must be able to parse
> > get the resume vector in FACS in order to support wakeup, right? So this
> > means that we need to be able to parse RSDP and FACT.  These happen to
> > be the only things that need anything not addressed by ADD and SUB so
> > ... maybe a couple of hardcoded quirks just to allocate these correctly
> > is cleaner.
> 
> Heh, it's actually pretty easy: let's just ask qemu
> to give us the address of the resume vector
> in a file with a pre-defined name.
> Linker can patch table offset there in the
> regular way.
> 
Seabios can find resume vector address the same way OSPM does: by following
pointers in memory. What QEMU has to do with it?

--
Gleb.

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Tue, Apr 23, 2013 at 08:54:17PM -0400, Kevin O'Connor wrote:
> On Tue, Apr 23, 2013 at 06:22:12PM +0300, Michael S. Tsirkin wrote:
> > So here's the version with shift, that should do it
> > correctly. Add mere 8 lines of code.
> > I'll look into alignment and fseg now.
> [...]
> > --- /dev/null
> > +++ b/src/linker.h
> > @@ -0,0 +1,29 @@
> > +#include "types.h" // u8
> > +#include "util.h" // romfile_s
> > +
> > +/* ROM file linker interface. Linker uses little endian format */
> > +struct linker_entry_s {
> > +u8 size; /* size in bytes including the header */
> > +u8 bytes; /* How many bytes to change. Can be 1,2,4 or 8. */
> > +u8 shift; /* Shift source address right by this many bits. 0-63. */
> > +u8 type;
> > +u8 format;
> > +u8 reserved1;
> > +u16 reserved2;
> > +u64 dst_offset; /* Offset in destination. Little endian format. */
> > +/* Followed by source and destination, optionally padded by
> > + * 0, up to the total of entry_len - 4 bytes.
> > + * Strings are 0-terminated. */
> > +char src_dst[];
> > +} PACKED;
> 
> As in my previous email, I think the main emphasis should be on
> getting the content into QEMU, so I don't want to nitpick on the
> details of the seabios side.  However, if I understand your proposal,
> an RSDT with 10 table entries would require 50 "linker" entries (10 to
> add the pointers and 40 to update the checksum) - that seems
> excessive.

Well that's not a lot of memory, is it?
And it's freed immediately after we link.

> If the goal is to do a checksum - might as well just
> define a checksum action.

I agree it makes the interface more more straight-forward
to use.

> Also, the variable length filenames aren't
> needed - the QemuCfgFile interface already limits the filename to a
> max of 56 bytes.

True. Two reasons I did not do it this way:
- this makes the code generic. Maybe others will want to reuse this?
- a bit less memory - we dont have long file names in practice.
Valid concerns?

> 
> If you really feel QEMU should direct the pointer and checksum
> updates, you might want to consider something like:
> 
> struct tabledeploy_s {
> u32 command;
> union {
> // COMMAND_ALLOC - allocate a table from the given "file"
> struct {
> char alloc_file[FILESZ];
> u32 alloc_align;
> u8 alloc_zone;
> };
> 
> // COMMAND_PATCH - patch the table (originating from
> // "dest_file") to a pointer to the table originating from
> // "src_file".
> struct {
> char patch_dest_file[FILESZ];
> char patch_src_file[FILESZ];
> u32 patch_offset;
> u8 patch_size;
> };
> 

I think ADD is better since this way we can use not the start of table,
but something in the middle. For example, a single fw_cfg can give us
multiple tables, it should be up to QEMU.

It's equivalent to PATCH if the original data is 0.

> // COMMAND_CHECKSUM - Update a checksum in a table
> struct {
> char cksum_file[FILESZ];
> u32 cksum_offset;
> u32 cksum_start;
> u32 cksum_length;
> };
> 

Same here. So I would do:

COMMAND_ADD_POINTER

COMMAND_ADD_CHECKSUM

> // PAD
> char pad[124];
> };
> } PACKED;
> 
> -Kevin

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Tue, Apr 23, 2013 at 08:23:44PM +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote:
> > On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
> > > On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
> > > > On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
> > > > > Okay I'm pretty close to posting some patches
> > > > > that advance this project further, but wanted to
> > > > > check something beforehand: there are several tables
> > > > > that point to other tables (for example: FADT points
> > > > > to DSDT). What I did is provide a list of fixups
> > > > > such that bios can patch in pointers without
> > > > > any need to understand what's what.
> > > > > Thoughts?
> > > > 
> > > > For the RSDP, RSDT, and FADT I think SeaBIOS should just update those
> > > > tables to set the pointers within them and then recalculate the
> > > > checksum.  I don't think anything complex is needed - it's easy for
> > > > SeaBIOS to recognize those special tables and modify them.
> > > 
> > > True, that's simple enough.  My worry is we can add more such tables.
> > > For example, we can decide to switch to XSDT in the future.
> > 
> > I know of the following quirks that would have to be handled:
> > 
> > 1 - the RSDP must be in the f-segment (where as all other tables can
> > go into "high" memory).
> > 
> > 2 - the RSDP has a checksum in a different location from the other
> > tables and (with an XSDT) it can have two checksums.
> > 
> > 3 - the RSDP has a pointer to the RSDT (and to the XSDT if present).
> > 
> > 4 - the RSDT (and XSDT if present) has pointers to all the other
> > tables (except RSDP, RSDT, DSDT, and FACS).  The FADT pointer must be
> > first in the list.
> > 
> > 5 - the FADT table has pointers to DSDT and FACS.
> > 
> > 6 - the FACS table must be 64 byte aligned.
> > 
> > So, will a generic scheme really be able to handle all of the above
> > quirks, or will we just be mixing some hardcoded quirks with some
> > generic quirks?  And, will the code to handle the above quirks in a
> > generic fashion be of a higher complexity than simply hard-coding it?
> > 
> > -Kevin
> 
> -->
> 
> So here's an implementation for align and FSEG.
> Not a big deal as you see.
> 
> I really have doubts about it however: BIOS still must be able to parse
> get the resume vector in FACS in order to support wakeup, right? So this
> means that we need to be able to parse RSDP and FACT.  These happen to
> be the only things that need anything not addressed by ADD and SUB so
> ... maybe a couple of hardcoded quirks just to allocate these correctly
> is cleaner.

Heh, it's actually pretty easy: let's just ask qemu
to give us the address of the resume vector
in a file with a pre-defined name.
Linker can patch table offset there in the
regular way.



> Signed-off-by: Michael S. Tsirkin 
> 
> ---
> 
> diff --git a/src/linker.c b/src/linker.c
> index a1f473d..22a0dff 100644
> --- a/src/linker.c
> +++ b/src/linker.c
> @@ -34,7 +34,9 @@ void linker_link(const char *name)
>   if (entry->shift > 63)
>  continue;
>  if (entry->type != LINKER_ENTRY_TYPE_ADD &&
> -entry->type != LINKER_ENTRY_TYPE_SUB)
> +entry->type != LINKER_ENTRY_TYPE_SUB &&
> +entry->type != LINKER_ENTRY_TYPE_ALIGN &&
> +entry->type != LINKER_ENTRY_TYPE_FSEG)
>  continue;
>  if (entry->format != LINKER_ENTRY_FORMAT_LE)
>  continue;
> @@ -44,17 +46,55 @@ void linker_link(const char *name)
>  continue;
>  }
>  lsrc = strlen(entry->src_dst);
> -if (!lsrc || lsrc + 1 +  sizeof *entry >= entry->size) {
> +if (!lsrc) {
>  warn_internalerror();
>  continue;
>  }
>  src = romfile_find(entry->src_dst);
> +if (!src) {
> +warn_internalerror();
> +continue;
> +}
> +if (!src->data) {
> +warn_internalerror();
> +continue;
> +}
> + if (entry->type == LINKER_ENTRY_TYPE_ALIGN ||
> + entry->type == LINKER_ENTRY_TYPE_FSEG) {
> + void *data;
> + u32 align;
> + struct zone_s *zone;
> +
> + if (entry->shift > 31) {
> + warn_internalerror();
> + continue;
> + }
> + align = 0x1 << entry->shift;
> + if (align < MALLOC_MIN_ALIGN)
> + align = MALLOC_MIN_ALIGN;
> + zone = entry->type == LINKER_ENTRY_TYPE_FSEG ?
> + &ZoneFSeg : &ZoneHigh;
> + data = pmm_malloc(zone, PMM_DEFAULT_HANDLE, src->size, align);
> + if (!data) {
> + warn_internalerror();
> + continue;
> + }
> + memcpy(data, src->data, src->size);
> + free(src->data);
> + src->data = data;

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Kevin O'Connor]

On Tue, Apr 23, 2013 at 06:22:12PM +0300, Michael S. Tsirkin wrote:
> So here's the version with shift, that should do it
> correctly. Add mere 8 lines of code.
> I'll look into alignment and fseg now.
[...]
> --- /dev/null
> +++ b/src/linker.h
> @@ -0,0 +1,29 @@
> +#include "types.h" // u8
> +#include "util.h" // romfile_s
> +
> +/* ROM file linker interface. Linker uses little endian format */
> +struct linker_entry_s {
> +u8 size; /* size in bytes including the header */
> +u8 bytes; /* How many bytes to change. Can be 1,2,4 or 8. */
> +u8 shift; /* Shift source address right by this many bits. 0-63. */
> +u8 type;
> +u8 format;
> +u8 reserved1;
> +u16 reserved2;
> +u64 dst_offset; /* Offset in destination. Little endian format. */
> +/* Followed by source and destination, optionally padded by
> + * 0, up to the total of entry_len - 4 bytes.
> + * Strings are 0-terminated. */
> +char src_dst[];
> +} PACKED;

As in my previous email, I think the main emphasis should be on
getting the content into QEMU, so I don't want to nitpick on the
details of the seabios side.  However, if I understand your proposal,
an RSDT with 10 table entries would require 50 "linker" entries (10 to
add the pointers and 40 to update the checksum) - that seems
excessive.  If the goal is to do a checksum - might as well just
define a checksum action.  Also, the variable length filenames aren't
needed - the QemuCfgFile interface already limits the filename to a
max of 56 bytes.

If you really feel QEMU should direct the pointer and checksum
updates, you might want to consider something like:

struct tabledeploy_s {
u32 command;
union {
// COMMAND_ALLOC - allocate a table from the given "file"
struct {
char alloc_file[FILESZ];
u32 alloc_align;
u8 alloc_zone;
};

// COMMAND_PATCH - patch the table (originating from
// "dest_file") to a pointer to the table originating from
// "src_file".
struct {
char patch_dest_file[FILESZ];
char patch_src_file[FILESZ];
u32 patch_offset;
u8 patch_size;
};

// COMMAND_CHECKSUM - Update a checksum in a table
struct {
char cksum_file[FILESZ];
u32 cksum_offset;
u32 cksum_start;
u32 cksum_length;
};

// PAD
char pad[124];
};
} PACKED;

-Kevin

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Kevin O'Connor]

On Tue, Apr 23, 2013 at 11:49:57AM +0300, Michael S. Tsirkin wrote:
> > I know of the following quirks that would have to be handled:
> > 
> > 1 - the RSDP must be in the f-segment (where as all other tables can
> > go into "high" memory).
> > 
> > 2 - the RSDP has a checksum in a different location from the other
> > tables and (with an XSDT) it can have two checksums.
> > 
> > 3 - the RSDP has a pointer to the RSDT (and to the XSDT if present).
> > 
> > 4 - the RSDT (and XSDT if present) has pointers to all the other
> > tables (except RSDP, RSDT, DSDT, and FACS).  The FADT pointer must be
> > first in the list.
> > 
> > 5 - the FADT table has pointers to DSDT and FACS.
> > 
> > 6 - the FACS table must be 64 byte aligned.
> 
> And of course newer ACPI has lots of other pointer quirks,
> I assume you are aware of this.

I haven't had a chance to look through the newer acpi specs, but it's
a shame more of the pointer quirks have been added.

> > So, will a generic scheme really be able to handle all of the above
> > quirks, or will we just be mixing some hardcoded quirks with some
> > generic quirks?  And, will the code to handle the above quirks in a
> > generic fashion be of a higher complexity than simply hard-coding it?
> 
> I wanted to handle checksums and pointers in a generic fashion,
> and allocation rules in a table specific version
> (since I only found two such examples in all of the spec:
> FACS and RSDP). It's not hard to add generic handlers for these two,
> and it's not much more code. You think it's preferable then?

I don't know if it's perferable or not.  I guess one advantage of it
is that the same mechanism could then also be used for smbios,
mptable, and pir.

I think the seabios part of this will be straight forward no matter
which direction is taken.  The real work will be on getting the tables
created in qemu.

-Kevin

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote:
> On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
> > On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
> > > On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
> > > > Okay I'm pretty close to posting some patches
> > > > that advance this project further, but wanted to
> > > > check something beforehand: there are several tables
> > > > that point to other tables (for example: FADT points
> > > > to DSDT). What I did is provide a list of fixups
> > > > such that bios can patch in pointers without
> > > > any need to understand what's what.
> > > > Thoughts?
> > > 
> > > For the RSDP, RSDT, and FADT I think SeaBIOS should just update those
> > > tables to set the pointers within them and then recalculate the
> > > checksum.  I don't think anything complex is needed - it's easy for
> > > SeaBIOS to recognize those special tables and modify them.
> > 
> > True, that's simple enough.  My worry is we can add more such tables.
> > For example, we can decide to switch to XSDT in the future.
> 
> I know of the following quirks that would have to be handled:
> 
> 1 - the RSDP must be in the f-segment (where as all other tables can
> go into "high" memory).
> 
> 2 - the RSDP has a checksum in a different location from the other
> tables and (with an XSDT) it can have two checksums.
> 
> 3 - the RSDP has a pointer to the RSDT (and to the XSDT if present).
> 
> 4 - the RSDT (and XSDT if present) has pointers to all the other
> tables (except RSDP, RSDT, DSDT, and FACS).  The FADT pointer must be
> first in the list.
> 
> 5 - the FADT table has pointers to DSDT and FACS.
> 
> 6 - the FACS table must be 64 byte aligned.
> 
> So, will a generic scheme really be able to handle all of the above
> quirks, or will we just be mixing some hardcoded quirks with some
> generic quirks?  And, will the code to handle the above quirks in a
> generic fashion be of a higher complexity than simply hard-coding it?
> 
> -Kevin

-->

So here's an implementation for align and FSEG.
Not a big deal as you see.

I really have doubts about it however: BIOS still must be able to parse
get the resume vector in FACS in order to support wakeup, right? So this
means that we need to be able to parse RSDP and FACT.  These happen to
be the only things that need anything not addressed by ADD and SUB so
... maybe a couple of hardcoded quirks just to allocate these correctly
is cleaner.

Signed-off-by: Michael S. Tsirkin 

---

diff --git a/src/linker.c b/src/linker.c
index a1f473d..22a0dff 100644
--- a/src/linker.c
+++ b/src/linker.c
@@ -34,7 +34,9 @@ void linker_link(const char *name)
if (entry->shift > 63)
 continue;
 if (entry->type != LINKER_ENTRY_TYPE_ADD &&
-entry->type != LINKER_ENTRY_TYPE_SUB)
+entry->type != LINKER_ENTRY_TYPE_SUB &&
+entry->type != LINKER_ENTRY_TYPE_ALIGN &&
+entry->type != LINKER_ENTRY_TYPE_FSEG)
 continue;
 if (entry->format != LINKER_ENTRY_FORMAT_LE)
 continue;
@@ -44,17 +46,55 @@ void linker_link(const char *name)
 continue;
 }
 lsrc = strlen(entry->src_dst);
-if (!lsrc || lsrc + 1 +  sizeof *entry >= entry->size) {
+if (!lsrc) {
 warn_internalerror();
 continue;
 }
 src = romfile_find(entry->src_dst);
+if (!src) {
+warn_internalerror();
+continue;
+}
+if (!src->data) {
+warn_internalerror();
+continue;
+}
+   if (entry->type == LINKER_ENTRY_TYPE_ALIGN ||
+   entry->type == LINKER_ENTRY_TYPE_FSEG) {
+   void *data;
+   u32 align;
+   struct zone_s *zone;
+
+   if (entry->shift > 31) {
+   warn_internalerror();
+   continue;
+   }
+   align = 0x1 << entry->shift;
+   if (align < MALLOC_MIN_ALIGN)
+   align = MALLOC_MIN_ALIGN;
+   zone = entry->type == LINKER_ENTRY_TYPE_FSEG ?
+   &ZoneFSeg : &ZoneHigh;
+   data = pmm_malloc(zone, PMM_DEFAULT_HANDLE, src->size, align);
+   if (!data) {
+   warn_internalerror();
+   continue;
+   }
+   memcpy(data, src->data, src->size);
+   free(src->data);
+   src->data = data;
+   continue;
+   }
+
+if (lsrc + 1 +  sizeof *entry >= entry->size) {
+warn_internalerror();
+continue;
+}
 dst = romfile_find(entry->src_dst + lsrc + 1);
-if (!src || !dst) {
+if (!dst) {
 warn_internalerror();
 continue;
 }
-if (!src->data || !dst->data) {
+if (!dst->data) {
 warn_internalerror();

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Mon, Apr 22, 2013 at 08:42:18PM -0400, Kevin O'Connor wrote:
> On Mon, Apr 22, 2013 at 08:23:39PM +0300, Michael S. Tsirkin wrote:
> > > @@ -0,0 +1,28 @@
> > > +#include "types.h" // u8
> > > +#include "util.h" // romfile_s
> > > +
> > > +/* ROM file linker interface. Linker uses little endian format */
> > > +struct linker_entry_s {
> > > +u8 size; /* size in bytes including the header */
> > > +u8 bytes; /* How many bytes to change. Can be 1,2,4 or 8. */
> > > +u8 type;
> > > +u8 format;
> > > +u32 reserved;
> > > +u64 src_offset; /* Offset in source. Little endian format. */
> > 
> > src_offset is not needed: host can pre-fill destination
> > table with the offset value.
> > Will probably drop it.
> > 
> > > +u64 dst_offset; /* Offset in destination. Little endian format. */
> > > +/* Followed by source and destination
> > 
> > Add "file names"
> > 
> > >, optionally padded by
> > > + * 0, up to the total of entry_len - 4 bytes.
> > > + * Strings are 0-terminated. */
> > > +char src_dst[];
> > > +} PACKED;
> > > +
> > > +enum linker_entry_type {
> > 
> > Documentation:
> > 
> > ADD: increment value in DST by the address of SRC
> >  useful e.g. to fill in pointer values in ACPI tables
> > SUB: decrement value in DST by the address of SRC
> >  useful e.g. to fix up checksum values in ACPI tables
> 
> I don't see how one could implement a checksum with just a
> subtraction.  If the table is at 0x12345678 the checksum isn't
> (oldcksum-0x12345678), it's (oldcksum-0x12-0x34-0x56-0x78).
> 
> -Kevin

So here's the version with shift, that should do it
correctly. Add mere 8 lines of code.
I'll look into alignment and fseg now.


diff --git a/Makefile b/Makefile
index 759..020fb2f 100644
--- a/Makefile
+++ b/Makefile
@@ -18,7 +18,7 @@ SRC16=$(SRCBOTH) system.c disk.c font.c
 SRC32FLAT=$(SRCBOTH) post.c shadow.c memmap.c pmm.c coreboot.c boot.c \
 acpi.c smm.c mptable.c pirtable.c smbios.c pciinit.c optionroms.c mtrr.c \
 lzmadecode.c bootsplash.c jpeg.c usb-hub.c paravirt.c \
-biostables.c xen.c bmp.c romfile.c csm.c
+biostables.c xen.c bmp.c romfile.c csm.c linker.c
 SRC32SEG=util.c output.c pci.c pcibios.c apm.c stacks.c
 
 # Default compiler flags
diff --git a/src/linker.c b/src/linker.c
new file mode 100644
index 000..a1f473d
--- /dev/null
+++ b/src/linker.c
@@ -0,0 +1,79 @@
+#include "linker.h"
+#include "byteorder.h" // le64_to_cpu
+
+void linker_link(const char *name)
+{
+struct linker_entry_s *entry;
+int offset = 0;
+int size, lsrc;
+void *data = romfile_loadfile(name, &size);
+struct romfile_s *src, *dst;
+u32 dst_offset;
+u64 val, buf;
+if (!data)
+return;
+
+for (offset = 0; offset < size; offset += entry->size) {
+entry = data + offset;
+/* Entry must have some data. If not - skip it. */
+if (entry->size <= sizeof *entry)
+continue;
+/* Check that entry fits in buffer. */
+if (offset + entry->size > size) {
+warn_internalerror();
+break;
+}
+/* Skip any command that we don't recognize. */
+if (entry->reserved1 || entry->reserved2)
+continue;
+if (entry->bytes != 1 &&
+entry->bytes != 2 &&
+entry->bytes != 4 &&
+entry->bytes != 8)
+continue;
+   if (entry->shift > 63)
+continue;
+if (entry->type != LINKER_ENTRY_TYPE_ADD &&
+entry->type != LINKER_ENTRY_TYPE_SUB)
+continue;
+if (entry->format != LINKER_ENTRY_FORMAT_LE)
+continue;
+/* Last byte must be 0 */
+if (entry->src_dst[entry->size - 1]) {
+warn_internalerror();
+continue;
+}
+lsrc = strlen(entry->src_dst);
+if (!lsrc || lsrc + 1 +  sizeof *entry >= entry->size) {
+warn_internalerror();
+continue;
+}
+src = romfile_find(entry->src_dst);
+dst = romfile_find(entry->src_dst + lsrc + 1);
+if (!src || !dst) {
+warn_internalerror();
+continue;
+}
+if (!src->data || !dst->data) {
+warn_internalerror();
+continue;
+}
+dst_offset = le32_to_cpu(entry->dst_offset);
+if (dst->size < dst_offset + entry->bytes) {
+warn_internalerror();
+continue;
+}
+   buf = 0;
+   memcpy(&buf, dst->data + dst_offset, entry->bytes);
+val = ((u64)(unsigned long)src->data) >> entry->shift;
+   buf = le64_to_cpu(buf);
+if (entry->type == LINKER_ENTRY_TYPE_ADD)
+buf += val;
+else
+buf -= val;
+   buf = cpu_to_le64(val);
+   memcpy(dst->data + dst_offset, &buf, entry->bytes);
+}
+
+free(data);
+}
diff --git a/src/linker.h b/src/linker.h
new file mode 100644
index 000..2bb376d
--- /dev/null
+++ b/src/linker.h
@@

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Mon, Apr 22, 2013 at 08:42:18PM -0400, Kevin O'Connor wrote:
> On Mon, Apr 22, 2013 at 08:23:39PM +0300, Michael S. Tsirkin wrote:
> > > @@ -0,0 +1,28 @@
> > > +#include "types.h" // u8
> > > +#include "util.h" // romfile_s
> > > +
> > > +/* ROM file linker interface. Linker uses little endian format */
> > > +struct linker_entry_s {
> > > +u8 size; /* size in bytes including the header */
> > > +u8 bytes; /* How many bytes to change. Can be 1,2,4 or 8. */
> > > +u8 type;
> > > +u8 format;
> > > +u32 reserved;
> > > +u64 src_offset; /* Offset in source. Little endian format. */
> > 
> > src_offset is not needed: host can pre-fill destination
> > table with the offset value.
> > Will probably drop it.
> > 
> > > +u64 dst_offset; /* Offset in destination. Little endian format. */
> > > +/* Followed by source and destination
> > 
> > Add "file names"
> > 
> > >, optionally padded by
> > > + * 0, up to the total of entry_len - 4 bytes.
> > > + * Strings are 0-terminated. */
> > > +char src_dst[];
> > > +} PACKED;
> > > +
> > > +enum linker_entry_type {
> > 
> > Documentation:
> > 
> > ADD: increment value in DST by the address of SRC
> >  useful e.g. to fill in pointer values in ACPI tables
> > SUB: decrement value in DST by the address of SRC
> >  useful e.g. to fix up checksum values in ACPI tables
> 
> I don't see how one could implement a checksum with just a
> subtraction.  If the table is at 0x12345678 the checksum isn't
> (oldcksum-0x12345678), it's (oldcksum-0x12-0x34-0x56-0x78).
> 
> -Kevin

True, it's a bug. I'll add a shift option to fix it.

-- 
MST

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote:
> On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
> > On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
> > > On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
> > > > Okay I'm pretty close to posting some patches
> > > > that advance this project further, but wanted to
> > > > check something beforehand: there are several tables
> > > > that point to other tables (for example: FADT points
> > > > to DSDT). What I did is provide a list of fixups
> > > > such that bios can patch in pointers without
> > > > any need to understand what's what.
> > > > Thoughts?
> > > 
> > > For the RSDP, RSDT, and FADT I think SeaBIOS should just update those
> > > tables to set the pointers within them and then recalculate the
> > > checksum.  I don't think anything complex is needed - it's easy for
> > > SeaBIOS to recognize those special tables and modify them.
> > 
> > True, that's simple enough.  My worry is we can add more such tables.
> > For example, we can decide to switch to XSDT in the future.
> 
> I know of the following quirks that would have to be handled:
> 
> 1 - the RSDP must be in the f-segment (where as all other tables can
> go into "high" memory).
> 
> 2 - the RSDP has a checksum in a different location from the other
> tables and (with an XSDT) it can have two checksums.
> 
> 3 - the RSDP has a pointer to the RSDT (and to the XSDT if present).
> 
> 4 - the RSDT (and XSDT if present) has pointers to all the other
> tables (except RSDP, RSDT, DSDT, and FACS).  The FADT pointer must be
> first in the list.
> 
> 5 - the FADT table has pointers to DSDT and FACS.
> 
> 6 - the FACS table must be 64 byte aligned.

And of course newer ACPI has lots of other pointer quirks,
I assume you are aware of this.

> So, will a generic scheme really be able to handle all of the above
> quirks, or will we just be mixing some hardcoded quirks with some
> generic quirks?  And, will the code to handle the above quirks in a
> generic fashion be of a higher complexity than simply hard-coding it?
> 
> -Kevin

I wanted to handle checksums and pointers in a generic fashion,
and allocation rules in a table specific version
(since I only found two such examples in all of the spec:
FACS and RSDP). It's not hard to add generic handlers for these two,
and it's not much more code. You think it's preferable then?

-- 
MST

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Kevin O'Connor]

On Mon, Apr 22, 2013 at 08:23:39PM +0300, Michael S. Tsirkin wrote:
> > @@ -0,0 +1,28 @@
> > +#include "types.h" // u8
> > +#include "util.h" // romfile_s
> > +
> > +/* ROM file linker interface. Linker uses little endian format */
> > +struct linker_entry_s {
> > +u8 size; /* size in bytes including the header */
> > +u8 bytes; /* How many bytes to change. Can be 1,2,4 or 8. */
> > +u8 type;
> > +u8 format;
> > +u32 reserved;
> > +u64 src_offset; /* Offset in source. Little endian format. */
> 
> src_offset is not needed: host can pre-fill destination
> table with the offset value.
> Will probably drop it.
> 
> > +u64 dst_offset; /* Offset in destination. Little endian format. */
> > +/* Followed by source and destination
> 
> Add "file names"
> 
> >, optionally padded by
> > + * 0, up to the total of entry_len - 4 bytes.
> > + * Strings are 0-terminated. */
> > +char src_dst[];
> > +} PACKED;
> > +
> > +enum linker_entry_type {
> 
> Documentation:
> 
> ADD: increment value in DST by the address of SRC
>  useful e.g. to fill in pointer values in ACPI tables
> SUB: decrement value in DST by the address of SRC
>  useful e.g. to fix up checksum values in ACPI tables

I don't see how one could implement a checksum with just a
subtraction.  If the table is at 0x12345678 the checksum isn't
(oldcksum-0x12345678), it's (oldcksum-0x12-0x34-0x56-0x78).

-Kevin

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Kevin O'Connor]

On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
> On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
> > On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
> > > Okay I'm pretty close to posting some patches
> > > that advance this project further, but wanted to
> > > check something beforehand: there are several tables
> > > that point to other tables (for example: FADT points
> > > to DSDT). What I did is provide a list of fixups
> > > such that bios can patch in pointers without
> > > any need to understand what's what.
> > > Thoughts?
> > 
> > For the RSDP, RSDT, and FADT I think SeaBIOS should just update those
> > tables to set the pointers within them and then recalculate the
> > checksum.  I don't think anything complex is needed - it's easy for
> > SeaBIOS to recognize those special tables and modify them.
> 
> True, that's simple enough.  My worry is we can add more such tables.
> For example, we can decide to switch to XSDT in the future.

I know of the following quirks that would have to be handled:

1 - the RSDP must be in the f-segment (where as all other tables can
go into "high" memory).

2 - the RSDP has a checksum in a different location from the other
tables and (with an XSDT) it can have two checksums.

3 - the RSDP has a pointer to the RSDT (and to the XSDT if present).

4 - the RSDT (and XSDT if present) has pointers to all the other
tables (except RSDP, RSDT, DSDT, and FACS).  The FADT pointer must be
first in the list.

5 - the FADT table has pointers to DSDT and FACS.

6 - the FACS table must be 64 byte aligned.

So, will a generic scheme really be able to handle all of the above
quirks, or will we just be mixing some hardcoded quirks with some
generic quirks?  And, will the code to handle the above quirks in a
generic fashion be of a higher complexity than simply hard-coding it?

-Kevin

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Laszlo Ersek]

On 04/22/13 17:37, Michael S. Tsirkin wrote:
> On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
>> On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
>>> On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
 Okay I'm pretty close to posting some patches
 that advance this project further, but wanted to
 check something beforehand: there are several tables
 that point to other tables (for example: FADT points
 to DSDT). What I did is provide a list of fixups
 such that bios can patch in pointers without
 any need to understand what's what.
 Thoughts?
>>>
>>> Great!
>>>
>>> For the RSDP, RSDT, and FADT I think SeaBIOS should just update those
>>> tables to set the pointers within them and then recalculate the
>>> checksum.  I don't think anything complex is needed - it's easy for
>>> SeaBIOS to recognize those special tables and modify them.
>>>
>>> -Kevin
>>
>> True, that's simple enough.  My worry is we can add more such tables.
>> For example, we can decide to switch to XSDT in the future.
>> If you look at the latest ACPI spec, there are tons of
>> pointer tables not implemented in seabios.
>> If we need to add support in seabios as well as qemu
>> for each of them, this will kind of defeat the purpose.
> 
> And just to show how simple it can be, here's that patch in the
> series: basically ACPI tables will be in-memory, so all we need to do to
> link them together is keep a pointer from file to the table.
> Basically all linker needs is ability to add or subtract.
> What I like with this approach is host is in complete control of table
> layout. All BIOS does is allocate memory for them, it never changes the
> tables, including the checksum.
> Note that it's coded very defensively, which makes it
> a bit bigger than absolutely necessary, but hopefully
> more future proof.
> 
> --->
> linker: utility to patch in-memory ROM files
> 
> Add ability for a ROM file to point to
> it's image in memory. When file is in memory,
> add utility that can patch it, storing
> pointers to one file within another file.
> 
> Signed-off-by: Michael S. Tsirkin 
> 
> ---
> 
> Makefile |2 -
> src/linker.c |   78 
> +++
> src/linker.h |   28 +
> src/util.h   |1 
> 4 files changed, 108 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/Makefile b/Makefile
> index 759..020fb2f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -18,7 +18,7 @@ SRC16=$(SRCBOTH) system.c disk.c font.c
>  SRC32FLAT=$(SRCBOTH) post.c shadow.c memmap.c pmm.c coreboot.c boot.c \
>  acpi.c smm.c mptable.c pirtable.c smbios.c pciinit.c optionroms.c mtrr.c 
> \
>  lzmadecode.c bootsplash.c jpeg.c usb-hub.c paravirt.c \
> -biostables.c xen.c bmp.c romfile.c csm.c
> +biostables.c xen.c bmp.c romfile.c csm.c linker.c
>  SRC32SEG=util.c output.c pci.c pcibios.c apm.c stacks.c
>  
>  # Default compiler flags
> diff --git a/src/linker.c b/src/linker.c
> new file mode 100644
> index 000..5009f88
> --- /dev/null
> +++ b/src/linker.c
> @@ -0,0 +1,78 @@
> +#include "linker.h"
> +#include "byteorder.h" // le64_to_cpu
> +
> +void linker_link(const char *name)
> +{
> +struct linker_entry_s *entry;
> +int offset = 0;
> +int size, lsrc;
> +void *data = romfile_loadfile(name, &size);
> +struct romfile_s *src, *dst;
> +u32 src_offset, dst_offset;
> +u64 val, buf;
> +if (!data)
> +return;
> +
> +for (offset = 0; offset < size; offset += entry->size) {
> +entry = data + offset;
> +/* Entry must have some data. If not - skip it. */
> +if (entry->size <= sizeof *entry)
> +continue;
> +/* Check that entry fits in buffer. */
> +if (offset + entry->size > size) {
> +warn_internalerror();
> +break;
> +}
> +/* Skip any command that we don't recognize. */
> +if (entry->reserved)
> +continue;
> +if (entry->bytes != 1 &&
> +entry->bytes != 2 &&
> +entry->bytes != 4 &&
> +entry->bytes != 8)
> +continue;
> +if (entry->type != LINKER_ENTRY_TYPE_ADD &&
> +entry->type != LINKER_ENTRY_TYPE_SUB)
> +continue;
> +if (entry->format != LINKER_ENTRY_FORMAT_LE)
> +continue;
> +/* Last byte must be 0 */
> +if (entry->src_dst[entry->size - 1]) {
> +warn_internalerror();
> +continue;
> +}
> +lsrc = strlen(entry->src_dst);
> +if (!lsrc || lsrc + 1 +  sizeof *entry >= entry->size) {
> +warn_internalerror();
> +continue;
> +}
> +src = romfile_find(entry->src_dst);
> +dst = romfile_find(entry->src_dst + lsrc + 1);
> +if (!src || !dst) {
> +warn_internalerror();
> +continue;
> +}
> +if (!src->data || !ds

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Mon, Apr 22, 2013 at 06:37:32PM +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
> > On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
> > > On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
> > > > Okay I'm pretty close to posting some patches
> > > > that advance this project further, but wanted to
> > > > check something beforehand: there are several tables
> > > > that point to other tables (for example: FADT points
> > > > to DSDT). What I did is provide a list of fixups
> > > > such that bios can patch in pointers without
> > > > any need to understand what's what.
> > > > Thoughts?
> > > 
> > > Great!
> > > 
> > > For the RSDP, RSDT, and FADT I think SeaBIOS should just update those
> > > tables to set the pointers within them and then recalculate the
> > > checksum.  I don't think anything complex is needed - it's easy for
> > > SeaBIOS to recognize those special tables and modify them.
> > > 
> > > -Kevin
> > 
> > True, that's simple enough.  My worry is we can add more such tables.
> > For example, we can decide to switch to XSDT in the future.
> > If you look at the latest ACPI spec, there are tons of
> > pointer tables not implemented in seabios.
> > If we need to add support in seabios as well as qemu
> > for each of them, this will kind of defeat the purpose.
> 
> And just to show how simple it can be, here's that patch in the
> series: basically ACPI tables will be in-memory, so all we need to do to
> link them together is keep a pointer from file to the table.
> Basically all linker needs is ability to add or subtract.
> What I like with this approach is host is in complete control of table
> layout. All BIOS does is allocate memory for them, it never changes the
> tables, including the checksum.
> Note that it's coded very defensively, which makes it
> a bit bigger than absolutely necessary, but hopefully
> more future proof.

Addressing some comments by Laszlo (sent off list)/.

> --->
> linker: utility to patch in-memory ROM files
> 
> Add ability for a ROM file to point to
> it's image in memory. When file is in memory,
> add utility that can patch it, storing
> pointers to one file within another file.
> 
> Signed-off-by: Michael S. Tsirkin 
> 
> ---
> 
> Makefile |2 -
> src/linker.c |   78 
> +++
> src/linker.h |   28 +
> src/util.h   |1 
> 4 files changed, 108 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/Makefile b/Makefile
> index 759..020fb2f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -18,7 +18,7 @@ SRC16=$(SRCBOTH) system.c disk.c font.c
>  SRC32FLAT=$(SRCBOTH) post.c shadow.c memmap.c pmm.c coreboot.c boot.c \
>  acpi.c smm.c mptable.c pirtable.c smbios.c pciinit.c optionroms.c mtrr.c 
> \
>  lzmadecode.c bootsplash.c jpeg.c usb-hub.c paravirt.c \
> -biostables.c xen.c bmp.c romfile.c csm.c
> +biostables.c xen.c bmp.c romfile.c csm.c linker.c
>  SRC32SEG=util.c output.c pci.c pcibios.c apm.c stacks.c
>  
>  # Default compiler flags
> diff --git a/src/linker.c b/src/linker.c
> new file mode 100644
> index 000..5009f88
> --- /dev/null
> +++ b/src/linker.c
> @@ -0,0 +1,78 @@
> +#include "linker.h"
> +#include "byteorder.h" // le64_to_cpu
> +
> +void linker_link(const char *name)
> +{
> +struct linker_entry_s *entry;
> +int offset = 0;
> +int size, lsrc;
> +void *data = romfile_loadfile(name, &size);
> +struct romfile_s *src, *dst;
> +u32 src_offset, dst_offset;
> +u64 val, buf;
> +if (!data)
> +return;
> +
> +for (offset = 0; offset < size; offset += entry->size) {
> +entry = data + offset;
> +/* Entry must have some data. If not - skip it. */
> +if (entry->size <= sizeof *entry)
> +continue;
> +/* Check that entry fits in buffer. */
> +if (offset + entry->size > size) {
> +warn_internalerror();
> +break;
> +}
> +/* Skip any command that we don't recognize. */
> +if (entry->reserved)
> +continue;
> +if (entry->bytes != 1 &&
> +entry->bytes != 2 &&
> +entry->bytes != 4 &&
> +entry->bytes != 8)
> +continue;
> +if (entry->type != LINKER_ENTRY_TYPE_ADD &&
> +entry->type != LINKER_ENTRY_TYPE_SUB)
> +continue;
> +if (entry->format != LINKER_ENTRY_FORMAT_LE)
> +continue;
> +/* Last byte must be 0 */
> +if (entry->src_dst[entry->size - 1]) {
> +warn_internalerror();
> +continue;
> +}
> +lsrc = strlen(entry->src_dst);
> +if (!lsrc || lsrc + 1 +  sizeof *entry >= entry->size) {
> +warn_internalerror();
> +continue;
> +}
> +src = romfile_find(entry->src_dst);
> +dst = romfile_find(entry->src_dst + lsrc + 1);
> +

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
> On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
> > On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
> > > Okay I'm pretty close to posting some patches
> > > that advance this project further, but wanted to
> > > check something beforehand: there are several tables
> > > that point to other tables (for example: FADT points
> > > to DSDT). What I did is provide a list of fixups
> > > such that bios can patch in pointers without
> > > any need to understand what's what.
> > > Thoughts?
> > 
> > Great!
> > 
> > For the RSDP, RSDT, and FADT I think SeaBIOS should just update those
> > tables to set the pointers within them and then recalculate the
> > checksum.  I don't think anything complex is needed - it's easy for
> > SeaBIOS to recognize those special tables and modify them.
> > 
> > -Kevin
> 
> True, that's simple enough.  My worry is we can add more such tables.
> For example, we can decide to switch to XSDT in the future.
> If you look at the latest ACPI spec, there are tons of
> pointer tables not implemented in seabios.
> If we need to add support in seabios as well as qemu
> for each of them, this will kind of defeat the purpose.

And just to show how simple it can be, here's that patch in the
series: basically ACPI tables will be in-memory, so all we need to do to
link them together is keep a pointer from file to the table.
Basically all linker needs is ability to add or subtract.
What I like with this approach is host is in complete control of table
layout. All BIOS does is allocate memory for them, it never changes the
tables, including the checksum.
Note that it's coded very defensively, which makes it
a bit bigger than absolutely necessary, but hopefully
more future proof.

--->
linker: utility to patch in-memory ROM files

Add ability for a ROM file to point to
it's image in memory. When file is in memory,
add utility that can patch it, storing
pointers to one file within another file.

Signed-off-by: Michael S. Tsirkin 

---

Makefile |2 -
src/linker.c |   78 +++
src/linker.h |   28 +
src/util.h   |1 
4 files changed, 108 insertions(+), 1 deletion(-)


diff --git a/Makefile b/Makefile
index 759..020fb2f 100644
--- a/Makefile
+++ b/Makefile
@@ -18,7 +18,7 @@ SRC16=$(SRCBOTH) system.c disk.c font.c
 SRC32FLAT=$(SRCBOTH) post.c shadow.c memmap.c pmm.c coreboot.c boot.c \
 acpi.c smm.c mptable.c pirtable.c smbios.c pciinit.c optionroms.c mtrr.c \
 lzmadecode.c bootsplash.c jpeg.c usb-hub.c paravirt.c \
-biostables.c xen.c bmp.c romfile.c csm.c
+biostables.c xen.c bmp.c romfile.c csm.c linker.c
 SRC32SEG=util.c output.c pci.c pcibios.c apm.c stacks.c
 
 # Default compiler flags
diff --git a/src/linker.c b/src/linker.c
new file mode 100644
index 000..5009f88
--- /dev/null
+++ b/src/linker.c
@@ -0,0 +1,78 @@
+#include "linker.h"
+#include "byteorder.h" // le64_to_cpu
+
+void linker_link(const char *name)
+{
+struct linker_entry_s *entry;
+int offset = 0;
+int size, lsrc;
+void *data = romfile_loadfile(name, &size);
+struct romfile_s *src, *dst;
+u32 src_offset, dst_offset;
+u64 val, buf;
+if (!data)
+return;
+
+for (offset = 0; offset < size; offset += entry->size) {
+entry = data + offset;
+/* Entry must have some data. If not - skip it. */
+if (entry->size <= sizeof *entry)
+continue;
+/* Check that entry fits in buffer. */
+if (offset + entry->size > size) {
+warn_internalerror();
+break;
+}
+/* Skip any command that we don't recognize. */
+if (entry->reserved)
+continue;
+if (entry->bytes != 1 &&
+entry->bytes != 2 &&
+entry->bytes != 4 &&
+entry->bytes != 8)
+continue;
+if (entry->type != LINKER_ENTRY_TYPE_ADD &&
+entry->type != LINKER_ENTRY_TYPE_SUB)
+continue;
+if (entry->format != LINKER_ENTRY_FORMAT_LE)
+continue;
+/* Last byte must be 0 */
+if (entry->src_dst[entry->size - 1]) {
+warn_internalerror();
+continue;
+}
+lsrc = strlen(entry->src_dst);
+if (!lsrc || lsrc + 1 +  sizeof *entry >= entry->size) {
+warn_internalerror();
+continue;
+}
+src = romfile_find(entry->src_dst);
+dst = romfile_find(entry->src_dst + lsrc + 1);
+if (!src || !dst) {
+warn_internalerror();
+continue;
+}
+if (!src->data || !dst->data) {
+warn_internalerror();
+continue;
+}
+src_offset = le32_to_cpu(entry->src_offset);
+dst_offset = le32_to_cpu(entry->dst_offset);
+if (dst->size < dst_offset + entry->bytes) {
+warn

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
> On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
> > Okay I'm pretty close to posting some patches
> > that advance this project further, but wanted to
> > check something beforehand: there are several tables
> > that point to other tables (for example: FADT points
> > to DSDT). What I did is provide a list of fixups
> > such that bios can patch in pointers without
> > any need to understand what's what.
> > Thoughts?
> 
> Great!
> 
> For the RSDP, RSDT, and FADT I think SeaBIOS should just update those
> tables to set the pointers within them and then recalculate the
> checksum.  I don't think anything complex is needed - it's easy for
> SeaBIOS to recognize those special tables and modify them.
> 
> -Kevin

True, that's simple enough.  My worry is we can add more such tables.
For example, we can decide to switch to XSDT in the future.
If you look at the latest ACPI spec, there are tons of
pointer tables not implemented in seabios.
If we need to add support in seabios as well as qemu
for each of them, this will kind of defeat the purpose.

-- 
MST

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Kevin O'Connor]

On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
> Okay I'm pretty close to posting some patches
> that advance this project further, but wanted to
> check something beforehand: there are several tables
> that point to other tables (for example: FADT points
> to DSDT). What I did is provide a list of fixups
> such that bios can patch in pointers without
> any need to understand what's what.
> Thoughts?

Great!

For the RSDP, RSDT, and FADT I think SeaBIOS should just update those
tables to set the pointers within them and then recalculate the
checksum.  I don't think anything complex is needed - it's easy for
SeaBIOS to recognize those special tables and modify them.

-Kevin

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Sun, Mar 24, 2013 at 05:55:22PM -0400, Kevin O'Connor wrote:
> On Sun, Mar 24, 2013 at 09:45:54PM +0200, Michael S. Tsirkin wrote:
> > On Sun, Mar 24, 2013 at 03:21:08PM -0400, Kevin O'Connor wrote:
> > > On Sun, Mar 24, 2013 at 09:14:47PM +0200, Michael S. Tsirkin wrote:
> > > > I don't exactly understand what do you mean by "file".
> > > 
> > > A fw_cfg entry added using QEMU's fw_cfg_add_file() instead of
> > > fw_cfg_add_bytes().
> > 
> > Looks good overall.  Some further proposals:
> > 
> > 1. I think we need runtime patching so we'll need
> > another one that adds stuff from memory but yes it
> > can look like a file for seabios.
> > 2. paths exposed to seabios are relative at the moment.
> >So "acpi/*" ?
> 
> fw_cfg_add_file() does read from memory.  It's not tied to the host
> filesystem or guest filesystem.
> 
> > 3. add option to disable builtin tables at compile time
> 
> Agreed.
> 
> > 4. to keep the ability to develop seabios code
> >in-tree, make it depend on compile option
> >disabled by default.
> > 
> > 5. similarly in QEMU, put code there disabled first,
> >so developers can play with it.
> > 
> > 6. when all is ready, flip the switch to enable in both places.
> 
> Sure - we can do that if needed.
> 
> -Kevin

Okay I'm pretty close to posting some patches
that advance this project further, but wanted to
check something beforehand: there are several tables
that point to other tables (for example: FADT points
to DSDT). What I did is provide a list of fixups
such that bios can patch in pointers without
any need to understand what's what.
Thoughts?

-- 
MST

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Laszlo Ersek]

On 03/24/13 20:14, Michael S. Tsirkin wrote:
> On Sun, Mar 24, 2013 at 01:17:38PM -0400, Kevin O'Connor wrote:

>> What do you think about using approach 2 as I outline at:
>> http://www.seabios.org/pipermail/seabios/2013-March/006020.html ?
>>
>> The existing fw_cfg acpi table passing mechanism is pretty hokey, and
>> new fw_cfg entries will be needed for smbios, pir, and mptable anyway.
>> Converting to all new fw_cfg tables may make the transition simpler
>> and provide both forward and backwards compatibility.

> I don't exactly understand what do you mean by "file".

Probably a key in the  [FW_CFG_FILE_FIRST, FW_CFG_FILE_FIRST +
FW_CFG_FILE_SLOTS - 1] range, with an associated entry (name) in the
FWCfgFiles array (FW_CFG_FILE_DIR).

Laszlo

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Kevin O'Connor]

On Sun, Mar 24, 2013 at 09:45:54PM +0200, Michael S. Tsirkin wrote:
> On Sun, Mar 24, 2013 at 03:21:08PM -0400, Kevin O'Connor wrote:
> > On Sun, Mar 24, 2013 at 09:14:47PM +0200, Michael S. Tsirkin wrote:
> > > I don't exactly understand what do you mean by "file".
> > 
> > A fw_cfg entry added using QEMU's fw_cfg_add_file() instead of
> > fw_cfg_add_bytes().
> 
> Looks good overall.  Some further proposals:
> 
> 1. I think we need runtime patching so we'll need
>   another one that adds stuff from memory but yes it
>   can look like a file for seabios.
> 2. paths exposed to seabios are relative at the moment.
>So "acpi/*" ?

fw_cfg_add_file() does read from memory.  It's not tied to the host
filesystem or guest filesystem.

> 3. add option to disable builtin tables at compile time

Agreed.

> 4. to keep the ability to develop seabios code
>in-tree, make it depend on compile option
>disabled by default.
> 
> 5. similarly in QEMU, put code there disabled first,
>so developers can play with it.
> 
> 6. when all is ready, flip the switch to enable in both places.

Sure - we can do that if needed.

-Kevin

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Sun, Mar 24, 2013 at 03:21:08PM -0400, Kevin O'Connor wrote:
> On Sun, Mar 24, 2013 at 09:14:47PM +0200, Michael S. Tsirkin wrote:
> > I don't exactly understand what do you mean by "file".
> 
> A fw_cfg entry added using QEMU's fw_cfg_add_file() instead of
> fw_cfg_add_bytes().
> 
> -Kevin

Looks good overall.  Some further proposals:

1. I think we need runtime patching so we'll need
another one that adds stuff from memory but yes it
can look like a file for seabios.
2. paths exposed to seabios are relative at the moment.
   So "acpi/*" ?

3. add option to disable builtin tables at compile time

4. to keep the ability to develop seabios code
   in-tree, make it depend on compile option
   disabled by default.

5. similarly in QEMU, put code there disabled first,
   so developers can play with it.

6. when all is ready, flip the switch to enable in both places.


___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Kevin O'Connor]

On Sun, Mar 24, 2013 at 09:14:47PM +0200, Michael S. Tsirkin wrote:
> I don't exactly understand what do you mean by "file".

A fw_cfg entry added using QEMU's fw_cfg_add_file() instead of
fw_cfg_add_bytes().

-Kevin

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Sun, Mar 24, 2013 at 01:17:38PM -0400, Kevin O'Connor wrote:
> On Sun, Mar 24, 2013 at 03:07:40PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Mar 21, 2013 at 08:04:38PM -0400, Kevin O'Connor wrote:
> > > So, I see two ways to do this:
> > > 
> > > 1 - update SeaBIOS with a patch series that makes it capable of
> > > handling all tables via existing and new fw_cfg entries (including
> > > flags to disable all internal tables), update QEMU to use that SeaBIOS
> > > rev, and then apply a patch series to QEMU that has it create all the
> > > tables (with the final patch flagging to seabios that it should no
> > > longer create any internal tables).
> > > 
> > > or, 2 - apply a patch series to QEMU that has it create all the tables
> > > via new fw_cfg entries, then apply a patch series to seabios that
> > > updates it to use the new fw_cfg entries instead of its existing
> > > internal tables, and then apply that new rev of seabios to qemu.
> > > 
> > > Were you proposing one of the above paths, or did you have something
> > > else in mind?
> > 
> > Exactly, I'm proposing 1, on top of this, a set of compile-time flags to
> > optionally stip unused tables from the bios binary.
> 
> What do you think about using approach 2 as I outline at:
> http://www.seabios.org/pipermail/seabios/2013-March/006020.html ?
> 
> The existing fw_cfg acpi table passing mechanism is pretty hokey, and
> new fw_cfg entries will be needed for smbios, pir, and mptable anyway.
> Converting to all new fw_cfg tables may make the transition simpler
> and provide both forward and backwards compatibility.
> 
> -Kevin

I don't exactly understand what do you mean by "file".

-- 
MST

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Kevin O'Connor]

On Sun, Mar 24, 2013 at 03:07:40PM +0200, Michael S. Tsirkin wrote:
> On Thu, Mar 21, 2013 at 08:04:38PM -0400, Kevin O'Connor wrote:
> > So, I see two ways to do this:
> > 
> > 1 - update SeaBIOS with a patch series that makes it capable of
> > handling all tables via existing and new fw_cfg entries (including
> > flags to disable all internal tables), update QEMU to use that SeaBIOS
> > rev, and then apply a patch series to QEMU that has it create all the
> > tables (with the final patch flagging to seabios that it should no
> > longer create any internal tables).
> > 
> > or, 2 - apply a patch series to QEMU that has it create all the tables
> > via new fw_cfg entries, then apply a patch series to seabios that
> > updates it to use the new fw_cfg entries instead of its existing
> > internal tables, and then apply that new rev of seabios to qemu.
> > 
> > Were you proposing one of the above paths, or did you have something
> > else in mind?
> 
> Exactly, I'm proposing 1, on top of this, a set of compile-time flags to
> optionally stip unused tables from the bios binary.

What do you think about using approach 2 as I outline at:
http://www.seabios.org/pipermail/seabios/2013-March/006020.html ?

The existing fw_cfg acpi table passing mechanism is pretty hokey, and
new fw_cfg entries will be needed for smbios, pir, and mptable anyway.
Converting to all new fw_cfg tables may make the transition simpler
and provide both forward and backwards compatibility.

-Kevin

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Thu, Mar 21, 2013 at 08:04:38PM -0400, Kevin O'Connor wrote:
> On Thu, Mar 21, 2013 at 03:26:26PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Mar 21, 2013 at 01:14:38PM +, David Woodhouse wrote:
> > > On Thu, 2013-03-21 at 15:12 +0200, Michael S. Tsirkin wrote:
> > > > Anyway, I am not against such runtime flags.
> > > > 
> > > > If we add to this an option to build a minimal BIOS
> > > > that only works with the new QEMU, do we have a deal?
> > > 
> > > Yeah, definitely. The code for SeaBIOS to build its own should certainly
> > > be optional. It should be possible to build a minimal SeaBIOS which
> > > *can't*.
> 
> Agreed.
> 
> > Okay that's exactly what me and Laszlo are working on.
> > We want to add a special way to build qemu and seabios
> > such that they work with tables in qemu, then add
> > runtime detection on top.
> > 
> > The advantage is that we can make progress
> > without keeping lots of patches out of tree.
> > Unless of course Kevin nacks this all ...
> 
> I think we need to have a plan for what the final interface will look
> like.
> 
> Right now, we can't change QEMU to pass tables via the existing fw_cfg
> entries unless we upgrade SeaBIOS in QEMU first (otherwise things like
> duplicate MADT entries occur).  If we're going to upgrade SeaBIOS in
> QEMU, we really want that upgrade to be the final version.  We don't
> want to have to bump the seabios rev in qemu multiple times in
> lock-step with the changes.
> 
> So, I see two ways to do this:
> 
> 1 - update SeaBIOS with a patch series that makes it capable of
> handling all tables via existing and new fw_cfg entries (including
> flags to disable all internal tables), update QEMU to use that SeaBIOS
> rev, and then apply a patch series to QEMU that has it create all the
> tables (with the final patch flagging to seabios that it should no
> longer create any internal tables).
> 
> or, 2 - apply a patch series to QEMU that has it create all the tables
> via new fw_cfg entries, then apply a patch series to seabios that
> updates it to use the new fw_cfg entries instead of its existing
> internal tables, and then apply that new rev of seabios to qemu.
> 
> Were you proposing one of the above paths, or did you have something
> else in mind?
> 
> -Kevin

Exactly, I'm proposing 1, on top of this, a set of compile-time flags to
optionally stip unused tables from the bios binary.

-- 
MST

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Fri, Mar 22, 2013 at 08:02:21PM +0100, Paolo Bonzini wrote:
> Il 21/03/2013 14:12, Michael S. Tsirkin ha scritto:
> > On Thu, Mar 21, 2013 at 01:04:35PM +, David Woodhouse wrote:
> >> On Thu, 2013-03-21 at 13:56 +0100, Laszlo Ersek wrote:
> >>> - for an earlier qemu, the option must be set,
> >>> - for a later qemu the option must be clear &&
> >>>   (no -acpitable switch must be specified on the qemu cmldine ||
> >>>one -acpitable switch must load a MADT)
> >>
> >> Hm, that sounds like it won't be possible to build one version of
> >> SeaBIOS that works for *both* old and new qemu. That doesn't seem like a
> >> great idea. I'd prefer something like:
> >>
> >> - If Qemu provides the 'core' ACPI tables (i.e. not just SSDT) then 
> >>   SeaBIOS uses them.
> >> - Otherwise, it makes its own.
> >>
> >>
> >> -- 
> >> dwmw2
> > 
> > It might simplify life for someone bisecting qemu
> > as you don't need to rebuild seabios after each
> > bisect but is this really a common workflow?
> 
> It would help me working on RHEL's qemu-kvm, which I often do on a
> Fedora box.
> 
> The bios.bin in the RHEL qemu-kvm tree is obsolete (like Gerd showed for
> Fedora, RHEL's bios.bin comes from a separate package), and using
> whatever happens to be in /usr or in my local SeaBIOS clone is the
> simplest thing to do.  I'd like this not to break.
> 
> Paolo

Sure, but be warned that the seabios copy of ACPI tables will bitrot
with time, it will only be used for old qemu binaries. We might not be
able to keep it alive for the full multi-year RHEL lifecycle.

> > Anyway, I am not against such runtime flags.
> > 
> > If we add to this an option to build a minimal BIOS
> > that only works with the new QEMU, do we have a deal?
> > 
> > Then the plan is to make progress and apply patches step by step without
> > deciding on the detection interface first.
> > Before QEMU is switched to the new configuration,
> > we'll add the runtime thing for the benefit of developers
> > that bisect.
> > 
> > Makes sense?
> > 

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Paolo Bonzini]

Il 21/03/2013 14:12, Michael S. Tsirkin ha scritto:
> On Thu, Mar 21, 2013 at 01:04:35PM +, David Woodhouse wrote:
>> On Thu, 2013-03-21 at 13:56 +0100, Laszlo Ersek wrote:
>>> - for an earlier qemu, the option must be set,
>>> - for a later qemu the option must be clear &&
>>>   (no -acpitable switch must be specified on the qemu cmldine ||
>>>one -acpitable switch must load a MADT)
>>
>> Hm, that sounds like it won't be possible to build one version of
>> SeaBIOS that works for *both* old and new qemu. That doesn't seem like a
>> great idea. I'd prefer something like:
>>
>> - If Qemu provides the 'core' ACPI tables (i.e. not just SSDT) then 
>>   SeaBIOS uses them.
>> - Otherwise, it makes its own.
>>
>>
>> -- 
>> dwmw2
> 
> It might simplify life for someone bisecting qemu
> as you don't need to rebuild seabios after each
> bisect but is this really a common workflow?

It would help me working on RHEL's qemu-kvm, which I often do on a
Fedora box.

The bios.bin in the RHEL qemu-kvm tree is obsolete (like Gerd showed for
Fedora, RHEL's bios.bin comes from a separate package), and using
whatever happens to be in /usr or in my local SeaBIOS clone is the
simplest thing to do.  I'd like this not to break.

Paolo

> Anyway, I am not against such runtime flags.
> 
> If we add to this an option to build a minimal BIOS
> that only works with the new QEMU, do we have a deal?
> 
> Then the plan is to make progress and apply patches step by step without
> deciding on the detection interface first.
> Before QEMU is switched to the new configuration,
> we'll add the runtime thing for the benefit of developers
> that bisect.
> 
> Makes sense?
> 


___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Paolo Bonzini]

Il 22/03/2013 00:22, Kevin O'Connor ha scritto:
 This also needs
 > >> to be resolved for SSDT tables, which can have zero, one, or more
 > >> instances.
>> > 
>> > Same argument as for the MADT.
> The issue with the SSDT is that there can be many of them.  QEMU may
> wish to pass in 2 or more.  If QEMU does pass in one or more SSDTs,
> how will SeaBIOS know if it should also generate it's own SSDT?

I think it's a fair assumption that having one SSDT in fw_cfg disables
generation of _all_ SSDTs in SeaBIOS.

Paolo


___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Laszlo Ersek]

On 03/22/13 01:04, Kevin O'Connor wrote:
> On Thu, Mar 21, 2013 at 03:26:26PM +0200, Michael S. Tsirkin wrote:

>> The advantage is that we can make progress
>> without keeping lots of patches out of tree.
>> Unless of course Kevin nacks this all ...
> 
> I think we need to have a plan for what the final interface will look
> like.
> 
> Right now, we can't change QEMU to pass tables via the existing fw_cfg
> entries unless we upgrade SeaBIOS in QEMU first (otherwise things like
> duplicate MADT entries occur).  If we're going to upgrade SeaBIOS in
> QEMU, we really want that upgrade to be the final version.  We don't
> want to have to bump the seabios rev in qemu multiple times in
> lock-step with the changes.
> 
> So, I see two ways to do this:
> 
> 1 - update SeaBIOS with a patch series that makes it capable of
> handling all tables via existing and new fw_cfg entries (including
> flags to disable all internal tables), update QEMU to use that SeaBIOS
> rev, and then apply a patch series to QEMU that has it create all the
> tables (with the final patch flagging to seabios that it should no
> longer create any internal tables).
> 
> or, 2 - apply a patch series to QEMU that has it create all the tables
> via new fw_cfg entries, then apply a patch series to seabios that
> updates it to use the new fw_cfg entries instead of its existing
> internal tables, and then apply that new rev of seabios to qemu.
> 
> Were you proposing one of the above paths, or did you have something
> else in mind?

Both of these say "apply a patch series to QEMU that has it create all
the tables". I think it would be preferable (for whoever develops the
tables in qemu) to submit a standalone series per table, testable in
isolation. A huge series covering all tables would likely go up to v5+,
and waste a lot of developer & reviewer effort.

In
,

On 03/22/13 00:22, Kevin O'Connor wrote:

> In practice, one never wants to mix QEMU generated ACPI tables with
> SeaBIOS generated ACPI tables.

AIUI, this is exactly what we'd like to do during development.

But also in general I see no problem with this; *what* should be in any
given ACPI table is independent from the table's producer's identity.
(The producer only needs access to the dependencies, and that access is
producer-specific, like global variables or functions in qemu, fw_cfg in
SeaBIOS & OVMF).

The reason for the move is just that we don't want to duplicate work
between SeaBIOS and OVMF; a table being produced in qemu versus boot
firmware is not inherently right or wrong. (At least it wasn't
bothering/intriguing anyone until OVMF started to care about ACPI tables
in earnest).

Laszlo

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Gerd Hoffmann]

>> As this is quite a bunch of work I would tend to avoid a flag day like
>> this so we can switch over tables one by one without building up big
>> patch queues.
> 
> True.  I'm certainly open to ideas.
> 
> In practice, one never wants to mix QEMU generated ACPI tables with
> SeaBIOS generated ACPI tables.

Yea, that would only be the case for the transition phase.

But maybe it isn't the best idea to attempt doing the switch without
breaking anything along the way.  We could use the existing logic vs.
"all tables from qemu" depending on a fw_cfg file being present or not
as you've suggested.  Make that runtime-switchable in qemu for easy testing.

Then start the qemu-provided tables with the minimal feature set needed
to boot up (i.e. no cpu hotplug, no pci hotplug, no ssdt patching,
static smbios, ...).  That could be easy enougth to be added in a single
patch series.

ovmf + coreboot can be updated to also use the qemu tables at this point.

Once we are there start generating/patching tables on the qemu side to
get back to feature parity with current seabios-provided tables.  Can
easily be done incrementally, even without having to patch seabios /
ovmf / coreboot for each step (well, in theory, in practice there are
probably cases where we have to fixup something overseen earlier ...).

Sounds sane?

cheers,
  Gerd



___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Kevin O'Connor]

On Thu, Mar 21, 2013 at 03:26:26PM +0200, Michael S. Tsirkin wrote:
> On Thu, Mar 21, 2013 at 01:14:38PM +, David Woodhouse wrote:
> > On Thu, 2013-03-21 at 15:12 +0200, Michael S. Tsirkin wrote:
> > > Anyway, I am not against such runtime flags.
> > > 
> > > If we add to this an option to build a minimal BIOS
> > > that only works with the new QEMU, do we have a deal?
> > 
> > Yeah, definitely. The code for SeaBIOS to build its own should certainly
> > be optional. It should be possible to build a minimal SeaBIOS which
> > *can't*.

Agreed.

> Okay that's exactly what me and Laszlo are working on.
> We want to add a special way to build qemu and seabios
> such that they work with tables in qemu, then add
> runtime detection on top.
> 
> The advantage is that we can make progress
> without keeping lots of patches out of tree.
> Unless of course Kevin nacks this all ...

I think we need to have a plan for what the final interface will look
like.

Right now, we can't change QEMU to pass tables via the existing fw_cfg
entries unless we upgrade SeaBIOS in QEMU first (otherwise things like
duplicate MADT entries occur).  If we're going to upgrade SeaBIOS in
QEMU, we really want that upgrade to be the final version.  We don't
want to have to bump the seabios rev in qemu multiple times in
lock-step with the changes.

So, I see two ways to do this:

1 - update SeaBIOS with a patch series that makes it capable of
handling all tables via existing and new fw_cfg entries (including
flags to disable all internal tables), update QEMU to use that SeaBIOS
rev, and then apply a patch series to QEMU that has it create all the
tables (with the final patch flagging to seabios that it should no
longer create any internal tables).

or, 2 - apply a patch series to QEMU that has it create all the tables
via new fw_cfg entries, then apply a patch series to seabios that
updates it to use the new fw_cfg entries instead of its existing
internal tables, and then apply that new rev of seabios to qemu.

Were you proposing one of the above paths, or did you have something
else in mind?

-Kevin

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Kevin O'Connor]

On Thu, Mar 21, 2013 at 09:18:50AM +0100, Gerd Hoffmann wrote:
> On 03/21/13 07:23, Michael S. Tsirkin wrote:
> > On Wed, Mar 20, 2013 at 08:22:30PM -0400, Kevin O'Connor wrote:
> >> On Wed, Mar 20, 2013 at 10:53:05PM +0100, Laszlo Ersek wrote:
> >>>
> >>> Signed-off-by: Laszlo Ersek 
> >>
> >> I think we need to figure out what the final fw_cfg interface for
> >> ACPI, SMBIOS, mptable, and PIR will be.
> >>
> >> We can use the current fw_cfg ACPI table passing mechanism for ACPI,
> >> but there are a couple of things that need to be worked out.  For each
> >> table, there must be a way to determine if SeaBIOS should build it, or
> >> if the table should not be present at all.  For example, if MADT isn't
> >> present in the fw_cfg, is that because SeaBIOS is expected to build it
> >> or because it is not expected to be present at all?
> 
> I think we always have a MADT, don't we?  So why worry about the case
> that we might have no MADT?  I think the logic is just fine here.

It's not always the case that we have an HPET or an SRAT though.  (And
potentially one may wish to test cases where the MADT is not present.)

> >> This also needs
> >> to be resolved for SSDT tables, which can have zero, one, or more
> >> instances.
> 
> Same argument as for the MADT.

The issue with the SSDT is that there can be many of them.  QEMU may
wish to pass in 2 or more.  If QEMU does pass in one or more SSDTs,
how will SeaBIOS know if it should also generate it's own SSDT?

> >> For mptable and
> >> PIR, there is no current mechanism, so we can add new fw_cfg "files"
> >> for them.  However, for all of these SeaBIOS needs to be able to
> >> determine when it needs to create the table and when no table should
> >> be published at all.
> 
> Same argument as for the MADT.

I'm not sure about that.  Right now the PIR table on q35 is totally
bogus - it would assuredly be better to not produce a PIR table then
to produce an incorrect one.  Maybe the right solution here is to fix
the PIR on q35, but I would not be surprised if at some point it
became impossible to define a valid PIR for new hardware.

> >> One possible way to accomplish the above would be to add an
> >> "all_tables_from_qemu" fw_cfg entry that turned off all of the
> >> existing SeaBIOS code.  There are probably other ways.
> 
> As this is quite a bunch of work I would tend to avoid a flag day like
> this so we can switch over tables one by one without building up big
> patch queues.

True.  I'm certainly open to ideas.

In practice, one never wants to mix QEMU generated ACPI tables with
SeaBIOS generated ACPI tables.  To do so would be chaos - the
signatures and versions would likely not match, various code
assumptions could conflict, and basic things like knowing where to
report a bug would be overly complex.

So, I wouldn't want to have to merge all patches in one big "flag
day", but maybe, for example, the new QEMU tables should be sent via
new fw_cfg entries and SeaBIOS should only be updated to use these new
entries once everything is produced by QEMU.

As an aside, there is some mixing of tables already because of the
qemu "-acpitable" option.  However, this is relatively limited and
done solely to import external system tables to appease certain guests
- it will always be an external mixing of tables because of this.
(The DSDT import isn't mixing tables - the DSDT is generated in
SeaBIOS to be used with other SeaBIOS tables - it's just a means of
reducing the size of the SeaBIOS binary.)

-Kevin

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Kevin O'Connor]

On Thu, Mar 21, 2013 at 03:04:37PM +0100, Gerd Hoffmann wrote:
> Today you can clone upstream seabios, build it, and the resulting image
> will work on pretty much any qemu version since 0.12 or so.  You don't
> have to pick the correct config switches for your particular qemu
> version, it just works.  I think it should stay that way by default.
> 
> Having config options to turn off support for older qemu versions is
> fine, so we can strip the bios binaries bundled with qemu by leaving out
> code which would not be used anyway.

Yes.  Exactly.

-Kevin

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[David Woodhouse]

On Wed, 2013-03-20 at 20:22 -0400, Kevin O'Connor wrote:
> On Wed, Mar 20, 2013 at 10:53:05PM +0100, Laszlo Ersek wrote:
> > 
> > Signed-off-by: Laszlo Ersek 
> 
> I think we need to figure out what the final fw_cfg interface for
> ACPI, SMBIOS, mptable, and PIR will be.

Once we have consensus, we can implement this on the OVMF side too. Is
anyone (Laszlo?) looking at that, or should I?


> For SMBIOS, I don't think we should use the existing fw_cfg mechanism.
> It's too complicated for what is needed.  Instead, one fw_cfg "file"
> entry with the whole smbios table should suffice. 

Agreed. I'd already looked at doing this in OVMF, and run away
screaming.

>  For mptable and PIR, there is no current mechanism, so we can add new
> fw_cfg "files" for them.  However, for all of these SeaBIOS needs to
> be able to determine when it needs to create the table and when no
> table should be published at all.

I'd make it all-or-nothing. Except for a few historical qemu commits if
you're bisecting, why would qemu ever provide a *partial* set of tables?

-- 
   Sent with MeeGo's ActiveSync support.

David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation


smime.p7s
Description: S/MIME cryptographic signature
___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Gerd Hoffmann]

  Hi,

> But I'm not sure I see any point in doing it table-by-table. Surely it
> can be all or nothing?

It allows to merge changes piecewise and avoids piling up long patch
queues.  It makes bisecting regressions easier.  Also the logic "if
table $foo is provided by qemu, just use it, otherwise generate it / use
compiled-in" seems to be simple enougth.

But at the end of the day it is the call of the one actually doing the
work.  Maybe I'm over-estimating the work needed to do the switch.

cheers,
  Gerd



___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Gerd Hoffmann]

On 03/21/13 14:01, Michael S. Tsirkin wrote:
> On Thu, Mar 21, 2013 at 01:49:36PM +0100, Gerd Hoffmann wrote:
>>   Hi,
>>
> How about we don't bother to determine this at runtime at all?

 Because it will be a PITA for testers + developers to figure the correct
 .config switches of the day during the transition phase?
>>>
>>> Why is it a PITA?  Are you developing QEMU?  Just use the makefile from
>>> roms/config.seabios Are you using QEMU binary?  Just use the defaults.
>>
>> SeaBIOS binaries are running on a wide range of qemu versions today.
>> Changing that is a big deal.
> 
> Chaqnging what?  Legacy QEMU will keep working if you build in ACPI
> tables.

Today you can clone upstream seabios, build it, and the resulting image
will work on pretty much any qemu version since 0.12 or so.  You don't
have to pick the correct config switches for your particular qemu
version, it just works.  I think it should stay that way by default.

Having config options to turn off support for older qemu versions is
fine, so we can strip the bios binaries bundled with qemu by leaving out
code which would not be used anyway.

>>  For starters the usual way to
>> package seabios and qemu in distros is to have separate packages ...

> Which ones? That's just crazy.  Fedora packages them together:

No:

[root@fedora ~]# ll /usr/share/qemu/bios.bin
lrwxrwxrwx. 1 root root 19 Feb 13 15:49 /usr/share/qemu/bios.bin ->
../seabios/bios.bin
[root@fedora ~]# rpm -qf /usr/share/*/bios.bin
qemu-system-x86-1.2.2-6.fc18.x86_64
seabios-bin-1.7.1-4.fc18.noarch

cheers,
  Gerd


___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Thu, Mar 21, 2013 at 01:14:38PM +, David Woodhouse wrote:
> On Thu, 2013-03-21 at 15:12 +0200, Michael S. Tsirkin wrote:
> > On Thu, Mar 21, 2013 at 01:04:35PM +, David Woodhouse wrote:
> > > On Thu, 2013-03-21 at 13:56 +0100, Laszlo Ersek wrote:
> > > > - for an earlier qemu, the option must be set,
> > > > - for a later qemu the option must be clear &&
> > > >   (no -acpitable switch must be specified on the qemu cmldine ||
> > > >one -acpitable switch must load a MADT)
> > > 
> > > Hm, that sounds like it won't be possible to build one version of
> > > SeaBIOS that works for *both* old and new qemu. That doesn't seem like a
> > > great idea. I'd prefer something like:
> > > 
> > > - If Qemu provides the 'core' ACPI tables (i.e. not just SSDT) then 
> > >   SeaBIOS uses them.
> > > - Otherwise, it makes its own.
> > > 
> > > 
> > > -- 
> > > dwmw2
> > 
> > It might simplify life for someone bisecting qemu
> > as you don't need to rebuild seabios after each
> > bisect but is this really a common workflow?
> 
> Not bisection, but perhaps switching between an old and a new version of
> qemu.
> 
> > Anyway, I am not against such runtime flags.
> > 
> > If we add to this an option to build a minimal BIOS
> > that only works with the new QEMU, do we have a deal?
> 
> Yeah, definitely. The code for SeaBIOS to build its own should certainly
> be optional. It should be possible to build a minimal SeaBIOS which
> *can't*.

Okay that's exactly what me and Laszlo are working on.
We want to add a special way to build qemu and seabios
such that they work with tables in qemu, then add
runtime detection on top.

The advantage is that we can make progress
without keeping lots of patches out of tree.
Unless of course Kevin nacks this all ...

> -- 
> dwmw2



___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[David Woodhouse]

On Thu, 2013-03-21 at 15:12 +0200, Michael S. Tsirkin wrote:
> On Thu, Mar 21, 2013 at 01:04:35PM +, David Woodhouse wrote:
> > On Thu, 2013-03-21 at 13:56 +0100, Laszlo Ersek wrote:
> > > - for an earlier qemu, the option must be set,
> > > - for a later qemu the option must be clear &&
> > >   (no -acpitable switch must be specified on the qemu cmldine ||
> > >one -acpitable switch must load a MADT)
> > 
> > Hm, that sounds like it won't be possible to build one version of
> > SeaBIOS that works for *both* old and new qemu. That doesn't seem like a
> > great idea. I'd prefer something like:
> > 
> > - If Qemu provides the 'core' ACPI tables (i.e. not just SSDT) then 
> >   SeaBIOS uses them.
> > - Otherwise, it makes its own.
> > 
> > 
> > -- 
> > dwmw2
> 
> It might simplify life for someone bisecting qemu
> as you don't need to rebuild seabios after each
> bisect but is this really a common workflow?

Not bisection, but perhaps switching between an old and a new version of
qemu.

> Anyway, I am not against such runtime flags.
> 
> If we add to this an option to build a minimal BIOS
> that only works with the new QEMU, do we have a deal?

Yeah, definitely. The code for SeaBIOS to build its own should certainly
be optional. It should be possible to build a minimal SeaBIOS which
*can't*.

-- 
dwmw2


smime.p7s
Description: S/MIME cryptographic signature
___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[David Woodhouse]

On Thu, 2013-03-21 at 13:56 +0100, Laszlo Ersek wrote:
> - for an earlier qemu, the option must be set,
> - for a later qemu the option must be clear &&
>   (no -acpitable switch must be specified on the qemu cmldine ||
>one -acpitable switch must load a MADT)

Hm, that sounds like it won't be possible to build one version of
SeaBIOS that works for *both* old and new qemu. That doesn't seem like a
great idea. I'd prefer something like:

- If Qemu provides the 'core' ACPI tables (i.e. not just SSDT) then 
  SeaBIOS uses them.
- Otherwise, it makes its own.


-- 
dwmw2


smime.p7s
Description: S/MIME cryptographic signature
___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Thu, Mar 21, 2013 at 01:04:35PM +, David Woodhouse wrote:
> On Thu, 2013-03-21 at 13:56 +0100, Laszlo Ersek wrote:
> > - for an earlier qemu, the option must be set,
> > - for a later qemu the option must be clear &&
> >   (no -acpitable switch must be specified on the qemu cmldine ||
> >one -acpitable switch must load a MADT)
> 
> Hm, that sounds like it won't be possible to build one version of
> SeaBIOS that works for *both* old and new qemu. That doesn't seem like a
> great idea. I'd prefer something like:
> 
> - If Qemu provides the 'core' ACPI tables (i.e. not just SSDT) then 
>   SeaBIOS uses them.
> - Otherwise, it makes its own.
> 
> 
> -- 
> dwmw2

It might simplify life for someone bisecting qemu
as you don't need to rebuild seabios after each
bisect but is this really a common workflow?

Anyway, I am not against such runtime flags.

If we add to this an option to build a minimal BIOS
that only works with the new QEMU, do we have a deal?

Then the plan is to make progress and apply patches step by step without
deciding on the detection interface first.
Before QEMU is switched to the new configuration,
we'll add the runtime thing for the benefit of developers
that bisect.

Makes sense?

-- 
MST

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Thu, Mar 21, 2013 at 12:52:17PM +, David Woodhouse wrote:
> On Thu, 2013-03-21 at 13:49 +0100, Gerd Hoffmann wrote:
> > >>> How about we don't bother to determine this at runtime at all?
> > >>
> > >> Because it will be a PITA for testers + developers to figure the correct
> > >> .config switches of the day during the transition phase?
> > > 
> > > Why is it a PITA?  Are you developing QEMU?  Just use the makefile from
> > > roms/config.seabios Are you using QEMU binary?  Just use the defaults.
> > 
> > SeaBIOS binaries are running on a wide range of qemu versions today.
> > Changing that is a big deal.  People are not used to it, and even when
> > writing it to the README people will stumble over it.  It also is pretty
> > inconvenient in a number of cases.  For starters the usual way to
> > package seabios and qemu in distros is to have separate packages ...
> 
> I agree that for the foreseeable future, we should be able to build
> SeaBIOS such that it can cope with old versions of Qemu that *don't*
> provide ACPI tables. 
> 
> And of course we should make it cope with new versions of Qemu that
> *do*.
> 
> But I'm not sure I see any point in doing it table-by-table. Surely it
> can be all or nothing?
> 
> -- 
> dwmw2

If someone wants to add a feature where same bios works with
old and new qemu, I don't see a problem here.
I also don't see why we should not allow building a
minimal bios that only works with the new specific qemu.
This is the only option that distros will actually ship,
so runtime detection is a developer's tool really,
seems quite sane to allow saving some resources
by removing it.

No?

-- 
MST

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Laszlo Ersek]

On 03/21/13 13:52, David Woodhouse wrote:
> On Thu, 2013-03-21 at 13:49 +0100, Gerd Hoffmann wrote:
> How about we don't bother to determine this at runtime at all?

 Because it will be a PITA for testers + developers to figure the correct
 .config switches of the day during the transition phase?
>>>
>>> Why is it a PITA?  Are you developing QEMU?  Just use the makefile from
>>> roms/config.seabios Are you using QEMU binary?  Just use the defaults.
>>
>> SeaBIOS binaries are running on a wide range of qemu versions today.
>> Changing that is a big deal.  People are not used to it, and even when
>> writing it to the README people will stumble over it.  It also is pretty
>> inconvenient in a number of cases.  For starters the usual way to
>> package seabios and qemu in distros is to have separate packages ...
> 
> I agree that for the foreseeable future, we should be able to build
> SeaBIOS such that it can cope with old versions of Qemu that *don't*
> provide ACPI tables. 
> 
> And of course we should make it cope with new versions of Qemu that
> *do*.

I'm fine with that as well.

I can't say anything about tables "in general". Regarding the MADT, we
always must have one (at least under the "APIC interrupt model", if I
remember the ACPI spec correctly), and for that this 2/2 should be OK.
The debug message regarding the table's origin I do agree with, I can
repost if that's the way to go.

Wrt. the interface. I think the current "mass dump of tables in one
fw_cfg file" is a bit inconvenient but I didn't want to change it. If we
had a separate fw_cfg *file* (not key of course) for each table, we
could define its format like:
- file not present: qemu is old, seabios should install the table,
- file is present with a special first byte: seabios should *not*
install the table,
- file is present with another first byte and a trailing blob: seabios
should install the blob as the table.

(Very coarse idea of admittedly, maybe some more headers will be necessary.)

Combining the current "big dump of tables's contents" with the more
flexible "Y/N file per table" approach seems ugly.

> 
> But I'm not sure I see any point in doing it table-by-table. Surely it
> can be all or nothing?

Not while we're implementing those "few historical commits" :)

Thanks
Laszlo


___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Thu, Mar 21, 2013 at 01:49:36PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> >>> How about we don't bother to determine this at runtime at all?
> >>
> >> Because it will be a PITA for testers + developers to figure the correct
> >> .config switches of the day during the transition phase?
> > 
> > Why is it a PITA?  Are you developing QEMU?  Just use the makefile from
> > roms/config.seabios Are you using QEMU binary?  Just use the defaults.
> 
> SeaBIOS binaries are running on a wide range of qemu versions today.
> Changing that is a big deal.

Chaqnging what?  Legacy QEMU will keep working if you build in ACPI
tables.

> People are not used to it, and even when
> writing it to the README people will stumble over it.  It also is pretty
> inconvenient in a number of cases.

Could you give me one example?

I don't get it. If you are working on both QEMU and seabios,
you better build seabios in the way that qemu build process
uses. Anything else is just wrong on so many levels...

>  For starters the usual way to
> package seabios and qemu in distros is to have separate packages ...
> 
> cheers,
>   Gerd

Which ones? That's just crazy.  Fedora packages them together:
qemu-system-x86-1.2.2-6.fc18.i686 bios is a firmware flashed on the
motherboard. You want to change motherboard but use the same firmware?

-- 
MST

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Laszlo Ersek]

On 03/21/13 13:23, David Woodhouse wrote:
> On Wed, 2013-03-20 at 20:22 -0400, Kevin O'Connor wrote:
>> On Wed, Mar 20, 2013 at 10:53:05PM +0100, Laszlo Ersek wrote:
>>>
>>> Signed-off-by: Laszlo Ersek 
>>
>> I think we need to figure out what the final fw_cfg interface for
>> ACPI, SMBIOS, mptable, and PIR will be.
> 
> Once we have consensus, we can implement this on the OVMF side too. Is
> anyone (Laszlo?) looking at that, or should I?

I figured we should design and implement the thing first between qemu
and seabios (seabios being the more widely used firmware ATM I reckon),
soak it in user testing, and then implement a "ready plan" in OVMF. I
expect a lot of back&forth in this, so let's not double it :)

As long as qemu is exporting the current fw_cfg keys, present code in
OVMF shouldn't break even if we export some more stuff.

>> For SMBIOS, I don't think we should use the existing fw_cfg mechanism.
>> It's too complicated for what is needed.  Instead, one fw_cfg "file"
>> entry with the whole smbios table should suffice. 
> 
> Agreed. I'd already looked at doing this in OVMF, and run away
> screaming.

In light of this, your offer above is even more appreciated :)

I wouldn't mind doing the OVMF side from an emotional standpoint, I just
feel overloaded / overwhelmed already by what I'm looking at. So
certainly don't wait for me (but consider that only work that is not
done is not lost). If you pick it up I'm thankful, if you don't, I can
still tack it to my todo list.

>>  For mptable and PIR, there is no current mechanism, so we can add new
>> fw_cfg "files" for them.  However, for all of these SeaBIOS needs to
>> be able to determine when it needs to create the table and when no
>> table should be published at all.
> 
> I'd make it all-or-nothing. Except for a few historical qemu commits if
> you're bisecting, why would qemu ever provide a *partial* set of tables?

I can visualize myself implementing Michael's build time option
suggestion: if the option is set, SeaBIOS installs its own MADT (and any
MADT from qemu will create a duplicate); if the option is clear, SeaBIOS
won't install its own MADT (and if qemu doesn't provide one either, no
MADT will be present). So,
- for an earlier qemu, the option must be set,
- for a later qemu the option must be clear &&
  (no -acpitable switch must be specified on the qemu cmldine ||
   one -acpitable switch must load a MADT)

Thanks
Laszlo

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[David Woodhouse]

On Thu, 2013-03-21 at 13:49 +0100, Gerd Hoffmann wrote:
> >>> How about we don't bother to determine this at runtime at all?
> >>
> >> Because it will be a PITA for testers + developers to figure the correct
> >> .config switches of the day during the transition phase?
> > 
> > Why is it a PITA?  Are you developing QEMU?  Just use the makefile from
> > roms/config.seabios Are you using QEMU binary?  Just use the defaults.
> 
> SeaBIOS binaries are running on a wide range of qemu versions today.
> Changing that is a big deal.  People are not used to it, and even when
> writing it to the README people will stumble over it.  It also is pretty
> inconvenient in a number of cases.  For starters the usual way to
> package seabios and qemu in distros is to have separate packages ...

I agree that for the foreseeable future, we should be able to build
SeaBIOS such that it can cope with old versions of Qemu that *don't*
provide ACPI tables. 

And of course we should make it cope with new versions of Qemu that
*do*.

But I'm not sure I see any point in doing it table-by-table. Surely it
can be all or nothing?

-- 
dwmw2


smime.p7s
Description: S/MIME cryptographic signature
___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Gerd Hoffmann]

  Hi,

>>> How about we don't bother to determine this at runtime at all?
>>
>> Because it will be a PITA for testers + developers to figure the correct
>> .config switches of the day during the transition phase?
> 
> Why is it a PITA?  Are you developing QEMU?  Just use the makefile from
> roms/config.seabios Are you using QEMU binary?  Just use the defaults.

SeaBIOS binaries are running on a wide range of qemu versions today.
Changing that is a big deal.  People are not used to it, and even when
writing it to the README people will stumble over it.  It also is pretty
inconvenient in a number of cases.  For starters the usual way to
package seabios and qemu in distros is to have separate packages ...

cheers,
  Gerd


___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Thu, Mar 21, 2013 at 12:23:45PM +, David Woodhouse wrote:
> On Wed, 2013-03-20 at 20:22 -0400, Kevin O'Connor wrote:
> > On Wed, Mar 20, 2013 at 10:53:05PM +0100, Laszlo Ersek wrote:
> > > 
> > > Signed-off-by: Laszlo Ersek 
> > 
> > I think we need to figure out what the final fw_cfg interface for
> > ACPI, SMBIOS, mptable, and PIR will be.
> 
> Once we have consensus, we can implement this on the OVMF side too. Is
> anyone (Laszlo?) looking at that, or should I?
> 
> 
> > For SMBIOS, I don't think we should use the existing fw_cfg mechanism.
> > It's too complicated for what is needed.  Instead, one fw_cfg "file"
> > entry with the whole smbios table should suffice. 
> 
> Agreed. I'd already looked at doing this in OVMF, and run away
> screaming.
> 
> >  For mptable and PIR, there is no current mechanism, so we can add new
> > fw_cfg "files" for them.  However, for all of these SeaBIOS needs to
> > be able to determine when it needs to create the table and when no
> > table should be published at all.
> 
> I'd make it all-or-nothing. Except for a few historical qemu commits if
> you're bisecting, why would qemu ever provide a *partial* set of tables?

I think we should just get the config from qemu. Pls take a look
at the README patch I posted. It will also help future
extensions.


> -- 
>Sent with MeeGo's ActiveSync support.
> 
> David WoodhouseOpen Source Technology Centre
> david.woodho...@intel.com  Intel Corporation



___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Wed, Mar 20, 2013 at 10:53:05PM +0100, Laszlo Ersek wrote:
> 
> Signed-off-by: Laszlo Ersek 

I think this is a bit too aggressive.
Let's do what I did for DSDT, add a config
option and default to yes.
In QEMU, override it to remove MADT from bios.

> ---
>  src/acpi.c |   19 ---
>  1 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/src/acpi.c b/src/acpi.c
> index 8bbc92b..611553e 100644
> --- a/src/acpi.c
> +++ b/src/acpi.c
> @@ -797,13 +797,13 @@ acpi_setup(void)
>  struct fadt_descriptor_rev1 *fadt = build_fadt(pci);
>  ACPI_INIT_TABLE(fadt);
>  ACPI_INIT_TABLE(build_ssdt());
> -ACPI_INIT_TABLE(build_madt());
>  ACPI_INIT_TABLE(build_hpet());
>  ACPI_INIT_TABLE(build_srat());
>  if (pci->device == PCI_DEVICE_ID_INTEL_ICH9_LPC)
>  ACPI_INIT_TABLE(build_mcfg_q35());
>  
>  struct romfile_s *file = NULL;
> +int madt_found = 0;
>  for (;;) {
>  file = romfile_findprefix("acpi/", file);
>  if (!file)
> @@ -816,13 +816,19 @@ acpi_setup(void)
>  int ret = file->copy(file, table, file->size);
>  if (ret <= sizeof(*table))
>  continue;
> -if (table->signature == DSDT_SIGNATURE) {
> +switch (table->signature) {
> +case DSDT_SIGNATURE:
>  if (fadt) {
>  fill_dsdt(fadt, table);
>  }
> -} else {
> +break;
> +case APIC_SIGNATURE:
> +madt_found = 1;
> +/* fall through */
> +default:
>  ACPI_INIT_TABLE(table);
>  }
> +
>  if (tbl_idx == MAX_ACPI_TABLES) {
>  warn_noalloc();
>  break;
> @@ -838,6 +844,13 @@ acpi_setup(void)
>  memcpy(dsdt, AmlCode, sizeof(AmlCode));
>  fill_dsdt(fadt, dsdt);
>  }
> +if (!madt_found) {
> +if (tbl_idx == MAX_ACPI_TABLES) {
> +warn_noalloc();
> +return;
> +}
> +ACPI_INIT_TABLE(build_madt());
> +}
>  
>  // Build final rsdt table
>  struct rsdt_descriptor_rev1 *rsdt;
> -- 
> 1.7.1

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Thu, Mar 21, 2013 at 09:18:50AM +0100, Gerd Hoffmann wrote:
> On 03/21/13 07:23, Michael S. Tsirkin wrote:
> > On Wed, Mar 20, 2013 at 08:22:30PM -0400, Kevin O'Connor wrote:
> >> On Wed, Mar 20, 2013 at 10:53:05PM +0100, Laszlo Ersek wrote:
> >>>
> >>> Signed-off-by: Laszlo Ersek 
> >>
> >> I think we need to figure out what the final fw_cfg interface for
> >> ACPI, SMBIOS, mptable, and PIR will be.
> >>
> >> We can use the current fw_cfg ACPI table passing mechanism for ACPI,
> >> but there are a couple of things that need to be worked out.  For each
> >> table, there must be a way to determine if SeaBIOS should build it, or
> >> if the table should not be present at all.  For example, if MADT isn't
> >> present in the fw_cfg, is that because SeaBIOS is expected to build it
> >> or because it is not expected to be present at all?
> 
> I think we always have a MADT, don't we?  So why worry about the case
> that we might have no MADT?  I think the logic is just fine here.
> 
> Having a debug printk saying which table came from there would be nice
> for trouble shooting though.
> 
> >> This also needs
> >> to be resolved for SSDT tables, which can have zero, one, or more
> >> instances.
> 
> Same argument as for the MADT.
> 
> > How about we don't bother to determine this at runtime at all?
> 
> Because it will be a PITA for testers + developers to figure the correct
> .config switches of the day during the transition phase?

Why is it a PITA?  Are you developing QEMU?  Just use the makefile from
roms/config.seabios Are you using QEMU binary?  Just use the defaults.

> >> For SMBIOS, I don't think we should use the existing fw_cfg mechanism.
> >> It's too complicated for what is needed.  Instead, one fw_cfg "file"
> >> entry with the whole smbios table should suffice.
> 
> Agree.
> 
> >> For mptable and
> >> PIR, there is no current mechanism, so we can add new fw_cfg "files"
> >> for them.  However, for all of these SeaBIOS needs to be able to
> >> determine when it needs to create the table and when no table should
> >> be published at all.
> 
> Same argument as for the MADT.
> 
> >> One possible way to accomplish the above would be to add an
> >> "all_tables_from_qemu" fw_cfg entry that turned off all of the
> >> existing SeaBIOS code.  There are probably other ways.
> 
> As this is quite a bunch of work I would tend to avoid a flag day like
> this so we can switch over tables one by one without building up big
> patch queues.
> 
> Once all is done switched over we can add a .config option for the
> seabios acpi table generation code, so it can be turned off altogether
> for qemu versions new enougth.
> 
> cheers,
>   Gerd

Agree. But no need for a runtime hack.  People building seabios for qemu
should use the makefile from roms/config.seabios we can flip switches
there atomically with adding tables to QEMU.


-- 
MST

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Gerd Hoffmann]

On 03/21/13 07:23, Michael S. Tsirkin wrote:
> On Wed, Mar 20, 2013 at 08:22:30PM -0400, Kevin O'Connor wrote:
>> On Wed, Mar 20, 2013 at 10:53:05PM +0100, Laszlo Ersek wrote:
>>>
>>> Signed-off-by: Laszlo Ersek 
>>
>> I think we need to figure out what the final fw_cfg interface for
>> ACPI, SMBIOS, mptable, and PIR will be.
>>
>> We can use the current fw_cfg ACPI table passing mechanism for ACPI,
>> but there are a couple of things that need to be worked out.  For each
>> table, there must be a way to determine if SeaBIOS should build it, or
>> if the table should not be present at all.  For example, if MADT isn't
>> present in the fw_cfg, is that because SeaBIOS is expected to build it
>> or because it is not expected to be present at all?

I think we always have a MADT, don't we?  So why worry about the case
that we might have no MADT?  I think the logic is just fine here.

Having a debug printk saying which table came from there would be nice
for trouble shooting though.

>> This also needs
>> to be resolved for SSDT tables, which can have zero, one, or more
>> instances.

Same argument as for the MADT.

> How about we don't bother to determine this at runtime at all?

Because it will be a PITA for testers + developers to figure the correct
.config switches of the day during the transition phase?

>> For SMBIOS, I don't think we should use the existing fw_cfg mechanism.
>> It's too complicated for what is needed.  Instead, one fw_cfg "file"
>> entry with the whole smbios table should suffice.

Agree.

>> For mptable and
>> PIR, there is no current mechanism, so we can add new fw_cfg "files"
>> for them.  However, for all of these SeaBIOS needs to be able to
>> determine when it needs to create the table and when no table should
>> be published at all.

Same argument as for the MADT.

>> One possible way to accomplish the above would be to add an
>> "all_tables_from_qemu" fw_cfg entry that turned off all of the
>> existing SeaBIOS code.  There are probably other ways.

As this is quite a bunch of work I would tend to avoid a flag day like
this so we can switch over tables one by one without building up big
patch queues.

Once all is done switched over we can add a .config option for the
seabios acpi table generation code, so it can be turned off altogether
for qemu versions new enougth.

cheers,
  Gerd


___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Michael S. Tsirkin]

On Wed, Mar 20, 2013 at 08:22:30PM -0400, Kevin O'Connor wrote:
> On Wed, Mar 20, 2013 at 10:53:05PM +0100, Laszlo Ersek wrote:
> > 
> > Signed-off-by: Laszlo Ersek 
> 
> I think we need to figure out what the final fw_cfg interface for
> ACPI, SMBIOS, mptable, and PIR will be.
> 
> We can use the current fw_cfg ACPI table passing mechanism for ACPI,
> but there are a couple of things that need to be worked out.  For each
> table, there must be a way to determine if SeaBIOS should build it, or
> if the table should not be present at all.  For example, if MADT isn't
> present in the fw_cfg, is that because SeaBIOS is expected to build it
> or because it is not expected to be present at all?  This also needs
> to be resolved for SSDT tables, which can have zero, one, or more
> instances.

How about we don't bother to determine this at runtime at all?
Long term all tables will be in qemu so why bother with
a runtime mechanism just for an intermediate stage?
Just build BIOS with the correct flags.

> Finally, the mechanism should define how the RSDP
> signatures is set as well as support both RSDT and XSDT tables (with
> signatures defined by QEMU for these as well).
>
> For SMBIOS, I don't think we should use the existing fw_cfg mechanism.
> It's too complicated for what is needed.  Instead, one fw_cfg "file"
> entry with the whole smbios table should suffice.  For mptable and
> PIR, there is no current mechanism, so we can add new fw_cfg "files"
> for them.  However, for all of these SeaBIOS needs to be able to
> determine when it needs to create the table and when no table should
> be published at all.
> 
> One possible way to accomplish the above would be to add an
> "all_tables_from_qemu" fw_cfg entry that turned off all of the
> existing SeaBIOS code.  There are probably other ways.
> 
> Thoughts?
> 
> -Kevin


Yes I agree these tables need some thought.  But first things first, we
can move quite a lot of code out to qemu with just fw_cfg.

-- 
MST

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/2] accept MADT over fw_cfg

[Kevin O'Connor]

On Wed, Mar 20, 2013 at 10:53:05PM +0100, Laszlo Ersek wrote:
> 
> Signed-off-by: Laszlo Ersek 

I think we need to figure out what the final fw_cfg interface for
ACPI, SMBIOS, mptable, and PIR will be.

We can use the current fw_cfg ACPI table passing mechanism for ACPI,
but there are a couple of things that need to be worked out.  For each
table, there must be a way to determine if SeaBIOS should build it, or
if the table should not be present at all.  For example, if MADT isn't
present in the fw_cfg, is that because SeaBIOS is expected to build it
or because it is not expected to be present at all?  This also needs
to be resolved for SSDT tables, which can have zero, one, or more
instances.  Finally, the mechanism should define how the RSDP
signatures is set as well as support both RSDT and XSDT tables (with
signatures defined by QEMU for these as well).

For SMBIOS, I don't think we should use the existing fw_cfg mechanism.
It's too complicated for what is needed.  Instead, one fw_cfg "file"
entry with the whole smbios table should suffice.  For mptable and
PIR, there is no current mechanism, so we can add new fw_cfg "files"
for them.  However, for all of these SeaBIOS needs to be able to
determine when it needs to create the table and when no table should
be published at all.

One possible way to accomplish the above would be to add an
"all_tables_from_qemu" fw_cfg entry that turned off all of the
existing SeaBIOS code.  There are probably other ways.

Thoughts?

-Kevin

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios