* Re: [PATCH] KVM: nVMX: Sync L2 guest CET states between L1/L2
2021-02-09 8:37 [PATCH] KVM: nVMX: Sync L2 guest CET states between L1/L2 Yang Weijiang
@ 2021-02-09 10:53 ` kernel test robot
2021-02-11 17:18 ` Sean Christopherson
1 sibling, 0 replies; 4+ messages in thread
From: kernel test robot @ 2021-02-09 10:53 UTC (permalink / raw)
To: Yang Weijiang, pbonzini, seanjc, kvm, linux-kernel
Cc: kbuild-all, Yang Weijiang
[-- Attachment #1: Type: text/plain, Size: 11714 bytes --]
Hi Yang,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on kvm/linux-next]
[also build test ERROR on v5.11-rc6 next-20210125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Yang-Weijiang/KVM-nVMX-Sync-L2-guest-CET-states-between-L1-L2/20210209-162909
base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: x86_64-rhel (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/892519e752407d6c2c5fd732108f397291d3eb97
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Yang-Weijiang/KVM-nVMX-Sync-L2-guest-CET-states-between-L1-L2/20210209-162909
git checkout 892519e752407d6c2c5fd732108f397291d3eb97
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
arch/x86/kvm/vmx/nested.c: In function 'prepare_vmcs02':
>> arch/x86/kvm/vmx/nested.c:2575:34: error: 'VM_ENTRY_LOAD_CET_STATE' undeclared (first use in this function); did you mean 'VM_ENTRY_LOAD_IA32_PAT'?
2575 | if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_CET_STATE) {
| ^~~~~~~~~~~~~~~~~~~~~~~
| VM_ENTRY_LOAD_IA32_PAT
arch/x86/kvm/vmx/nested.c:2575:34: note: each undeclared identifier is reported only once for each function it appears in
>> arch/x86/kvm/vmx/nested.c:2576:15: error: 'GUEST_SSP' undeclared (first use in this function); did you mean 'GUEST_RSP'?
2576 | vmcs_writel(GUEST_SSP, vmcs12->guest_ssp);
| ^~~~~~~~~
| GUEST_RSP
>> arch/x86/kvm/vmx/nested.c:2576:34: error: 'struct vmcs12' has no member named 'guest_ssp'; did you mean 'guest_rsp'?
2576 | vmcs_writel(GUEST_SSP, vmcs12->guest_ssp);
| ^~~~~~~~~
| guest_rsp
>> arch/x86/kvm/vmx/nested.c:2577:15: error: 'GUEST_INTR_SSP_TABLE' undeclared (first use in this function); did you mean 'GUEST_INTR_STATUS'?
2577 | vmcs_writel(GUEST_INTR_SSP_TABLE, vmcs12->guest_ssp_tbl);
| ^~~~~~~~~~~~~~~~~~~~
| GUEST_INTR_STATUS
>> arch/x86/kvm/vmx/nested.c:2577:43: error: 'struct vmcs12' has no member named 'guest_ssp_tbl'
2577 | vmcs_writel(GUEST_INTR_SSP_TABLE, vmcs12->guest_ssp_tbl);
| ^~
>> arch/x86/kvm/vmx/nested.c:2578:15: error: 'GUEST_S_CET' undeclared (first use in this function); did you mean 'GUEST_CR4'?
2578 | vmcs_writel(GUEST_S_CET, vmcs12->guest_s_cet);
| ^~~~~~~~~~~
| GUEST_CR4
>> arch/x86/kvm/vmx/nested.c:2578:36: error: 'struct vmcs12' has no member named 'guest_s_cet'; did you mean 'guest_cr0'?
2578 | vmcs_writel(GUEST_S_CET, vmcs12->guest_s_cet);
| ^~~~~~~~~~~
| guest_cr0
arch/x86/kvm/vmx/nested.c: In function 'sync_vmcs02_to_vmcs12':
arch/x86/kvm/vmx/nested.c:4113:34: error: 'VM_ENTRY_LOAD_CET_STATE' undeclared (first use in this function); did you mean 'VM_ENTRY_LOAD_IA32_PAT'?
4113 | if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_CET_STATE) {
| ^~~~~~~~~~~~~~~~~~~~~~~
| VM_ENTRY_LOAD_IA32_PAT
arch/x86/kvm/vmx/nested.c:4114:11: error: 'struct vmcs12' has no member named 'guest_ssp'; did you mean 'guest_rsp'?
4114 | vmcs12->guest_ssp = vmcs_readl(GUEST_SSP);
| ^~~~~~~~~
| guest_rsp
arch/x86/kvm/vmx/nested.c:4114:34: error: 'GUEST_SSP' undeclared (first use in this function); did you mean 'GUEST_RSP'?
4114 | vmcs12->guest_ssp = vmcs_readl(GUEST_SSP);
| ^~~~~~~~~
| GUEST_RSP
arch/x86/kvm/vmx/nested.c:4115:9: error: 'struct vmcs12' has no member named 'guest_ssp_tbl'
4115 | vmcs12->guest_ssp_tbl = vmcs_readl(GUEST_INTR_SSP_TABLE);
| ^~
arch/x86/kvm/vmx/nested.c:4115:38: error: 'GUEST_INTR_SSP_TABLE' undeclared (first use in this function); did you mean 'GUEST_INTR_STATUS'?
4115 | vmcs12->guest_ssp_tbl = vmcs_readl(GUEST_INTR_SSP_TABLE);
| ^~~~~~~~~~~~~~~~~~~~
| GUEST_INTR_STATUS
arch/x86/kvm/vmx/nested.c:4116:11: error: 'struct vmcs12' has no member named 'guest_s_cet'; did you mean 'guest_cr0'?
4116 | vmcs12->guest_s_cet = vmcs_readl(GUEST_S_CET);
| ^~~~~~~~~~~
| guest_cr0
arch/x86/kvm/vmx/nested.c:4116:36: error: 'GUEST_S_CET' undeclared (first use in this function); did you mean 'GUEST_CR4'?
4116 | vmcs12->guest_s_cet = vmcs_readl(GUEST_S_CET);
| ^~~~~~~~~~~
| GUEST_CR4
vim +2575 arch/x86/kvm/vmx/nested.c
2490
2491 /*
2492 * prepare_vmcs02 is called when the L1 guest hypervisor runs its nested
2493 * L2 guest. L1 has a vmcs for L2 (vmcs12), and this function "merges" it
2494 * with L0's requirements for its guest (a.k.a. vmcs01), so we can run the L2
2495 * guest in a way that will both be appropriate to L1's requests, and our
2496 * needs. In addition to modifying the active vmcs (which is vmcs02), this
2497 * function also has additional necessary side-effects, like setting various
2498 * vcpu->arch fields.
2499 * Returns 0 on success, 1 on failure. Invalid state exit qualification code
2500 * is assigned to entry_failure_code on failure.
2501 */
2502 static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
2503 enum vm_entry_failure_code *entry_failure_code)
2504 {
2505 struct vcpu_vmx *vmx = to_vmx(vcpu);
2506 struct hv_enlightened_vmcs *hv_evmcs = vmx->nested.hv_evmcs;
2507 bool load_guest_pdptrs_vmcs12 = false;
2508
2509 if (vmx->nested.dirty_vmcs12 || hv_evmcs) {
2510 prepare_vmcs02_rare(vmx, vmcs12);
2511 vmx->nested.dirty_vmcs12 = false;
2512
2513 load_guest_pdptrs_vmcs12 = !hv_evmcs ||
2514 !(hv_evmcs->hv_clean_fields &
2515 HV_VMX_ENLIGHTENED_CLEAN_FIELD_GUEST_GRP1);
2516 }
2517
2518 if (vmx->nested.nested_run_pending &&
2519 (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS)) {
2520 kvm_set_dr(vcpu, 7, vmcs12->guest_dr7);
2521 vmcs_write64(GUEST_IA32_DEBUGCTL, vmcs12->guest_ia32_debugctl);
2522 } else {
2523 kvm_set_dr(vcpu, 7, vcpu->arch.dr7);
2524 vmcs_write64(GUEST_IA32_DEBUGCTL, vmx->nested.vmcs01_debugctl);
2525 }
2526 if (kvm_mpx_supported() && (!vmx->nested.nested_run_pending ||
2527 !(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)))
2528 vmcs_write64(GUEST_BNDCFGS, vmx->nested.vmcs01_guest_bndcfgs);
2529 vmx_set_rflags(vcpu, vmcs12->guest_rflags);
2530
2531 /* EXCEPTION_BITMAP and CR0_GUEST_HOST_MASK should basically be the
2532 * bitwise-or of what L1 wants to trap for L2, and what we want to
2533 * trap. Note that CR0.TS also needs updating - we do this later.
2534 */
2535 update_exception_bitmap(vcpu);
2536 vcpu->arch.cr0_guest_owned_bits &= ~vmcs12->cr0_guest_host_mask;
2537 vmcs_writel(CR0_GUEST_HOST_MASK, ~vcpu->arch.cr0_guest_owned_bits);
2538
2539 if (vmx->nested.nested_run_pending &&
2540 (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT)) {
2541 vmcs_write64(GUEST_IA32_PAT, vmcs12->guest_ia32_pat);
2542 vcpu->arch.pat = vmcs12->guest_ia32_pat;
2543 } else if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
2544 vmcs_write64(GUEST_IA32_PAT, vmx->vcpu.arch.pat);
2545 }
2546
2547 vmcs_write64(TSC_OFFSET, vcpu->arch.tsc_offset);
2548
2549 if (kvm_has_tsc_control)
2550 decache_tsc_multiplier(vmx);
2551
2552 nested_vmx_transition_tlb_flush(vcpu, vmcs12, true);
2553
2554 if (nested_cpu_has_ept(vmcs12))
2555 nested_ept_init_mmu_context(vcpu);
2556
2557 /*
2558 * This sets GUEST_CR0 to vmcs12->guest_cr0, possibly modifying those
2559 * bits which we consider mandatory enabled.
2560 * The CR0_READ_SHADOW is what L2 should have expected to read given
2561 * the specifications by L1; It's not enough to take
2562 * vmcs12->cr0_read_shadow because on our cr0_guest_host_mask we we
2563 * have more bits than L1 expected.
2564 */
2565 vmx_set_cr0(vcpu, vmcs12->guest_cr0);
2566 vmcs_writel(CR0_READ_SHADOW, nested_read_cr0(vmcs12));
2567
2568 vmx_set_cr4(vcpu, vmcs12->guest_cr4);
2569 vmcs_writel(CR4_READ_SHADOW, nested_read_cr4(vmcs12));
2570
2571 vcpu->arch.efer = nested_vmx_calc_efer(vmx, vmcs12);
2572 /* Note: may modify VM_ENTRY/EXIT_CONTROLS and GUEST/HOST_IA32_EFER */
2573 vmx_set_efer(vcpu, vcpu->arch.efer);
2574
> 2575 if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_CET_STATE) {
> 2576 vmcs_writel(GUEST_SSP, vmcs12->guest_ssp);
> 2577 vmcs_writel(GUEST_INTR_SSP_TABLE, vmcs12->guest_ssp_tbl);
> 2578 vmcs_writel(GUEST_S_CET, vmcs12->guest_s_cet);
2579 }
2580
2581 /*
2582 * Guest state is invalid and unrestricted guest is disabled,
2583 * which means L1 attempted VMEntry to L2 with invalid state.
2584 * Fail the VMEntry.
2585 */
2586 if (CC(!vmx_guest_state_valid(vcpu))) {
2587 *entry_failure_code = ENTRY_FAIL_DEFAULT;
2588 return -EINVAL;
2589 }
2590
2591 /* Shadow page tables on either EPT or shadow page tables. */
2592 if (nested_vmx_load_cr3(vcpu, vmcs12->guest_cr3, nested_cpu_has_ept(vmcs12),
2593 entry_failure_code))
2594 return -EINVAL;
2595
2596 /*
2597 * Immediately write vmcs02.GUEST_CR3. It will be propagated to vmcs12
2598 * on nested VM-Exit, which can occur without actually running L2 and
2599 * thus without hitting vmx_load_mmu_pgd(), e.g. if L1 is entering L2 with
2600 * vmcs12.GUEST_ACTIVITYSTATE=HLT, in which case KVM will intercept the
2601 * transition to HLT instead of running L2.
2602 */
2603 if (enable_ept)
2604 vmcs_writel(GUEST_CR3, vmcs12->guest_cr3);
2605
2606 /* Late preparation of GUEST_PDPTRs now that EFER and CRs are set. */
2607 if (load_guest_pdptrs_vmcs12 && nested_cpu_has_ept(vmcs12) &&
2608 is_pae_paging(vcpu)) {
2609 vmcs_write64(GUEST_PDPTR0, vmcs12->guest_pdptr0);
2610 vmcs_write64(GUEST_PDPTR1, vmcs12->guest_pdptr1);
2611 vmcs_write64(GUEST_PDPTR2, vmcs12->guest_pdptr2);
2612 vmcs_write64(GUEST_PDPTR3, vmcs12->guest_pdptr3);
2613 }
2614
2615 if (!enable_ept)
2616 vcpu->arch.walk_mmu->inject_page_fault = vmx_inject_page_fault_nested;
2617
2618 if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL) &&
2619 WARN_ON_ONCE(kvm_set_msr(vcpu, MSR_CORE_PERF_GLOBAL_CTRL,
2620 vmcs12->guest_ia32_perf_global_ctrl)))
2621 return -EINVAL;
2622
2623 kvm_rsp_write(vcpu, vmcs12->guest_rsp);
2624 kvm_rip_write(vcpu, vmcs12->guest_rip);
2625 return 0;
2626 }
2627
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45659 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] KVM: nVMX: Sync L2 guest CET states between L1/L2
2021-02-09 8:37 [PATCH] KVM: nVMX: Sync L2 guest CET states between L1/L2 Yang Weijiang
2021-02-09 10:53 ` kernel test robot
@ 2021-02-11 17:18 ` Sean Christopherson
2021-02-13 2:03 ` Yang Weijiang
1 sibling, 1 reply; 4+ messages in thread
From: Sean Christopherson @ 2021-02-11 17:18 UTC (permalink / raw)
To: Yang Weijiang; +Cc: pbonzini, kvm, linux-kernel
On Tue, Feb 09, 2021, Yang Weijiang wrote:
> When L2 guest status has been changed by L1 QEMU/KVM, sync the change back
> to L2 guest before the later's next vm-entry. On the other hand, if it's
> changed due to L2 guest, sync it back so as to let L1 guest see the change.
>
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
> arch/x86/kvm/vmx/nested.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 9728efd529a1..b9d8db8facea 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2602,6 +2602,12 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
> /* Note: may modify VM_ENTRY/EXIT_CONTROLS and GUEST/HOST_IA32_EFER */
> vmx_set_efer(vcpu, vcpu->arch.efer);
>
> + if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_CET_STATE) {
> + vmcs_writel(GUEST_SSP, vmcs12->guest_ssp);
> + vmcs_writel(GUEST_INTR_SSP_TABLE, vmcs12->guest_ssp_tbl);
> + vmcs_writel(GUEST_S_CET, vmcs12->guest_s_cet);
> + }
> +
This is incomplete. If VM_ENTRY_LOAD_CET_STATE is not set, then CET state needs
to be propagated from vmcs01 to vmcs02. See nested.vmcs01_debugctl and
nested.vmcs01_guest_bndcfgs.
It's tempting to say that we should add machinery to simplify implementing new
fields that are conditionally loading, e.g. define an array that specifies the
field, its control, and its offset in vmcs12, then process the array at the
appropriate time. That might be overkill though...
> /*
> * Guest state is invalid and unrestricted guest is disabled,
> * which means L1 attempted VMEntry to L2 with invalid state.
> @@ -4152,6 +4158,12 @@ static void sync_vmcs02_to_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>
> if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_IA32_EFER)
> vmcs12->guest_ia32_efer = vcpu->arch.efer;
> +
> + if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_CET_STATE) {
This is wrong, guest state is saved on VM-Exit if the control is _supported_,
it doesn't have to be enabled.
If the processor supports the 1-setting of the “load CET” VM-entry control,
the contents of the IA32_S_CET and IA32_INTERRUPT_SSP_TABLE_ADDR MSRs are
saved into the corresponding fields. On processors that do not support Intel
64 architecture, bits 63:32 of these MSRs are not saved.
And I'm pretty sure we should define these fields as a so called "rare" fields,
i.e. add 'em to the case statement in is_vmcs12_ext_field() and process them in
sync_vmcs02_to_vmcs12_rare(). CET isn't easily emulated, so they should almost
never be read/written by a VMM, and thus aren't with synchronizing to vmcs12 on
every exit.
> + vmcs12->guest_ssp = vmcs_readl(GUEST_SSP);
> + vmcs12->guest_ssp_tbl = vmcs_readl(GUEST_INTR_SSP_TABLE);
> + vmcs12->guest_s_cet = vmcs_readl(GUEST_S_CET);
> + }
> }
>
> /*
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 4+ messages in thread