Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ce305ca6-959f-8b39-79e0-ba77b000413f@amd.com>
Date:   Mon, 24 Oct 2022 09:09:39 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.2
Subject: Re: [PATCH v3 2/4] x86/sev: Change snp_guest_issue_request's fw_err
Content-Language: en-US
To:     Dionna Glaze <dionnaglaze@google.com>,
        linux-kernel@vger.kernel.org, x86@kernel.org
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Joerg Roedel <jroedel@suse.de>,
        Peter Gonda <pgonda@google.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Dave Hansen <dave.hansen@linux.intel.com>
References: <20221021165943.1968044-1-dionnaglaze@google.com>
 <20221021165943.1968044-3-dionnaglaze@google.com>
From:   Tom Lendacky <thomas.lendacky@amd.com>
In-Reply-To: <20221021165943.1968044-3-dionnaglaze@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZGtCMWw3dlplNC9OYVBFNW1XODNNb0g3d2JXd3p2eDNTcUtITURLVnpyOHZi?=
 =?utf-8?B?NFF6YkwyUVZjUkJhN1VnT0l6QmxZUFJtQllDT1VuZkNtbG9vcXlFVmhTWWt3?=
 =?utf-8?B?MXZPb2JRK1BheEZBandpeTZEL0JWNThwWEVUNUQ1WXpoRy9DbVFoNHpuZVFU?=
 =?utf-8?B?dHdQU05oNkFtK01DaVlhRFlvb1pJOGZoU0hvb0x5aUlNWHIxRlRUaWxHMWtN?=
 =?utf-8?B?QVZmNlpETVhLZzJNK3k5YVErdkhlSTRHZHZGVUQ0MVZmdlNkQ3ZsSHh1eDNG?=
 =?utf-8?B?aXBobG4rcmVpaSsrNkFDUHR0cThXSGllR1kwMkxnbUxoZWRMZlh3OEtSL25U?=
 =?utf-8?B?RDdzTnBMQUl5azVWK0F1ZGE4U1M1aHQ3cW1HaEVxSy9uakk3MzNkb0lFMFRZ?=
 =?utf-8?B?UldPOGl1UzREa0dwbHRMNEN6VWJ5b2luNGtkbEF0cUpPVnJ4c3NUT0tKVTNH?=
 =?utf-8?B?VHJtWGl5Zmd4OFVmdklRbHJaYmtMZGpKUkR6bGhiUkdWTDgwNHcva0g5amxJ?=
 =?utf-8?B?L2RXNnVkNmROdTMveWlUa0hiWjYxaFpjREM5UGdEWHZFMUNXbUxuSENmK3pK?=
 =?utf-8?B?c3NVVkxtTG9VOFlYN0dYN1ByMFdLNjJDVXh0U01DdDcvb1RMd0llcGF2bE9G?=
 =?utf-8?B?b3RsTS9za2pEbk5iNXl4UDZ4cGtOa1RVM2lPZXJLbE5Jd0MzakFickVwRUM3?=
 =?utf-8?B?Tkx4Z0NTc2xUYUxqMk9zK29xNWgyRGhLbmhJZjU2eVpOTXZoOFN4L1NkUC9L?=
 =?utf-8?B?UWxBUkhNT1NuMndoaEZkSVNVY25JSW1wVTJINjIweFlRb2xVc01GeTgwUnNn?=
 =?utf-8?B?Q3FPcEswQXQ5T3RiWURaL1RMK203Lyt4YklpVzZtRHYvUVZYOXIvRWtPdEJw?=
 =?utf-8?B?MTZaTXg5ODBrNGxRYi96bk1MQTFyTWJ0dFVnV0FUL0xjenRWNGVMUUNxVTVu?=
 =?utf-8?B?RVNEZEh6bEtISzE0TzFPQStWK1c2WFd4L3hJOXNiRU5zRmw1U0IxbVdHeVhU?=
 =?utf-8?B?TXp3dEYvOWNibWZ5R24ydDVFS0VKejF6bWFadnpzRHVVMklGbVM0Sm1UdTdM?=
 =?utf-8?B?TFBYbnJXT0Q4TVo0RVVPZmV2T0hSbEtLRkpNL0FpQ3RiSThNQ2JHbjV1dlpX?=
 =?utf-8?B?QWdaMEplR25xcDhmcTFvQi8zT2xJVGwvNUVuUHZQVkhLZFZsN25uZmJkNHlM?=
 =?utf-8?B?RVlQQnZoUkUvdGx5TWpRTkd5T243MU15SGNoT2lTUTYxcGE2N1V5dHNURFpr?=
 =?utf-8?B?RFZNSFAveVRhcnlaV3hqWGdoSStYQ2g0eE4xN1VWc241Q2VXYlpXOEgyQ0NX?=
 =?utf-8?B?T1VkcU80MVhZWlZlVEg3ZkFUS28ySXduNWU0ZFJiVDhFdnlEOVZ5aVVKNW1l?=
 =?utf-8?B?SUgrd2RsUlc1VGE2VCt0dGdmaG1IRFo4UzZLL1ZScXZ6TnBqRVRSc0hSWUhW?=
 =?utf-8?B?cjJqM3BpWmxieFZDTmFnZFpnNVkrOGd2TzRSbTRaTlhUdnNORHV5UHFnaDlL?=
 =?utf-8?B?ZVJSdThZdVRVdUM0L2xIcnFTczBEMERERi8wVHBQNTQ2cjM5cTVuRGVKZ0FJ?=
 =?utf-8?B?VHlNSDJrd0R5WWdOTjBYRHZ5Q2F2d2FhZ2lXa0ZLbHlhWVZxUTZKbFY1U2FQ?=
 =?utf-8?B?MXcwK1J3ZlJEanRqREpvNndsOHdRY0h3VWlDaC9weWV5Ky9SM2hkNnMyYmlT?=
 =?utf-8?B?eXV6RnQyeGlPdmNwNGRmT2dHeVJaclV6bFFpYjdsNWc2UXJSMEFWMVc4dE8w?=
 =?utf-8?B?djhaL01HbXpuTlRZajZRenhMejFSNjZhNVV1U3BkTFhOM3BJeG8vMSt4WUJk?=
 =?utf-8?B?Z2k5SVlSakRXbzlQZ0Y0K1FOemh6TkIzUHJPVG01NThqOG1NUm54OVJrSXhH?=
 =?utf-8?B?WWFZeXdVZVMralZmOENhaU5QTHdybjNaby9QU1BMd1VRVjBXb0NJKzloWkxG?=
 =?utf-8?B?UEdEeFZYdG9HN0F0blJyQ3RmOW91TVMvZm41TC91cTRBNW5jaVpvaEt3UnQw?=
 =?utf-8?B?RjV0cTRVNWw2aEQyYUp5dHE1T3BPMUx0KzJIUDIrQXhGa3IvaEtvMXowZC93?=
 =?utf-8?B?YUJOT1Fwc2ZieXRiOVFIYk1jSTV0TDFrc2dsT1lKRWdPTFpMdXlTcmsybWt5?=
 =?utf-8?Q?BOVnFjAfh+1pHfJVf98gD+I8F?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bb18c84f-f770-4f57-b72d-08dab5c96512
X-MS-Exchange-CrossTenant-AuthSource: DM4PR12MB5229.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Oct 2022 14:09:41.5906
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: CUnh/ZmOARWw2c2XalXZ8MWY1Mhco7S0k9XxyGVJDEu2NuOnhTMUy+bAgjhLxedcV7YttNQUwJ6WxwWYi/B06Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR12MB5208
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 10/21/22 11:59, Dionna Glaze wrote:
> The GHCB specification declares that the firmware error value for a
> guest request will be stored in the lower 32 bits of EXIT_INFO_2.
> The upper 32 bits are for the VMM's own error code. The fw_err argument
> is thus a misnomer, and callers will need access to all 64 bits.
> 
> The type of unsigned long also causes problems, since sw_exit_info2 is
> u64 (unsigned long long) vs the argument's previous unsigned long*.
> The signature change requires the follow-up change to
> drivers/virt/coco/sev-guest to use the new expected type in order to
> compile.
> 
> The firmware might not even be called, so we bookend the call with the
> no firmware call error and clearing the error.
> 
> Cc: Tom Lendacky <Thomas.Lendacky@amd.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Cc: Peter Gonda <pgonda@google.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> 
> Fixes: d5af44dde546 ("x86/sev: Provide support for SNP guest request NAEs")
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>   arch/x86/include/asm/sev.h |  4 ++--
>   arch/x86/kernel/sev.c      | 11 ++++++++---
>   2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index ebc271bb6d8e..8ebd78b6a57c 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -196,7 +196,7 @@ void snp_set_memory_private(unsigned long vaddr, unsigned int npages);
>   void snp_set_wakeup_secondary_cpu(void);
>   bool snp_init(struct boot_params *bp);
>   void __init __noreturn snp_abort(void);
> -int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned long *fw_err);
> +int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, u64 *exitinfo2);
>   #else
>   static inline void sev_es_ist_enter(struct pt_regs *regs) { }
>   static inline void sev_es_ist_exit(void) { }
> @@ -217,7 +217,7 @@ static inline void snp_set_wakeup_secondary_cpu(void) { }
>   static inline bool snp_init(struct boot_params *bp) { return false; }
>   static inline void snp_abort(void) { }
>   static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input,
> -					  unsigned long *fw_err)
> +					  unsigned long *exitinfo2)


s/unsigned long/u64/

>   {
>   	return -ENOTTY;
>   }
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index a428c62330d3..5a402df7549e 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -22,6 +22,7 @@
>   #include <linux/efi.h>
>   #include <linux/platform_device.h>
>   #include <linux/io.h>
> +#include <linux/psp-sev.h>
>   
>   #include <asm/cpu_entry_area.h>
>   #include <asm/stacktrace.h>
> @@ -2175,7 +2176,7 @@ static int __init init_sev_config(char *str)
>   }
>   __setup("sev=", init_sev_config);
>   
> -int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned long *fw_err)
> +int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, u64 *exitinfo2)
>   {
>   	struct ghcb_state state;
>   	struct es_em_ctxt ctxt;
> @@ -2186,9 +2187,11 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned
>   	if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
>   		return -ENODEV;
>   
> -	if (!fw_err)
> +	if (!exitinfo2)
>   		return -EINVAL;
>   
> +	*exitinfo2 = SEV_RET_NO_FW_CALL;
> +
>   	/*
>   	 * __sev_get_ghcb() needs to run with IRQs disabled because it is using
>   	 * a per-CPU GHCB.
> @@ -2218,9 +2221,11 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned
>   		    ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN)
>   			input->data_npages = ghcb_get_rbx(ghcb);
>   
> -		*fw_err = ghcb->save.sw_exit_info_2;
> +		*exitinfo2 = ghcb->save.sw_exit_info_2;

Looks like you can do "*exitinfo2 = ghcb->save.sw_exit_info_2;" just 
before the "if (ghcb->save.sw_exit_info_2) {". That would eliminate the 
need to assign it in each branch of the if.

Thanks,
Tom

>   
>   		ret = -EIO;
> +	} else {
> +		*exitinfo2 = 0;
>   	}
>   
>   e_put: