2023-01-27 02:53:08

by Jarkko Sakkinen

[permalink] [raw]
Subject: [PATCH RFC 3/8] KVM: SVM: write back corrected CPUID page

From: Tom Dohrmann <[email protected]>

When doing a launch update for a CPUID page the firmware checks that the values
conform to the policy laid out in the processor programming manual. If the
values don't conform, the firmware will return SEV_RET_INVALID_PARAM.
In addition to returning an error the firmware will choose some acceptable
values and write them back to the page that was used for the launch update, so
that the VMM can inspect the changes and try again with the corrected values.
This is specified in section 8.17.2.6 in the SEV-SNP Firmware ABI spec.
Because launch updates are always done on the private UPM mappings, the pages
are first copied from the shared mappings to the private mappings. When the
firmware corrects the values, the corrected values are in the private mappings,
inaccessible to userspace. In order to make the corrected values accessible to
userspace, the page containing them must be copied from the private mappings
back to the shared mappings.

[[email protected]: fixed checkpatch.pl errors]
Link: https://lore.kernel.org/lkml/Y76%2FI6Nrh7xEAAwv@notebook/
Signed-off-by: Tom Dohrmann <[email protected]>
Signed-off-by: Jarkko Sakkinen <[email protected]>
---
arch/x86/kvm/svm/sev.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 6d3162853c33..4a8e552d8cfe 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2230,6 +2230,23 @@ static int snp_launch_update_gfn_handler(struct kvm *kvm,
pr_err("SEV-SNP launch update failed, ret: 0x%x, fw_error: 0x%x\n",
ret, *error);
snp_page_reclaim(pfns[i]);
+
+ /*
+ * When invalid CPUID function entries are detected, the firmware
+ * corrects these entries. In that case write the page back to
+ * userspace.
+ */
+ if (params.page_type == SNP_PAGE_TYPE_CPUID &&
+ *error == SEV_RET_INVALID_PARAM) {
+ int ret;
+
+ host_rmp_make_shared(pfns[i], PG_LEVEL_4K, true);
+
+ ret = kvm_write_guest_page(kvm, gfn, kvaddr, 0, PAGE_SIZE);
+ if (ret)
+ pr_err("Guest write failed, ret: 0x%x\n", ret);
+ }
+
goto e_release;
}
}
--
2.38.1



2023-01-27 22:46:49

by Tom Lendacky

[permalink] [raw]
Subject: Re: [PATCH RFC 3/8] KVM: SVM: write back corrected CPUID page

On 1/26/23 20:52, Jarkko Sakkinen wrote:
> From: Tom Dohrmann <[email protected]>
>
> When doing a launch update for a CPUID page the firmware checks that the values
> conform to the policy laid out in the processor programming manual. If the
> values don't conform, the firmware will return SEV_RET_INVALID_PARAM.
> In addition to returning an error the firmware will choose some acceptable
> values and write them back to the page that was used for the launch update, so
> that the VMM can inspect the changes and try again with the corrected values.
> This is specified in section 8.17.2.6 in the SEV-SNP Firmware ABI spec.
> Because launch updates are always done on the private UPM mappings, the pages
> are first copied from the shared mappings to the private mappings. When the
> firmware corrects the values, the corrected values are in the private mappings,
> inaccessible to userspace. In order to make the corrected values accessible to
> userspace, the page containing them must be copied from the private mappings
> back to the shared mappings.
>
> [[email protected]: fixed checkpatch.pl errors]
> Link: https://lore.kernel.org/lkml/Y76%2FI6Nrh7xEAAwv@notebook/
> Signed-off-by: Tom Dohrmann <[email protected]>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> ---
> arch/x86/kvm/svm/sev.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 6d3162853c33..4a8e552d8cfe 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2230,6 +2230,23 @@ static int snp_launch_update_gfn_handler(struct kvm *kvm,
> pr_err("SEV-SNP launch update failed, ret: 0x%x, fw_error: 0x%x\n",
> ret, *error);
> snp_page_reclaim(pfns[i]);
> +
> + /*
> + * When invalid CPUID function entries are detected, the firmware
> + * corrects these entries. In that case write the page back to
> + * userspace.

I would additionally add that the firmware does not encrypt the page,
which allows the hypervisor to copy the page back to userspace.

Thanks,
Tom

> + */
> + if (params.page_type == SNP_PAGE_TYPE_CPUID &&
> + *error == SEV_RET_INVALID_PARAM) {
> + int ret;
> +
> + host_rmp_make_shared(pfns[i], PG_LEVEL_4K, true);
> +
> + ret = kvm_write_guest_page(kvm, gfn, kvaddr, 0, PAGE_SIZE);
> + if (ret)
> + pr_err("Guest write failed, ret: 0x%x\n", ret);
> + }
> +
> goto e_release;
> }
> }