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.
> 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*, 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. The steps depend on PointerFile having been
installed as a whole blob.
The edk2 interface (EFI_ACPI_TABLE_PROTOCOL) only works with individual
tables. That's the reason I need to parse them out of the fw_cfg files.
The installed versions of the tables are guaranteed to be discontiguous
in guest RAM (guest-phys address space).
> RSDP specifically is not pointed to by anything, so if you
> use it this way, it will be skipped automatically for free,
> and there would be no need to look for signatures.
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
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).
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.
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
------------------------------------------------------------------------------
"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
