Re: [PATCH v3 4/6] x86/sev: Add interface to re-enable RMP optimizations.

[Dave Hansen <dave.hansen@intel.com>]

The subject seems rather imprecise. This both adds a function to
"re-enable RMP optimizations" *AND* calls it.

> RMPOPT table is a per-processor table which indicates if 1GB regions of
> physical memory are entirely hypervisor-owned or not.

It's per-core, right? Why not just be precise about it?

> When performing host memory accesses in hypervisor mode as well as
> non-SNP guest mode, the processor may consult the RMPOPT table to
> potentially skip an RMP access and improve performance.
> 
> Events such as RMPUPDATE or SNP_INIT can clear RMP optimizations. Add
> an interface to re-enable those optimizations.


> +int snp_perform_rmp_optimization(void)
> +{
> +	if (!cpu_feature_enabled(X86_FEATURE_RMPOPT))
> +		return -EINVAL;
> +
> +	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
> +		return -EINVAL;
> +
> +	if (!(rmp_cfg & MSR_AMD64_SEG_RMP_ENABLED))
> +		return -EINVAL;

This seems wrong. How about we just make 'X86_FEATURE_RMPOPT' the one
true source of RMP support?

If you don't have CC_ATTR_HOST_SEV_SNP you:

	setup_clear_cpu_cap(X86_FEATURE_RMPOPT)

Ditto for MSR_AMD64_SEG_RMP_ENABLED.

It could also potentially replace the 'rmpopt_wq' checks.

> +	rmpopt_all_physmem(FALSE);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(snp_perform_rmp_optimization);
> +
>  void __snp_leak_pages(u64 pfn, unsigned int npages, bool dump_rmp)
>  {
>  	struct page *page = pfn_to_page(pfn);
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index aebf4dad545e..0cbe828d204c 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1476,6 +1476,10 @@ static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
>  	}
>  
>  	snp_hv_fixed_pages_state_update(sev, HV_FIXED);
> +
> +	/* SNP_INIT clears the RMPOPT table, re-enable RMP optimizations */
> +	snp_perform_rmp_optimization();

Ahhh, so this isn't happening at boot, it happens when kvm_amd.ko gets
loaded? That escaped me until now. It would be nice to mention
somewhere, please.

There is basically no naming difference between
snp_perform_rmp_optimization() and rmpopt_all_physmem(). Can you just
get this all down to a single function, please?

If you really have a reason to have a scan now and scan later mode, just
do this:

	rmpopt_all_physmem(RMPOPT_SCAN_NOW);

and:

	rmpopt_all_physmem(RMPOPT_SCAN_LATER);

*That* function can do the X86_FEATURE_RMPOPT check.