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

[Avi Kivity]

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.

--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.