Received-SPF: pass (google.com: domain of linux-kernel+bounces-122848-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <f6bb6f62-c114-4a82-bbaf-9994da8999cd@linux.microsoft.com>
Date: Thu, 28 Mar 2024 12:51:17 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 5/5] x86/CPU/AMD: Track SNP host status with
 cc_platform_*()
To: Borislav Petkov <bp@alien8.de>, X86 ML <x86@kernel.org>
Cc: LKML <linux-kernel@vger.kernel.org>, KVM <kvm@vger.kernel.org>,
 Ashish Kalra <ashish.kalra@amd.com>, Joerg Roedel <joro@8bytes.org>,
 Michael Roth <michael.roth@amd.com>, Tom Lendacky <thomas.lendacky@amd.com>
References: <20240327154317.29909-1-bp@alien8.de>
 <20240327154317.29909-6-bp@alien8.de>
Content-Language: en-CA
From: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
In-Reply-To: <20240327154317.29909-6-bp@alien8.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 27/03/2024 16:43, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> 
> The host SNP worthiness can determined later, after alternatives have
> been patched, in snp_rmptable_init() depending on cmdline options like
> iommu=pt which is incompatible with SNP, for example.
> 
> Which means that one cannot use X86_FEATURE_SEV_SNP and will need to
> have a special flag for that control.
> 
> Use that newly added CC_ATTR_HOST_SEV_SNP in the appropriate places.
> 
> Move kdump_sev_callback() to its rightfull place, while at it.
> 
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> ---
>  arch/x86/include/asm/sev.h         |  4 ++--
>  arch/x86/kernel/cpu/amd.c          | 38 ++++++++++++++++++------------
>  arch/x86/kernel/cpu/mtrr/generic.c |  2 +-
>  arch/x86/kernel/sev.c              | 10 --------
>  arch/x86/kvm/svm/sev.c             |  2 +-
>  arch/x86/virt/svm/sev.c            | 26 +++++++++++++-------
>  drivers/crypto/ccp/sev-dev.c       |  2 +-
>  drivers/iommu/amd/init.c           |  4 +++-
>  8 files changed, 49 insertions(+), 39 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 9477b4053bce..780182cda3ab 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -228,7 +228,6 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct sn
>  void snp_accept_memory(phys_addr_t start, phys_addr_t end);
>  u64 snp_get_unsupported_features(u64 status);
>  u64 sev_get_status(void);
> -void kdump_sev_callback(void);
>  void sev_show_status(void);
>  #else
>  static inline void sev_es_ist_enter(struct pt_regs *regs) { }
> @@ -258,7 +257,6 @@ static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *in
>  static inline void snp_accept_memory(phys_addr_t start, phys_addr_t end) { }
>  static inline u64 snp_get_unsupported_features(u64 status) { return 0; }
>  static inline u64 sev_get_status(void) { return 0; }
> -static inline void kdump_sev_callback(void) { }
>  static inline void sev_show_status(void) { }
>  #endif
>  
> @@ -270,6 +268,7 @@ int psmash(u64 pfn);
>  int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, u32 asid, bool immutable);
>  int rmp_make_shared(u64 pfn, enum pg_level level);
>  void snp_leak_pages(u64 pfn, unsigned int npages);
> +void kdump_sev_callback(void);
>  #else
>  static inline bool snp_probe_rmptable_info(void) { return false; }
>  static inline int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) { return -ENODEV; }
> @@ -282,6 +281,7 @@ static inline int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, u32 as
>  }
>  static inline int rmp_make_shared(u64 pfn, enum pg_level level) { return -ENODEV; }
>  static inline void snp_leak_pages(u64 pfn, unsigned int npages) {}
> +static inline void kdump_sev_callback(void) { }
>  #endif
>  
>  #endif
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 6d8677e80ddb..9bf17c9c29da 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -345,6 +345,28 @@ static void srat_detect_node(struct cpuinfo_x86 *c)
>  #endif
>  }
>  
> +static void bsp_determine_snp(struct cpuinfo_x86 *c)
> +{
> +#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
> +	cc_vendor = CC_VENDOR_AMD;

Shouldn't this line be inside the cpu_has(c, X86_FEATURE_SEV_SNP) check?

> +
> +	if (cpu_has(c, X86_FEATURE_SEV_SNP)) {
> +		/*
> +		 * RMP table entry format is not architectural and is defined by the
> +		 * per-processor PPR. Restrict SNP support on the known CPU models
> +		 * for which the RMP table entry format is currently defined for.
> +		 */> +		if (!cpu_has(c, X86_FEATURE_HYPERVISOR) &&

How about turning this into a more specific check:

  if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP) &&

Thanks,
Jeremi

> +		    c->x86 >= 0x19 && snp_probe_rmptable_info()) {
> +			cc_platform_set(CC_ATTR_HOST_SEV_SNP);
> +		} else {
> +			setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
> +			cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
> +		}
> +	}
> +#endif
> +}
> +
>  static void bsp_init_amd(struct cpuinfo_x86 *c)
>  {
>  	if (cpu_has(c, X86_FEATURE_CONSTANT_TSC)) {
> @@ -452,21 +474,7 @@ static void bsp_init_amd(struct cpuinfo_x86 *c)
>  		break;
>  	}
>  
> -	if (cpu_has(c, X86_FEATURE_SEV_SNP)) {
> -		/*
> -		 * RMP table entry format is not architectural and it can vary by processor
> -		 * and is defined by the per-processor PPR. Restrict SNP support on the
> -		 * known CPU model and family for which the RMP table entry format is
> -		 * currently defined for.
> -		 */
> -		if (!boot_cpu_has(X86_FEATURE_ZEN3) &&
> -		    !boot_cpu_has(X86_FEATURE_ZEN4) &&
> -		    !boot_cpu_has(X86_FEATURE_ZEN5))
> -			setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
> -		else if (!snp_probe_rmptable_info())
> -			setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
> -	}
> -
> +	bsp_determine_snp(c);
>  	return;
>  
>  warn: