Messages in this thread | | | Date | Tue, 16 Apr 2024 16:45:42 +0200 | From | Borislav Petkov <> | 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
| |