| Message ID | 20210424004645.3950558-43-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: vCPU RESET/INIT fixes and consolidation | expand |
On 24/04/21 02:46, Sean Christopherson wrote: > Don't waste time writing zeros via VMWRITE during vCPU RESET, the VMCS > is zero allocated. Is this guaranteed to be valid, or could the VMCS in principle use some weird encoding? (Like it does for the access rights, even though this does not matter for this patch). Paolo > No functional change intended. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/vmx/vmx.c | 29 ----------------------------- > 1 file changed, 29 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 78d17adce7e6..74258ba4832a 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -4427,13 +4427,6 @@ static void init_vmcs(struct vcpu_vmx *vmx) > } > > if (kvm_vcpu_apicv_active(&vmx->vcpu)) { > - vmcs_write64(EOI_EXIT_BITMAP0, 0); > - vmcs_write64(EOI_EXIT_BITMAP1, 0); > - vmcs_write64(EOI_EXIT_BITMAP2, 0); > - vmcs_write64(EOI_EXIT_BITMAP3, 0); > - > - vmcs_write16(GUEST_INTR_STATUS, 0); > - > vmcs_write16(POSTED_INTR_NV, POSTED_INTR_VECTOR); > vmcs_write64(POSTED_INTR_DESC_ADDR, __pa((&vmx->pi_desc))); > } > @@ -4444,23 +4437,9 @@ static void init_vmcs(struct vcpu_vmx *vmx) > vmx->ple_window_dirty = true; > } > > - vmcs_write32(PAGE_FAULT_ERROR_CODE_MASK, 0); > - vmcs_write32(PAGE_FAULT_ERROR_CODE_MATCH, 0); > - vmcs_write32(CR3_TARGET_COUNT, 0); /* 22.2.1 */ > - > - vmcs_write16(HOST_FS_SELECTOR, 0); /* 22.2.4 */ > - vmcs_write16(HOST_GS_SELECTOR, 0); /* 22.2.4 */ > vmx_set_constant_host_state(vmx); > - vmcs_writel(HOST_FS_BASE, 0); /* 22.2.4 */ > - vmcs_writel(HOST_GS_BASE, 0); /* 22.2.4 */ > > - if (cpu_has_vmx_vmfunc()) > - vmcs_write64(VM_FUNCTION_CONTROL, 0); > - > - vmcs_write32(VM_EXIT_MSR_STORE_COUNT, 0); > - vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, 0); > vmcs_write64(VM_EXIT_MSR_LOAD_ADDR, __pa(vmx->msr_autoload.host.val)); > - vmcs_write32(VM_ENTRY_MSR_LOAD_COUNT, 0); > vmcs_write64(VM_ENTRY_MSR_LOAD_ADDR, __pa(vmx->msr_autoload.guest.val)); > > if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) > @@ -4493,7 +4472,6 @@ static void init_vmcs(struct vcpu_vmx *vmx) > memset(&vmx->pt_desc, 0, sizeof(vmx->pt_desc)); > /* Bit[6~0] are forced to 1, writes are ignored. */ > vmx->pt_desc.guest.output_mask = 0x7F; > - vmcs_write64(GUEST_IA32_RTIT_CTL, 0); > } > > vmx_setup_uret_msrs(vmx); > @@ -4536,13 +4514,6 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) > vmcs_write32(GUEST_LDTR_LIMIT, 0xffff); > vmcs_write32(GUEST_LDTR_AR_BYTES, 0x00082); > > - if (!init_event) { > - vmcs_write32(GUEST_SYSENTER_CS, 0); > - vmcs_writel(GUEST_SYSENTER_ESP, 0); > - vmcs_writel(GUEST_SYSENTER_EIP, 0); > - vmcs_write64(GUEST_IA32_DEBUGCTL, 0); > - } > - > vmcs_writel(GUEST_GDTR_BASE, 0); > vmcs_write32(GUEST_GDTR_LIMIT, 0xffff); > >
On Mon, May 24, 2021 at 2:15 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 24/04/21 02:46, Sean Christopherson wrote: > > Don't waste time writing zeros via VMWRITE during vCPU RESET, the VMCS > > is zero allocated. > > Is this guaranteed to be valid, or could the VMCS in principle use some > weird encoding? (Like it does for the access rights, even though this > does not matter for this patch). I see nothing in the SDM that would indicate that zero must be encoded as zero.
On Mon, May 24, 2021, Paolo Bonzini wrote: > On 24/04/21 02:46, Sean Christopherson wrote: > > Don't waste time writing zeros via VMWRITE during vCPU RESET, the VMCS > > is zero allocated. > > Is this guaranteed to be valid, or could the VMCS in principle use some > weird encoding? (Like it does for the access rights, even though this does > not matter for this patch). Phooey. In principle, the CPU can do whatever it wants, e.g. the SDM states that software should never write to the data portion of the VMCS under any circumstance. In practice, I would be flabbergasted if Intel ever ships a CPU that doesn't play nice with zero initiazing the VMCS via software writes. I'd bet dollars to donuts that KVM isn't the only software that relies on that behavior. That said, I'm not against switching to VMWRITE for everything, but regardless of which route we choose, we should commit to one or the other. I.e. double down on memset() and bet that Intel won't break KVM, or replace the memset() in alloc_vmcs_cpu() with a sequence that writes all known (possible?) fields. The current approach of zeroing the memory in software but initializing _some_ fields is the worst option, e.g. I highly doubt vmcs01 and vmcs02 do VMWRITE(..., 0) on the same fields.
On Mon, May 24, 2021 at 3:28 PM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, May 24, 2021, Paolo Bonzini wrote: > > On 24/04/21 02:46, Sean Christopherson wrote: > > > Don't waste time writing zeros via VMWRITE during vCPU RESET, the VMCS > > > is zero allocated. > > > > Is this guaranteed to be valid, or could the VMCS in principle use some > > weird encoding? (Like it does for the access rights, even though this does > > not matter for this patch). > > Phooey. In principle, the CPU can do whatever it wants, e.g. the SDM states that > software should never write to the data portion of the VMCS under any circumstance. > > In practice, I would be flabbergasted if Intel ever ships a CPU that doesn't play > nice with zero initiazing the VMCS via software writes. I'd bet dollars to > donuts that KVM isn't the only software that relies on that behavior. It's not just Intel. It's any manufacturer of physical or virtual CPUs that implement VT-x. Non-architected behavior isn't guaranteed. > That said, I'm not against switching to VMWRITE for everything, but regardless > of which route we choose, we should commit to one or the other. I.e. double down > on memset() and bet that Intel won't break KVM, or replace the memset() in > alloc_vmcs_cpu() with a sequence that writes all known (possible?) fields. The > current approach of zeroing the memory in software but initializing _some_ fields > is the worst option, e.g. I highly doubt vmcs01 and vmcs02 do VMWRITE(..., 0) on > the same fields. The memset should probably be dropped, unless it is there to prevent information leakage. However, it is not necessary to VMWRITE all known (or possible) fields--just those that aren't guarded by an enable bit.
On Mon, May 24, 2021, Jim Mattson wrote: > On Mon, May 24, 2021 at 3:28 PM Sean Christopherson <seanjc@google.com> wrote: > > That said, I'm not against switching to VMWRITE for everything, but regardless > > of which route we choose, we should commit to one or the other. I.e. double down > > on memset() and bet that Intel won't break KVM, or replace the memset() in > > alloc_vmcs_cpu() with a sequence that writes all known (possible?) fields. The > > current approach of zeroing the memory in software but initializing _some_ fields > > is the worst option, e.g. I highly doubt vmcs01 and vmcs02 do VMWRITE(..., 0) on > > the same fields. > > The memset should probably be dropped, unless it is there to prevent > information leakage. However, it is not necessary to VMWRITE all known > (or possible) fields--just those that aren't guarded by an enable bit. Yeah, I was thinking of defense-in-depth, e.g. better to have VM-Enter consume '0' than random garbage because KVM botched an enabling sequence. We essentially get that today via the memset(). I'll fiddle with the sequence and see how much overhead a paranoid and/or really paranoid approach would incur.
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 78d17adce7e6..74258ba4832a 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -4427,13 +4427,6 @@ static void init_vmcs(struct vcpu_vmx *vmx) } if (kvm_vcpu_apicv_active(&vmx->vcpu)) { - vmcs_write64(EOI_EXIT_BITMAP0, 0); - vmcs_write64(EOI_EXIT_BITMAP1, 0); - vmcs_write64(EOI_EXIT_BITMAP2, 0); - vmcs_write64(EOI_EXIT_BITMAP3, 0); - - vmcs_write16(GUEST_INTR_STATUS, 0); - vmcs_write16(POSTED_INTR_NV, POSTED_INTR_VECTOR); vmcs_write64(POSTED_INTR_DESC_ADDR, __pa((&vmx->pi_desc))); } @@ -4444,23 +4437,9 @@ static void init_vmcs(struct vcpu_vmx *vmx) vmx->ple_window_dirty = true; } - vmcs_write32(PAGE_FAULT_ERROR_CODE_MASK, 0); - vmcs_write32(PAGE_FAULT_ERROR_CODE_MATCH, 0); - vmcs_write32(CR3_TARGET_COUNT, 0); /* 22.2.1 */ - - vmcs_write16(HOST_FS_SELECTOR, 0); /* 22.2.4 */ - vmcs_write16(HOST_GS_SELECTOR, 0); /* 22.2.4 */ vmx_set_constant_host_state(vmx); - vmcs_writel(HOST_FS_BASE, 0); /* 22.2.4 */ - vmcs_writel(HOST_GS_BASE, 0); /* 22.2.4 */ - if (cpu_has_vmx_vmfunc()) - vmcs_write64(VM_FUNCTION_CONTROL, 0); - - vmcs_write32(VM_EXIT_MSR_STORE_COUNT, 0); - vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, 0); vmcs_write64(VM_EXIT_MSR_LOAD_ADDR, __pa(vmx->msr_autoload.host.val)); - vmcs_write32(VM_ENTRY_MSR_LOAD_COUNT, 0); vmcs_write64(VM_ENTRY_MSR_LOAD_ADDR, __pa(vmx->msr_autoload.guest.val)); if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) @@ -4493,7 +4472,6 @@ static void init_vmcs(struct vcpu_vmx *vmx) memset(&vmx->pt_desc, 0, sizeof(vmx->pt_desc)); /* Bit[6~0] are forced to 1, writes are ignored. */ vmx->pt_desc.guest.output_mask = 0x7F; - vmcs_write64(GUEST_IA32_RTIT_CTL, 0); } vmx_setup_uret_msrs(vmx); @@ -4536,13 +4514,6 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) vmcs_write32(GUEST_LDTR_LIMIT, 0xffff); vmcs_write32(GUEST_LDTR_AR_BYTES, 0x00082); - if (!init_event) { - vmcs_write32(GUEST_SYSENTER_CS, 0); - vmcs_writel(GUEST_SYSENTER_ESP, 0); - vmcs_writel(GUEST_SYSENTER_EIP, 0); - vmcs_write64(GUEST_IA32_DEBUGCTL, 0); - } - vmcs_writel(GUEST_GDTR_BASE, 0); vmcs_write32(GUEST_GDTR_LIMIT, 0xffff);
Don't waste time writing zeros via VMWRITE during vCPU RESET, the VMCS is zero allocated. No functional change intended. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/vmx/vmx.c | 29 ----------------------------- 1 file changed, 29 deletions(-)