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

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Apr 26, 2013 at 01:13:28PM +0200, Laszlo Ersek wrote:
> On 04/25/13 21:03, Anthony Liguori wrote:
> > Laszlo Ersek <lersek@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 <kraxel@redhat.com>
> >>   Acked-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>   Acked-by: Jason Baron <jbaron@akamai.com>
> >>   Acked-by: David Woodhouse <David.Woodhouse@intel.com>
> >>   Acked-by: Gleb Natapov <gleb@redhat.com>
> >>   Acked-by: Marcelo Tosatti <mtosatti@redhat.com>
> >>   Acked-by: Dave Frodin <dave.frodin@se-eng.com>
> >>   Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> >>   Acked-by: Kevin O'Connor <kevin@koconnor.net>
> >>   Acked-by: Laszlo Ersek <lersek@redhat.com>
> >>   Acked-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
> >>   Acked-by: Isaku Yamahata <yamahata@valinux.co.jp>
> >>   Acked-by: Magnus Christensson <magnus.christensson@intel.com>
> >>   Acked-by: Hu Tao <hutao@cn.fujitsu.com>
> >>   Acked-by: Eduardo Habkost <ehabkost@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 <lersek@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