Patchwork [PATCHv2] load hpet info for HPET ACPI table from qemu

login
register
mail settings
Submitter Gleb Natapov
Date June 14, 2010, 8:30 a.m.
Message ID <20100614083053.GC21797@redhat.com>
Download mbox | patch
Permalink /patch/55490/
State New
Headers show

Comments

Gleb Natapov - June 14, 2010, 8:30 a.m.
Load hpet info for HPET ACPI table from qemu instead of using hardcoded
values. Use hardcoded values anyway if old qemu is detected. 

Signed-off-by: Gleb Natapov <gleb@redhat.com>
--
			Gleb.
Kevin O'Connor - June 14, 2010, 1:54 p.m.
On Mon, Jun 14, 2010 at 11:30:53AM +0300, Gleb Natapov wrote:
> Load hpet info for HPET ACPI table from qemu instead of using hardcoded
> values. Use hardcoded values anyway if old qemu is detected.

The current code does a lot of mixing of qemu provided and seabios
provided data to build the acpi tables.  This patch extends that.
Unfortunately, I find it confusing because someone then needs to look
through both the seabios and qemu code to understand how the acpi
tables are generated.

Could we just have qemu build the hpet tables and pass them through to
seabios?  Perhaps using the qemu_cfg_acpi_additional_tables() method.

-Kevin
Gleb Natapov - June 14, 2010, 2:09 p.m.
On Mon, Jun 14, 2010 at 09:54:25AM -0400, Kevin O'Connor wrote:
> On Mon, Jun 14, 2010 at 11:30:53AM +0300, Gleb Natapov wrote:
> > Load hpet info for HPET ACPI table from qemu instead of using hardcoded
> > values. Use hardcoded values anyway if old qemu is detected.
> 
> The current code does a lot of mixing of qemu provided and seabios
> provided data to build the acpi tables.  This patch extends that.
That is the point of ACPI/MP/SMBIOS tables. Those tables describe HW, so if
HW may vary we need a way to tell seabios about HW configuration if hardware
itself is un-discoverable by any other means.

> Unfortunately, I find it confusing because someone then needs to look
> through both the seabios and qemu code to understand how the acpi
> tables are generated.
What is hard to understand? Seabios itself does not have enough information
to generate ACPI tables. On real HW you rebuild bios image for each
board. Seabios + qemu can be better then that.

> 
> Could we just have qemu build the hpet tables and pass them through to
> seabios?  Perhaps using the qemu_cfg_acpi_additional_tables() method.
> 
Possible, and I considered that. I personally prefer to pass minimum
information required for seabios to discover underlying HW and leave
ACPI table creation to seabios. That is how things done for HW that
seabios can actually detect. If we will go your way pretty soon we will
move creation of ACPI/SMBIOS/MP tables into qemu and IMHO this will be
step backworkds.

--
			Gleb.
Jamie Lokier - June 14, 2010, 2:40 p.m.
Gleb Natapov wrote:
> On Mon, Jun 14, 2010 at 09:54:25AM -0400, Kevin O'Connor wrote:
> > Could we just have qemu build the hpet tables and pass them through to
> > seabios?  Perhaps using the qemu_cfg_acpi_additional_tables() method.
> > 
> Possible, and I considered that. I personally prefer to pass minimum
> information required for seabios to discover underlying HW and leave
> ACPI table creation to seabios. That is how things done for HW that
> seabios can actually detect. If we will go your way pretty soon we will
> move creation of ACPI/SMBIOS/MP tables into qemu and IMHO this will be
> step backworkds.

Why would creation of all the tables in qemu be a bad thing or a step
in the wrong direction?

Crude argument in favour of doing it in qemu: They're both C code, so
sharing code between qemu and SeaBIOS may not be as traumatic as it
would be for an asm BIOS.  Doing in SeaBIOS forces the existence of
another API, in effect, to pass the qemu-specific hardware
information, so doing it in qemu means one less interface API that
needs designing and maintaining.

Argument in favour of doing it all in SeaBIOS: I'm not sure, what is it?

Indeed, why not build all the tables outside qemu using a separate
tool ("qemu-build-acpi-tables < machine-description.txt"), invoked
from qemu when it starts up, making it easier to examine the tables
using ACPI tools?

-- Jamie
Avi Kivity - June 14, 2010, 2:51 p.m.
On 06/14/2010 05:09 PM, Gleb Natapov wrote:
>> Could we just have qemu build the hpet tables and pass them through to
>> seabios?  Perhaps using the qemu_cfg_acpi_additional_tables() method.
>>
>>      
> Possible, and I considered that. I personally prefer to pass minimum
> information required for seabios to discover underlying HW and leave
> ACPI table creation to seabios. That is how things done for HW that
> seabios can actually detect. If we will go your way pretty soon we will
> move creation of ACPI/SMBIOS/MP tables into qemu and IMHO this will be
> step backworkds.
>    

I agree.  ACPI is a firmware/OS interface.  If we move ACPI table 
generation into qemu, it becomes a mixed hardware/firmware/OS interface.

Better keep those interfaces separate: hardware/firmware (fwcfg) and 
firmware/OS (acpi).
Gleb Natapov - June 14, 2010, 4:03 p.m.
On Mon, Jun 14, 2010 at 03:40:16PM +0100, Jamie Lokier wrote:
> Gleb Natapov wrote:
> > On Mon, Jun 14, 2010 at 09:54:25AM -0400, Kevin O'Connor wrote:
> > > Could we just have qemu build the hpet tables and pass them through to
> > > seabios?  Perhaps using the qemu_cfg_acpi_additional_tables() method.
> > > 
> > Possible, and I considered that. I personally prefer to pass minimum
> > information required for seabios to discover underlying HW and leave
> > ACPI table creation to seabios. That is how things done for HW that
> > seabios can actually detect. If we will go your way pretty soon we will
> > move creation of ACPI/SMBIOS/MP tables into qemu and IMHO this will be
> > step backworkds.
> 
> Why would creation of all the tables in qemu be a bad thing or a step
> in the wrong direction?
> 
See Avi's answer. All those tables are firmware/OS interface and as such
may (and some definitely do) contain information known only to BIOS.

--
			Gleb.
Kevin O'Connor - June 14, 2010, 6:25 p.m.
On Mon, Jun 14, 2010 at 05:51:27PM +0300, Avi Kivity wrote:
> On 06/14/2010 05:09 PM, Gleb Natapov wrote:
> >>Could we just have qemu build the hpet tables and pass them through to
> >>seabios?  Perhaps using the qemu_cfg_acpi_additional_tables() method.
> >>
> >Possible, and I considered that. I personally prefer to pass minimum
> >information required for seabios to discover underlying HW and leave
> >ACPI table creation to seabios. That is how things done for HW that
> >seabios can actually detect. If we will go your way pretty soon we will
> >move creation of ACPI/SMBIOS/MP tables into qemu and IMHO this will be
> >step backworkds.
> 
> I agree.  ACPI is a firmware/OS interface.  If we move ACPI table
> generation into qemu, it becomes a mixed hardware/firmware/OS
> interface.

This seems to be a philosophical distinction.  Lets go over the
practical implications.

It seems there was a change in qemu to the hpet functionality.
Although the change is solely between qemu and the OS, it's necessary
to patch both qemu and seabios for the OS to see the change.  This
means creating and reviewing patches for two separate repos.  This
also requires release coordination - the seabios change has to be
committed and released, and then qemu needs to be released with the
new seabios.  Additional changes in seabios tip will get merged into
qemu, which could complicate testing.

> Better keep those interfaces separate: hardware/firmware (fwcfg) and
> firmware/OS (acpi).

One could look at the current hpet patch as implementing:
qemu -> struct hpet_fw_entry -> seabios -> struct acpi_20_hpet -> OS.

I'm suggesting that we do the following instead:
qemu -> struct acpi_20_hpet -> seabios -> struct acpi_20_hpet -> OS.

I'm not suggesting a radical rethink of fwcfg, but I fail to see the
advantage in introducing the arbitrary "struct hpet_fw_entry" when
there is a perfectly good, well defined, "struct acpi_20_hpet" that
already exists.  This new arbitrary intermediate format just
introduces "make work" for all of us.

-Kevin
Gleb Natapov - June 14, 2010, 6:56 p.m.
On Mon, Jun 14, 2010 at 02:25:21PM -0400, Kevin O'Connor wrote:
> On Mon, Jun 14, 2010 at 05:51:27PM +0300, Avi Kivity wrote:
> > On 06/14/2010 05:09 PM, Gleb Natapov wrote:
> > >>Could we just have qemu build the hpet tables and pass them through to
> > >>seabios?  Perhaps using the qemu_cfg_acpi_additional_tables() method.
> > >>
> > >Possible, and I considered that. I personally prefer to pass minimum
> > >information required for seabios to discover underlying HW and leave
> > >ACPI table creation to seabios. That is how things done for HW that
> > >seabios can actually detect. If we will go your way pretty soon we will
> > >move creation of ACPI/SMBIOS/MP tables into qemu and IMHO this will be
> > >step backworkds.
> > 
> > I agree.  ACPI is a firmware/OS interface.  If we move ACPI table
> > generation into qemu, it becomes a mixed hardware/firmware/OS
> > interface.
> 
> This seems to be a philosophical distinction.  Lets go over the
> practical implications.
> 
> It seems there was a change in qemu to the hpet functionality.
> Although the change is solely between qemu and the OS, it's necessary
> to patch both qemu and seabios for the OS to see the change.  This
> means creating and reviewing patches for two separate repos.  This
> also requires release coordination - the seabios change has to be
> committed and released, and then qemu needs to be released with the
> new seabios.  Additional changes in seabios tip will get merged into
> qemu, which could complicate testing.
> 
My patch is completely unrelated to functionality change in qemu. In
fact I wrote it before the change and had to rebase. Seabios/qemu have
a bug that HPET is always advertise through ACPI table even when qemu
haven't created one. So your description above is not accurate.
But even if it was accurate propagation features through software stack is
common operation everywhere. Think about adding system call to the
kernel and updating libc, or adding feature to kvm kernel module and
adding patch to use it in qemu.

> > Better keep those interfaces separate: hardware/firmware (fwcfg) and
> > firmware/OS (acpi).
> 
> One could look at the current hpet patch as implementing:
> qemu -> struct hpet_fw_entry -> seabios -> struct acpi_20_hpet -> OS.
> 
> I'm suggesting that we do the following instead:
> qemu -> struct acpi_20_hpet -> seabios -> struct acpi_20_hpet -> OS.
> 
> I'm not suggesting a radical rethink of fwcfg, but I fail to see the
> advantage in introducing the arbitrary "struct hpet_fw_entry" when
> there is a perfectly good, well defined, "struct acpi_20_hpet" that
> already exists.  This new arbitrary intermediate format just
> introduces "make work" for all of us.
> 
Then qemu will have to create ACPI header too and will have to fill
details like oem_id/oem_table_id and so on. Now if I want to change them
in my bios version it is not enough to edit CONFIG_APPNAME in seabios.
If seabios will decide to move to more resent version of ACPI spec it
will not be able to do so since it will not fully control table creation.

--
			Gleb.
Anthony Liguori - June 14, 2010, 7:38 p.m.
On 06/14/2010 01:25 PM, Kevin O'Connor wrote:
> On Mon, Jun 14, 2010 at 05:51:27PM +0300, Avi Kivity wrote:
>    
>> On 06/14/2010 05:09 PM, Gleb Natapov wrote:
>>      
>>>> Could we just have qemu build the hpet tables and pass them through to
>>>> seabios?  Perhaps using the qemu_cfg_acpi_additional_tables() method.
>>>>
>>>>          
>>> Possible, and I considered that. I personally prefer to pass minimum
>>> information required for seabios to discover underlying HW and leave
>>> ACPI table creation to seabios. That is how things done for HW that
>>> seabios can actually detect. If we will go your way pretty soon we will
>>> move creation of ACPI/SMBIOS/MP tables into qemu and IMHO this will be
>>> step backworkds.
>>>        
>> I agree.  ACPI is a firmware/OS interface.  If we move ACPI table
>> generation into qemu, it becomes a mixed hardware/firmware/OS
>> interface.
>>      
> This seems to be a philosophical distinction.  Lets go over the
> practical implications.
>
> It seems there was a change in qemu to the hpet functionality.
> Although the change is solely between qemu and the OS, it's necessary
> to patch both qemu and seabios for the OS to see the change.  This
> means creating and reviewing patches for two separate repos.  This
> also requires release coordination - the seabios change has to be
> committed and released, and then qemu needs to be released with the
> new seabios.  Additional changes in seabios tip will get merged into
> qemu, which could complicate testing.
>    

I think we can be pretty flexible as long as we're careful about 
releases.  For instance, I've applied Gleb's current patch but won't 
update SeaBIOS until the interface is worked out.  If we decide to 
implement a new interface, there's no harm since we've never had a qemu 
build that had a combination of SeaBIOS and fw_cfg that didn't work.

Regards,

Anthony Liguori
Kevin O'Connor - June 14, 2010, 8:12 p.m.
On Mon, Jun 14, 2010 at 09:56:15PM +0300, Gleb Natapov wrote:
> On Mon, Jun 14, 2010 at 02:25:21PM -0400, Kevin O'Connor wrote:
> > It seems there was a change in qemu to the hpet functionality.
> My patch is completely unrelated to functionality change in qemu. In
> fact I wrote it before the change and had to rebase. Seabios/qemu have
> a bug that HPET is always advertise through ACPI table even when qemu
> haven't created one. So your description above is not accurate.

I apologize for the confusion.  However, I feel this only furthers my
proposal.  (There was a defect in hpet table generation - seabios
knows/cares nothing about the hpet - but now we need to review, patch,
and coordinate two different projects.)

> But even if it was accurate propagation features through software stack is
> common operation everywhere. Think about adding system call to the
> kernel and updating libc, or adding feature to kvm kernel module and
> adding patch to use it in qemu.

I don't see why the above would deter us from optimizing seabios/qemu
maintenance work.

> > I'm not suggesting a radical rethink of fwcfg, but I fail to see the
> > advantage in introducing the arbitrary "struct hpet_fw_entry" when
> > there is a perfectly good, well defined, "struct acpi_20_hpet" that
> > already exists.  This new arbitrary intermediate format just
> > introduces "make work" for all of us.
> > 
> Then qemu will have to create ACPI header too and will have to fill
> details like oem_id/oem_table_id and so on. Now if I want to change them
> in my bios version it is not enough to edit CONFIG_APPNAME in seabios.

Easily solved - if there exists a table via
qemu_cfg_acpi_additional_tables(), then SeaBIOS could use the oem_ids
from that table for all tables SeaBIOS creates.

> If seabios will decide to move to more resent version of ACPI spec it
> will not be able to do so since it will not fully control table creation.

Well, SeaBIOS already doesn't fully control table creation.

But.. in order to move to a newer ACPI spec, there would be qemu
changes anyway.  (If nothing else, so that qemu can tell seabios if
it's okay to use the new rev.)  At that point we're stuck changing
both repos anyway - nothing gained, nothing lost.

I still think there is an opportunity to reduce the load on the bulk
of acpi changes - most of these changes have no dependence on seabios
at all.

-Kevin
Paul Brook - June 15, 2010, 12:54 a.m.
> > >>Could we just have qemu build the hpet tables and pass them through to
> > >>seabios?  Perhaps using the qemu_cfg_acpi_additional_tables() method.
> > >
> > >Possible, and I considered that. I personally prefer to pass minimum
> > >information required for seabios to discover underlying HW and leave
> > >ACPI table creation to seabios. That is how things done for HW that
> > >seabios can actually detect. If we will go your way pretty soon we will
> > >move creation of ACPI/SMBIOS/MP tables into qemu and IMHO this will be
> > >step backworkds.
> > 
> > I agree.  ACPI is a firmware/OS interface.  If we move ACPI table
> > generation into qemu, it becomes a mixed hardware/firmware/OS
> > interface.
> 
> This seems to be a philosophical distinction.  Lets go over the
> practical implications.
> 
> It seems there was a change in qemu to the hpet functionality.
> Although the change is solely between qemu and the OS, it's necessary
> to patch both qemu and seabios for the OS to see the change.  This
> means creating and reviewing patches for two separate repos.  This
> also requires release coordination - the seabios change has to be
> committed and released, and then qemu needs to be released with the
> new seabios.  Additional changes in seabios tip will get merged into
> qemu, which could complicate testing.

I don't think you can draw a clear conclusion one way or the other. ACPI may 
defined and an API between the firmware and OS, but its primary purpose is to 
allow the OS to interface with the hardware.

While I do think it's desirable to have a single bios image work across the 
different machines qemu emulates, I don't think it's realistic to 
develop/release the two independently.  The rate of change may be such that we 
have a fair amount of slack, and it may often be possible to make bios images 
backwards compatible.  However I think it's entirely reasonable to require 
people use matching bios and qemu.

Paul
Avi Kivity - June 15, 2010, 4:41 a.m.
On 06/14/2010 09:25 PM, Kevin O'Connor wrote:
> On Mon, Jun 14, 2010 at 05:51:27PM +0300, Avi Kivity wrote:
>    
>> On 06/14/2010 05:09 PM, Gleb Natapov wrote:
>>      
>>>> Could we just have qemu build the hpet tables and pass them through to
>>>> seabios?  Perhaps using the qemu_cfg_acpi_additional_tables() method.
>>>>
>>>>          
>>> Possible, and I considered that. I personally prefer to pass minimum
>>> information required for seabios to discover underlying HW and leave
>>> ACPI table creation to seabios. That is how things done for HW that
>>> seabios can actually detect. If we will go your way pretty soon we will
>>> move creation of ACPI/SMBIOS/MP tables into qemu and IMHO this will be
>>> step backworkds.
>>>        
>> I agree.  ACPI is a firmware/OS interface.  If we move ACPI table
>> generation into qemu, it becomes a mixed hardware/firmware/OS
>> interface.
>>      
> This seems to be a philosophical distinction.  Lets go over the
> practical implications.
>    

In my experience, well-defined interfaces ("philosophical distinctions") 
are more important in the long term than practicalities.  The 
practicalities change, but confusion over incorrect interfaces, or 
problems when wrong interfaces are used, are forever.


> It seems there was a change in qemu to the hpet functionality.
> Although the change is solely between qemu and the OS, it's necessary
> to patch both qemu and seabios for the OS to see the change.  This
> means creating and reviewing patches for two separate repos.  This
> also requires release coordination - the seabios change has to be
> committed and released, and then qemu needs to be released with the
> new seabios.  Additional changes in seabios tip will get merged into
> qemu, which could complicate testing.
>
>    

If a table needs to refer to some other information which is in a table 
that is generated by seabios, we cannot generate this table from qemu.  
That's much worse that reviewing and applying two patches.

>> Better keep those interfaces separate: hardware/firmware (fwcfg) and
>> firmware/OS (acpi).
>>      
> One could look at the current hpet patch as implementing:
> qemu ->  struct hpet_fw_entry ->  seabios ->  struct acpi_20_hpet ->  OS.
>
> I'm suggesting that we do the following instead:
> qemu ->  struct acpi_20_hpet ->  seabios ->  struct acpi_20_hpet ->  OS.
>
> I'm not suggesting a radical rethink of fwcfg, but I fail to see the
> advantage in introducing the arbitrary "struct hpet_fw_entry" when
> there is a perfectly good, well defined, "struct acpi_20_hpet" that
> already exists.  This new arbitrary intermediate format just
> introduces "make work" for all of us.
>    

Choosing an existing format is fine.  But seabios blindly copying qemu 
provided data is wrong IMO.
Avi Kivity - June 15, 2010, 4:47 a.m.
On 06/14/2010 10:38 PM, Anthony Liguori wrote:
>
> I think we can be pretty flexible as long as we're careful about 
> releases.  For instance, I've applied Gleb's current patch but won't 
> update SeaBIOS until the interface is worked out.  If we decide to 
> implement a new interface, there's no harm since we've never had a 
> qemu build that had a combination of SeaBIOS and fw_cfg that didn't work.

Or we can choose a new interface number if the interface changes.

One of Kevin's points was that the ACPI tables are a documented 
interface.  AFAIR, the firmware configuration interface isn't.  We need 
to start documenting it (and reject patches without accompanying 
documentation).
Gleb Natapov - June 15, 2010, 6:37 a.m.
On Mon, Jun 14, 2010 at 04:12:32PM -0400, Kevin O'Connor wrote:
> On Mon, Jun 14, 2010 at 09:56:15PM +0300, Gleb Natapov wrote:
> > On Mon, Jun 14, 2010 at 02:25:21PM -0400, Kevin O'Connor wrote:
> > > It seems there was a change in qemu to the hpet functionality.
> > My patch is completely unrelated to functionality change in qemu. In
> > fact I wrote it before the change and had to rebase. Seabios/qemu have
> > a bug that HPET is always advertise through ACPI table even when qemu
> > haven't created one. So your description above is not accurate.
> 
> I apologize for the confusion.  However, I feel this only furthers my
> proposal.  (There was a defect in hpet table generation - seabios
> knows/cares nothing about the hpet - but now we need to review, patch,
> and coordinate two different projects.)
> 
Seabios "knows/cares nothing about the hpet" is just another
bug/missing features. See hpet spec at www.intel.com/hardwaredesign/hpetspec_1.pdf 
and search for System BIOS (System BIOS need to do this, System BIOS need
to mark that).

> > But even if it was accurate propagation features through software stack is
> > common operation everywhere. Think about adding system call to the
> > kernel and updating libc, or adding feature to kvm kernel module and
> > adding patch to use it in qemu.
> 
> I don't see why the above would deter us from optimizing seabios/qemu
> maintenance work.

So why not go further? In theory qemu needs seabios only for legacy bios
functionality. Qemu is perfectly capable of configuring HW to OS usable
state by itself, so we can have coreboot functionality completely inside
qemu and use seabios only for legacy function just like coreboot does.

Firmware/HW is tightly coupled by design. If you do not want seabios to
be qemu's firmware and just what it to have only legacy bios
functionality we can yank all qemu support from seabios and move it to
coreboot project and use seabios only for legacy bios just like coreboot
does.

> 
> > > I'm not suggesting a radical rethink of fwcfg, but I fail to see the
> > > advantage in introducing the arbitrary "struct hpet_fw_entry" when
> > > there is a perfectly good, well defined, "struct acpi_20_hpet" that
> > > already exists.  This new arbitrary intermediate format just
> > > introduces "make work" for all of us.
> > > 
> > Then qemu will have to create ACPI header too and will have to fill
> > details like oem_id/oem_table_id and so on. Now if I want to change them
> > in my bios version it is not enough to edit CONFIG_APPNAME in seabios.
> 
> Easily solved - if there exists a table via
> qemu_cfg_acpi_additional_tables(), then SeaBIOS could use the oem_ids
> from that table for all tables SeaBIOS creates.
Why create problem to solve them easily later?

> 
> > If seabios will decide to move to more resent version of ACPI spec it
> > will not be able to do so since it will not fully control table creation.
> 
> Well, SeaBIOS already doesn't fully control table creation.
> 
Users are not suppose to abuse ACPI table passing interface. If they do
that and things breaks they should fix them by themselves. If we start
using this interface internally in qemu that's different.

> But.. in order to move to a newer ACPI spec, there would be qemu
> changes anyway.  (If nothing else, so that qemu can tell seabios if
> it's okay to use the new rev.)  At that point we're stuck changing
> both repos anyway - nothing gained, nothing lost.
I don't see why qemu should care what ACPI rev Seabios uses. You don't
changed you HW when you update you BIOS in non virtualized world. You do
change you BIOS with each new HW though.

> 
> I still think there is an opportunity to reduce the load on the bulk
> of acpi changes - most of these changes have no dependence on seabios
> at all.
> 
That depends on how you view seabios project. If you consider it to be
legacy bios functionality provider only then I agree and we should move
to coreboot model. If you consider it to be legacy bios + qemu firmware
(like old BOCHS bios was) then by definition it's seabios job to
describe underlying HW to an OS.

--
			Gleb.
Gleb Natapov - June 15, 2010, 6:50 a.m.
On Tue, Jun 15, 2010 at 07:47:55AM +0300, Avi Kivity wrote:
> On 06/14/2010 10:38 PM, Anthony Liguori wrote:
> >
> >I think we can be pretty flexible as long as we're careful about
> >releases.  For instance, I've applied Gleb's current patch but
> >won't update SeaBIOS until the interface is worked out.  If we
> >decide to implement a new interface, there's no harm since we've
> >never had a qemu build that had a combination of SeaBIOS and
> >fw_cfg that didn't work.
> 
> Or we can choose a new interface number if the interface changes.
> 
> One of Kevin's points was that the ACPI tables are a documented
> interface.  AFAIR, the firmware configuration interface isn't.  We
> need to start documenting it (and reject patches without
> accompanying documentation).
> 
ACPI tables are, indeed, documented interface (it doesn't make it good
interface though :)), but it is interface between firmware and OS, so
it may (and it does) have things like "if firmware support that, then
this bit should be set". Using it as interface to describe HW to
firmware is just plain abuse.

--
			Gleb.
Kevin O'Connor - June 17, 2010, 12:55 a.m.
On Tue, Jun 15, 2010 at 07:41:02AM +0300, Avi Kivity wrote:
> On 06/14/2010 09:25 PM, Kevin O'Connor wrote:
> >This seems to be a philosophical distinction.  Lets go over the
> >practical implications.
> In my experience, well-defined interfaces ("philosophical
> distinctions") are more important in the long term than
> practicalities.

I agree with the importance of clean interfaces.  However, I feel the
current approach to acpi table handling between qemu and seabios is
not ideal.  I'm proposing an interface which I believe is an
improvement.

> If a table needs to refer to some other information which is in a
> table that is generated by seabios, we cannot generate this table
> from qemu.  That's much worse that reviewing and applying two
> patches.

I understand.  Such tables would not make sense to generate in qemu.

I also understand and appreciate the desire for qemu to not touch
guest memory.  This means rsdp and rsdt are the domain of seabios.
The only other table that directly addresses the memory location of
another table (that I know of) is fadt - which is also tied to
seabios' smi handler - so this too is in seabios' domain.

I'm not aware of any dependencies to seabios in any of the other
tables (eg, madt, srat, hpet, ssdt, dsdt).

> >I'm not suggesting a radical rethink of fwcfg, but I fail to see the
> >advantage in introducing the arbitrary "struct hpet_fw_entry" when
> >there is a perfectly good, well defined, "struct acpi_20_hpet" that
> >already exists.  This new arbitrary intermediate format just
> >introduces "make work" for all of us.
> 
> Choosing an existing format is fine.  But seabios blindly copying
> qemu provided data is wrong IMO.

Okay.  For "struct acpi_20_hpet", what transformations or checks do
you think seabios needs to perform?

-Kevin
Kevin O'Connor - June 17, 2010, 1:22 a.m.
On Tue, Jun 15, 2010 at 09:37:07AM +0300, Gleb Natapov wrote:
> On Mon, Jun 14, 2010 at 04:12:32PM -0400, Kevin O'Connor wrote:
> > But.. in order to move to a newer ACPI spec, there would be qemu
> > changes anyway.  (If nothing else, so that qemu can tell seabios if
> > it's okay to use the new rev.)  At that point we're stuck changing
> > both repos anyway - nothing gained, nothing lost.
> I don't see why qemu should care what ACPI rev Seabios uses.

A change in ACPI rev would likely break old OSs.  Only the user would
know this, and so a method of propagating that info from qemu to
seabios would be needed.  (However, it's much more likely that a new
ACPI rev would require more data which qemu would then also need to
pass to seabios.)

> > I still think there is an opportunity to reduce the load on the bulk
> > of acpi changes - most of these changes have no dependence on seabios
> > at all.
> That depends on how you view seabios project. If you consider it to be
> legacy bios functionality provider only then I agree and we should move
> to coreboot model. If you consider it to be legacy bios + qemu firmware
> (like old BOCHS bios was) then by definition it's seabios job to
> describe underlying HW to an OS.

I don't think this is that "cut and dry".  A real machine just ships
with these acpi tables compiled in.  This is what BOCHS bios did and
it is what seabios did up until about 8 months ago.

However, qemu isn't a simple machine emulator - it can emulate a whole
class of x86 machines.  It's not practical to compile a seabios.bin
file for every permutation of x86 machine that qemu can emulate.  So,
we pass info from qemu to seabios so that it can support all the
classes of hardware.  This isn't what real machines do, and it's not
what bochs bios did.

I do view SeaBIOS as primarily a legacy bios interface and a boot
loader.  I also think it makes sense to handle qemu and kvm firmware
needs - some initialization wants to be done from the guest and
seabios is a good place to do that.

This hpet thing is really rather minor, but it has me puzzled.  The
guest OS wants the info in ACPI form, and only qemu has the info.  I
don't understand why there is a desire to pass the info in this new
arbitrary form instead of passing it in the form that the OS wants it
in.

A couple of emails back you stated you considered using the existing
qemu_cfg_acpi_additional_tables() format but dismissed the idea.
Maybe you could explain why you dismissed it and/or what the
deficiencies of this mechanism are?

-Kevin
Kevin O'Connor - June 17, 2010, 1:47 a.m.
On Tue, Jun 15, 2010 at 09:50:48AM +0300, Gleb Natapov wrote:
> On Tue, Jun 15, 2010 at 07:47:55AM +0300, Avi Kivity wrote:
> > One of Kevin's points was that the ACPI tables are a documented
> > interface.  AFAIR, the firmware configuration interface isn't.  We
> > need to start documenting it (and reject patches without
> > accompanying documentation).
> ACPI tables are, indeed, documented interface (it doesn't make it good
> interface though :)), but it is interface between firmware and OS, so
> it may (and it does) have things like "if firmware support that, then
> this bit should be set". Using it as interface to describe HW to
> firmware is just plain abuse.

I also don't much like the ACPI spec.  In particular, the DSDT stuff
is just crazy.  However, the other tables (eg, hpet, srat, madt, fadt,
facs) are just your bog standard binary structs.  There really is no
fundamental difference between these formats and the structs currently
passed via fwcfg.

I don't think it is an abuse to configure the firmware with these
structures.  They define necessary information that can't be
auto-detected, so they're going to have a lot of overlap with info
that needs to be passed between qemu and firmware.  The tables are not
perfect, but neither are the alternatives.  The mature documentation
and the fact that it is an industry standard makes up for many of
their deficiencies.

BTW, it's been said several times now that ACPI is an interface
between OS and firmware.  I don't see this at all - ACPI defines how
the OS can interact with the hardware.  The only place I know of where
seabios has involvement is with it's tiny (16 asm statement) SMI stub.
Everything else describes the hardware and enables interactions
directly with the hardware.

-Kevin
Peter Stuge - June 17, 2010, 1:58 a.m.
My background is that of a strong coreboot proponent.

Gleb Natapov wrote:
> So why not go further? In theory qemu needs seabios only for legacy bios
> functionality. Qemu is perfectly capable of configuring HW to OS usable
> state by itself, so we can have coreboot functionality completely inside
> qemu and use seabios only for legacy function just like coreboot does.

I think this is a really interesting idea, IMO it makes qemu an even
more complete product.


> Firmware/HW is tightly coupled by design. If you do not want seabios
> to be qemu's firmware and just what it to have only legacy bios
> functionality we can yank all qemu support from seabios and move it to
> coreboot project and use seabios only for legacy bios just like
> coreboot does.

Personally I believe the life of SeaBIOS would be easier if it didn't
have to be both a qemu firmware and a BIOS service provider, but note
that I am not a SeaBIOS developer.

I think it would be very cool if qemu required a coreboot payload for
startup instead of a complete firmware. Then SeaBIOS could focus only
on the coreboot model, and only on being an excellent BIOS service
provider.

But at some point the firmware/init part might be better placed in
coreboot, if the coreboot infrastructure is useful enough. I don't
know enough about "proper" qemu init to say if that's actually the
case though.


I think it's important to keep the coreboot model in mind, where
firmware and BIOS are very much distinct entities.

> You don't changed you HW when you update you BIOS in non
> virtualized world.

No, but you may not be able to get the very latest firmware either,
because of limitations in the hardware that the BIOS developers knew
about, and thus chose not to include in the updated BIOS.

The information required to make this choice comes from the hardware
department.

For qemu firmware (whether in qemu, coreboot or SeaBIOS) it means
that info about the hardware must be available in order to
selectively enable or implement stuff in the firmware and/or in the
BIOS.


> You do change you BIOS with each new HW though.

But that is not because of any technical reasons, only because the
BIOS developers keep one code base per hardware, and because the
hardware can not completely describe itself.

qemu could do that, and if firmware and BIOS service provider
understands the description then there's no reason to change firmware
nor BIOS just because the hardware changes. I think that would be
very elegant. :)


Kevin O'Connor wrote:
> > That depends on how you view seabios project. If you consider it to be
> > legacy bios functionality provider only then I agree and we should move
> > to coreboot model. If you consider it to be legacy bios + qemu firmware
> > (like old BOCHS bios was) then by definition it's seabios job to
> > describe underlying HW to an OS.
> 
> I don't think this is that "cut and dry".  A real machine just ships
> with these acpi tables compiled in.  This is what BOCHS bios did and
> it is what seabios did up until about 8 months ago.
> 
> However, qemu isn't a simple machine emulator - it can emulate a whole
> class of x86 machines.  It's not practical to compile a seabios.bin
> file for every permutation of x86 machine that qemu can emulate.  So,
> we pass info from qemu to seabios so that it can support all the
> classes of hardware.  This isn't what real machines do, and it's not
> what bochs bios did.
> 
> I do view SeaBIOS as primarily a legacy bios interface and a boot
> loader.  I also think it makes sense to handle qemu and kvm firmware
> needs - some initialization wants to be done from the guest and
> seabios is a good place to do that.

I don't care at all strongly about this, but when considering the
coreboot model, then then qemu firmware part seems to fit better in
the coreboot project. But again let me emphasize that I'm just as
happy with it being in SeaBIOS. :)


> This hpet thing is really rather minor, but it has me puzzled.  The
> guest OS wants the info in ACPI form, and only qemu has the info.  I
> don't understand why there is a desire to pass the info in this new
> arbitrary form instead of passing it in the form that the OS wants it
> in.

I guess the reason is to create a separate interface between hardware
and firmware. This is information that is normally communicated
out-of-band between hardware engineers and firmware engineers, and
now it wants to be done at runtime. I think this is a great
improvement, and maybe it can even benefit PC hardware universally. :)


//Peter
Avi Kivity - June 17, 2010, 3:58 a.m.
On 06/17/2010 04:47 AM, Kevin O'Connor wrote:
>
> BTW, it's been said several times now that ACPI is an interface
> between OS and firmware.  I don't see this at all - ACPI defines how
> the OS can interact with the hardware.  The only place I know of where
> seabios has involvement is with it's tiny (16 asm statement) SMI stub.
> Everything else describes the hardware and enables interactions
> directly with the hardware.
>    

In general, ACPI code can work with memory or device registers that have 
been initialized by the BIOS and depend on them.  It's possible to write 
ACPI code that depends on preceding BIOS code.  I don't know if that's 
the case with our ACPI implementation.
Gleb Natapov - June 17, 2010, 6:44 a.m.
On Wed, Jun 16, 2010 at 08:55:05PM -0400, Kevin O'Connor wrote:
> On Tue, Jun 15, 2010 at 07:41:02AM +0300, Avi Kivity wrote:
> > On 06/14/2010 09:25 PM, Kevin O'Connor wrote:
> > >This seems to be a philosophical distinction.  Lets go over the
> > >practical implications.
> > In my experience, well-defined interfaces ("philosophical
> > distinctions") are more important in the long term than
> > practicalities.
> 
> I agree with the importance of clean interfaces.  However, I feel the
> current approach to acpi table handling between qemu and seabios is
> not ideal.  I'm proposing an interface which I believe is an
> improvement.
> 
What is it? If you mean passing ACPI tables, then this is huge step
backwards. Qemu started like that and then Fabrice moved everything to
BIOS where it should be. Check these commit in qemu git:
2146e8389f267a5fb751106b0dfc6421808ccbd0
362ee297a6a977801758302c68b4ef5d1af76ab3
a0910fa4a5371c2cc70cb9f63ccc65b66decbb22
71b300df920a39db4368723765eed533f5fc209b

> > If a table needs to refer to some other information which is in a
> > table that is generated by seabios, we cannot generate this table
> > from qemu.  That's much worse that reviewing and applying two
> > patches.
> 
> I understand.  Such tables would not make sense to generate in qemu.
> 
So now we generate part of the tables in qemu and part in seabios. This
is not sane.

> I also understand and appreciate the desire for qemu to not touch
> guest memory.  This means rsdp and rsdt are the domain of seabios.
> The only other table that directly addresses the memory location of
> another table (that I know of) is fadt - which is also tied to
> seabios' smi handler - so this too is in seabios' domain.
> 
You see now you are starting to rationalize about which tables should
be generated by bios and which are not and the fact is that ACPI was
designed with assumption that firmware generates tables, so how can you
be sure that you will not find flaws in your rationalization later on?

> I'm not aware of any dependencies to seabios in any of the other
> tables (eg, madt, srat, hpet, ssdt, dsdt).
With certain HW configs firmware may need to configure hpet to function
in legacy mode (replacing RTC/PIT) so if seabios is unaware of hpet no
doesn't mean it will stay that way.

> 
> > >I'm not suggesting a radical rethink of fwcfg, but I fail to see the
> > >advantage in introducing the arbitrary "struct hpet_fw_entry" when
> > >there is a perfectly good, well defined, "struct acpi_20_hpet" that
> > >already exists.  This new arbitrary intermediate format just
> > >introduces "make work" for all of us.
> > 
> > Choosing an existing format is fine.  But seabios blindly copying
> > qemu provided data is wrong IMO.
> 
> Okay.  For "struct acpi_20_hpet", what transformations or checks do
> you think seabios needs to perform?
> 
Lets concentrate on principles and not hpet in particular. Just because
proposed interface is similar to how HPET ACPI table looks doesn't mean
we should build ACPI tables in qemu. I could have been lazy and pass
only "has hpet" flag to the seabios, but I tried to make interface
general to cover future qemu enhancements.

But lets get back to principles. Since qemu is not one platform but very
dynamic system and firmware is tightly coupled with the platform it runs
on and we do not want to have separate firmware for each possible
configuration the only solution is to make firmware to be dynamically
adoptable to platform it runs on. For that every bit of information
about underlying HW should be discoverable by firmware. Not all HW was
designed to be discoverable by software, so we need to create a channel
between qemu and firmware to pass information about otherwise undiscoverable
devices. Hpet is only one of such devices, so we need a way to pass platform
device tree into firmware. We can find generic way to do this for all
devices, or we may introduce separate interface for each device when
needed like propose patch does.

--
			Gleb.
Peter Stuge - June 17, 2010, 6:57 a.m.
Avi Kivity wrote:
> In general, ACPI code can work with memory or device registers that have 
> been initialized by the BIOS and depend on them.  It's possible to write 
> ACPI code that depends on preceding BIOS code.

It's also possible to write C code that makes extensive use of goto. :)

To be fair, ACPI bytecode for actual hardware may need to rely on the
firmware because of limitations in the hardware. But I think qemu is
one instance where any ACPI bytecode can be kept simple.


> I don't know if that's the case with our ACPI implementation.

Where is that code? Who has the ASL for qemu? Who wrote it? Is it big?


//Peter
Gleb Natapov - June 17, 2010, 7:45 a.m.
On Wed, Jun 16, 2010 at 09:22:09PM -0400, Kevin O'Connor wrote:
> On Tue, Jun 15, 2010 at 09:37:07AM +0300, Gleb Natapov wrote:
> > On Mon, Jun 14, 2010 at 04:12:32PM -0400, Kevin O'Connor wrote:
> > > But.. in order to move to a newer ACPI spec, there would be qemu
> > > changes anyway.  (If nothing else, so that qemu can tell seabios if
> > > it's okay to use the new rev.)  At that point we're stuck changing
> > > both repos anyway - nothing gained, nothing lost.
> > I don't see why qemu should care what ACPI rev Seabios uses.
> 
> A change in ACPI rev would likely break old OSs.  Only the user would
In that case new ACPI would never be adopted. No HW manufacturer would
risk to not be able to run WindowsXP on their HW. Real BIOS may have
config option to choose what ACPI version to use though. We can add this
too.

> know this, and so a method of propagating that info from qemu to
> seabios would be needed.  (However, it's much more likely that a new
> ACPI rev would require more data which qemu would then also need to
> pass to seabios.)
Why do you think so? But anyway my position is that we need to pass
maximum information from qemu to firmware. On real HW bios knows
everything about underlying hardware.


> 
> > > I still think there is an opportunity to reduce the load on the bulk
> > > of acpi changes - most of these changes have no dependence on seabios
> > > at all.
> > That depends on how you view seabios project. If you consider it to be
> > legacy bios functionality provider only then I agree and we should move
> > to coreboot model. If you consider it to be legacy bios + qemu firmware
> > (like old BOCHS bios was) then by definition it's seabios job to
> > describe underlying HW to an OS.
> 
> I don't think this is that "cut and dry".  A real machine just ships
> with these acpi tables compiled in.  This is what BOCHS bios did and
> it is what seabios did up until about 8 months ago.
That was because qemu was stale project for a couple of years. Now pace of
qemu development is very fast, so the same is required from firmware
too. When qemu development started to accelerate BOCHS bios was
essentially forked to allow for faster development.

> 
> However, qemu isn't a simple machine emulator - it can emulate a whole
> class of x86 machines.  It's not practical to compile a seabios.bin
> file for every permutation of x86 machine that qemu can emulate.  So,
> we pass info from qemu to seabios so that it can support all the
> classes of hardware.  This isn't what real machines do, and it's not
> what bochs bios did.
BOCHS bios didn't do it because when qemu development accelerated we
switched to seabios. I agree with paragraph above otherwise. We just not
agree in what form information should be passed. You think we should
pass HPET ACPI table (my guess is just because we already have a way to
pass ACPI table to seabios) and I think this is abuse of ACPI spec. fw cfg
interface was designed to be extendible, why oppose adding things to it?
It is not like if we build HPET table in qemu we will not have to patch
seabios and coordinate changes. Seabios creates HPET table
unconditionally now and we will have to fix it to not do that if HPET
table is passed from qemu (and for that seabios will have to expect all tables
that it receives over fw cfg interface, something it doesn't do now) and
it will have to detect old qemu somehow and create HPET unconditionally
to preserve old behaviour on old qemus. In the end the change to seabios
will be bigger that proposed patch.

> 
> I do view SeaBIOS as primarily a legacy bios interface and a boot
> loader.  
This is worrying statement for qemu project.

>          I also think it makes sense to handle qemu and kvm firmware
> needs - 
Good, but qemu needs are growing in the pace of qemu development and
this is fast these days.

>          some initialization wants to be done from the guest and
> seabios is a good place to do that.
> 
HW does not initialize itself. So the only sane place to do _all_
initialization is from guest.

> This hpet thing is really rather minor, but it has me puzzled.  The
> guest OS wants the info in ACPI form, and only qemu has the info.  I
> don't understand why there is a desire to pass the info in this new
> arbitrary form instead of passing it in the form that the OS wants it
> in.
Because OS does not talk directly to qemu. It has mediator in the form
of seabios. We have spec that define interface between seabios and an OS
(ACPI spec) and we define interface between seabios and qemu by ourselves.
Why intentionally blur this separation?

> 
> A couple of emails back you stated you considered using the existing
> qemu_cfg_acpi_additional_tables() format but dismissed the idea.
> Maybe you could explain why you dismissed it and/or what the
> deficiencies of this mechanism are?
> 
I dismissed it (very quickly) on the premiss that this is layering
violation. I saw that I need to specify value that qemu should have
nothing to do with to build header and to support old qemu with new
seabios I need to add new fw cfg key anyway.

--
			Gleb.

Patch

diff --git a/src/acpi.c b/src/acpi.c
index 0559443..864f1a8 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -469,7 +469,7 @@  build_ssdt(void)
 
 #define HPET_SIGNATURE 0x54455048 //HPET
 static void*
-build_hpet(void)
+build_hpet(struct hpet_fw_entry *e, u8 id)
 {
     struct acpi_20_hpet *hpet = malloc_high(sizeof(*hpet));
     if (!hpet) {
@@ -478,11 +478,11 @@  build_hpet(void)
     }
 
     memset(hpet, 0, sizeof(*hpet));
-    /* Note timer_block_id value must be kept in sync with value advertised by
-     * emulated hpet
-     */
-    hpet->timer_block_id = cpu_to_le32(0x8086a201);
-    hpet->addr.address = cpu_to_le32(ACPI_HPET_ADDRESS);
+    hpet->timer_block_id = cpu_to_le32(e->event_timer_block_id);
+    hpet->addr.address = cpu_to_le32(e->address);
+    hpet->min_tick = cpu_to_le32(e->min_tick);
+    hpet->hpet_number = id;
+    hpet->page_protect = e->page_prot;
     build_header((void*)hpet, HPET_SIGNATURE, sizeof(*hpet), 1);
 
     return hpet;
@@ -637,9 +637,27 @@  acpi_bios_init(void)
     ACPI_INIT_TABLE(build_fadt(bdf));
     ACPI_INIT_TABLE(build_ssdt());
     ACPI_INIT_TABLE(build_madt());
-    ACPI_INIT_TABLE(build_hpet());
     ACPI_INIT_TABLE(build_srat());
 
+    u8 hpet_id, c =  qemu_cfg_hpet_entries();
+    struct hpet_fw_entry e;
+
+    if (c == ~0) {
+        /* qemu do not provide hpet description */
+        e.event_timer_block_id = 0x8086a201;
+        e.address = ACPI_HPET_ADDRESS;
+        e.min_tick = 0;
+        c = 1;
+    } else if (c != 0)
+        qemu_cfg_hpet_load_next(&e);
+
+    while (c--) {
+        ACPI_INIT_TABLE(build_hpet(&e, hpet_id++));
+        if (c)
+            qemu_cfg_hpet_load_next(&e);
+    }
+
+
     u16 i, external_tables = qemu_cfg_acpi_additional_tables();
 
     for(i = 0; i < external_tables; i++) {
diff --git a/src/paravirt.c b/src/paravirt.c
index 5c77b5c..458ab08 100644
--- a/src/paravirt.c
+++ b/src/paravirt.c
@@ -149,6 +149,23 @@  void* qemu_cfg_e820_load_next(void *addr)
     return addr;
 }
 
+u8 qemu_cfg_hpet_entries(void)
+{
+    u8 cnt;
+
+    if (!qemu_cfg_present)
+        return 0;
+
+    /* read valid flags */
+    qemu_cfg_read_entry(&cnt, QEMU_CFG_HPET, sizeof(cnt));
+    return cnt;
+}
+
+void qemu_cfg_hpet_load_next(struct hpet_fw_entry *e)
+{
+    qemu_cfg_read((u8*)e, sizeof(*e));
+}
+
 struct smbios_header {
     u16 length;
     u8 type;
diff --git a/src/paravirt.h b/src/paravirt.h
index c46418f..272af81 100644
--- a/src/paravirt.h
+++ b/src/paravirt.h
@@ -37,6 +37,7 @@  static inline int kvm_para_available(void)
 #define QEMU_CFG_SMBIOS_ENTRIES		(QEMU_CFG_ARCH_LOCAL + 1)
 #define QEMU_CFG_IRQ0_OVERRIDE		(QEMU_CFG_ARCH_LOCAL + 2)
 #define QEMU_CFG_E820_TABLE		(QEMU_CFG_ARCH_LOCAL + 3)
+#define QEMU_CFG_HPET			(QEMU_CFG_ARCH_LOCAL + 4)
 
 extern int qemu_cfg_present;
 
@@ -68,10 +69,20 @@  struct e820_reservation {
     u32 type;
 };
 
+struct hpet_fw_entry
+{
+    u32 event_timer_block_id;
+    u64 address;
+    u16 min_tick;
+    u8 page_prot;
+} __attribute__ ((packed));
+
 u16 qemu_cfg_first_file(QemuCfgFile *entry);
 u16 qemu_cfg_next_file(QemuCfgFile *entry);
 u32 qemu_cfg_read_file(QemuCfgFile *entry, void *dst, u32 maxlen);
 u32 qemu_cfg_e820_entries(void);
 void* qemu_cfg_e820_load_next(void *addr);
+u8 qemu_cfg_hpet_entries(void);
+void qemu_cfg_hpet_load_next(struct hpet_fw_entry *e);
 
 #endif