qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v5 05/10] ACPI: Add Virtual Machine Generation I


From: Ben Warren
Subject: Re: [Qemu-devel] [PATCH v5 05/10] ACPI: Add Virtual Machine Generation ID support
Date: Thu, 9 Feb 2017 12:39:30 -0800

> On Feb 9, 2017, at 12:24 PM, Laszlo Ersek <address@hidden> wrote:
> 
> On 02/09/17 21:02, Ben Warren wrote:
>> 
>>> On Feb 9, 2017, at 11:27 AM, Laszlo Ersek <address@hidden
>>> <mailto:address@hidden>> wrote:
>>> 
>>> On 02/09/17 18:23, Igor Mammedov wrote:
>>>> On Wed, 8 Feb 2017 01:48:42 +0100
>>>> Laszlo Ersek <address@hidden <mailto:address@hidden>> wrote:
>>>> 
>>>>> On 02/05/17 10:12, address@hidden
>>>>> <mailto:address@hidden> wrote:
>>>>>> From: Ben Warren <address@hidden
>>>>>> <mailto:address@hidden>>
>>>> [...]
>>>> 
>>>>> (2) Structure of the vmgenid fw_cfg blob, AKA "why that darn offset 40
>>>>> decimal".
>>>>> 
>>>>> I explained it under points (6) and (7) in the following message:
>>>>> 
>>>>> Message-Id: <address@hidden
>>>>> <mailto:address@hidden>>
>>>>> URL: https://www.mail-archive.com/address@hidden/msg425226.html
>>>>> 
>>>>> The story is that *wherever* an ADD_POINTER command points to -- that
>>>>> is, at the *exact* target address --, OVMF will look for an ACPI table
>>>>> header, somewhat heuristically. If it finds a byte pattern that (a) fits
>>>>> into the remaining blob and (b) passes some superficial ACPI table
>>>>> header checks, such as length and checksum, then OVMF assumes that the
>>>>> blob contains an ACPI table there, and passes the address (again, the
>>>>> exact, relocated, absolute target address of ADD_POINTER) to
>>>>> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable().
>>>>> 
>>>>> We want to disable this heuristic for the vmgenid blob. *If* the blob
>>>>> contained only 16 bytes (for the GUID), then the heuristic would
>>>>> automatically disable itself, because the ACPI table header (36 bytes)
>>>>> is larger than 16 bytes, so OVMF wouldn't even look. However, for the
>>>>> caching and other VMGENID requirements, we need to allocate a full page
>>>>> with the blob (4KB), hence OVMF *will* look. If we placed the GUID right
>>>>> at the start of the page, then OVMF would sanity-check it as an ACPI
>>>>> table header. The check would *most likely* not pass, so things would be
>>>>> fine in practice, but we can do better than that: just put 40 zero bytes
>>>>> at the front of the blob.
>>>>> 
>>>>> And this is why the ADD_POINTER command has to point to the beginning of
>>>>> the blob (i.e., 36 zero bytes), to "suppress the OVMF ACPI SDT header
>>>>> detection". (The other 4 bytes are for arriving at an address divisible
>>>>> by 8, which is a VMGENID requirement for the GUID.)
>>>>> 
>>>>> The consequence is that both the ADDR method and QEMU's guest memory
>>>>> access code have to add 40 manually.
>>>> The longer I look "suppress the OVMF ACPI SDT header detection",
>>>> the less I like approach.
>>>> 
>>>> It looks somewhat backwards where a firmware forces QEMU
>>>> to use non obvious offsets to workaround OVMF ACPI table detection
>>>> heuristics.
>>> 
>>> This is for historical reasons -- when the linker/loader commands were
>>> invented, it wasn't considered that in UEFI, ACPI tables cannot just be
>>> linked together in-place, instead they'd have to be passed to
>>> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() one by one, for copying. The
>>> commands didn't provide dedicated means for identifying individual
>>> tables in blobs. Hence the heuristics built upon ADD_POINTER.
>>> 
>>> And once you have heuristics, you want to suppress them occasionally, if
>>> you can find a way.
>>> 
>>>> Can we add and use explicit flag to mark blobs as ACPI tables,
>>>> so that OVMF won't have to guess whether to hand off table
>>>> as ACPI to UEFI stack or just keep it to yourself?
>>> 
>>> The ADD_POINTER-based heuristics cannot be turned off for ACPI table
>>> identification, because a single fw_cfg blob can (and does) contain
>>> multiple ACPI tables, and OVMF needs to figure out, somehow, where each
>>> of those tables start. Blob-level hints won't help with this.
>>> 
>>> The following could be an improvement though: a blob-level hint (perhaps
>>> in the ALLOCATE command) that the blob contains *no* ACPI tables. In
>>> this case, OVMF could turn off the table recognition heuristics for
>>> those ADD_POINTER commands that point into such blobs. (Plus mark the
>>> blob for permanent preservation at once, and not only when an
>>> ADD_POINTER "probe" into the blob fails.)
>>> 
>>> OVMF currently ignores the Zone field in the ALLOCATE command, so that
>>> could be extended / abused for such a hint, without breaking
>>> compatibility with OVMF. (Not sure about SeaBIOS.)
>>> 
>> Overloading the ALLOCATE command in theory could be done with a simple
>> change to SeaBIOS.  Here’s where the zone is handled:
>> 
>>    switch (entry->alloc_zone) {
>>        case ROMFILE_LOADER_ALLOC_ZONE_HIGH:
>>            zone = &ZoneHigh;
>>            break;
>>        case ROMFILE_LOADER_ALLOC_ZONE_FSEG:
>>            zone = &ZoneFSeg;
>>            break;
>>        default:
>>            goto err;
>>    }
>> 
>> ZONE_HIGH = 1, and ZONE_FSEG = 2 and alloc_zone is 8 bits.  Stealing the
>> MSB and masking it off would be dirty, but could work.
> 
> Even assuming that we don't horribly break cross-version compatibility
> with tricks like this, the problem is that such changes only look simple
> and easy in isolation. They add up, and their testing impact is not small.
> 
> This week I've been working practically every waking moment -- I wanted
> to provide quick and detailed feedback (as far as OVMF was concerned)
> under both this series and on the topic of DSDT / X_DSDT /
> multiply-pointed-to tables. I'm also "racing you" to create a good
> quality OVMF series for WRITE_POINTER (including replay at S3 resume),
> so that as soon you post v6, we can test it together with OVMF too.
> (Last night I thought I had WRITE_POINTER ready, and started testing it
> against your v5, but then I suddenly thought of device reset and S3...
> Sigh.)
> 
> We can certainly file RFEs for stuff like the above (maybe in RHBZ,
> maybe in Launchpad), but the scope creep is already significant for
> VMGENID (it's noone's fault, VMGENID has tentacles into everything).
> Igor's suggestion is good, it aims to decrease technical debt, but I'm
> grasping at cycles -- review is very time consuming (albeit just as
> necessary). Also, for QEMU, the 2.9 soft freeze is around the corner…

OK, well I’m fine with things as they are.  I’ll try to get v6 out tomorrow.  
This optimization can wait until later.

> Best I can recommend now is filing an RFE for this.
> 
> Thanks
> Laszlo
> 
>>> Otherwise, a new allocation command will be necessary. (Which should
>>> embed the current ALLOCATE command structure fully, at some offset.)
>>> 
>>> Thanks
>>> Laszlo
>> 
> 

Attachment: smime.p7s
Description: S/MIME cryptographic signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]