On 05/21/14 14:40, Michael S. Tsirkin wrote:
> On Wed, May 21, 2014 at 01:21:11PM +0200, Laszlo Ersek wrote:
>> On 05/21/14 12:30, Michael S. Tsirkin wrote:
>>> On Wed, May 21, 2014 at 12:16:47PM +0200, Laszlo Ersek wrote:
>>>> On 05/21/14 10:19, Michael S. Tsirkin wrote:
>>>>> On Wed, May 21, 2014 at 12:51:47AM +0200, Laszlo Ersek wrote:
>>>>>> In one of the next patches we'll start scanning all fw_cfg files that
>>>>>> QEMU
>>>>>> advertises as carrying ACPI tables, not just "etc/acpi/tables".
>>>>>>
>>>>>> The RSD PTR table is known to occur in the "etc/acpi/rsdp" fw_cfg file.
>>>>>> Since edk2 handles RSD PTR automatically, similarly to RSDT and XSDT,
>>>>>> let's exclude RSD PTR too from the manually installed tables.
>>>>>>
>>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>>>>>
>>>>> Please don't rely on this name, there's a better way:
>>>>> check the zone flag, anything that does not set it
>>>>> to 0x1 is not an ACPI table.
>>>>> In particular, RSDP is in ZONE_FSEG = 0x2.
>>>>>
>>>>> You can then safely ignore it without bothering parsing.
>>>>
>>>> (Your messages reached edk2-devel after all!)
>>>>
>>>> - I don't use "etc/acpi/rsdp" in the source; the commit message mentions
>>>> it as illustration. The patchset makes the OVMF code independent of
>>>> individual "etc/acpi/*" names.
>>>
>>> Good I missed this.
>>>
>>>> - I noticed that the fw_cfg file containing the RSDP differs from the
>>>> rest of the files in the Zone allocation hint, but I didn't want to
>>>> directly associate that hint with the RSDP's presence. I'd prefer to
>>>> leave the patchset as-is, unless Jordan insists that we should filter
>>>> out the RSDP by looking at the Zone hint (rather than checking
>>>> signatures in the payload).
>>>>
>>>> Thanks
>>>> Laszlo
>>>
>>> I think the way you locate the start of tables is not guaranteed to work,
>>> because QEMU can place padding between the tables.
>>
>> Trailing zero pad at the end of the fw_cfg files (not at the end of each
>> ACPI table within an fw_cfg file) is already handled. Padding between
>> tables within the same fw_cfg file has not been sighted yet. If qemu
>> doesn't actively do that right now, I'd prefer to adapt the parsing when
>> it becomes necessary.
>
> Well it will be too late if someone ships this firmware, we
> will have to stick to what we happen to do now.
I still don't understand this. What do you mean by "if someone ships
this firmware"? They better make sure that the qemu they *also* ship can
talk to it. Emulator and boot firmware is usually shipped in lock-step.
Unless there's an official spec like the virtio spec that both sides can
refer to independently.
>>> You are looking at the "allocate" commands but you really
>>> should look for "pointer" commands.
>>> That gives you start of each table.
>>
>> I don't see how. The pointer command incorporates the following
>> instructions:
>> (1) Here's an fw_cfg file name, let's call it PointerFile.
>> (2) At offset PointerOffset in this fw_cfg *file*,
>
> Let's call this file SourceFile
>
>> for a length of
>> PointerSize, there's a pointer field. This field belongs to one of the
>> *tables* contained in PointerFile.
>> (3) Here's another fw_cfg file name, let's call it PointeeFile.
>> (4) After you have installed PointerFile as a *whole blob* in guest RAM,
>> and after you have installed PointeeFile as a *whole blob* in guest RAM,
>> do the following:
>>
>> increment(guest_phys_addr(PointerFile) + PointerOffset,
>> PointerSize,
>> guest_phys_addr(PointeeFile));
>>
>> In other words, increment the pointer field (size-correctly) in the
>> RAM-installed instance of PointerFile, at PointerOffset, for PointerSize
>> bytes, with the base address of the RAM-installed instance of PointeeFile.
>>
>> Instructions (1) to (4) don't speak about *tables*. The pointer to
>> increment may fall in any one of the several tables contained in (and
>> installed from) PointerFile.
>
> Yes but in practice ACPI spec does not have pointers to
> middle of the table. It is always to beginning of the table.
>
>> The steps depend on PointerFile having been
>> installed as a whole blob.
>
> but we reword it like this:
>
> - read value (sized correctly) at PointerOffset in SourceFile
> - this value is the offset in PointerFile to
(offset in PointeeFile (as in, pointed-to-file))
> start of table
Yes, that's how I understood it.
>
> put like this it's pretty clear how to use that to
> find start of each table, right?
Yes, the algorithm is clear; I tried to outline it below:
[snip]
>> Are you suggesting the following?
>> - Execute all Allocate commands (they are guaranteed to come first),
>> and build a dictionary from fw_cfg file name to fw_cfg file contents
>> (basically, base address of temporary copy, and size).
>> dict: fw_cfg_name -> {base, size}
>>
>> - for each pointer command
>> {PointerFile, PointeeFile, PointerOffset, PointerSize}:
>> - memcpy(&relativeOffset,
>> dict(PointerFile).base + PointerOffset,
>> PointerSize);
>> - (dict(PointeeFile).base + relativeOffset) points to some ACPI table,
>> simply by virtue of some pointer field pointing at it
I just find its implementation too costly in practice, for little actual
benefit.
>> That seems doable of course in theory, but also much more complicated in
>> practice than what we have now:
>> - ACPI table header validation needs to be done the same, just at
>> different addresses
>> - allocate commands must be traversed just the same
>> - fw_cfg_name -> (base, size) dictionary needs to be built in addition
>> - pointer commands need to be parsed in addition
>>
>> Finally, what if there's a linked object that is not an ACPI table (ie.
>> a pointer points at it, but the target doesn't start with an ACPI table
>> header)?
>>
>> One example is the BGRT (5.2.22 Boot Graphics Resource Table). The Image
>> Address field points to an image bitmap, not an ACPI table. The current
>> code will reject it of course (and roll back all individual ACPI tables
>> installed until then), but the proposed change won't solve it either
>> (despite the additional complexity).
>
> Hmm true, but with your approach it's not solvable at all, is it?
Correct.
> Using pointer field at least will make it solvable:
> you need to know that source is in BGRT which you
> can figure out when you install it.
This means that beyond realizing the BGRT is "some ACPI table" and
installing it, the parser would have to incorporate knowledge about
BGRT. I might prefer simply failing.
>
>> If I understand the proposal correctly, then I wouldn't like to
>> implement it; it seems to future-proof the code mostly against
>> gratuitous / frivolous changes in the wire format, at a quite high price
>> to me personally.
>
> I think it is a good idea to use BGRT as a litmus test
> for which parts of interface to use.
(Oops!)
>
>>
>> We have no natural-language spec for the interface, and the qemu source
>> doesn't seem to point out explicitly what is considered stable and what
>> is not. What's contract and what's an implementation detail is anyone's
>> best guess therefore. I'd like to avoid several proactive iterations of
>> fixing up the OVMF-side client code, in the absence of actual need. I
>> don't intend to tie the QEMU-side generator in a bind -- if you have a
>> good reason to change the format, go ahead, and let OVMF break. I'll try
>> to fix it then. (This has happened before several times, eg. with the
>> bootorder fw_cfg file acquiring the HALT keyword, and libvirt starting
>> to specify it unconditionally, happily breaking the OVMF parser.)
>>
>> Thanks
>> Laszlo
>
> Yes but when e.g. qemu starts to actually ship this, this will become a
> problem, won't it?
I never considered qemu actually *bundling* OVMF, like it bundles
SeaBIOS. But it's great that you mention it, because it highlights a big
difference between the SeaBIOS and OVMF developer communities, and how
they relate to the qemu developer base.
The qemu developer community is huge.
The SeaBIOS community is appropriately sized I guess, considering the
size of the project.
The above two communities overlap; the same people regularly write (and
sometimes design) both sides of a feature or another.
OVMF depends on an incredible amount of generic edk2 code. So while
OvmfPkg itself might not be very big, the stuff that goes into OVMF.fd
is huge. There's only a handful of OVMF contributors, and OVMF keeps
playing tag with qemu and SeaBIOS.
This disparity in developer resources is a much graver problem than
interface breakage between qemu and OVMF. Your mentioning qemu
potentially bundling OVMF is useful exactly for this reason -- once that
happens, qemu developers will be *forced* to contribute to OVMF, same as
they contribute now to SeaBIOS.
I'll be very happy if someone implements the OVMF/QEMU ACPI client code
in a way that matches your taste. (Same as I was very happy to see that
Gabriel did all three sides of the SMBIOS stuff.)
I'm unable to implement OVMF client code according to your taste for a
spec that is handed down to me after being designed with only Qemu and
SeaBIOS in mind.
I was obviously not excluded from the design of the wire format. My
answer is (same as I said to Gabriel when he worked with Kevin O'Connor
et al on SMBIOS) -- I usually don't even have time to attend to the
*design* of the protocol. I can't just throw aside everything I'm bogged
down in whenever the next protocol design comes up. Best I can try is
pay some attention to the emails and shoot out warnings. (I managed that
a few times in the SMBIOS case IIRC.)
> That's why my idea was to make OVMF do mostly what seabios does.
> Yes, it has a table linking fw cfg to files, this is
> used to execute the linker commands.
>
> If OVMF is still at a point where breaking it in minor ways
> is acceptable, then I'm fine with this patchset for now.
Thanks.
As far as my capacity is concerned, it's either this patchset (with
minor fixups if reviewers request them) or what we have in-tree now.
Thanks,
Laszlo
------------------------------------------------------------------------------
"Accelerate Dev Cycles with Automated Cross-Browser Testing - For FREE
Instantly run your Selenium tests across 300+ browser/OS combos.
Get unparalleled scalability from the best Selenium testing platform available
Simple to use. Nothing to install. Get started now for free."
http://p.sf.net/sfu/SauceLabs
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
