Re: [PATCH V3] OvmfPkg/ResetVector: Removing SEV-ES CPUID bit check

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

On 6/1/22 07:25, Ard Biesheuvel wrote:
> On Tue, 31 May 2022 at 16:40, Peter Gonda <pgonda@google.com> wrote:
>>
>> The SEV-ES bit of Fn800-001F[EAX] - Bit 3 is used for a host to
>> determine support for running SEV-ES guests. It should not be checked by
>> a guest to determine if it is running under SEV-ES. The guest should use
>> the SEV_STATUS MSR Bit 1 to determine if SEV-ES is enabled. This check
>> was not part of the original SEV-ES support and was added in
>> a91b700e38. Removing the check makes this code consistent with the
>> Linux kernel
>>
>> Fixes: a91b700e38 (Ovmf/ResetVector: Simplify and consolidate the SEV features checks)
>>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>> Cc: Marc Orr <marcorr@google.com>
>> Signed-off-by: Peter Gonda <pgonda@google.com>
>> Acked-by: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> ---
>>   OvmfPkg/ResetVector/Ia32/AmdSev.asm | 8 --------
>>   1 file changed, 8 deletions(-)
>>
>> diff --git a/OvmfPkg/ResetVector/Ia32/AmdSev.asm b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
>> index 1f827da3b9..77692db27e 100644
>> --- a/OvmfPkg/ResetVector/Ia32/AmdSev.asm
>> +++ b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
>> @@ -265,14 +265,6 @@ CheckSevFeatures:
>>       ; Set the work area header to indicate that the SEV is enabled
>>       mov     byte[WORK_AREA_GUEST_TYPE], 1
>>
>> -    ; Check for SEV-ES memory encryption feature:
>> -    ; CPUID  Fn8000_001F[EAX] - Bit 3
>> -    ;   CPUID raises a #VC exception if running as an SEV-ES guest
>> -    mov       eax, 0x8000001f
>> -    cpuid
>> -    bt        eax, 3
>> -    jnc       GetSevEncBit
>> -
>>       ; Check if SEV-ES is enabled
>>       ;  MSR_0xC0010131 - Bit 1 (SEV-ES enabled)
>>       mov       ecx, SEV_STATUS_MSR
> 
> Thanks Peter, I have queued this up.
> 
> I did wonder, though: the only remaining reference to GetSevEncBit is
> a conditional branch that just precedes the label itself. This appears
> to be a leftover from commit 63c50d3ff2854a76 ("OvmfPkg/ResetVector:
> cache the SEV status MSR value in workarea") but it looks a bit dodgy.

Yes, it looks like the rdmsr and the GetSevEncBit: label can all be 
removed since the MSR value is now cached (a few lines above) and used for 
checks.

Thanks,
Tom