Re: [Qemu-devel] [PATCH 0/3] acpi: more infrastructure cleanups

[Laszlo Ersek]

On 04/15/13 13:25, Michael S. Tsirkin wrote:
> On Mon, Apr 15, 2013 at 12:37:37PM +0300, Michael S. Tsirkin wrote:
>> I wanted to help Laszlo completely move ACPI tables out of seabios in
>> time for 1.5, but had to put out some fires so didn't manage to do this
>> in time, and going offline for now.
>>
>> Meanwhile, here is some refactoring that I did complete.  This does not
>> do much by itself, but works fine for me and will be helpful down the
>> road.
>>
>> This includes a couple of patches I posted previously
>> and another one on top.
>>
>> Pls consider for 1.5.
> 
> Just two clarifications
> 1. This conflicts with Laszlo's MADT change. As such if Laszlo's patch is 
> applied,
>    this will need to be rebased on top.
> 2. This was lightly tested but there's a holiday here so I run out of
>    time for testing. Will report in a couple of days.

First, thank you very much for the help!

Second, 2/3 breaks the binary format exported under the
FW_CFG_ACPI_TABLES key. The format is as follows:

  * number of tables in entire fw_cfg blob : uint16_t
  for each table:
    * ACPI payload size belonging to this table : uint16_t
    * standard ACPI header
    * ACPI table contents

whereas 2/3 changes the format to:

  * zero-filled uint16_t (i)
  for each table:
    * uint16_t storing value 1 in LE order (ii)
    * number of bytes in blob segment belonging to this table : uint16_t
    * standard ACPI header
    * ACPI table contents

(ii) shouldn't exist in per-table storage, the increments should target
(i) actually.


Another note: 2/3 introduces two ways to spell "has_header" in the
leading comments. The first use is "&&address@hidden" (correct with @, but
whitespace missing). The second use is "&& !has_header" (from my last
series, also see "bloblen"), which has the whitespace OK, but misses the
at-sign @ customary in comments when referring to function paramteres.


Finally, regarding the purpose of this patch. It seems to be to prepend
a standard ACPI header to a "headerles" table.

I think we can do without the reallocation. When preparing any ACPI
table, we know in advance the size of the ACPI header we're going to
need. So we can take it into account when allocating the buffer: fill in
the table body, and then call a function that puts the header at front,
into existing storage. See pc_acpi_install() in "[Qemu-devel] [qemu
PATCH 5/5] i386/pc: build ACPI MADT (APIC) for fw_cfg clients".

I propose the following:

(1) I'll resend my "[qemu PATCH 0/5] publish etc/acpi/APIC in fw_cfg"
series from last Monday, rebased to current (or then) master, with the
following changes suggested by Michael in private:
(1a) separating some stuff out into different files,
(1b) adding licensing bits related to SeaBIOS,
(1c) I'll try to consider 1/3 and 3/3 in this series that Paolo has
already picked up (may not match or be useful any longer),

(2) Then, please review / apply that updated series of mine,

(3) then please rebase 2/3 from here on top of (2). Again, I suggest
that acpi_table_add_hdr(), if any, fill in preallocated space; this is
consistent with updating the checksum too.

(4) I *do* anticipate that my code in (2) is not / will not be flexible
enough for all tables. However, please split up functions / extract bits
/ reorganize them only in a series that puts those new extracted
functions to use *at once* (probably near the end of said series). I
can't reason about "librarization" without actual use.

I'll wait for Michael's answer before doing anything.

Thanks
Laszlo