Re: [edk2-devel] [RFC PATCH v3 37/43] OvmfPkg: Reserve a page in memory for the SEV-ES AP reset vector

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 11/22/19 10:06 AM, Laszlo Ersek wrote:
> On 11/21/19 23:49, Tom Lendacky wrote:
>> On 11/21/19 1:27 PM, Laszlo Ersek wrote:
>>> On 11/20/19 21:06, Lendacky, Thomas wrote:
> 
>>>> +; sevEsResetBlock:
>>>> +;   For the initial boot of an AP under SEV-ES, the "reset" RIP must be
>>>> +;   programmed to the RAM area defined by SEV_ES_RESET_IP. A known offset
>>>> +;   and GUID will be used to locate this block in the firmware and extract
>>>> +;   the build time RIP value. The GUID must always be 48 bytes from the
>>>> +;   end of the firmware.
>>>> +;
>>>> +;   0xffffffca (-0x36) - IP value
>>>> +;   0xffffffcc (-0x34) - CS selector value
>>>
>>> (5) I think the documentation of these four bytes is incorrect.
>>> (Similarly in the previous patch, in the commit message.) We populate
>>> these bytes with the *linear address* of the "reset trampoline" where
>>> the host will have to boot the AP for the first time. It's not
>>> expressed in real mode CS:IP notation / meaning.
>>
>> This is correct. The code will be in "big real mode" as you note below
>> (the reset vector is actually 0xfffffff0, just below 4GB and running
>> in 16-bit mode).
>>
>> If you look in Qemu, target/i386/cpu.c, x86_cpu_reset(), you'll see
>> where the CS segment is being loaded with a base of 0xffff0000 and the
>> eip gets loaded with 0xfff0.
> 
> That still doesn't explain the above field names to me ("IP value" and
> "CS selector value"). Once the firmware image is built, we have:
> 
> - CS: 0x0080
> - IP: 0xB000
> 
> I don't understand the CS=0x80 part as
> - either a genuine segment selector
> - or the linear segment base address 0x800 (formed per real-address
>    mode).
> 
>> So the change to Qemu takes the "CS selector value" and effectively
>> left shifts it 16-bits to set the base
> 
> This logic seems correct, as it translates 0x80 to 0x80_0000, i.e. 8 MB.
> 
> However, I still think it's wrong to call the 16-bit value 0x0080 that
> we store at 0xffff_ffcc a "CS selector value". It is not a CS selector
> value, because it neither points into a segment descriptor table, nor is
> it left-shifted by 4-bits to form a linear base address, as in
> real-address mode.
> 
> I would call this field
> 
>    most significant 16 bits of the CS base address                    [1]
> 
>> and sets the eip to the "IP value" (in actuality, Qemu reads this as a
>> 32-bit value starting at the IP value and ANDs it with 0xffff0000 to
>> get the base and 0x0000ffff to get the eip.
> 
> That seems correct to me.
> 
> So... to summarize my point (5), *all* I'm asking for is to change the
> field description from "CS selector value" to [1] (or something to the
> same effect).
> 
>>
>>>
>>>
>>> (6) Which brings me to my main point... Value 0x00B000 (for the
>>> "base" PCD) is relative to the base address of FD.MEMFD, namely 8MB
>>> (see MEMFD_BASE_ADDRESS).
>>>
>>> IOW, the ultimate value of SEV_ES_RESET_IP will be 0x0x0080_B000.
>>> That address is larger than 1MB.
>>
>> Which is ok since Qemu will set up CS appropriately.
>>
>>>
>>> Therefore, the host will have to launch the AP (for the first time)
>>> above 1MB (in guest-phys address space).
>>
>> Which is also OK, just like the BSP was launched at 0xFFFF_FFF0.
>>
>>>
>>> How can that work, if the AP is supposed to start in real mode? The
>>> linear address 0x0x0080_B000 cannot be expressed in 16-bit real mode
>>> CS:IP notation at all.
>>>
>>> Does the hypervisor start the AP in "big real mode" (16-bit mode with
>>> 4GB segment limits, and CS containing a segment selector, not the
>>> actual segment base)?
> 
> Actually, I have to retract my question (6), because here I got lost in
> the weeds. I keep forgetting the following quirk in the Intel SDM:
> 
>      9.1.4 First Instruction Executed
> 
>      The first instruction that is fetched and executed following a
>      hardware reset is located at physical address FFFFFFF0H. This
>      address is 16 bytes below the processors uppermost physical address.
>      The EPROM containing the software- initialization code must be
>      located at this address.
> 
>      The address FFFFFFF0H is beyond the 1-MByte addressable range of the
>      processor while in real-address mode. The processor is initialized
>      to this starting address as follows. The CS register has two parts:
>      the visible segment selector part and the hidden base address part.
>      In real-address mode, the base address is normally formed by
>      shifting the 16-bit segment selector value 4 bits to the left to
>      produce a 20-bit base address. However, during a hardware reset, the
>      segment selector in the CS register is loaded with F000H and the
>      base address is loaded with FFFF0000H. The starting address is thus
>      formed by adding the base address to the value in the EIP register
>      (that is, FFFF0000 + FFF0H = FFFFFFF0H).
> 
>      The first time the CS register is loaded with a new value after a
>      hardware reset, the processor will follow the normal rule for
>      address translation in real-address mode (that is, [CS base address
>      = CS segment selector * 16]). To insure that the base address in the
>      CS register remains unchanged until the EPROM based software-
>      initialization code is completed, the code must not contain a far
>      jump or far call or allow an interrupt to occur (which would cause
>      the CS selector value to be changed).
> 
> I need to be reminded of this every once in a while!
> 
> While reviewing your patch, I thought that in real mode, the CS base
> address played no part, only the CS selector did -- which would be
> left-shifted by 4 bits directly, to form the segment base, in
> real-address mode.
> 
> However, the fact is that the CS base address *always* plays a part
> (even in real mode). The "left shifting by 4 bits" does not occur when
> the CS:IP *reference* is made, but when the CS register is *loaded*
> explicitly, in real-address mode.
> 
> And, at reset, CS is *pre-loaded*
> - with selector=0xF000, and
> - (as a quirk!) with base=0xFFFF_0000 (and *not* with base=0xF_0000, as
>    the selector would otherwise dictate in real-address mode).
> 
> And the QEMU change is that, at first AP boot, the CS will be pre-loaded
> - with selector=0x80 (which is a complete happenstance -- it's
>    irrelevant!),
> - and, importantly, with base=0x80_0000 (and *not* with base=0x800, as
>    the selector would otherwise dictate in real-address mode).
> 
> So, it's all clear now (as long as you can please confirm that my
> understanding is finally correct).
> 
> Thus, question (6) falls away.
> 
>>> ... I guess that would answer my question (6) -- and the last few
>>> patches in this series, before this one, *do* suggest "big real mode"
>>> to me --, but the documentation around (5), and in the commit message
>>> of the previous patch, still doesn't match that.
>>
>> I'll expand on the commit message to indicate that Qemu, or others,
>> must do something similar relative to the CS register setup.
> 
> To summarize, my request would be:
> 
> - in the previous patch, please replace, in the commit message,
> 
>      Reset CS
> 
>    with
> 
>      most significant 16 bits of the reset CS base address
> 
> - in this patch, please replace, in the code comment,
> 
>      CS selector value
> 
>    with
> 
>      most significant 16 bits of the CS base address
> 
> (I mean... you *could* keep the current language in both places, but
> then you would have to describe this *entirely new* quirk for forming
> the CS base address from the CS selector value. This new SEV-ES / QEMU
> method does not match *any* { CS selector --> CS base address }
> transformation from the SDM, so you'd have to be quite verbose in the
> documenation.
> 
> Given that the CS selector value 0x80 is completely irrelevant here, and
> we only care about the ultimate CS base address 0x80_0000, I'd suggest
> defining the 16-bit field at 0xffffffcc in terms of the desired CS base
> address [1], and not once mentioning "selector".)

Let me work on the wording and see what I come up with. I'll pass it by
you to see if it makes sense.

Thanks,
Tom

> 
> Thanks!
> Laszlo
>