| Message ID | 7845d453af6344d0b156493eb4555399aad78615.1655761627.git.ashish.kalra@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) | expand |
On Mon, Jun 20, 2022 at 5:11 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote: > > From: Brijesh Singh <brijesh.singh@amd.com> > > On VMGEXIT, sev_handle_vmgexit() creates a host mapping for the GHCB GPA, > and unmaps it just before VM-entry. This long-lived GHCB map is used by > the VMGEXIT handler through accessors such as ghcb_{set_get}_xxx(). > > A long-lived GHCB map can cause issue when SEV-SNP is enabled. When > SEV-SNP is enabled the mapped GPA needs to be protected against a page > state change. > > To eliminate the long-lived GHCB mapping, update the GHCB sync operations > to explicitly map the GHCB before access and unmap it after access is > complete. This requires that the setting of the GHCBs sw_exit_info_{1,2} > fields be done during sev_es_sync_to_ghcb(), so create two new fields in > the vcpu_svm struct to hold these values when required to be set outside > of the GHCB mapping. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > arch/x86/kvm/svm/sev.c | 131 ++++++++++++++++++++++++++--------------- > arch/x86/kvm/svm/svm.c | 12 ++-- > arch/x86/kvm/svm/svm.h | 24 +++++++- > 3 files changed, 111 insertions(+), 56 deletions(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 01ea257e17d6..c70f3f7e06a8 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -2823,15 +2823,40 @@ void sev_free_vcpu(struct kvm_vcpu *vcpu) > kvfree(svm->sev_es.ghcb_sa); > } > > +static inline int svm_map_ghcb(struct vcpu_svm *svm, struct kvm_host_map *map) > +{ > + struct vmcb_control_area *control = &svm->vmcb->control; > + u64 gfn = gpa_to_gfn(control->ghcb_gpa); > + > + if (kvm_vcpu_map(&svm->vcpu, gfn, map)) { > + /* Unable to map GHCB from guest */ > + pr_err("error mapping GHCB GFN [%#llx] from guest\n", gfn); > + return -EFAULT; > + } > + > + return 0; > +} There is a perf cost to this suggestion but it might make accessing the GHCB safer for KVM. Have you thought about just using kvm_read_guest() or copy_from_user() to fully copy out the GCHB into a KVM owned buffer, then copying it back before the VMRUN. That way the KVM doesn't need to guard against page_state_changes on the GHCBs, that could be a perf improvement in a follow up. Since we cannot unmap GHCBs I don't think UPM will help here so we probably want to make these patches safe against malicious guests making GHCBs private. But maybe UPM does help?
[AMD Official Use Only - General] Hello Peter, >> From: Brijesh Singh <brijesh.singh@amd.com> >> >> On VMGEXIT, sev_handle_vmgexit() creates a host mapping for the GHCB >> GPA, and unmaps it just before VM-entry. This long-lived GHCB map is >> used by the VMGEXIT handler through accessors such as ghcb_{set_get}_xxx(). >> >> A long-lived GHCB map can cause issue when SEV-SNP is enabled. When >> SEV-SNP is enabled the mapped GPA needs to be protected against a page >> state change. >> >> To eliminate the long-lived GHCB mapping, update the GHCB sync >> operations to explicitly map the GHCB before access and unmap it after >> access is complete. This requires that the setting of the GHCBs >> sw_exit_info_{1,2} fields be done during sev_es_sync_to_ghcb(), so >> create two new fields in the vcpu_svm struct to hold these values when >> required to be set outside of the GHCB mapping. >> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> arch/x86/kvm/svm/sev.c | 131 >> ++++++++++++++++++++++++++--------------- >> arch/x86/kvm/svm/svm.c | 12 ++-- >> arch/x86/kvm/svm/svm.h | 24 +++++++- >> 3 files changed, 111 insertions(+), 56 deletions(-) >> >> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index >> 01ea257e17d6..c70f3f7e06a8 100644 >> --- a/arch/x86/kvm/svm/sev.c >> +++ b/arch/x86/kvm/svm/sev.c >> @@ -2823,15 +2823,40 @@ void sev_free_vcpu(struct kvm_vcpu *vcpu) >> kvfree(svm->sev_es.ghcb_sa); >> } >> >> +static inline int svm_map_ghcb(struct vcpu_svm *svm, struct >> +kvm_host_map *map) { >> + struct vmcb_control_area *control = &svm->vmcb->control; >> + u64 gfn = gpa_to_gfn(control->ghcb_gpa); >> + >> + if (kvm_vcpu_map(&svm->vcpu, gfn, map)) { >> + /* Unable to map GHCB from guest */ >> + pr_err("error mapping GHCB GFN [%#llx] from guest\n", gfn); >> + return -EFAULT; >> + } >> + >> + return 0; >> +} >There is a perf cost to this suggestion but it might make accessing the GHCB safer for KVM. Have you thought about just using >kvm_read_guest() or copy_from_user() to fully copy out the GCHB into a KVM owned buffer, then copying it back before the VMRUN. That way the KVM doesn't need to guard against page_state_changes on the GHCBs, that could be a perf ?>improvement in a follow up. Along with the performance costs you mentioned, the main concern here will be the GHCB write-back path (copying it back) before VMRUN: this will again hit the issue we have currently with kvm_write_guest() / copy_to_user(), when we use it to sync the scratch buffer back to GHCB. This can fail if guest RAM is mapped using huge-page(s) and RMP is 4K. Please refer to the patch/fix mentioned below, kvm_write_guest() potentially can fail before VMRUN in case of SNP : commit 94ed878c2669532ebae8eb9b4503f19aa33cd7aa Author: Ashish Kalra <ashish.kalra@amd.com> Date: Mon Jun 6 22:28:01 2022 +0000 KVM: SVM: Sync the GHCB scratch buffer using already mapped ghcb Using kvm_write_guest() to sync the GHCB scratch buffer can fail due to host mapping being 2M, but RMP being 4K. The page fault handling in do_user_addr_fault() fails to split the 2M page to handle RMP fault due to it being called here in a non-preemptible context. Instead use the already kernel mapped ghcb to sync the scratch buffer when the scratch buffer is contained within the GHCB. Thanks, Ashish >Since we cannot unmap GHCBs I don't think UPM will help here so we probably want to make these patches safe against malicious guests making GHCBs private. But maybe UPM does help?
On Fri, Jun 24, 2022 at 2:14 PM Kalra, Ashish <Ashish.Kalra@amd.com> wrote: > > [AMD Official Use Only - General] > > Hello Peter, > > >> From: Brijesh Singh <brijesh.singh@amd.com> > >> > >> On VMGEXIT, sev_handle_vmgexit() creates a host mapping for the GHCB > >> GPA, and unmaps it just before VM-entry. This long-lived GHCB map is > >> used by the VMGEXIT handler through accessors such as ghcb_{set_get}_xxx(). > >> > >> A long-lived GHCB map can cause issue when SEV-SNP is enabled. When > >> SEV-SNP is enabled the mapped GPA needs to be protected against a page > >> state change. > >> > >> To eliminate the long-lived GHCB mapping, update the GHCB sync > >> operations to explicitly map the GHCB before access and unmap it after > >> access is complete. This requires that the setting of the GHCBs > >> sw_exit_info_{1,2} fields be done during sev_es_sync_to_ghcb(), so > >> create two new fields in the vcpu_svm struct to hold these values when > >> required to be set outside of the GHCB mapping. > >> > >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > >> --- > >> arch/x86/kvm/svm/sev.c | 131 > >> ++++++++++++++++++++++++++--------------- > >> arch/x86/kvm/svm/svm.c | 12 ++-- > >> arch/x86/kvm/svm/svm.h | 24 +++++++- > >> 3 files changed, 111 insertions(+), 56 deletions(-) > >> > >> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index > >> 01ea257e17d6..c70f3f7e06a8 100644 > >> --- a/arch/x86/kvm/svm/sev.c > >> +++ b/arch/x86/kvm/svm/sev.c > >> @@ -2823,15 +2823,40 @@ void sev_free_vcpu(struct kvm_vcpu *vcpu) > >> kvfree(svm->sev_es.ghcb_sa); > >> } > >> > >> +static inline int svm_map_ghcb(struct vcpu_svm *svm, struct > >> +kvm_host_map *map) { > >> + struct vmcb_control_area *control = &svm->vmcb->control; > >> + u64 gfn = gpa_to_gfn(control->ghcb_gpa); > >> + > >> + if (kvm_vcpu_map(&svm->vcpu, gfn, map)) { > >> + /* Unable to map GHCB from guest */ > >> + pr_err("error mapping GHCB GFN [%#llx] from guest\n", gfn); > >> + return -EFAULT; > >> + } > >> + > >> + return 0; > >> +} > > >There is a perf cost to this suggestion but it might make accessing the GHCB safer for KVM. Have you thought about just using > >kvm_read_guest() or copy_from_user() to fully copy out the GCHB into a KVM owned buffer, then copying it back before the VMRUN. That way the KVM doesn't need to guard against page_state_changes on the GHCBs, that could be a perf ?>improvement in a follow up. > > Along with the performance costs you mentioned, the main concern here will be the GHCB write-back path (copying it back) before VMRUN: this will again hit the issue we have currently with > kvm_write_guest() / copy_to_user(), when we use it to sync the scratch buffer back to GHCB. This can fail if guest RAM is mapped using huge-page(s) and RMP is 4K. Please refer to the patch/fix > mentioned below, kvm_write_guest() potentially can fail before VMRUN in case of SNP : > > commit 94ed878c2669532ebae8eb9b4503f19aa33cd7aa > Author: Ashish Kalra <ashish.kalra@amd.com> > Date: Mon Jun 6 22:28:01 2022 +0000 > > KVM: SVM: Sync the GHCB scratch buffer using already mapped ghcb > > Using kvm_write_guest() to sync the GHCB scratch buffer can fail > due to host mapping being 2M, but RMP being 4K. The page fault handling > in do_user_addr_fault() fails to split the 2M page to handle RMP fault due > to it being called here in a non-preemptible context. Instead use > the already kernel mapped ghcb to sync the scratch buffer when the > scratch buffer is contained within the GHCB. Ah I didn't see that issue thanks for the pointer. The patch description says "When SEV-SNP is enabled the mapped GPA needs to be protected against a page state change." since if the guest were to convert the GHCB page to private when the host is using the GHCB the host could get an RMP violation right? That RMP violation would cause the host to crash unless we use some copy_to_user() type protections. I don't see anything mechanism for this patch to add the page state change protection discussed. Can't another vCPU still convert the GHCB to private? I was wrong about the importance of this though seanjc@ walked me through how UPM will solve this issue so no worries about this until the series is rebased on to UPM. > > Thanks, > Ashish > > >Since we cannot unmap GHCBs I don't think UPM will help here so we probably want to make these patches safe against malicious guests making GHCBs private. But maybe UPM does help?
[AMD Official Use Only - General] Hello Peter, >> >There is a perf cost to this suggestion but it might make accessing >> >the GHCB safer for KVM. Have you thought about just using >> >kvm_read_guest() or copy_from_user() to fully copy out the GCHB into a KVM owned buffer, then copying it back before the VMRUN. That way the KVM doesn't need to guard against page_state_changes on the GHCBs, that could be a perf ?>>improvement in a follow up. >> >> Along with the performance costs you mentioned, the main concern here >> will be the GHCB write-back path (copying it back) before VMRUN: this >> will again hit the issue we have currently with >> kvm_write_guest() / copy_to_user(), when we use it to sync the scratch >> buffer back to GHCB. This can fail if guest RAM is mapped using huge-page(s) and RMP is 4K. Please refer to the patch/fix mentioned below, kvm_write_guest() potentially can fail before VMRUN in case of SNP : >> >> commit 94ed878c2669532ebae8eb9b4503f19aa33cd7aa >> Author: Ashish Kalra <ashish.kalra@amd.com> >> Date: Mon Jun 6 22:28:01 2022 +0000 >> >> KVM: SVM: Sync the GHCB scratch buffer using already mapped ghcb >> >> Using kvm_write_guest() to sync the GHCB scratch buffer can fail >> due to host mapping being 2M, but RMP being 4K. The page fault handling >> in do_user_addr_fault() fails to split the 2M page to handle RMP fault due >> to it being called here in a non-preemptible context. Instead use >> the already kernel mapped ghcb to sync the scratch buffer when the >> scratch buffer is contained within the GHCB. >Ah I didn't see that issue thanks for the pointer. >The patch description says "When SEV-SNP is enabled the mapped GPA needs to be protected against a page state change." since if the guest were to convert the GHCB page to private when the host is using the GHCB the host could get an RMP violation right? Right. >That RMP violation would cause the host to crash unless we use some copy_to_user() type protections. As such copy_to_user() will only swallow the RMP violation and return failure, so the host can retry the write. > I don't see anything mechanism for this patch to add the page state change protection discussed. Can't another vCPU still convert the GHCB to private? We do have the protections for GHCB getting mapped to private specifically, there are new post_{map|unmap}_gfn functions added to verify if it is safe to map GHCB pages. There is a PSC spinlock added which protects again page state change for these mapped pages. Below is the reference to this patch: https://lore.kernel.org/lkml/cover.1655761627.git.ashish.kalra@amd.com/T/#mafcaac7296eb9a92c0ea58730dbd3ca47a8e0756 But do note that there is protection only for GHCB pages and there is a need to add generic post_{map,unmap}_gfn() ops that can be used to verify that it's safe to map a given guest page in the hypervisor. This is a TODO right now and probably this is something which UPM can address more cleanly. >I was wrong about the importance of this though seanjc@ walked me through how UPM will solve this issue so no worries about this until the series is rebased on to UPM. Thanks, Ashish
On Thu, Jul 7, 2022 at 2:31 PM Kalra, Ashish <Ashish.Kalra@amd.com> wrote: > > [AMD Official Use Only - General] > > Hello Peter, > > >> >There is a perf cost to this suggestion but it might make accessing > >> >the GHCB safer for KVM. Have you thought about just using > >> >kvm_read_guest() or copy_from_user() to fully copy out the GCHB into a KVM owned buffer, then copying it back before the VMRUN. That way the KVM doesn't need to guard against page_state_changes on the GHCBs, that could be a perf ?>>improvement in a follow up. > >> > >> Along with the performance costs you mentioned, the main concern here > >> will be the GHCB write-back path (copying it back) before VMRUN: this > >> will again hit the issue we have currently with > >> kvm_write_guest() / copy_to_user(), when we use it to sync the scratch > >> buffer back to GHCB. This can fail if guest RAM is mapped using huge-page(s) and RMP is 4K. Please refer to the patch/fix mentioned below, kvm_write_guest() potentially can fail before VMRUN in case of SNP : > >> > >> commit 94ed878c2669532ebae8eb9b4503f19aa33cd7aa > >> Author: Ashish Kalra <ashish.kalra@amd.com> > >> Date: Mon Jun 6 22:28:01 2022 +0000 > >> > >> KVM: SVM: Sync the GHCB scratch buffer using already mapped ghcb > >> > >> Using kvm_write_guest() to sync the GHCB scratch buffer can fail > >> due to host mapping being 2M, but RMP being 4K. The page fault handling > >> in do_user_addr_fault() fails to split the 2M page to handle RMP fault due > >> to it being called here in a non-preemptible context. Instead use > >> the already kernel mapped ghcb to sync the scratch buffer when the > >> scratch buffer is contained within the GHCB. > > >Ah I didn't see that issue thanks for the pointer. > > >The patch description says "When SEV-SNP is enabled the mapped GPA needs to be protected against a page state change." since if the guest were to convert the GHCB page to private when the host is using the GHCB the host could get an RMP violation right? > > Right. > > >That RMP violation would cause the host to crash unless we use some copy_to_user() type protections. > > As such copy_to_user() will only swallow the RMP violation and return failure, so the host can retry the write. > > > I don't see anything mechanism for this patch to add the page state change protection discussed. Can't another vCPU still convert the GHCB to private? > > We do have the protections for GHCB getting mapped to private specifically, there are new post_{map|unmap}_gfn functions added to verify if it is safe to map > GHCB pages. There is a PSC spinlock added which protects again page state change for these mapped pages. > Below is the reference to this patch: > https://lore.kernel.org/lkml/cover.1655761627.git.ashish.kalra@amd.com/T/#mafcaac7296eb9a92c0ea58730dbd3ca47a8e0756 > > But do note that there is protection only for GHCB pages and there is a need to add generic post_{map,unmap}_gfn() ops that can be used to verify > that it's safe to map a given guest page in the hypervisor. This is a TODO right now and probably this is something which UPM can address more cleanly. Thank you Ashish. I had missed that. Can you help me understand why its OK to use kvm_write_guest() for the |snp_certs_data| inside of snp_handle_ext_guest_request() in patch 42/49? I would have thought we'd have the same 2M vs 4K mapping issues. > > >I was wrong about the importance of this though seanjc@ walked me through how UPM will solve this issue so no worries about this until the series is rebased on to UPM. > > Thanks, > Ashish
[AMD Official Use Only - General] Hello Peter, >> > I don't see anything mechanism for this patch to add the page state change protection discussed. Can't another vCPU still convert the GHCB to private? >> >> We do have the protections for GHCB getting mapped to private >> specifically, there are new post_{map|unmap}_gfn functions added to verify if it is safe to map GHCB pages. There is a PSC spinlock added which protects again page state change for these mapped pages. >> Below is the reference to this patch: >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore >> .kernel.org%2Flkml%2Fcover.1655761627.git.ashish.kalra%40amd.com%2FT%2 >> F%23mafcaac7296eb9a92c0ea58730dbd3ca47a8e0756&data=05%7C01%7CAshis >> h.Kalra%40amd.com%7C647218cdb2a040bf354e08da60fa2968%7C3dd8961fe4884e6 >> 08e11a82d994e183d%7C0%7C0%7C637928924845082803%7CUnknown%7CTWFpbGZsb3d >> 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C >> 3000%7C%7C%7C&sdata=ss8%2F5qualccXQero9phARIG2wvYhtp8SMdve3GglZeU% >> 3D&reserved=0 >> >> But do note that there is protection only for GHCB pages and there is >> a need to add generic post_{map,unmap}_gfn() ops that can be used to verify that it's safe to map a given guest page in the hypervisor. This is a TODO right now and probably this is something which UPM can address more cleanly. >Thank you Ashish. I had missed that. >Can you help me understand why its OK to use kvm_write_guest() for the >|snp_certs_data| inside of snp_handle_ext_guest_request() in patch >42/49? I would have thought we'd have the same 2M vs 4K mapping issues. Preemption is not disabled there, hence the RMP page fault handler can do the split of 2M to 4K on host pages without any issues. Thanks, Ashish
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 01ea257e17d6..c70f3f7e06a8 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -2823,15 +2823,40 @@ void sev_free_vcpu(struct kvm_vcpu *vcpu) kvfree(svm->sev_es.ghcb_sa); } +static inline int svm_map_ghcb(struct vcpu_svm *svm, struct kvm_host_map *map) +{ + struct vmcb_control_area *control = &svm->vmcb->control; + u64 gfn = gpa_to_gfn(control->ghcb_gpa); + + if (kvm_vcpu_map(&svm->vcpu, gfn, map)) { + /* Unable to map GHCB from guest */ + pr_err("error mapping GHCB GFN [%#llx] from guest\n", gfn); + return -EFAULT; + } + + return 0; +} + +static inline void svm_unmap_ghcb(struct vcpu_svm *svm, struct kvm_host_map *map) +{ + kvm_vcpu_unmap(&svm->vcpu, map, true); +} + static void dump_ghcb(struct vcpu_svm *svm) { - struct ghcb *ghcb = svm->sev_es.ghcb; + struct kvm_host_map map; unsigned int nbits; + struct ghcb *ghcb; + + if (svm_map_ghcb(svm, &map)) + return; + + ghcb = map.hva; /* Re-use the dump_invalid_vmcb module parameter */ if (!dump_invalid_vmcb) { pr_warn_ratelimited("set kvm_amd.dump_invalid_vmcb=1 to dump internal KVM state.\n"); - return; + goto e_unmap; } nbits = sizeof(ghcb->save.valid_bitmap) * 8; @@ -2846,12 +2871,21 @@ static void dump_ghcb(struct vcpu_svm *svm) pr_err("%-20s%016llx is_valid: %u\n", "sw_scratch", ghcb->save.sw_scratch, ghcb_sw_scratch_is_valid(ghcb)); pr_err("%-20s%*pb\n", "valid_bitmap", nbits, ghcb->save.valid_bitmap); + +e_unmap: + svm_unmap_ghcb(svm, &map); } -static void sev_es_sync_to_ghcb(struct vcpu_svm *svm) +static bool sev_es_sync_to_ghcb(struct vcpu_svm *svm) { struct kvm_vcpu *vcpu = &svm->vcpu; - struct ghcb *ghcb = svm->sev_es.ghcb; + struct kvm_host_map map; + struct ghcb *ghcb; + + if (svm_map_ghcb(svm, &map)) + return false; + + ghcb = map.hva; /* * The GHCB protocol so far allows for the following data @@ -2865,13 +2899,24 @@ static void sev_es_sync_to_ghcb(struct vcpu_svm *svm) ghcb_set_rbx(ghcb, vcpu->arch.regs[VCPU_REGS_RBX]); ghcb_set_rcx(ghcb, vcpu->arch.regs[VCPU_REGS_RCX]); ghcb_set_rdx(ghcb, vcpu->arch.regs[VCPU_REGS_RDX]); + + /* + * Copy the return values from the exit_info_{1,2}. + */ + ghcb_set_sw_exit_info_1(ghcb, svm->sev_es.ghcb_sw_exit_info_1); + ghcb_set_sw_exit_info_2(ghcb, svm->sev_es.ghcb_sw_exit_info_2); + + trace_kvm_vmgexit_exit(svm->vcpu.vcpu_id, ghcb); + + svm_unmap_ghcb(svm, &map); + + return true; } -static void sev_es_sync_from_ghcb(struct vcpu_svm *svm) +static void sev_es_sync_from_ghcb(struct vcpu_svm *svm, struct ghcb *ghcb) { struct vmcb_control_area *control = &svm->vmcb->control; struct kvm_vcpu *vcpu = &svm->vcpu; - struct ghcb *ghcb = svm->sev_es.ghcb; u64 exit_code; /* @@ -2915,20 +2960,25 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm) memset(ghcb->save.valid_bitmap, 0, sizeof(ghcb->save.valid_bitmap)); } -static int sev_es_validate_vmgexit(struct vcpu_svm *svm) +static int sev_es_validate_vmgexit(struct vcpu_svm *svm, u64 *exit_code) { - struct kvm_vcpu *vcpu; + struct kvm_vcpu *vcpu = &svm->vcpu; + struct kvm_host_map map; struct ghcb *ghcb; - u64 exit_code; u64 reason; - ghcb = svm->sev_es.ghcb; + if (svm_map_ghcb(svm, &map)) + return -EFAULT; + + ghcb = map.hva; + + trace_kvm_vmgexit_enter(vcpu->vcpu_id, ghcb); /* * Retrieve the exit code now even though it may not be marked valid * as it could help with debugging. */ - exit_code = ghcb_get_sw_exit_code(ghcb); + *exit_code = ghcb_get_sw_exit_code(ghcb); /* Only GHCB Usage code 0 is supported */ if (ghcb->ghcb_usage) { @@ -3021,6 +3071,9 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm) goto vmgexit_err; } + sev_es_sync_from_ghcb(svm, ghcb); + + svm_unmap_ghcb(svm, &map); return 0; vmgexit_err: @@ -3031,10 +3084,10 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm) ghcb->ghcb_usage); } else if (reason == GHCB_ERR_INVALID_EVENT) { vcpu_unimpl(vcpu, "vmgexit: exit code %#llx is not valid\n", - exit_code); + *exit_code); } else { vcpu_unimpl(vcpu, "vmgexit: exit code %#llx input is not valid\n", - exit_code); + *exit_code); dump_ghcb(svm); } @@ -3044,6 +3097,8 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm) ghcb_set_sw_exit_info_1(ghcb, 2); ghcb_set_sw_exit_info_2(ghcb, reason); + svm_unmap_ghcb(svm, &map); + /* Resume the guest to "return" the error code. */ return 1; } @@ -3053,23 +3108,20 @@ void sev_es_unmap_ghcb(struct vcpu_svm *svm) /* Clear any indication that the vCPU is in a type of AP Reset Hold */ svm->sev_es.ap_reset_hold_type = AP_RESET_HOLD_NONE; - if (!svm->sev_es.ghcb) + if (!svm->sev_es.ghcb_in_use) return; /* Sync the scratch buffer area. */ if (svm->sev_es.ghcb_sa_sync) { kvm_write_guest(svm->vcpu.kvm, - ghcb_get_sw_scratch(svm->sev_es.ghcb), + svm->sev_es.ghcb_sa_gpa, svm->sev_es.ghcb_sa, svm->sev_es.ghcb_sa_len); svm->sev_es.ghcb_sa_sync = false; } - trace_kvm_vmgexit_exit(svm->vcpu.vcpu_id, svm->sev_es.ghcb); - sev_es_sync_to_ghcb(svm); - kvm_vcpu_unmap(&svm->vcpu, &svm->sev_es.ghcb_map, true); - svm->sev_es.ghcb = NULL; + svm->sev_es.ghcb_in_use = false; } void pre_sev_run(struct vcpu_svm *svm, int cpu) @@ -3099,7 +3151,6 @@ void pre_sev_run(struct vcpu_svm *svm, int cpu) static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) { struct vmcb_control_area *control = &svm->vmcb->control; - struct ghcb *ghcb = svm->sev_es.ghcb; u64 ghcb_scratch_beg, ghcb_scratch_end; u64 scratch_gpa_beg, scratch_gpa_end; @@ -3178,8 +3229,8 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) return 0; e_scratch: - ghcb_set_sw_exit_info_1(ghcb, 2); - ghcb_set_sw_exit_info_2(ghcb, GHCB_ERR_INVALID_SCRATCH_AREA); + svm_set_ghcb_sw_exit_info_1(&svm->vcpu, 2); + svm_set_ghcb_sw_exit_info_2(&svm->vcpu, GHCB_ERR_INVALID_SCRATCH_AREA); return 1; } @@ -3316,7 +3367,6 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu) struct vcpu_svm *svm = to_svm(vcpu); struct vmcb_control_area *control = &svm->vmcb->control; u64 ghcb_gpa, exit_code; - struct ghcb *ghcb; int ret; /* Validate the GHCB */ @@ -3331,29 +3381,14 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu) return 1; } - if (kvm_vcpu_map(vcpu, ghcb_gpa >> PAGE_SHIFT, &svm->sev_es.ghcb_map)) { - /* Unable to map GHCB from guest */ - vcpu_unimpl(vcpu, "vmgexit: error mapping GHCB [%#llx] from guest\n", - ghcb_gpa); - - /* Without a GHCB, just return right back to the guest */ - return 1; - } - - svm->sev_es.ghcb = svm->sev_es.ghcb_map.hva; - ghcb = svm->sev_es.ghcb_map.hva; - - trace_kvm_vmgexit_enter(vcpu->vcpu_id, ghcb); - - exit_code = ghcb_get_sw_exit_code(ghcb); - - ret = sev_es_validate_vmgexit(svm); + ret = sev_es_validate_vmgexit(svm, &exit_code); if (ret) return ret; - sev_es_sync_from_ghcb(svm); - ghcb_set_sw_exit_info_1(ghcb, 0); - ghcb_set_sw_exit_info_2(ghcb, 0); + svm->sev_es.ghcb_in_use = true; + + svm_set_ghcb_sw_exit_info_1(vcpu, 0); + svm_set_ghcb_sw_exit_info_2(vcpu, 0); switch (exit_code) { case SVM_VMGEXIT_MMIO_READ: @@ -3393,20 +3428,20 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu) break; case 1: /* Get AP jump table address */ - ghcb_set_sw_exit_info_2(ghcb, sev->ap_jump_table); + svm_set_ghcb_sw_exit_info_2(vcpu, sev->ap_jump_table); break; default: pr_err("svm: vmgexit: unsupported AP jump table request - exit_info_1=%#llx\n", control->exit_info_1); - ghcb_set_sw_exit_info_1(ghcb, 2); - ghcb_set_sw_exit_info_2(ghcb, GHCB_ERR_INVALID_INPUT); + svm_set_ghcb_sw_exit_info_1(vcpu, 2); + svm_set_ghcb_sw_exit_info_2(vcpu, GHCB_ERR_INVALID_INPUT); } ret = 1; break; } case SVM_VMGEXIT_HV_FEATURES: { - ghcb_set_sw_exit_info_2(ghcb, GHCB_HV_FT_SUPPORTED); + svm_set_ghcb_sw_exit_info_2(vcpu, GHCB_HV_FT_SUPPORTED); ret = 1; break; @@ -3537,7 +3572,7 @@ void sev_vcpu_deliver_sipi_vector(struct kvm_vcpu *vcpu, u8 vector) * Return from an AP Reset Hold VMGEXIT, where the guest will * set the CS and RIP. Set SW_EXIT_INFO_2 to a non-zero value. */ - ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, 1); + svm_set_ghcb_sw_exit_info_2(vcpu, 1); break; case AP_RESET_HOLD_MSR_PROTO: /* diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index 18e2cd4d9559..b24e0171cbf2 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -2720,14 +2720,14 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) static int svm_complete_emulated_msr(struct kvm_vcpu *vcpu, int err) { struct vcpu_svm *svm = to_svm(vcpu); - if (!err || !sev_es_guest(vcpu->kvm) || WARN_ON_ONCE(!svm->sev_es.ghcb)) + if (!err || !sev_es_guest(vcpu->kvm) || WARN_ON_ONCE(!svm->sev_es.ghcb_in_use)) return kvm_complete_insn_gp(vcpu, err); - ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, 1); - ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, - X86_TRAP_GP | - SVM_EVTINJ_TYPE_EXEPT | - SVM_EVTINJ_VALID); + svm_set_ghcb_sw_exit_info_1(vcpu, 1); + svm_set_ghcb_sw_exit_info_2(vcpu, + X86_TRAP_GP | + SVM_EVTINJ_TYPE_EXEPT | + SVM_EVTINJ_VALID); return 1; } diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h index bd0db4d4a61e..c80352c9c0d6 100644 --- a/arch/x86/kvm/svm/svm.h +++ b/arch/x86/kvm/svm/svm.h @@ -189,8 +189,7 @@ struct svm_nested_state { struct vcpu_sev_es_state { /* SEV-ES support */ struct sev_es_save_area *vmsa; - struct ghcb *ghcb; - struct kvm_host_map ghcb_map; + bool ghcb_in_use; bool received_first_sipi; unsigned int ap_reset_hold_type; @@ -200,6 +199,13 @@ struct vcpu_sev_es_state { u64 ghcb_sa_gpa; u32 ghcb_sa_alloc_len; bool ghcb_sa_sync; + + /* + * SEV-ES support to hold the sw_exit_info return values to be + * sync'ed to the GHCB when mapped. + */ + u64 ghcb_sw_exit_info_1; + u64 ghcb_sw_exit_info_2; }; struct vcpu_svm { @@ -614,6 +620,20 @@ void nested_sync_control_from_vmcb02(struct vcpu_svm *svm); void nested_vmcb02_compute_g_pat(struct vcpu_svm *svm); void svm_switch_vmcb(struct vcpu_svm *svm, struct kvm_vmcb_info *target_vmcb); +static inline void svm_set_ghcb_sw_exit_info_1(struct kvm_vcpu *vcpu, u64 val) +{ + struct vcpu_svm *svm = to_svm(vcpu); + + svm->sev_es.ghcb_sw_exit_info_1 = val; +} + +static inline void svm_set_ghcb_sw_exit_info_2(struct kvm_vcpu *vcpu, u64 val) +{ + struct vcpu_svm *svm = to_svm(vcpu); + + svm->sev_es.ghcb_sw_exit_info_2 = val; +} + extern struct kvm_x86_nested_ops svm_nested_ops; /* avic.c */