On Fri, Apr 26, 2013 at 01:13:28PM +0200, Laszlo Ersek wrote:
> On 04/25/13 21:03, Anthony Liguori wrote:
> > Laszlo Ersek <ler...@redhat.com> writes:
> >
> >> This patch reuses some code from SeaBIOS, which was originally under
> >> LGPLv2 and then relicensed to GPLv3 or LGPLv3, in QEMU under GPLv2+. This
> >> relicensing has been acked by all contributors that had contributed to the
> >> code since the v2->v3 relicense. ACKs approving the v2+ relicensing are
> >> listed below. The list might include ACKs from people not holding
> >> copyright on any parts of the reused code, but it's better to err on the
> >> side of caution and include them.
> >>
> >> Affected SeaBIOS files (GPLv2+ license headers added)
> >> <http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/5949>:
> >>
> >> src/acpi-dsdt-cpu-hotplug.dsl | 15 +++++++++++++++
> >> src/acpi-dsdt-dbug.dsl | 15 +++++++++++++++
> >> src/acpi-dsdt-hpet.dsl | 15 +++++++++++++++
> >> src/acpi-dsdt-isa.dsl | 15 +++++++++++++++
> >> src/acpi-dsdt-pci-crs.dsl | 15 +++++++++++++++
> >> src/acpi.c | 14 +++++++++++++-
> >> src/acpi.h | 14 ++++++++++++++
> >> src/ssdt-misc.dsl | 15 +++++++++++++++
> >> src/ssdt-pcihp.dsl | 15 +++++++++++++++
> >> src/ssdt-proc.dsl | 15 +++++++++++++++
> >> tools/acpi_extract.py | 13 ++++++++++++-
> >> tools/acpi_extract_preprocess.py | 13 ++++++++++++-
> >> 12 files changed, 171 insertions(+), 3 deletions(-)
> >>
> >> Each one of the listed people agreed to the following:
> >>
> >>> If you allow the use of your contribution in QEMU under the
> >>> terms of GPLv2 or later as proposed by this patch,
> >>> please respond to this mail including the line:
> >>>
> >>> Acked-by: Name <email address>
> >>
> >> Acked-by: Gerd Hoffmann <kra...@redhat.com>
> >> Acked-by: Jan Kiszka <jan.kis...@siemens.com>
> >> Acked-by: Jason Baron <jba...@akamai.com>
> >> Acked-by: David Woodhouse <david.woodho...@intel.com>
> >> Acked-by: Gleb Natapov <g...@redhat.com>
> >> Acked-by: Marcelo Tosatti <mtosa...@redhat.com>
> >> Acked-by: Dave Frodin <dave.fro...@se-eng.com>
> >> Acked-by: Paolo Bonzini <pbonz...@redhat.com>
> >> Acked-by: Kevin O'Connor <ke...@koconnor.net>
> >> Acked-by: Laszlo Ersek <ler...@redhat.com>
> >> Acked-by: Kenji Kaneshige <kaneshige.ke...@jp.fujitsu.com>
> >> Acked-by: Isaku Yamahata <yamah...@valinux.co.jp>
> >> Acked-by: Magnus Christensson <magnus.christens...@intel.com>
> >> Acked-by: Hu Tao <hu...@cn.fujitsu.com>
> >> Acked-by: Eduardo Habkost <ehabk...@redhat.com>
> >>
> >> The patch incorporates ideas/suggestions from Michael Tsirkin's prototype
> >> code:
> >> - "hw/i386/pc.c" is too big, create new file "hw/i386/acpi.c" with
> >> i386-specific ACPI table stuff,
> >> - separate preparation of individual tables from their installation as
> >> fw_cfg files,
> >> - install these fw_cfg files inside pc_memory_init(), which is shared by
> >> piix4/q35,
> >> - add the above licensing-related block to the commit message.
> >>
> >> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> >> ---
> >> configure | 12 ++++
> >> hw/i386/Makefile.objs | 1 +
> >> hw/i386/acpi.h | 9 +++
> >> hw/i386/acpi.c | 159
> >> +++++++++++++++++++++++++++++++++++++++++++++++++
> >> hw/i386/pc.c | 23 +++++++
> >> 5 files changed, 204 insertions(+), 0 deletions(-)
> >> create mode 100644 hw/i386/acpi.h
> >> create mode 100644 hw/i386/acpi.c
> >>
> >> diff --git a/configure b/configure
> >> index ed49f91..45a5f55 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -241,6 +241,7 @@ gtk=""
> >> gtkabi="2.0"
> >> tpm="no"
> >> libssh2=""
> >> +dynamic_acpi="no"
> >>
> >> # parse CC options first
> >> for opt do
> >> @@ -928,6 +929,10 @@ for opt do
> >> ;;
> >> --enable-libssh2) libssh2="yes"
> >> ;;
> >> + --disable-dynamic-acpi) dynamic_acpi="no"
> >> + ;;
> >> + --enable-dynamic-acpi) dynamic_acpi="yes"
> >> + ;;
> >> *) echo "ERROR: unknown option $opt"; show_help="yes"
> >> ;;
> >> esac
> >> @@ -1195,6 +1200,8 @@ echo " --gcov=GCOV use specified gcov
> >> [$gcov_tool]"
> >> echo " --enable-tpm enable TPM support"
> >> echo " --disable-libssh2 disable ssh block device support"
> >> echo " --enable-libssh2 enable ssh block device support"
> >> +echo " --disable-dynamic-acpi disable dynamic ACPI table generation
> >> (default)"
> >> +echo " --enable-dynamic-acpi enable dynamic ACPI table generation
> >> (work in progress)"
> >> echo ""
> >> echo "NOTE: The object files are built at the place where configure is
> >> launched"
> >> exit 1
> >> @@ -3573,6 +3580,7 @@ echo "gcov enabled $gcov"
> >> echo "TPM support $tpm"
> >> echo "libssh2 support $libssh2"
> >> echo "TPM passthrough $tpm_passthrough"
> >> +echo "dynamic ACPI tables $dynamic_acpi"
> >>
> >> if test "$sdl_too_old" = "yes"; then
> >> echo "-> Your SDL version is too old - please upgrade to have SDL support"
> >> @@ -3958,6 +3966,10 @@ if test "$virtio_blk_data_plane" = "yes" ; then
> >> echo 'CONFIG_VIRTIO_BLK_DATA_PLANE=$(CONFIG_VIRTIO)' >> $config_host_mak
> >> fi
> >>
> >> +if test "$dynamic_acpi" = "yes"; then
> >> + echo "CONFIG_DYN_ACPI=y" >> $config_host_mak
> >> +fi
> >> +
> >> # USB host support
> >> case "$usb" in
> >> linux)
> >
> > No reason for this to be a configure option.
>
> :-/
>
> I added this from v3 to v4 because Michael asked me for it
> <http://thread.gmane.org/gmane.comp.emulators.qemu/206435/focus=207146>.
>
> >From the SeaBIOS side, I believe Kevin O'Connor also wants to keep out
> related code from SeaBIOS until a full set of tables is passed. I
> disagree with flipping a big switch in the end (ie. I agree a config
> option (let alone a separate SeaBIOS branch) is unwarranted, which is
> why I didn't add the former in v3), but I have no say in it.
>
> Do you want me to rip out these hunks (and adapt the dependencies)?
Essentially, what seabios wants to do is operate in two
modes:
- (mostly) use builtin acpi tables
- use acpi tables from hypervisor
in particular, seabios wants to interpret presence
of any file in etc/acpi as a signal not to generate
its own tables.
So merging this patch but without the config option will break
this plan. The only two ways I see are:
- merge this last patch with the config option, disabled by default
the idea being we can improve it in-tree, gradually.
- keep this patch out of tree until we have a complete
set of tables.
Both are fine with me.
> >> +static void acpi_table_fill_hdr(AcpiTableStdHdr *std_hdr, size_t
> >> blob_size,
> >> + const char *sig)
> >> +{
> >> + g_assert(blob_size >= sizeof *std_hdr);
> >> +
> >> + *std_hdr = acpi_dfl_hdr;
> >> + strncpy(std_hdr->sig, sig, sizeof std_hdr->sig);
> >
> > Should use () with sizeof and avoid strncpy. It almost never has the
> > behavior you want with respect to NULL termination (unless you
> > explicitly want to not NULL terminate when hitting bufsize).
>
> Correct, I explicitly wanted to avoid NUL-termination here. The
> destination ACPI fields are not NUL-terminated but fixed size. The
> caller specifies a C string. The bytes not covered by that string in the
> ACPI field, ie. coming from the default header, should be overwritten by
> NULs.
>
>
> >> +#ifdef CONFIG_DYN_ACPI
> >> +static void pc_acpi_madt(FWCfgState *fw_cfg)
> >> +{
> >> + unsigned num_lapic;
> >> + char unsigned *blob;
> >> + size_t blob_size;
> >> +
> >> + /* see note on FW_CFG_MAX_CPUS in bochs_bios_init() */
> >> + num_lapic = pc_apic_id_limit(max_cpus);
> >> +
> >> + acpi_build_madt(&blob, &blob_size, num_lapic);
> >
> > I'd be a lot happier if we were passing more information to this routine
> > and not hard coding it. For instance, the PCI interrupt assignments,
> > the APIC ids, the number of available CPUs, etc.
>
> I can try to extract all dependencies as parameters, but I think it will
> lead us down an unpleasant path. In my understanding the migration of
> ACPI tables from boot firmware(s) to qemu is *also* motivated that ACPI
> tables are very quirky ^W flexible, and passing down every bit of info
> to build them is (a) a chore, because there are many parameters erecting
> the whole bunch of tables, (b) the set of parameters is a constantly
> moving target, as tables are added or their ACPI versions are upgraded
> (eg. from ACPI 1.0 to ACPI 2.0).
>
> Hence my thinking about this went as the polar opposite of yours. In
> these ACPI preparation functions, where we're composing a "portable"
> description of the machine, we're fishing from a global pool of
> information. Nothing should be off limits. Just as well as I can call
> kvm_allows_irq0_override(), I should be able to access whatever global
> data and functions, without explicitly specifying in a function
> prototype what I need. I need *everything* -- that's (also) why we're
> doing the tables in QEMU. Because everything is accessible there without
> jumping through hoops.
>
> There's nothing regular in the kinds of information stored in various
> ACPI tables; they are arbitrary. A function prototype according to your
> suggestion would be justified by nothing more than "well, that's what's
> required for the MADT", and if we bump an ACPI version (maybe not for
> the MADT but for another table), we'll have to adapt the prototype and
> the caller too.
>
> (I already dislike the "num_lapic" parameter; IMO the MADT function
> should directly call x86_cpu_apic_id_from_index() with both max_cpus and
> smp_cpus, or *maybe* export and call pc_apic_id_limit() for the former.)
>
> Of course this is just my two cents.
>
> > The commit message also doesn't provide any reason about why we would
> > want this. The cover letter provides a reference at least but cover
> > letters don't end up in git history.
>
> Well I'll need help wording that. From my perspective there are two goals:
> - primary goal: stop OVMF chasing SeaBIOS's tail in ACPI features --
> prepare the ACPI tables in QEMU and let whatever boot firmware use the
> same set of tables,
> - secondary goal: stop bumping into fw_cfg boundaries when needing new
> ACPI dependencies in boot firmware (see above).
>
> I believe that Michael is interested in the ACPI move because of a new
> chipset he's introducing (or some such, please excuse my ignorance).
>
> Thanks!
> Laszlo
Yes, and hotplug for PCI bridge as well.
--
MST
