linux-kernel - Re: [PATCHv3 5/8] KVM: SVM: Add VNMI support in inject

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d30192e4-9e12-f770-e944-e3c38b9514b8@maciej.szmigiero.name>
Date:   Fri, 26 Aug 2022 14:20:45 +0200
From:   "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
To:     "Shukla, Santosh" <santosh.shukla@....com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Tom Lendacky <thomas.lendacky@....com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, mlevitsk@...hat.com
Subject: Re: [PATCHv3 5/8] KVM: SVM: Add VNMI support in inject_nmi

On 26.08.2022 11:35, Shukla, Santosh wrote:
> On 8/25/2022 7:46 PM, Maciej S. Szmigiero wrote:
>> On 25.08.2022 16:05, Shukla, Santosh wrote:
>>> On 8/25/2022 6:15 PM, Maciej S. Szmigiero wrote:
>>>> On 25.08.2022 12:56, Shukla, Santosh wrote:
>>>>> On 8/24/2022 6:26 PM, Maciej S. Szmigiero wrote:
>>>>>> On 24.08.2022 14:13, Shukla, Santosh wrote:
>>>>>>> Hi Maciej,
>>>>>>>
>>>>>>> On 8/11/2022 2:54 AM, Maciej S. Szmigiero wrote:
>>>>>>>> On 10.08.2022 08:12, Santosh Shukla wrote:
>>>>>>>>> Inject the NMI by setting V_NMI in the VMCB interrupt control. processor
>>>>>>>>> will clear V_NMI to acknowledge processing has started and will keep the
>>>>>>>>> V_NMI_MASK set until the processor is done with processing the NMI event.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Santosh Shukla <santosh.shukla@....com>
>>>>>>>>> ---
>>>>>>>>> v3:
>>>>>>>>> - Removed WARN_ON check.
>>>>>>>>>
>>>>>>>>> v2:
>>>>>>>>> - Added WARN_ON check for vnmi pending.
>>>>>>>>> - use `get_vnmi_vmcb` to get correct vmcb so to inject vnmi.
>>>>>>>>>
>>>>>>>>>       arch/x86/kvm/svm/svm.c | 7 +++++++
>>>>>>>>>       1 file changed, 7 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>>>>>>>> index e260e8cb0c81..8c4098b8a63e 100644
>>>>>>>>> --- a/arch/x86/kvm/svm/svm.c
>>>>>>>>> +++ b/arch/x86/kvm/svm/svm.c
>>>>>>>>> @@ -3479,7 +3479,14 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
>>>>>>>>>       static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>>>>>>>>>       {
>>>>>>>>>           struct vcpu_svm *svm = to_svm(vcpu);
>>>>>>>>> +    struct vmcb *vmcb = NULL;
>>>>>>>>>       +    if (is_vnmi_enabled(svm)) {
>>>>>>>>
>>>>>>>> I guess this should be "is_vnmi_enabled(svm) && !svm->nmi_l1_to_l2"
>>>>>>>> since if nmi_l1_to_l2 is true then the NMI to be injected originally
>>>>>>>> comes from L1's VMCB12 EVENTINJ field.
>>>>>>>>
>>>>>>>
>>>>>>> Not sure if I understood the case fully.. so trying to sketch scenario here -
>>>>>>> if nmi_l1_to_l2 is true then event is coming from EVTINJ. .which could
>>>>>>> be one of following case -
>>>>>>> 1) L0 (vnmi enabled) and L1 (vnmi disabled)
>>>>>>
>>>>>> As far as I can see in this case:
>>>>>> is_vnmi_enabled() returns whether VMCB02's int_ctl has V_NMI_ENABLE bit set.
>>>>>>
>>>>>
>>>>> For L1 with vnmi disabled case - is_vnmi_enabled()->get_vnmi_vmcb() will return false so the
>>>>> execution path will opt EVTINJ model for re-injection.
>>>>
>>>> I guess by "get_vnmi_vmcb() will return false" you mean it will return NULL,
>>>> since this function returns a pointer, not a bool.
>>>>
>>>
>>> Yes, I meant is_vnmi_enabled() will return false if vnmi param is unset.
>>>
>>>> I can't see however, how this will happen:
>>>>> static inline struct vmcb *get_vnmi_vmcb(struct vcpu_svm *svm)
>>>>> {
>>>>>       if (!vnmi)
>>>>>           return NULL;
>>>>           ^ "vnmi" variable controls whether L0 uses vNMI,
>>>>          so this variable is true in our case
>>>>
>>>
>>> No.
>>>
>>> In L1 case (vnmi disabled) - vnmi param will be false.
>>
>> Perhaps there was a misunderstanding here - the case here
>> isn't the code under discussion running as L1, but as L0
>> where L1 not using vNMI - L1 here can be an old version of KVM,
>> or Hyper-V, or any other hypervisor.
>>
> 
> Ok.
> 
>> In this case L0 is re-injecting an EVENTINJ NMI into L2 on
>> the behalf of L1.
>> That's when "nmi_l1_to_l2" is true.
>   
> hmm,. trying to understand the event re-injection flow -
> First L1 (non-vnmi) injecting event to L2 guest, in-turn
> intercepted by L0, 

That's right, the L1's VMRUN of L2 gets intercepted by L0.

> L0 sees event injection through EVTINJ

It sees that L1 wants to inject an NMI into L2 via VMCB12 EVTINJ.

> so sets the 'nmi_l1_to_l2' var 

That's right, L0 needs to keep track of this fact.

> and then L0 calls svm_inject_nmi()

Not yet - at this point svm_inject_nmi() is NOT called
(rather than, VMCB12 EVTINJ is directly copied into VMCB02 EVTINJ).

Now L0 does the actual VMRUN of L2.

Let's say that there is an intervening VMExit during delivery of
that NMI to L2, of type which is handled by L0 (perhaps a NPF on
L2 IDT or so).

In this case the NMI will be returned in VMCB02 EXITINTINFO and
needs to be re-injected into L2 on the next VMRUN,
again via EVTINJ.

That's when svm_inject_nmi() will get called to re-inject
that NMI.

> to re-inject event in L2 - is that correct (nmi_l1_to_l2) flow?
Hope the flow is clear now.

> 
> Thanks,.
> Santosh

Thanks,
Maciej