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)? >> +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
