[Qemu-devel] Re: [SeaBIOS] [PATCHv2] load hpet info for HPET ACPI table from qemu

[Gleb Natapov]

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.