[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240416144542.GFZh6PFjPNT9Zt3iUl@fat_crate.local>
Date: Tue, 16 Apr 2024 16:45:42 +0200
From: Borislav Petkov <bp@...en8.de>
To: Nikunj A Dadhania <nikunj@....com>
Cc: linux-kernel@...r.kernel.org, thomas.lendacky@....com, x86@...nel.org,
kvm@...r.kernel.org, mingo@...hat.com, tglx@...utronix.de,
dave.hansen@...ux.intel.com, pgonda@...gle.com, seanjc@...gle.com,
pbonzini@...hat.com
Subject: Re: [PATCH v8 05/16] x86/sev: Cache the secrets page address
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
Powered by blists - more mailing lists