| Message ID | 20100614083053.GC21797@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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).
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.
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
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.
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
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
> > >>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
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.
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).
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.
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.
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
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
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
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
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.
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.
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
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.
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
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.