Move asid to svm->asid to allow for vmcb assignment
during svm_vcpu_run without regard to which level
guest is running.
Signed-off-by: Cathy Avery <[email protected]>
---
arch/x86/kvm/svm/svm.c | 4 +++-
arch/x86/kvm/svm/svm.h | 1 +
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index d4e18bda19c7..619980a5d540 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1101,6 +1101,7 @@ static void init_vmcb(struct vcpu_svm *svm)
save->cr4 = 0;
}
svm->asid_generation = 0;
+ svm->asid = 0;
svm->nested.vmcb = 0;
svm->vcpu.arch.hflags = 0;
@@ -1663,7 +1664,7 @@ static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd)
}
svm->asid_generation = sd->asid_generation;
- svm->vmcb->control.asid = sd->next_asid++;
+ svm->asid = sd->next_asid++;
vmcb_mark_dirty(svm->vmcb, VMCB_ASID);
}
@@ -3446,6 +3447,7 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
sync_lapic_to_cr8(vcpu);
+ svm->vmcb->control.asid = svm->asid;
svm->vmcb->save.cr2 = vcpu->arch.cr2;
/*
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index a798e1731709..862f0d2405e8 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -104,6 +104,7 @@ struct vcpu_svm {
struct vmcb *vmcb;
unsigned long vmcb_pa;
struct svm_cpu_data *svm_data;
+ u32 asid;
uint64_t asid_generation;
uint64_t sysenter_esp;
uint64_t sysenter_eip;
--
2.20.1
On Sun, Oct 11, 2020 at 02:48:17PM -0400, Cathy Avery wrote:
> Move asid to svm->asid to allow for vmcb assignment
This is misleading. The asid isn't being moved, it's being copied/tracked.
The "to allow" wording also confused me; I though this was just a prep patch
and the actual assignment was in a follow-up patch.
> during svm_vcpu_run without regard to which level
> guest is running.
> Signed-off-by: Cathy Avery <[email protected]>
> ---
> arch/x86/kvm/svm/svm.c | 4 +++-
> arch/x86/kvm/svm/svm.h | 1 +
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index d4e18bda19c7..619980a5d540 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1101,6 +1101,7 @@ static void init_vmcb(struct vcpu_svm *svm)
> save->cr4 = 0;
> }
> svm->asid_generation = 0;
> + svm->asid = 0;
>
> svm->nested.vmcb = 0;
> svm->vcpu.arch.hflags = 0;
> @@ -1663,7 +1664,7 @@ static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd)
> }
>
> svm->asid_generation = sd->asid_generation;
> - svm->vmcb->control.asid = sd->next_asid++;
> + svm->asid = sd->next_asid++;
> vmcb_mark_dirty(svm->vmcb, VMCB_ASID);
I know very little (ok, nothing) about SVM VMCB caching rules, but I strongly
suspect this is broken. The existing code explicitly marks VMCB_ASID dirty,
but there is no equivalent code for the case where there are multiple VMCBs,
e.g. if new_asid() is called while vmcb01 is active, then vmcb02 will pick up
the new ASID but will not mark it dirty.
> }
> @@ -3446,6 +3447,7 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
>
> sync_lapic_to_cr8(vcpu);
>
> + svm->vmcb->control.asid = svm->asid;
Related to the above, handling this in vcpu_run() feels wrong. There really
shouldn't be a need to track the ASID. vmcb01 will always exist if vmcb02
exits, e.g. the ASID can be copied and marked dirty when loading vmcb02.
For new_asid(), it can unconditionally update vmcb01 and conditionally update
vmcb02.
> svm->vmcb->save.cr2 = vcpu->arch.cr2;
>
> /*
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index a798e1731709..862f0d2405e8 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -104,6 +104,7 @@ struct vcpu_svm {
> struct vmcb *vmcb;
> unsigned long vmcb_pa;
> struct svm_cpu_data *svm_data;
> + u32 asid;
> uint64_t asid_generation;
> uint64_t sysenter_esp;
> uint64_t sysenter_eip;
> --
> 2.20.1
>
On 11/10/20 20:48, Cathy Avery wrote:
> Move asid to svm->asid to allow for vmcb assignment
> during svm_vcpu_run without regard to which level
> guest is running.
Slightly more verbose commit message:
KVM does not have separate ASIDs for L1 and L2; either the nested
hypervisor and nested guests share a single ASID, or on older processor
the ASID is used only to implement TLB flushing.
Either way, ASIDs are handled at the VM level. In preparation
for having different VMCBs passed to VMLOAD/VMRUN/VMSAVE for L1 and
L2, store the current ASID to struct vcpu_svm and only move it to
the VMCB in svm_vcpu_run. This way, TLB flushes can be applied
no matter which VMCB will be active during the next svm_vcpu_run.
> Signed-off-by: Cathy Avery <[email protected]>
> ---
> arch/x86/kvm/svm/svm.c | 4 +++-
> arch/x86/kvm/svm/svm.h | 1 +
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index d4e18bda19c7..619980a5d540 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1101,6 +1101,7 @@ static void init_vmcb(struct vcpu_svm *svm)
> save->cr4 = 0;
> }
> svm->asid_generation = 0;
> + svm->asid = 0;
>
> svm->nested.vmcb = 0;
> svm->vcpu.arch.hflags = 0;
> @@ -1663,7 +1664,7 @@ static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd)
> }
>
> svm->asid_generation = sd->asid_generation;
> - svm->vmcb->control.asid = sd->next_asid++;
> + svm->asid = sd->next_asid++;
>
> vmcb_mark_dirty(svm->vmcb, VMCB_ASID);
> }
This vmcb_mark_dirty must be delayed to svm_vcpu_run as well, because
the active VMCB could change:
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 48965bfa3d1e..3b53a7ead04b 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1756,12 +1756,11 @@ static void new_asid(struct vcpu_svm *svm,
struct svm_cpu_data *sd)
++sd->asid_generation;
sd->next_asid = sd->min_asid;
svm->vmcb->control.tlb_ctl = TLB_CONTROL_FLUSH_ALL_ASID;
+ vmcb_mark_dirty(svm->vmcb, VMCB_ASID);
}
svm->asid_generation = sd->asid_generation;
svm->asid = sd->next_asid++;
-
- vmcb_mark_dirty(svm->vmcb, VMCB_ASID);
}
static void svm_set_dr6(struct vcpu_svm *svm, unsigned long value)
@@ -3571,7 +3570,10 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct
kvm_vcpu *vcpu)
sync_lapic_to_cr8(vcpu);
- svm->vmcb->control.asid = svm->asid;
+ if (unlikely(svm->asid != svm->vmcb->control.asid)) {
+ svm->vmcb->control.asid = svm->asid;
+ vmcb_mark_dirty(svm->vmcb, VMCB_ASID);
+ }
svm->vmcb->save.cr2 = vcpu->arch.cr2;
/*
Queued with this change.
Paolo
On 13/10/20 03:29, Sean Christopherson wrote:
>> @@ -3446,6 +3447,7 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
>>
>> sync_lapic_to_cr8(vcpu);
>>
>> + svm->vmcb->control.asid = svm->asid;
>
> Related to the above, handling this in vcpu_run() feels wrong. There really
> shouldn't be a need to track the ASID. vmcb01 will always exist if vmcb02
> exits, e.g. the ASID can be copied and marked dirty when loading vmcb02.
> For new_asid(), it can unconditionally update vmcb01 and conditionally update
> vmcb02.
Yeah, it is a bit ugly and it is only needed on processors without
flush-by-ASID. On those processors the ASID is used by KVM as a sort of
TLB flush generation count. A TLB flush should affect both VMCB01 and
VMCB02, and that's the reason to have a global ASID in struct vcpu_svm.
On processors with flush-by-ASID, currently we bump the ASID on every
physical CPU change. However even that is not needed, in principle
svm->asid should never change and we could also have different ASID01
and ASID02, similar to VMX. So: long term, svm->asid should only be
used only on processors without flush-by-ASID. kvm-amd however is not
quite ready for that.
Paolo
From: Ashish Kalra <[email protected]>
This patch breaks SEV guests.
The patch stores current ASID in struct vcpu_svm and only moves it to VMCB in
svm_vcpu_run(), but by doing so, the ASID allocated for SEV guests and setup
in vmcb->control.asid by pre_sev_run() gets over-written by this ASID
stored in struct vcpu_svm and hence, VMRUN fails as SEV guest is bound/activated
on a different ASID then the one overwritten in vmcb->control.asid at VMRUN.
For example, asid#1 was activated for SEV guest and then vmcb->control.asid is
overwritten with asid#0 (svm->asid) as part of this patch in svm_vcpu_run() and
hence VMRUN fails.
On 29/11/20 10:41, Ashish Kalra wrote:
> From: Ashish Kalra <[email protected]>
>
> This patch breaks SEV guests.
>
> The patch stores current ASID in struct vcpu_svm and only moves it to VMCB in
> svm_vcpu_run(), but by doing so, the ASID allocated for SEV guests and setup
> in vmcb->control.asid by pre_sev_run() gets over-written by this ASID
> stored in struct vcpu_svm and hence, VMRUN fails as SEV guest is bound/activated
> on a different ASID then the one overwritten in vmcb->control.asid at VMRUN.
>
> For example, asid#1 was activated for SEV guest and then vmcb->control.asid is
> overwritten with asid#0 (svm->asid) as part of this patch in svm_vcpu_run() and
> hence VMRUN fails.
>
Thanks Ashish, I've sent a patch to fix it.
Would it be possible to add a minimal SEV test to
tools/testing/selftests/kvm? It doesn't have to do full attestation
etc., if you can just write an "out" instruction using SEV_DBG_ENCRYPT
and check that you can run it that's enough.
Paolo
Hello Paolo,
I believe one of my teammates is currently working on adding a KVM
selftest for SEV and SEV-ES.
Thanks,
Ashish
On Mon, Nov 30, 2020 at 03:41:41PM +0100, Paolo Bonzini wrote:
> On 29/11/20 10:41, Ashish Kalra wrote:
> > From: Ashish Kalra <[email protected]>
> >
> > This patch breaks SEV guests.
> >
> > The patch stores current ASID in struct vcpu_svm and only moves it to VMCB in
> > svm_vcpu_run(), but by doing so, the ASID allocated for SEV guests and setup
> > in vmcb->control.asid by pre_sev_run() gets over-written by this ASID
> > stored in struct vcpu_svm and hence, VMRUN fails as SEV guest is bound/activated
> > on a different ASID then the one overwritten in vmcb->control.asid at VMRUN.
> >
> > For example, asid#1 was activated for SEV guest and then vmcb->control.asid is
> > overwritten with asid#0 (svm->asid) as part of this patch in svm_vcpu_run() and
> > hence VMRUN fails.
> >
>
> Thanks Ashish, I've sent a patch to fix it.
>
> Would it be possible to add a minimal SEV test to
> tools/testing/selftests/kvm? It doesn't have to do full attestation etc.,
> if you can just write an "out" instruction using SEV_DBG_ENCRYPT and check
> that you can run it that's enough.
>
> Paolo
>