| Message ID | 20220301135526.136554-4-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SVM fixes + apic fix | expand |
Just "KVM: SVM:" for the shortlog, please. On Tue, Mar 01, 2022, Maxim Levitsky wrote: > Out of precation use vmcb01 when enabling host AVIC. > No functional change intended. > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > arch/x86/kvm/svm/avic.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c > index e23159f3a62ba..9656e192c646b 100644 > --- a/arch/x86/kvm/svm/avic.c > +++ b/arch/x86/kvm/svm/avic.c > @@ -167,7 +167,7 @@ int avic_vm_init(struct kvm *kvm) > > void avic_init_vmcb(struct vcpu_svm *svm) > { > - struct vmcb *vmcb = svm->vmcb; > + struct vmcb *vmcb = svm->vmcb01.ptr; I don't like this change. It's not bad code, but it'll be confusing because it implies that it's legal for svm->vmcb to be something other than svm->vmcb01.ptr when this is called. If we want to guard AVIC, I'd much rather we do something like: diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index 7038c76fa841..dcc856bd628d 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -992,8 +992,12 @@ static inline void init_vmcb_after_set_cpuid(struct kvm_vcpu *vcpu) static void init_vmcb(struct kvm_vcpu *vcpu) { struct vcpu_svm *svm = to_svm(vcpu); - struct vmcb_control_area *control = &svm->vmcb->control; - struct vmcb_save_area *save = &svm->vmcb->save; + struct vmcb *vmcb = svm->vmcb01.ptr; + struct vmcb_control_area *control = &vmcb->control; + struct vmcb_save_area *save = &vmcb->save; + + if (WARN_ON_ONCE(vmcb != svm->vmcb)) + svm_leave_nested(vcpu); svm_set_intercept(svm, INTERCEPT_CR0_READ); svm_set_intercept(svm, INTERCEPT_CR3_READ); On a related topic, init_vmcb_after_set_cpuid() is broken for nested, it needs to play nice with being called when svm->vmcb == &svm->nested.vmcb02, e.g. update vmcb01 and re-merge (or just recalc?) vmcb02's intercepts. > struct kvm_svm *kvm_svm = to_kvm_svm(svm->vcpu.kvm); > phys_addr_t bpa = __sme_set(page_to_phys(svm->avic_backing_page)); > phys_addr_t lpa = __sme_set(page_to_phys(kvm_svm->avic_logical_id_table_page)); > -- > 2.26.3 >
On Tue, 2022-03-01 at 16:21 +0000, Sean Christopherson wrote: > Just "KVM: SVM:" for the shortlog, please. > > On Tue, Mar 01, 2022, Maxim Levitsky wrote: > > Out of precation use vmcb01 when enabling host AVIC. > > No functional change intended. > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > arch/x86/kvm/svm/avic.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c > > index e23159f3a62ba..9656e192c646b 100644 > > --- a/arch/x86/kvm/svm/avic.c > > +++ b/arch/x86/kvm/svm/avic.c > > @@ -167,7 +167,7 @@ int avic_vm_init(struct kvm *kvm) > > > > void avic_init_vmcb(struct vcpu_svm *svm) > > { > > - struct vmcb *vmcb = svm->vmcb; > > + struct vmcb *vmcb = svm->vmcb01.ptr; > > I don't like this change. It's not bad code, but it'll be confusing because it > implies that it's legal for svm->vmcb to be something other than svm->vmcb01.ptr > when this is called. Honestly I don't see how you had reached this conclusion. I just think that code that always works on vmcb01 should use it, even if it happens that vmcb == vmcb01. If you insist I can drop this patch or add WARN_ON instead, I just think that this way is cleaner. Best regards, Maxim Levitsky > > If we want to guard AVIC, I'd much rather we do something like: > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > index 7038c76fa841..dcc856bd628d 100644 > --- a/arch/x86/kvm/svm/svm.c > +++ b/arch/x86/kvm/svm/svm.c > @@ -992,8 +992,12 @@ static inline void init_vmcb_after_set_cpuid(struct kvm_vcpu *vcpu) > static void init_vmcb(struct kvm_vcpu *vcpu) > { > struct vcpu_svm *svm = to_svm(vcpu); > - struct vmcb_control_area *control = &svm->vmcb->control; > - struct vmcb_save_area *save = &svm->vmcb->save; > + struct vmcb *vmcb = svm->vmcb01.ptr; > + struct vmcb_control_area *control = &vmcb->control; > + struct vmcb_save_area *save = &vmcb->save; > + > + if (WARN_ON_ONCE(vmcb != svm->vmcb)) > + svm_leave_nested(vcpu); > > svm_set_intercept(svm, INTERCEPT_CR0_READ); > svm_set_intercept(svm, INTERCEPT_CR3_READ); > > > On a related topic, init_vmcb_after_set_cpuid() is broken for nested, it needs to > play nice with being called when svm->vmcb == &svm->nested.vmcb02, e.g. update > vmcb01 and re-merge (or just recalc?) vmcb02's intercepts. > > > struct kvm_svm *kvm_svm = to_kvm_svm(svm->vcpu.kvm); > > phys_addr_t bpa = __sme_set(page_to_phys(svm->avic_backing_page)); > > phys_addr_t lpa = __sme_set(page_to_phys(kvm_svm->avic_logical_id_table_page)); > > -- > > 2.26.3 > >
On Tue, Mar 01, 2022, Maxim Levitsky wrote: > On Tue, 2022-03-01 at 16:21 +0000, Sean Christopherson wrote: > > Just "KVM: SVM:" for the shortlog, please. > > > > On Tue, Mar 01, 2022, Maxim Levitsky wrote: > > > Out of precation use vmcb01 when enabling host AVIC. > > > No functional change intended. > > > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > > --- > > > arch/x86/kvm/svm/avic.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c > > > index e23159f3a62ba..9656e192c646b 100644 > > > --- a/arch/x86/kvm/svm/avic.c > > > +++ b/arch/x86/kvm/svm/avic.c > > > @@ -167,7 +167,7 @@ int avic_vm_init(struct kvm *kvm) > > > > > > void avic_init_vmcb(struct vcpu_svm *svm) > > > { > > > - struct vmcb *vmcb = svm->vmcb; > > > + struct vmcb *vmcb = svm->vmcb01.ptr; > > > > I don't like this change. It's not bad code, but it'll be confusing because it > > implies that it's legal for svm->vmcb to be something other than svm->vmcb01.ptr > > when this is called. > > Honestly I don't see how you had reached this conclusion. There's exactly one caller, init_vmcb(), and that caller doesn't assert that the current VMCB is vmcb01, nor does it unconditionally use vmcb01. Adding code here without an assert implies that init_vmcb() may be called with vmcb02 active, otherwise why diverge from its one caller? > I just think that code that always works on vmcb01 > should use it, even if it happens that vmcb == vmcb01. I'm not disagreeing, I'm saying that the rule you want to enforce also applies to init_vmcb(), so rather than introduce inconsistent code in all the leafs, fix the problem at the root. I've no objection to adding a WARN in the AVIC code (though at that point I'd vote to just pass in @vmcb), I'm objecting to "silently" diverging.
On 3/1/22 18:25, Maxim Levitsky wrote: >> I don't like this change. It's not bad code, but it'll be confusing because it >> implies that it's legal for svm->vmcb to be something other than svm->vmcb01.ptr >> when this is called. > Honestly I don't see how you had reached this conclusion. > > I just think that code that always works on vmcb01 > should use it, even if it happens that vmcb == vmcb01. > > If you insist I can drop this patch or add WARN_ON instead, > I just think that this way is cleaner. > I do like the patch, but you should do the same in init_vmcb() and svm_hv_init_vmcb() as well. Paolo
On Wed, 2022-03-09 at 16:48 +0100, Paolo Bonzini wrote: > On 3/1/22 18:25, Maxim Levitsky wrote: > > > I don't like this change. It's not bad code, but it'll be confusing because it > > > implies that it's legal for svm->vmcb to be something other than svm->vmcb01.ptr > > > when this is called. > > Honestly I don't see how you had reached this conclusion. > > > > I just think that code that always works on vmcb01 > > should use it, even if it happens that vmcb == vmcb01. > > > > If you insist I can drop this patch or add WARN_ON instead, > > I just think that this way is cleaner. > > > > I do like the patch, but you should do the same in init_vmcb() and > svm_hv_init_vmcb() as well. I will do this. Thanks! Best regards, Maxim Levitsky > > Paolo >
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c index e23159f3a62ba..9656e192c646b 100644 --- a/arch/x86/kvm/svm/avic.c +++ b/arch/x86/kvm/svm/avic.c @@ -167,7 +167,7 @@ int avic_vm_init(struct kvm *kvm) void avic_init_vmcb(struct vcpu_svm *svm) { - struct vmcb *vmcb = svm->vmcb; + struct vmcb *vmcb = svm->vmcb01.ptr; struct kvm_svm *kvm_svm = to_kvm_svm(svm->vcpu.kvm); phys_addr_t bpa = __sme_set(page_to_phys(svm->avic_backing_page)); phys_addr_t lpa = __sme_set(page_to_phys(kvm_svm->avic_logical_id_table_page));
Out of precation use vmcb01 when enabling host AVIC. No functional change intended. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- arch/x86/kvm/svm/avic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)