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

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

On Mon, Apr 15, 2013 at 03:24:00PM +0200, Laszlo Ersek wrote:
> 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 "&&!@has_header" (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

I'm fine with this. FYI, I'm offline for the rest of today and
tomorrow.

-- 
MST