lkml.org 
[lkml]   [2020]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 1/2] KVM: SVM: Move asid to vcpu_svm
    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 <cavery@redhat.com>
    > ---
    > 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
    >

    \
     
     \ /
      Last update: 2020-10-13 03:31    [W:3.329 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site