Received-SPF: pass (google.com: domain of linux-kernel+bounces-147051-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 16 Apr 2024 16:45:42 +0200
From: Borislav Petkov <bp@alien8.de>
To: Nikunj A Dadhania <nikunj@amd.com>
Cc: linux-kernel@vger.kernel.org, thomas.lendacky@amd.com, x86@kernel.org,
	kvm@vger.kernel.org, mingo@redhat.com, tglx@linutronix.de,
	dave.hansen@linux.intel.com, pgonda@google.com, seanjc@google.com,
	pbonzini@redhat.com
Subject: Re: [PATCH v8 05/16] x86/sev: Cache the secrets page address
Message-ID: <20240416144542.GFZh6PFjPNT9Zt3iUl@fat_crate.local>
References: <20240215113128.275608-1-nikunj@amd.com>
 <20240215113128.275608-6-nikunj@amd.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240215113128.275608-6-nikunj@amd.com>

On Thu, Feb 15, 2024 at 05:01:17PM +0530, Nikunj A Dadhania wrote:
> +/* Secrets page physical address from the CC blob */
> +static u64 secrets_pa __ro_after_init;

Since you're going to use this during runtime (are you?), why don't you
put in here the result of:

	ioremap_encrypted(secrets_pa, PAGE_SIZE);

so that you can have it ready and not even have to ioremap each time?

And then you iounmap on driver teardown.

> +static void __init set_secrets_pa(const struct cc_blob_sev_info *cc_info)
> +{
> +	if (cc_info && cc_info->secrets_phys && cc_info->secrets_len == PAGE_SIZE)
> +		secrets_pa = cc_info->secrets_phys;
> +}

Why is this a separate function if it is called only once and it is
a trivial function at that?

Also, can the driver continue without secrets page?

If not, then you need to unwind.

>  bool __init snp_init(struct boot_params *bp)
>  {
>  	struct cc_blob_sev_info *cc_info;
> @@ -2099,6 +2079,8 @@ bool __init snp_init(struct boot_params *bp)
>  	if (!cc_info)
>  		return false;
>  
> +	set_secrets_pa(cc_info);
> +
>  	setup_cpuid_table(cc_info);
>  
>  	/*
> @@ -2246,16 +2228,16 @@ static struct platform_device sev_guest_device = {
>  static int __init snp_init_platform_device(void)
>  {
>  	struct sev_guest_platform_data data;
> -	u64 gpa;
>  
>  	if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
>  		return -ENODEV;
>  
> -	gpa = get_secrets_page();
> -	if (!gpa)
> +	if (!secrets_pa) {
> +		pr_err("SNP secrets page not found\n");
>  		return -ENODEV;
> +	}

Yeah, no, you need to error out in snp_init() and not drag it around to
snp_init_platform_device().

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette