I'm not convinced that the test, "if
(kvm_vcpu_apicv_active(&vmx->vcpu))," is entirely correct. This is the
same as "if (vcpu->arch.apic && vcpu->arch.apicv_active)." I don't
believe that L1 has to have lapic_in_kernel() for L0 to use the APICv
features of the hardware when running L2. I'm also not sure that
Hyper-V SynIC activation for L1 has any bearing on whether or not L0
can use the APICv features of the hardware when running L2.
Should this test be "if (enable_apicv)" instead? Even that gives me
pause, since it means that L1 cannot use any of the three features
{"process posted interrupts," "APIC-register virtualization,"
"virtual-interrupt delivery"} unless it can use all three of them.
Didn't some Ivy Bridge SKUs implement only two of them? (Of course, it
doesn't matter if you just want to run kvm as the L1 hypervisor, but I
hope we're getting past that. :-)
I think the code should actually implement the following:
1. Set vmx->nested.nested_vmx_secondary_ctls_high based on the
kvm-implemented capabilities reported by MSR_IA32_VMX_PROCBASED_CTLS2.
2. Clear the capability bits for "APIC-register virtualization" and
"virtual-interrupt delivery" if the module parameter, enable_apicv, is
zero.
Doing (2) means being able to access the actual module parameter,
irrespective of the clearing done by hardware_setup if the trio of
capabilities referenced above aren't all present.
On Wed, Nov 22, 2017 at 2:23 AM, Arbel Moshe <arbel.moshe@xxxxxxxxxx> wrote:
> Implementation of virtual APICv relies on L0 being able to use APICv.
> Therefore, if enable_apicv==false, we should not expose APICv to L1.
>
> This commit makes sure to not expose APICv Secondary CPU controls
> to L1 when enable_apicv==false.
>
> Signed-off-by: Arbel Moshe <arbel.moshe@xxxxxxxxxx>
> Reviewed-by: Liran Alon <liran.alon@xxxxxxxxxx>
> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
> ---
> arch/x86/kvm/vmx.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 0450fbdb97be..a2f157e9c33c 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2811,10 +2811,14 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
> SECONDARY_EXEC_DESC |
> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
> - SECONDARY_EXEC_APIC_REGISTER_VIRT |
> - SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
> SECONDARY_EXEC_WBINVD_EXITING;
>
> + if (kvm_vcpu_apicv_active(&vmx->vcpu)) {
> + vmx->nested.nested_vmx_secondary_ctls_high |=
> + (SECONDARY_EXEC_APIC_REGISTER_VIRT |
> + SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
> + }
> +
> if (enable_ept) {
> /* nested EPT: emulate EPT also to L1 */
> vmx->nested.nested_vmx_secondary_ctls_high |=
> --
> 2.14.1
>
