RE: [RFC PATCH V2 02/18] x86/hyperv: Add sev-snp enlightened guest specific config

["Michael Kelley (LINUX)" <mikelley@microsoft.com>]

From: Tianyu Lan <ltykernel@gmail.com> Sent: Friday, November 18, 2022 7:46 PM
> 
> Introduce static key isolation_type_en_snp for enlightened
> guest check and add some specific options in ms_hyperv_init_
> platform().
> 
> Signed-off-by: Tianyu Lan <tiala@microsoft.com>
> ---
>  arch/x86/hyperv/ivm.c           | 12 +++++++++++-
>  arch/x86/include/asm/mshyperv.h |  2 ++
>  arch/x86/kernel/cpu/mshyperv.c  | 29 ++++++++++++++++++++++++-----
>  drivers/hv/hv_common.c          |  7 +++++++
>  4 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
> index 1dbcbd9da74d..e9c30dad3419 100644
> --- a/arch/x86/hyperv/ivm.c
> +++ b/arch/x86/hyperv/ivm.c
> @@ -259,10 +259,20 @@ bool hv_is_isolation_supported(void)
>  }
> 
>  DEFINE_STATIC_KEY_FALSE(isolation_type_snp);
> +DEFINE_STATIC_KEY_FALSE(isolation_type_en_snp);
> +
> +/*
> + * hv_isolation_type_en_snp - Check system runs in the AMD SEV-SNP based
> + * isolation enlightened VM.
> + */
> +bool hv_isolation_type_en_snp(void)
> +{
> +	return static_branch_unlikely(&isolation_type_en_snp);
> +}
> 
>  /*
>   * hv_isolation_type_snp - Check system runs in the AMD SEV-SNP based
> - * isolation VM.
> + * isolation VM with vTOM support.
>   */
>  bool hv_isolation_type_snp(void)
>  {
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 61f0c206bff0..9b8c3f638845 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -14,6 +14,7 @@
>  union hv_ghcb;
> 
>  DECLARE_STATIC_KEY_FALSE(isolation_type_snp);
> +DECLARE_STATIC_KEY_FALSE(isolation_type_en_snp);
> 
>  typedef int (*hyperv_fill_flush_list_func)(
>  		struct hv_guest_mapping_flush_list *flush,
> @@ -32,6 +33,7 @@ extern u64 hv_current_partition_id;
> 
>  extern union hv_ghcb * __percpu *hv_ghcb_pg;
> 
> +extern bool hv_isolation_type_en_snp(void);

This file also has a declaration for hv_isolation_type_snp().  I
think this new declaration is near the beginning of this file so
that it can be referenced by hv_do_hypercall() and related
functions in Patch 6 of this series.  So maybe move the
declaration of hv_isolation_type_snp() up so it is adjacent
to this one?  It would make sense for the two to be together.

>  int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages);
>  int hv_call_add_logical_proc(int node, u32 lp_index, u32 acpi_id);
>  int hv_call_create_vp(int node, u64 partition_id, u32 vp_index, u32 flags);
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 831613959a92..2ea4f21c6172 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -273,6 +273,21 @@ static void __init ms_hyperv_init_platform(void)
>  	ms_hyperv.misc_features = cpuid_edx(HYPERV_CPUID_FEATURES);
>  	ms_hyperv.hints    = cpuid_eax(HYPERV_CPUID_ENLIGHTMENT_INFO);
> 
> +	/*
> +	 * Add custom configuration for SEV-SNP Enlightened guest
> +	 */
> +	if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP)) {
> +		ms_hyperv.features |= HV_ACCESS_FREQUENCY_MSRS;
> +		ms_hyperv.misc_features |= HV_FEATURE_FREQUENCY_MSRS_AVAILABLE;
> +		ms_hyperv.misc_features &= ~HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE;
> +		ms_hyperv.hints |= HV_DEPRECATING_AEOI_RECOMMENDED;
> +		ms_hyperv.hints |= HV_X64_APIC_ACCESS_RECOMMENDED;
> +		ms_hyperv.hints |= HV_X64_CLUSTER_IPI_RECOMMENDED;
> +	}
> +
> +	pr_info("Hyper-V: enlightment features 0x%x, hints 0x%x, misc 0x%x\n",
> +		ms_hyperv.features, ms_hyperv.hints, ms_hyperv.misc_features);
> +

What's the reason for this additional call to pr_info()?  There's a call to pr_info()
a couple of lines below that displays the same information, and a little bit more.
It seems like the above call should be deleted, as I think we should try to be as
consistent as possible in the output.

>  	hv_max_functions_eax = cpuid_eax(HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS);
> 
>  	pr_info("Hyper-V: privilege flags low 0x%x, high 0x%x, hints 0x%x, misc 0x%x\n",
> @@ -328,18 +343,22 @@ static void __init ms_hyperv_init_platform(void)
>  		ms_hyperv.shared_gpa_boundary =
>  			BIT_ULL(ms_hyperv.shared_gpa_boundary_bits);
> 
> -		pr_info("Hyper-V: Isolation Config: Group A 0x%x, Group B 0x%x\n",
> -			ms_hyperv.isolation_config_a, ms_hyperv.isolation_config_b);
> -
> -		if (hv_get_isolation_type() == HV_ISOLATION_TYPE_SNP) {
> +		if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP)) {
> +			static_branch_enable(&isolation_type_en_snp);
> +		} else if (hv_get_isolation_type() == HV_ISOLATION_TYPE_SNP) {
>  			static_branch_enable(&isolation_type_snp);
>  #ifdef CONFIG_SWIOTLB
>  			swiotlb_unencrypted_base = ms_hyperv.shared_gpa_boundary;
>  #endif
>  		}
> +
> +		pr_info("Hyper-V: Isolation Config: Group A 0x%x, Group B 0x%x\n",
> +			ms_hyperv.isolation_config_a, ms_hyperv.isolation_config_b);
> +

Is there a reason for moving this pr_info() down a few lines?  I can't see that the
intervening code changes any of the settings that are displayed, so it should be
good in the original location.  Just trying to minimize changes that don't add value ...

>  		/* Isolation VMs are unenlightened SEV-based VMs, thus this check: */
>  		if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)) {
> -			if (hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE)
> +			if (hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE
> +			    && !cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
>  				cc_set_vendor(CC_VENDOR_HYPERV);
>  		}
>  	}
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index ae68298c0dca..2c6602571c47 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -268,6 +268,13 @@ bool __weak hv_isolation_type_snp(void)
>  }
>  EXPORT_SYMBOL_GPL(hv_isolation_type_snp);
> 
> +bool __weak hv_isolation_type_en_snp(void)
> +{
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(hv_isolation_type_en_snp);
> +
> +

Nit: One blank line is sufficient.  Don't need to add two.

>  void __weak hv_setup_vmbus_handler(void (*handler)(void))
>  {
>  }
> --
> 2.25.1