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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id BF43CC7EE2F for ; Mon, 12 Jun 2023 16:12:20 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 610EE8E0003; Mon, 12 Jun 2023 12:12:20 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5C18C8E0002; Mon, 12 Jun 2023 12:12:20 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4B0278E0003; Mon, 12 Jun 2023 12:12:20 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 3B1EA8E0002 for ; Mon, 12 Jun 2023 12:12:20 -0400 (EDT) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 0F0141C75B8 for ; Mon, 12 Jun 2023 16:12:20 +0000 (UTC) X-FDA: 80894588040.05.F5AA763 Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by imf21.hostedemail.com (Postfix) with ESMTP id BF1021C0009 for ; Mon, 12 Jun 2023 16:12:15 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=WwuP42lj; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf21.hostedemail.com: domain of dave.hansen@intel.com designates 192.55.52.136 as permitted sender) smtp.mailfrom=dave.hansen@intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1686586337; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=pj36W/3auJ33f95K7ygSJ5vrwtfhAw3/S5usGHAZiAw=; b=ltb5RiVXtdJkRSXs6XKSlCXekhkH5WaBv8puHUDv0ywIFmdycsiWiRlvGF6qzWLX07qM2p Fkg+PcPb5UkvX0rdbvHgyAYViIuZ5g/6x1fULhb5vo6RyAhkCsmBOsAavcigx54Ny0XRRM aDwAVEvx+tLvd2251Jl5bzq2MpzQdZw= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=WwuP42lj; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf21.hostedemail.com: domain of dave.hansen@intel.com designates 192.55.52.136 as permitted sender) smtp.mailfrom=dave.hansen@intel.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1686586337; a=rsa-sha256; cv=none; b=hmn67U/zl/Y+8AbGBsQPdON7nsx/0cPXA3H4JdLtJk/5iM/Em0c3+SsRikdK1ZnTC6zWK/ NHBtLpZKs4DCRSclFTS9C6gNV7cbHjyZ62lkxWEgQQ1CBGdywlyxgcyWyVw1mbTG26talw pJ09EbZSVh45HamFu/G6MNny5PNRHtw= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1686586336; x=1718122336; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=5KFn1g9QXL7PSHf/uoACePAASUQ7zOXitZhS2hdbsts=; b=WwuP42ljWCYcQOsjwz40lNyFVlotEoVV3zHmcS/DYVlgwDj8LUbp99OD VtF/QIuM4vVOMK0CDdJ+SRXbmKlVh4eVp0zwJFHHrtuTnyOuA0Zq0ndfD HvN6Y8mXhVq7VxL0fG3myxen1nXbATjeNGjBd3uvuRvB3XUK+Jm4Ba7YA LkZaqsQIYDM/4JwZ6tE48R+ANKMPUKLf+nkA9wvu3nlFlWq+vZQXP9hTr iQZhuKr/0e8l8vPdu4iynCP/qlBUozgrJ2DERNwlLcv6CIhY/C/I39jlB IPaYGxiQTSYZwDrYce2m7TavLjW8/Dgpd2lUlO1KD6Nz1PAoFdpoa1bfj A==; X-IronPort-AV: E=McAfee;i="6600,9927,10739"; a="337723187" X-IronPort-AV: E=Sophos;i="6.00,236,1681196400"; d="scan'208";a="337723187" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Jun 2023 09:09:01 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10739"; a="801072508" X-IronPort-AV: E=Sophos;i="6.00,236,1681196400"; d="scan'208";a="801072508" Received: from spmantha-mobl1.amr.corp.intel.com (HELO [10.209.43.2]) ([10.209.43.2]) by fmsmga003-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Jun 2023 09:08:59 -0700 Message-ID: <59d5ca67-6a31-1929-8d2f-0e3314626893@intel.com> Date: Mon, 12 Jun 2023 09:08:58 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCH RFC v9 09/51] x86/sev: Add RMP entry lookup helpers Content-Language: en-US To: Michael Roth , kvm@vger.kernel.org Cc: linux-coco@lists.linux.dev, linux-mm@kvack.org, linux-crypto@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org, tglx@linutronix.de, mingo@redhat.com, jroedel@suse.de, thomas.lendacky@amd.com, hpa@zytor.com, ardb@kernel.org, pbonzini@redhat.com, seanjc@google.com, vkuznets@redhat.com, jmattson@google.com, luto@kernel.org, dave.hansen@linux.intel.com, slp@redhat.com, pgonda@google.com, peterz@infradead.org, srinivas.pandruvada@linux.intel.com, rientjes@google.com, dovmurik@linux.ibm.com, tobin@ibm.com, bp@alien8.de, vbabka@suse.cz, kirill@shutemov.name, ak@linux.intel.com, tony.luck@intel.com, marcorr@google.com, sathyanarayanan.kuppuswamy@linux.intel.com, alpergun@google.com, dgilbert@redhat.com, jarkko@kernel.org, ashish.kalra@amd.com, nikunj.dadhania@amd.com, liam.merwick@oracle.com, zhi.a.wang@intel.com, Brijesh Singh References: <20230612042559.375660-1-michael.roth@amd.com> <20230612042559.375660-10-michael.roth@amd.com> From: Dave Hansen In-Reply-To: <20230612042559.375660-10-michael.roth@amd.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: BF1021C0009 X-Stat-Signature: kpwrhd5q9b3d7nb8pr6zkfzjehzyouxp X-Rspam-User: X-HE-Tag: 1686586335-261982 X-HE-Meta: U2FsdGVkX19DeOviLGmI1Mbr9bTSuMZ+9vHOtvvvOwx0d4JPcwua0ybO6crToJIIQmCjfehE8wc/Xy7l/MB7csQFtjoQF3IcPWVqzXqdy7XawNzrLyh8KDwa+U0dSEnlSjT7S9dDkxjRPXt5XXACUaolN3m6NUQHJJGk7eIFs4AVDJyKsTUnifuSB/YW2WF4nC4I8EEgdh9FyqkAJ/oJumtvCHICZJaL3Iam7TSxTVLRK9ZWkwbpdqxc/1N2vo0SNmHcVvNrYPNi6AM2K7VVqDmSnKQLFBuyMWZGxeZFVr1qdu+dDt37+E/0yKGYlkcZe6KMbeLlFxAlw3gaqsriN/WnzZ2CDaUgFRdp8NMJMzHJe+NL+tXMQFymHb3ra2p+JNsn9cGxVTymeghENI98okT4wAL3nofRkGUkrU6TYL4X95QLwt9Io5n2Tsse6YXyylX/EQmkTx9HhBZd8Q2Ab/4gqsvt7m+NyDbJGI+XookEkrtXSnBu0jGfvbvLcHFocXh4AqW8gxd34tabI53Q4nN0uOdcVinjbZMOd045hnILqlORDGMVT83ZT38EBVFtpPRcL3GDVey9VnRCQXhrqzL4vCukQ50u1zM++tfWNTeglcVoC6Ip08d0z9xENmgV23w78NzhgP45mefwPPX1guiyvSngDtCgYWsAl88kgHX+yJ3iEirniI3ykeO5eiNfGyO39bhfSRTm2bAWpfIGrrbxvSUA1KhJl+96omPBp6QhVTWlM85mVAohZt3zzA80jwUfFCkq/lgP1oxLrWsIow3QF0FLvJOWxvCGsu2r3GskLtu+I2UV7rl8V5HicSYTbSoLE1mfIErEjXcGD3fB+LeVpRZdHwqClI4dqpriVTNpQNf/ADkU0PxzG3tIkdKCXUQ/YKTctZ/H+R1TrrhXWR+OsWNqV1bacOuduTOEAsvblNyzoPgCEm+Qjxo3gaXvtYKtEcP7mnRVdhw3tfx f9Y6nzci jbtzQAs+Jga6Zv0DmpPCYRMjga16+pD6OLoZ8VWDZpKEKe/rvtYE8/fPgvW5JgdpNHzGFgE4L8k9HFdjJTSzB5CGuFKQ4YXBsxmQ0fFAu0jMnd64VGQb61YFgWZR9OY+SWyk03SkWZ9cbqDs0bW1VsRslSEq3FDqJ70ECD9tmrmOH1QU= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 6/11/23 21:25, Michael Roth wrote: > +/* > + * The RMP entry format is not architectural. The format is defined in PPR > + * Family 19h Model 01h, Rev B1 processor. > + */ > +struct rmpentry { > + union { > + struct { > + u64 assigned : 1, > + pagesize : 1, > + immutable : 1, > + rsvd1 : 9, > + gpa : 39, > + asid : 10, > + vmsa : 1, > + validated : 1, > + rsvd2 : 1; > + } info; > + u64 low; > + }; > + u64 high; > +} __packed; What's 'high' used for? The PPR says it's reserved. Why not call it reserved? It _looks_ like it's only used for a debugging pr_info(). It makes the struct look kinda goofy. I'd much rather limit the goofiness to the "dumping" code, like: u64 *__e = (void *)e; .... pr_info("RMPEntry paddr 0x%llx: [high=0x%016llx low=0x%016llx]\n", pfn << PAGE_SHIFT, __e[0], __e[1]); BTW, why does it do any good to dump all these reserved fields? > /* > * The first 16KB from the RMP_BASE is used by the processor for the > * bookkeeping, the range needs to be added during the RMP entry lookup. > */ > #define RMPTABLE_CPU_BOOKKEEPING_SZ 0x4000 > +#define RMPENTRY_SHIFT 8 > +#define rmptable_page_offset(x) (RMPTABLE_CPU_BOOKKEEPING_SZ + \ > + (((unsigned long)x) >> RMPENTRY_SHIFT)) Is RMPENTRY_SHIFT just log2(sizeof(struct rmpentry))? If so, wouldn't this be a *LOT* more obvious to be written: unsigned long rmptable_offset(unsigned long paddr) { return RMPTABLE_CPU_BOOKKEEPING_SZ + paddr / sizeof(struct rmpentry); } ?? It removes a cast, gives 'x' a real name, removes a random magic number #define and is clearer to boot. > static unsigned long rmptable_start __ro_after_init; > static unsigned long rmptable_end __ro_after_init; > @@ -210,3 +235,63 @@ static int __init snp_rmptable_init(void) > * the page(s) used for DMA are hypervisor owned. > */ > fs_initcall(snp_rmptable_init); > + > +static inline unsigned int rmpentry_assigned(const struct rmpentry *e) > +{ > + return e->info.assigned; > +} > + > +static inline unsigned int rmpentry_pagesize(const struct rmpentry *e) > +{ > + return e->info.pagesize; > +} I think these are a little silly. If you're going to go to the trouble of using bitfields, you don't need helpers for every bitfield. I say either use bitfields without helpers or just regular old: #define RMPENTRY_ASSIGNED_MASK BIT(1) and then *maybe* you can make helpers for them. > +static int rmptable_entry(unsigned long paddr, struct rmpentry *entry) > +{ > + unsigned long vaddr; > + > + vaddr = rmptable_start + rmptable_page_offset(paddr); > + if (unlikely(vaddr > rmptable_end)) > + return -EFAULT; This seems like more of a WARN_ON_ONCE() kind of thing than just an error return. Isn't it a big deal if an invalid (non-RMP-covered) address makes it in here? > + *entry = *(struct rmpentry *)vaddr; > + > + return 0; > +} > + > +static int __snp_lookup_rmpentry(u64 pfn, struct rmpentry *entry, int *level) > +{ I've been bitten by pfn vs. paddr quite a few times. I'd really encourage you to add it to the names if you're going to pass them around, like __snp_lookup_rmpentry_pfn(). > + unsigned long paddr = pfn << PAGE_SHIFT; > + struct rmpentry large_entry; > + int ret; > + > + if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) > + return -ENXIO; OK, so if you're running on non-SNP hardware, you return -ENXIO here. Remember this please. > + ret = rmptable_entry(paddr, entry); > + if (ret) > + return ret; > + > + /* Read a large RMP entry to get the correct page level used in RMP entry. */ > + ret = rmptable_entry(paddr & PMD_MASK, &large_entry); > + if (ret) > + return ret; > + > + *level = RMP_TO_X86_PG_LEVEL(rmpentry_pagesize(&large_entry)); > + > + return 0; > +} This is a bit weird. Should it say something like this? To do an 4k RMP lookup the hardware looks at two places in the RMP: 1. The actual 4k RMP entry 2. The 2M entry that "covers" the 4k entry The page size is not indicated in the 4k entries at all. It is solely indicated by the 2M entry. > +int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) > +{ > + struct rmpentry e; > + int ret; > + > + ret = __snp_lookup_rmpentry(pfn, &e, level); > + if (ret) > + return ret; > + > + *assigned = !!rmpentry_assigned(&e); > + return 0; > +} > +EXPORT_SYMBOL_GPL(snp_lookup_rmpentry); > diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h > index b8357d6ecd47..bf0378136289 100644 > --- a/arch/x86/include/asm/sev-common.h > +++ b/arch/x86/include/asm/sev-common.h > @@ -171,4 +171,8 @@ struct snp_psc_desc { > #define GHCB_ERR_INVALID_INPUT 5 > #define GHCB_ERR_INVALID_EVENT 6 > > +/* RMP page size */ > +#define RMP_PG_SIZE_4K 0 > +#define RMP_TO_X86_PG_LEVEL(level) (((level) == RMP_PG_SIZE_4K) ? PG_LEVEL_4K : PG_LEVEL_2M) > + > #endif > diff --git a/arch/x86/include/asm/sev-host.h b/arch/x86/include/asm/sev-host.h > new file mode 100644 > index 000000000000..30d47e20081d > --- /dev/null > +++ b/arch/x86/include/asm/sev-host.h > @@ -0,0 +1,22 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * AMD SVM-SEV Host Support. > + * > + * Copyright (C) 2023 Advanced Micro Devices, Inc. > + * > + * Author: Ashish Kalra > + * > + */ > + > +#ifndef __ASM_X86_SEV_HOST_H > +#define __ASM_X86_SEV_HOST_H > + > +#include > + > +#ifdef CONFIG_KVM_AMD_SEV > +int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level); > +#else > +static inline int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) { return 0; } > +#endif Above, -ENXIO was returned when SEV-SNP was not supported. Here, 0 is returned when it is compiled out. That inconsistent. Is snp_lookup_rmpentry() acceptable when SEV-SNP is in play? I'd like to see consistency between when it is compiled out and when it is compiled in but unsupported on the CPU. > +#endif > diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h > index d34c46db7dd1..446fc7a9f7b0 100644 > --- a/arch/x86/include/asm/sev.h > +++ b/arch/x86/include/asm/sev.h > @@ -81,9 +81,6 @@ extern bool handle_vc_boot_ghcb(struct pt_regs *regs); > /* Software defined (when rFlags.CF = 1) */ > #define PVALIDATE_FAIL_NOUPDATE 255 > > -/* RMP page size */ > -#define RMP_PG_SIZE_4K 0 > - > #define RMPADJUST_VMSA_PAGE_BIT BIT(16) > > /* SNP Guest message request */