On 02/03/2023 16:33, Tom Lendacky wrote: > On 3/1/23 23:59, Dov Murik wrote: >> Hi Mike, Zhi, >> >> On 01/03/2023 23:20, Zhi Wang wrote: >>> On Mon, 20 Feb 2023 12:38:43 -0600 >>> Michael Roth <michael.roth@xxxxxxx> wrote: >>> >>>> From: Brijesh Singh <brijesh.singh@xxxxxxx> >>>> >>>> Add support to decrypt guest encrypted memory. These API interfaces can >>>> be used for example to dump VMCBs on SNP guest exit. >>>> >>> >>> What kinds of check will be applied from firmware when VMM decrypts this >>> page? I suppose there has to be kinda mechanism to prevent VMM to >>> decrypt >>> any page in the guest. It would be nice to have some introduction about >>> it in the comments. >>> >> >> The SNP ABI spec says (section 8.27.2 SNP_DBG_DECRYPT): >> >> The firmware checks that the guest's policy allows debugging. If not, >> the firmware returns POLICY_FAILURE. >> >> and in the Guest Policy (section 4.3): >> >> Bit 19 - DEBUG >> 0: Debugging is disallowed. >> 1: Debugging is allowed. >> >> In the kernel, that firmware error code is defined as >> SEV_RET_POLICY_FAILURE. >> >> >>>> Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx> >>>> Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx> >>>> [mdr: minor commit fixups] >>>> Signed-off-by: Michael Roth <michael.roth@xxxxxxx> >>>> --- >>>> drivers/crypto/ccp/sev-dev.c | 32 ++++++++++++++++++++++++++++++++ >>>> include/linux/psp-sev.h | 22 ++++++++++++++++++++-- >>>> 2 files changed, 52 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/crypto/ccp/sev-dev.c >>>> b/drivers/crypto/ccp/sev-dev.c >>>> index e65563bc8298..bf5167b2acfc 100644 >>>> --- a/drivers/crypto/ccp/sev-dev.c >>>> +++ b/drivers/crypto/ccp/sev-dev.c >>>> @@ -2017,6 +2017,38 @@ int sev_guest_df_flush(int *error) >>>> } >>>> EXPORT_SYMBOL_GPL(sev_guest_df_flush); >>>> +int snp_guest_dbg_decrypt_page(u64 gctx_pfn, u64 src_pfn, u64 >>>> dst_pfn, int *error) >>>> +{ >>>> + struct sev_data_snp_dbg data = {0}; >>>> + struct sev_device *sev; >>>> + int ret; >>>> + >>>> + if (!psp_master || !psp_master->sev_data) >>>> + return -ENODEV; >>>> + >>>> + sev = psp_master->sev_data; >>>> + >>>> + if (!sev->snp_initialized) >>>> + return -EINVAL; >>>> + >>>> + data.gctx_paddr = sme_me_mask | (gctx_pfn << PAGE_SHIFT); >>>> + data.src_addr = sme_me_mask | (src_pfn << PAGE_SHIFT); >>>> + data.dst_addr = sme_me_mask | (dst_pfn << PAGE_SHIFT); >> >> I guess this works, but I wonder why we need to turn on sme_me_mask on >> teh dst_addr. I thought that the firmware decrypts the guest page >> (src_addr) to a plaintext page. Couldn't find this requirement in the >> SNP spec. > > This sme_me_mask tells the firmware how to access the host memory > (similar to how DMA uses sme_me_mask when supplying addresses to devices > under SME). This needs to match the pagetable mapping being used by the > host, otherwise the contents will appears as ciphertext to the host if > they are not in sync. Since the default pagetable mapping is encrypted, > the sme_me_mask bit must be provided on the destination address. So it > is not a spec requirement, but an SME implementation requirement. > Ah, OK, that's clear now. Thanks Tom. -Dov