qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_c


From: Laszlo Ersek
Subject: Re: [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients
Date: Fri, 26 Apr 2013 13:13:28 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130329 Thunderbird/17.0.5

On 04/25/13 21:03, Anthony Liguori wrote:
> Laszlo Ersek <address@hidden> 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 <address@hidden>
>>   Acked-by: Jan Kiszka <address@hidden>
>>   Acked-by: Jason Baron <address@hidden>
>>   Acked-by: David Woodhouse <address@hidden>
>>   Acked-by: Gleb Natapov <address@hidden>
>>   Acked-by: Marcelo Tosatti <address@hidden>
>>   Acked-by: Dave Frodin <address@hidden>
>>   Acked-by: Paolo Bonzini <address@hidden>
>>   Acked-by: Kevin O'Connor <address@hidden>
>>   Acked-by: Laszlo Ersek <address@hidden>
>>   Acked-by: Kenji Kaneshige <address@hidden>
>>   Acked-by: Isaku Yamahata <address@hidden>
>>   Acked-by: Magnus Christensson <address@hidden>
>>   Acked-by: Hu Tao <address@hidden>
>>   Acked-by: Eduardo Habkost <address@hidden>
>>
>> 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 <address@hidden>
>> ---
>>  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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]