From: "Nikunj A. Dadhania" <nikunj@amd.com>
To: Borislav Petkov <bp@alien8.de>
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
Date: Wed, 17 Apr 2024 10:57:46 +0530 [thread overview]
Message-ID: <c08e7493-2fde-4624-9395-d1b8b5ab47c7@amd.com> (raw)
In-Reply-To: <20240416144542.GFZh6PFjPNT9Zt3iUl@fat_crate.local>
On 4/16/2024 8:15 PM, Borislav Petkov wrote:
> 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?),
Yes, this is used during runtime, during initial boot will be used by Secure TSC and later by sev-guest driver.
> 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?
Yes, that is a good idea. If I map in sev.c, what is the right place to iounmap ? Is it safe to keep it mapped until reboot/shutdown ?
> 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?
Sure, I will change it.
>
> Also, can the driver continue without secrets page?
No.
> If not, then you need to unwind.
>
By unwind, do you mean unmapping in the driver?
>> 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().
snp_init() is called from sme_enable(), and does not handle failure from snp_init()
How about the below diff?
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index e9925df21010..5e052f972138 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -114,7 +114,7 @@ struct snp_req_data {
};
struct sev_guest_platform_data {
- u64 secrets_gpa;
+ void *secrets_page;
};
/*
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 95003b809438..14c88e4f98ba 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -90,6 +90,9 @@ static struct ghcb *boot_ghcb __section(".data");
/* Bitmap of SEV features supported by the hypervisor */
static u64 sev_hv_features __ro_after_init;
+/* Secrets page address mapped from the CC blob physical address */
+static void *secrets_page __ro_after_init;
+
/* #VC handler runtime per-CPU data */
struct sev_es_runtime_data {
struct ghcb ghcb_page;
@@ -616,54 +619,16 @@ void noinstr __sev_es_nmi_complete(void)
__sev_put_ghcb(&state);
}
-static u64 __init get_secrets_page(void)
-{
- u64 pa_data = boot_params.cc_blob_address;
- struct cc_blob_sev_info info;
- void *map;
-
- /*
- * The CC blob contains the address of the secrets page, check if the
- * blob is present.
- */
- if (!pa_data)
- return 0;
-
- map = early_memremap(pa_data, sizeof(info));
- if (!map) {
- pr_err("Unable to locate SNP secrets page: failed to map the Confidential Computing blob.\n");
- return 0;
- }
- memcpy(&info, map, sizeof(info));
- early_memunmap(map, sizeof(info));
-
- /* smoke-test the secrets page passed */
- if (!info.secrets_phys || info.secrets_len != PAGE_SIZE)
- return 0;
-
- return info.secrets_phys;
-}
-
static u64 __init get_snp_jump_table_addr(void)
{
struct snp_secrets_page_layout *layout;
- void __iomem *mem;
- u64 pa, addr;
-
- pa = get_secrets_page();
- if (!pa)
- return 0;
+ u64 addr;
- mem = ioremap_encrypted(pa, PAGE_SIZE);
- if (!mem) {
- pr_err("Unable to locate AP jump table address: failed to map the SNP secrets page.\n");
+ if (!secrets_page)
return 0;
- }
-
- layout = (__force struct snp_secrets_page_layout *)mem;
+ layout = (__force struct snp_secrets_page_layout *)secrets_page;
addr = layout->os_area.ap_jump_table_pa;
- iounmap(mem);
return addr;
}
@@ -2118,6 +2083,14 @@ bool __init snp_init(struct boot_params *bp)
if (!cc_info)
return false;
+ if (cc_info->secrets_phys && cc_info->secrets_len == PAGE_SIZE) {
+ secrets_page = ioremap_encrypted(cc_info->secrets_phys, PAGE_SIZE);
+ if (!secrets_page) {
+ pr_err("Unable to map secrets page\n");
+ return false;
+ }
+ }
+
setup_cpuid_table(cc_info);
/*
@@ -2265,16 +2238,11 @@ 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 (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP) || !secrets_page)
return -ENODEV;
- data.secrets_gpa = gpa;
+ data.secrets_page = secrets_page;
if (platform_device_add_data(&sev_guest_device, &data, sizeof(data)))
return -ENODEV;
diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index dbc04229f7ac..4cef4e108130 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -820,12 +820,10 @@ static void unregister_sev_tsm(void *data)
static int __init sev_guest_probe(struct platform_device *pdev)
{
- struct snp_secrets_page_layout *layout;
struct sev_guest_platform_data *data;
struct device *dev = &pdev->dev;
struct snp_guest_dev *snp_dev;
struct miscdevice *misc;
- void __iomem *mapping;
int ret;
if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
@@ -835,28 +833,24 @@ static int __init sev_guest_probe(struct platform_device *pdev)
return -ENODEV;
data = (struct sev_guest_platform_data *)dev->platform_data;
- mapping = ioremap_encrypted(data->secrets_gpa, PAGE_SIZE);
- if (!mapping)
+ if (!data->secrets_page)
return -ENODEV;
- layout = (__force void *)mapping;
-
- ret = -ENOMEM;
snp_dev = devm_kzalloc(&pdev->dev, sizeof(struct snp_guest_dev), GFP_KERNEL);
if (!snp_dev)
- goto e_unmap;
+ return -ENOMEM;
ret = -EINVAL;
- snp_dev->layout = layout;
+ snp_dev->layout = (__force struct snp_secrets_page_layout *)data->secrets_page;
if (!snp_assign_vmpck(snp_dev, vmpck_id)) {
dev_err(dev, "invalid vmpck id %u\n", vmpck_id);
- goto e_unmap;
+ return ret;
}
/* Verify that VMPCK is not zero. */
if (snp_is_vmpck_empty(snp_dev)) {
dev_err(dev, "vmpck id %u is null\n", vmpck_id);
- goto e_unmap;
+ return ret;
}
platform_set_drvdata(pdev, snp_dev);
@@ -865,7 +859,7 @@ static int __init sev_guest_probe(struct platform_device *pdev)
/* Allocate secret request and response message for double buffering */
snp_dev->secret_request = kzalloc(SNP_GUEST_MSG_SIZE, GFP_KERNEL);
if (!snp_dev->secret_request)
- goto e_unmap;
+ return ret;
snp_dev->secret_response = kzalloc(SNP_GUEST_MSG_SIZE, GFP_KERNEL);
if (!snp_dev->secret_response)
@@ -925,8 +919,6 @@ static int __init sev_guest_probe(struct platform_device *pdev)
kfree(snp_dev->secret_response);
e_free_secret_req:
kfree(snp_dev->secret_request);
-e_unmap:
- iounmap(mapping);
return ret;
}
Regards
Nikunj
next prev parent reply other threads:[~2024-04-17 5:27 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-15 11:31 [PATCH v8 00/16] Add Secure TSC support for SNP guests Nikunj A Dadhania
2024-02-15 11:31 ` [PATCH v8 01/16] virt: sev-guest: Use AES GCM crypto library Nikunj A Dadhania
2024-02-27 18:25 ` Borislav Petkov
2024-02-15 11:31 ` [PATCH v8 02/16] virt: sev-guest: Replace dev_dbg with pr_debug Nikunj A Dadhania
2024-02-27 18:28 ` Borislav Petkov
2024-02-15 11:31 ` [PATCH v8 03/16] virt: sev-guest: Add SNP guest request structure Nikunj A Dadhania
2024-02-27 22:20 ` Tom Lendacky
2024-02-29 9:12 ` Nikunj A. Dadhania
2024-02-28 11:50 ` Borislav Petkov
2024-02-29 9:26 ` Nikunj A. Dadhania
2024-02-15 11:31 ` [PATCH v8 04/16] virt: sev-guest: Add vmpck_id to snp_guest_dev struct Nikunj A Dadhania
2024-04-09 10:23 ` Borislav Petkov
2024-04-16 5:57 ` Nikunj A. Dadhania
2024-04-16 9:06 ` Borislav Petkov
2024-04-17 4:18 ` Nikunj A. Dadhania
2024-02-15 11:31 ` [PATCH v8 05/16] x86/sev: Cache the secrets page address Nikunj A Dadhania
2024-04-16 14:45 ` Borislav Petkov
2024-04-17 5:27 ` Nikunj A. Dadhania [this message]
2024-04-17 7:59 ` Nikunj A. Dadhania
2024-02-15 11:31 ` [PATCH v8 06/16] virt: sev-guest: Move SNP Guest command mutex Nikunj A Dadhania
2024-04-22 13:00 ` Borislav Petkov
2024-04-23 4:22 ` Nikunj A. Dadhania
2024-04-23 10:28 ` Borislav Petkov
2024-04-23 10:42 ` Nikunj A. Dadhania
2024-04-23 11:21 ` Borislav Petkov
2024-02-15 11:31 ` [PATCH v8 07/16] x86/sev: Move and reorganize sev guest request api Nikunj A Dadhania
2024-04-22 13:14 ` Borislav Petkov
2024-04-23 4:26 ` Nikunj A. Dadhania
2024-04-23 13:22 ` Borislav Petkov
2024-02-15 11:31 ` [PATCH v8 08/16] x86/mm: Add generic guest initialization hook Nikunj A Dadhania
2024-04-22 13:20 ` Borislav Petkov
2024-04-23 4:34 ` Nikunj A. Dadhania
2024-02-15 11:31 ` [PATCH v8 09/16] x86/cpufeatures: Add synthetic Secure TSC bit Nikunj A Dadhania
2024-04-22 13:25 ` Borislav Petkov
2024-04-23 4:40 ` Nikunj A. Dadhania
2024-04-23 13:29 ` Borislav Petkov
2024-02-15 11:31 ` [PATCH v8 10/16] x86/sev: Add Secure TSC support for SNP guests Nikunj A Dadhania
2024-04-22 13:50 ` Borislav Petkov
2024-04-23 4:44 ` Nikunj A. Dadhania
2024-02-15 11:31 ` [PATCH v8 11/16] x86/sev: Change TSC MSR behavior for Secure TSC enabled guests Nikunj A Dadhania
2024-02-15 11:31 ` [PATCH v8 12/16] x86/sev: Prevent RDTSC/RDTSCP interception " Nikunj A Dadhania
2024-02-15 11:31 ` [PATCH v8 13/16] x86/kvmclock: Skip kvmclock when Secure TSC is available Nikunj A Dadhania
2024-02-15 11:31 ` [PATCH v8 14/16] x86/sev: Mark Secure TSC as reliable Nikunj A Dadhania
2024-02-15 11:31 ` [PATCH v8 15/16] x86/cpu/amd: Do not print FW_BUG for Secure TSC Nikunj A Dadhania
2024-02-15 11:31 ` [PATCH v8 16/16] x86/sev: Enable Secure TSC for SNP guests Nikunj A Dadhania
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c08e7493-2fde-4624-9395-d1b8b5ab47c7@amd.com \
--to=nikunj@amd.com \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=pbonzini@redhat.com \
--cc=pgonda@google.com \
--cc=seanjc@google.com \
--cc=tglx@linutronix.de \
--cc=thomas.lendacky@amd.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).