From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id F3081ECAAA0 for ; Thu, 25 Aug 2022 20:09:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S243525AbiHYUJq (ORCPT ); Thu, 25 Aug 2022 16:09:46 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46408 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231755AbiHYUJl (ORCPT ); Thu, 25 Aug 2022 16:09:41 -0400 Received: from mail-vs1-xe2b.google.com (mail-vs1-xe2b.google.com [IPv6:2607:f8b0:4864:20::e2b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 03ACBBFE97 for ; Thu, 25 Aug 2022 13:09:39 -0700 (PDT) Received: by mail-vs1-xe2b.google.com with SMTP id h67so21026868vsc.11 for ; Thu, 25 Aug 2022 13:09:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc; bh=/o5u98fJkAPGGPRvZ0JnL9MrzWUgTcXH/oohHXk3MD8=; b=Ezi3cLxl5SBn+0WFQppba1X7+DdHeAZwd+3+8yyHBkZ8p32tIpXd4owl1HlE83sJlO 35tGqyLcydDFnVxl+CllN9bqMX9roI42WPfmwop0omLlw8YncOW+c8HsYJX/9B/vbTyJ G0ggnVxnzwnyWSTVI870+xJVwDivl/otgYRGldLDtzhFC+NR8TQKY25dlVrK2AY5pxqg j8+1wMEzuZoPbCH0bnMmY/aShcHogX5ZkHm8OOmT0W1plozwcVxwRnNem25RQcP/v+8v cOYAMOSYJKqvLMMxImuVQpmqla9mEyo5zBXp/yPnp1NiDw1AX/9F/iOoYCV+y7vg+ryK bZNg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc; bh=/o5u98fJkAPGGPRvZ0JnL9MrzWUgTcXH/oohHXk3MD8=; b=hFnNvgfWsTedmBlwI7Ipdm2RViRGi49COCuzNsIH+ZRPub+UBKd/7CEgN6IZqsFBh/ FLGJ+66+RlXOikm2XTDrd3tYW1vR7SMl2CTfYx5CnXk+vIcekF1+7CJwwMCS6ubWd6Uu rg+RJ7SlYzK2z74PdQYv3li5jXvXqMc17ivsTg8fdp7nPB2PUkQhJMHv8vifE9bqs5AR TRWTeN4oVKEoacfMLnyBbJFB0I6BHY+YvOiqragL5M5/2mQoaPWmKH4/UKhywtU5NpIO cYxuO3t0EZyjpB3Z918UUUtpcKnaB6+ljjdAe7VIA4JVYl3p5nlQ+brCemBq7qQNK+qF VSUQ== X-Gm-Message-State: ACgBeo3WzKahPm5AJQ+E06+EWXz6pgBorbmoycPyVZx2At9MTkj7Di/6 sCO4qW0x+vcR5IDvwIC9keOLE+4+DCSBscq1s1P5cQ== X-Google-Smtp-Source: AA6agR7nyEzEvgpeTs849VA7+3zgZ0xGJ1Pxf0VO2EuGO24TVf9MLwl1oV6f4IzTsrHTAKLU/CRN4th7Cmzsf8Yp7TA= X-Received: by 2002:a67:fd0e:0:b0:390:1d9a:2455 with SMTP id f14-20020a67fd0e000000b003901d9a2455mr2104726vsr.78.1661458177919; Thu, 25 Aug 2022 13:09:37 -0700 (PDT) MIME-Version: 1.0 References: <20220307213356.2797205-1-brijesh.singh@amd.com> <20220307213356.2797205-44-brijesh.singh@amd.com> <51298b17-9e12-7a08-7322-594deac52f53@amd.com> In-Reply-To: <51298b17-9e12-7a08-7322-594deac52f53@amd.com> From: Peter Gonda Date: Thu, 25 Aug 2022 14:09:26 -0600 Message-ID: Subject: Re: [PATCH v12 43/46] virt: Add SEV-SNP guest driver To: Tom Lendacky Cc: Dionna Amalie Glaze , Brijesh Singh , "the arch/x86 maintainers" , LKML , "open list:X86 KVM CPUs" , linux-efi , platform-driver-x86@vger.kernel.org, linux-coco@lists.linux.dev, Linux Memory Management List , Thomas Gleixner , Ingo Molnar , Joerg Roedel , "H. Peter Anvin" , Ard Biesheuvel , Paolo Bonzini , Sean Christopherson , Vitaly Kuznetsov , Jim Mattson , Andy Lutomirski , Dave Hansen , Sergio Lopez , Peter Zijlstra , Srinivas Pandruvada , David Rientjes , Dov Murik , Tobin Feldman-Fitzthum , Borislav Petkov , Michael Roth , Vlastimil Babka , "Kirill A . Shutemov" , Andi Kleen , "Dr . David Alan Gilbert" , brijesh.ksingh@gmail.com, Tony Luck , Marc Orr , Kuppuswamy Sathyanarayanan Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Aug 25, 2022 at 12:54 PM Tom Lendacky wrote: > > On 8/24/22 14:28, Peter Gonda wrote: > > On Wed, Aug 24, 2022 at 12:01 PM Dionna Amalie Glaze > > wrote: > >> > >> Apologies for the necropost, but I noticed strange behavior testing my > >> own Golang-based wrapper around the /dev/sev-guest driver. > >> > >>> + > >>> +static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, int msg_ver, > >>> + u8 type, void *req_buf, size_t req_sz, void *resp_buf, > >>> + u32 resp_sz, __u64 *fw_err) > >>> +{ > >>> + unsigned long err; > >>> + u64 seqno; > >>> + int rc; > >>> + > >>> + /* Get message sequence and verify that its a non-zero */ > >>> + seqno = snp_get_msg_seqno(snp_dev); > >>> + if (!seqno) > >>> + return -EIO; > >>> + > >>> + memset(snp_dev->response, 0, sizeof(struct snp_guest_msg)); > >>> + > >>> + /* Encrypt the userspace provided payload */ > >>> + rc = enc_payload(snp_dev, seqno, msg_ver, type, req_buf, req_sz); > >>> + if (rc) > >>> + return rc; > >>> + > >>> + /* Call firmware to process the request */ > >>> + rc = snp_issue_guest_request(exit_code, &snp_dev->input, &err); > >>> + if (fw_err) > >>> + *fw_err = err; > >>> + > >>> + if (rc) > >>> + return rc; > >>> + > >> > >> The fw_err is written back regardless of rc, so since err is > >> uninitialized, you can end up with garbage written back. I've worked > >> around this by only caring about fw_err when the result is -EIO, but > >> thought that I should bring this up. > > > > I also noticed that we use a u64 in snp_guest_request_ioctl.fw_err and > > u32 in sev_issue_cmd.error when these should be errors from the > > sev_ret_code enum IIUC. > > The reason for the u64 is that the Extended Guest Request can return a > firmware error or a hypervisor error. To distinguish between the two, a > firmware error is contained in the lower 32-bits, while a hypervisor error > is contained in the upper 32-bits (e.g. when not enough contiguous pages > of memory have been supplied). Ah, makes sense. I was trying to think of a way to codify the state described above where we error so early in the IOCTL or call that the PSP is never called, something like below. I think using UINT32_MAX still works with how u64 of Extended Guest Request is spec'd. Is this interesting to clean up the PSP driver and internal calls, and the new sev-guest driver? diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index 63dc626627a0..d1e605567d5e 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -2177,6 +2178,8 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned if (!fw_err) return -EINVAL; + fw_err = SEV_RET_NO_FW_CALL; + /* * __sev_get_ghcb() needs to run with IRQs disabled because it is using * a per-CPU GHCB. @@ -2209,6 +2212,8 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned *fw_err = ghcb->save.sw_exit_info_2; ret = -EIO; + } else { + *fw_err = 0; } e_put: diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index 9f588c9728f8..e71d6e39aa2b 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -439,7 +439,7 @@ static int __sev_platform_init_locked(int *error) { struct psp_device *psp = psp_master; struct sev_device *sev; - int rc, psp_ret = -1; + int rc, psp_ret = SEV_RET_NO_FW_CALL; int (*init_function)(int *error); if (!psp || !psp->sev_data) diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h index 91b4c63d5cbf..b8f2c129d63d 100644 --- a/include/uapi/linux/psp-sev.h +++ b/include/uapi/linux/psp-sev.h @@ -36,6 +36,11 @@ enum { * SEV Firmware status code */ ...skipping... #include #include @@ -2177,6 +2178,8 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned if (!fw_err) return -EINVAL; + fw_err = SEV_RET_NO_FW_CALL; + /* * __sev_get_ghcb() needs to run with IRQs disabled because it is using * a per-CPU GHCB. @@ -2209,6 +2212,8 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned *fw_err = ghcb->save.sw_exit_info_2; ret = -EIO; + } else { + *fw_err = 0; } e_put: diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index 9f588c9728f8..e71d6e39aa2b 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -439,7 +439,7 @@ static int __sev_platform_init_locked(int *error) { struct psp_device *psp = psp_master; struct sev_device *sev; - int rc, psp_ret = -1; + int rc, psp_ret = SEV_RET_NO_FW_CALL; int (*init_function)(int *error); if (!psp || !psp->sev_data) diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h index 91b4c63d5cbf..b8f2c129d63d 100644 --- a/include/uapi/linux/psp-sev.h +++ b/include/uapi/linux/psp-sev.h @@ -36,6 +36,11 @@ enum { * SEV Firmware status code */ typedef enum { + /* + * This error code is not in the SEV spec but is added to convey that + * there was an error that prevented the SEV Firmware from being called. + */ + SEV_RET_NO_FW_CALL = -1, SEV_RET_SUCCESS = 0, SEV_RET_INVALID_PLATFORM_STATE, SEV_RET_INVALID_GUEST_STATE, > > > >> > >> -- > >> -Dionna Glaze, PhD (she/her)