Re: [RFC PATCH v2 02/10] OvmfPkg/ResetVector: add memory encryption mask when SEV is enabled

[Laszlo Ersek <lersek@redhat.com>]

On 03/23/17 17:48, Brijesh Singh wrote:
> On Thu, Mar 23, 2017 at 11:16 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 03/23/17 16:05, Brijesh Singh wrote:
>>> On Wed, Mar 22, 2017 at 3:20 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>>>> In the below logic, which branch exactly (to NoSev) will be taken on
>>>> Intel processors?
>>>>
>>>>
>>> The below check should branch to NoSev  on Intel processor
>>> (please note that 0x8000_001F is new leaf and may not exist on older AMD
>>> processor hence we will also branch to NoSev on AMD processor which does
>>> not support this leaf)
>>>
>>> *; Check if we have a valid (0x8000_001F) CPUID leaf
>>> **mov       eax, 0x80000000
>>> **cpuid
>>> **cmp       eax, 0x8000001f
>>> **jl        NoSev*
>>
>> Yes, I figured that maybe this "highest supported cpuid leaf" check
>> would catch Intel processors.
>>
>> But, what prevents a future Intel processor from exposing such a high
>> CPUID leaf, for a completely different purpose? Should we not perform
>> some kind of vendor-id check? (I'm quite unfamiliar with CPUID, as you
>> can tell, I just seem to recall a comment (from Liming perhaps?) for one
>> of your MdeModulePkg patches, where he said that directly using a
>> software-defined CPUID leaf (?) would not be portable across all CPU
>> vendors, or some such.)
>>
>>
> 
> I had similar question in my mind.  Before creating the patch I checked
> with
> AMD architecture team. The response was, CPUID 0x8000_001F definition
> is fixed for x86 architecture. So, if Intel ever decides to report this
> CPUID leaf
> then it will comply to exact same definition.

OK, this looks safe then. Can you please add it as a comment to the code?

Thanks!
Laszlo

> 
> I think vendor check is weak, e.g user could change the vendor string
> through the qemu property which will break this loop. Because of this, I
> wanted
> to avoid any kind of vendor string or hypervisor defined CPUID checks
> inside the code.
> 
> BTW, in my previous patches I was using KVM software defined CPUID leaf and
> I
> got rid of it because hypervisor's are flexiable to define that CPUID in
> whatever
> way they want.
> 
> -Brijesh
>