Re: [PATCH 2/2] OvmfPkg/AmdSev: update the fdf to use new workarea PCD

["Dov Murik" <dovmurik@linux.ibm.com>]

On 17/10/2021 1:32, Brijesh Singh wrote:
> 
> On 10/16/21 1:38 PM, Dov Murik wrote:
>> [+Tobin]
>>
>>
>> On 14/10/2021 21:17, Brijesh Singh wrote:
>>> The commit 80e67af9afca added support for the generic work area concept
>>> used mainly by the encrypted VMs but missed update the AmdSev package.
>>>
>>> Fixes: 80e67af9afca ("OvmfPkg: introduce a common work area")
>> Thanks Brijesh.
>>
>> The fix does allow me to launch SEV-ES guests, which is good news.
>> However, the guest's measurement has changed, so I wonder what this
>> change causes.
>>
>> The details:
>>
>> I tested 3 commits (always building the AmdSevX64 target):
>>
>> 1. commit 7b4a99be8a39 - edk2-stable202108
>>
>> I successfully launch SEV and SEV-ES guests and my measurement check
>> script verifies the digest correctly (including the "measured linux
>> boot" hashes table added by QEMU).
>>
>> 2. commit f10a112f08f3 - master (Oct 14)
>>
>> I successfully launch SEV guests, but SEV-ES guests crash with "error:
>> kvm run failed Invalid argument". The measurement check verifies digest
>> correctly.
>>
>> 3. master + this AmdSevX64.fdf patch
>>
>> I successfully launch SEV guests and measurement calculation is OK. As
>> far SEV-ES guests, the measurement check doesn't match what I expect. If
>> I ignore the mismatched measurement and continue the launch, the guest
>> runs OK with SEV-ES.
>>
>>
>> So this patch fixes the problem (SEV-ES guest crashes on launch) but
>> shows another problem (bad guest measurement).
>>
>>
>> Note that for this test, my measurement calculation script automatically
>> takes the OVMF image I'm using to boot the VM.  From my reading of the
>> QEMU code, the only pieces that should affect the measurement is the
>> OVMF image, the hashes table, and the VMSAs for each vcpu.  The OVMF
>> image is updated on every check, and the rest shouldn't have changed
>> between those 3 revisions that I tested.
>>
>>
>> It might be an issue with my measurement checking script which was
>> assuming something that has changed with the introduction of the new
>> work area, but I can't think of something like that. Note again that
>> plain SEV measurement is still working OK.
>>
> I assume you are including the IP for the APs during your VMSA hash
> computation. The IP for the AP is obtained through the SevEsResetGuid
> [1]. It points to Fixed(PcdSevEsWorkArea). After we introduced the
> generic Ovmf workarea concept the PcdSevEsWorkArea no longer start from
> the beginning of the workarea. See the hunk below
> 
> +##########################################################################################
> +# Set the SEV-ES specific work area PCDs
> +#
> +SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase = $(MEMFD_BASE_ADDRESS) +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHeader
> +SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHeader
> +##########################################################################################
> +
> 
> [1]
> https://github.com/tianocore/edk2/blob/master/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm#L109
> 
> Make sure you are using the correct value for the AP IP in your
> computation. If you have hard coded AP IP in your script then I would
> recommend to update the script to  retrieve the value from the OVMF_CODE.fd.
> 
> Hope this helps.
> 

Yep, that was indeed the issue. The VMSA for vcpu0 is identical to the
previous version, but there was a 4-byte shift in the IP field in VMSA
for the AP vcpus.

Thanks for your help debugging this.

So, FWIW:

Reviewed-by: Dov Murik <dovmurik@linux.ibm.com>
Tested-by: Dov Murik <dovmurik@linux.ibm.com>


-Dov

>> Do you encounter similar issues with VM measurement?
>>
>>
>> -Dov
>>
>>
>>
>>> 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: Erdem Aktas <erdemaktas@google.com>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Reported-by: Dov Murik <dovmurik@linux.ibm.com>
>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>> ---
>>>  OvmfPkg/AmdSev/AmdSevX64.fdf | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
>>> index 542722ac6b37..56626098862c 100644
>>> --- a/OvmfPkg/AmdSev/AmdSevX64.fdf
>>> +++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
>>> @@ -57,7 +57,7 @@ [FD.MEMFD]
>>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>>>  
>>>  0x00B000|0x001000
>>> -gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize
>>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize
>>>  
>>>  0x00C000|0x000C00
>>>  gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
>>> @@ -79,6 +79,13 @@ [FD.MEMFD]
>>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize
>>>  FV = DXEFV
>>>  
>>> +##########################################################################################
>>> +# Set the SEV-ES specific work area PCDs
>>> +#
>>> +SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase = $(MEMFD_BASE_ADDRESS) +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHeader
>>> +SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHeader
>>> +##########################################################################################
>>> +
>>>  ################################################################################
>>>  
>>>  [FV.SECFV]
>>>