From: Chenyi Qiang <chenyi.qiang@intel.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
Vitaly Kuznetsov <vkuznets@redhat.com>,
Wanpeng Li <wanpengli@tencent.com>,
Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
Xiaoyao Li <xiaoyao.li@intel.com>,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 3/3] KVM: VMX: Enable Notify VM exit
Date: Thu, 19 May 2022 18:38:16 +0800 [thread overview]
Message-ID: <a3212daa-16d8-71a8-ef65-f73af268c089@intel.com> (raw)
In-Reply-To: <YoVzf8tPgONxjmZM@google.com>
On 5/19/2022 6:30 AM, Sean Christopherson wrote:
> On Thu, Apr 21, 2022, Chenyi Qiang wrote:
>> @@ -1504,6 +1511,8 @@ struct kvm_x86_ops {
>> * Returns vCPU specific APICv inhibit reasons
>> */
>> unsigned long (*vcpu_get_apicv_inhibit_reasons)(struct kvm_vcpu *vcpu);
>> +
>> + bool has_notify_vmexit;
>
> I'm pretty sure I suggested this, but seeing it in code, it kinda sorta makes things
> worst if we don't first consolidate the existing flags. kvm_x86_ops works, but we'd
> definitely be taking liberties with the "ops" part.
>
> What about adding struct kvm_caps to collect these flags/settings that don't fit
> into kvm_cpu_caps because they're not a CPUID feature flag? kvm_x86_ops has the
> advantage of kinda being read-only after init since VMX modifies vmx_x86_ops,
> but IMO that's not enough reason to shove this into kvm_x86_ops. And long term,
> we might be able find a way to mark kvm_caps as full __ro_after_init.
>
> If no one objects, the attached patch can slide in before this patch, then
> has_notifiy_vmexit can land in kvm_caps.
>
> struct kvm_caps {
> /* control of guest tsc rate supported? */
> bool has_tsc_control;
> /* maximum supported tsc_khz for guests */
> u32 max_guest_tsc_khz;
> /* number of bits of the fractional part of the TSC scaling ratio */
> u8 tsc_scaling_ratio_frac_bits;
> /* maximum allowed value of TSC scaling ratio */
> u64 max_tsc_scaling_ratio;
> /* 1ull << kvm_caps.tsc_scaling_ratio_frac_bits */
> u64 default_tsc_scaling_ratio;
> /* bus lock detection supported? */
> bool has_bus_lock_exit;
>
> u64 supported_mce_cap;
> u64 supported_xcr0;
> u64 supported_xss;
> };
>
Thanks Sean for your patch. I think an unintentional change is mixed in it:
@@ -4739,7 +4725,8 @@ static int
kvm_vcpu_ready_for_interrupt_injection(struct kvm_vcpu *vcpu)
return (kvm_arch_interrupt_allowed(vcpu) &&
kvm_cpu_accept_dm_intr(vcpu) &&
!kvm_event_needs_reinjection(vcpu) &&
- !vcpu->arch.exception.pending);
+ !vcpu->arch.exception.pending &&
+ !kvm_test_request(KVM_REQ_TRIPLE_FAULT, vcpu));
}
Maybe this should belong to the patch 1?
>> @@ -6090,6 +6094,18 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>> }
>> mutex_unlock(&kvm->lock);
>> break;
>> + case KVM_CAP_X86_NOTIFY_VMEXIT:
>> + r = -EINVAL;
>> + if ((u32)cap->args[0] & ~KVM_X86_NOTIFY_VMEXIT_VALID_BITS)
>> + break;
>> + if (!kvm_x86_ops.has_notify_vmexit)
>> + break;
>> + if (!(u32)cap->args[0] & KVM_X86_NOTIFY_VMEXIT_ENABLED)
>> + break;
>> + kvm->arch.notify_window = cap->args[0] >> 32;
>
> Setting notify_vmexit and notify_vmexit_flags needs to be done under kvm->lock,
> and changing notify_window if kvm->created_vcpus > 0 needs to disallowed, otherwise
> init_vmcs() will use the wrong value.
>
> notify_vmexit_flags could be changed on the fly, but I doubt that's worth
> supporting as even the smallest amount of complexity will go unused.
>
> So I think this?
>
Make sense.
> case KVM_CAP_X86_NOTIFY_VMEXIT:
> r = -EINVAL;
> if ((u32)cap->args[0] & ~KVM_X86_NOTIFY_VMEXIT_VALID_BITS)
> break;
> if (!kvm_x86_ops.has_notify_vmexit)
> break;
> if (!(u32)cap->args[0] & KVM_X86_NOTIFY_VMEXIT_ENABLED)
> break;
> mutex_lock(&kvm->lock);
> if (!kvm->created_vcpus) {
> kvm->arch.notify_window = cap->args[0] >> 32;
> kvm->arch.notify_vmexit_flags = (u32)cap->args[0];
> r = 0;
> }
> mutex_unlock(&kvm->lock);
> break;
next prev parent reply other threads:[~2022-05-19 10:38 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-21 7:29 [PATCH v6 0/3] Introduce Notify VM exit Chenyi Qiang
2022-04-21 7:29 ` [PATCH v6 1/3] KVM: X86: Save&restore the triple fault request Chenyi Qiang
2022-05-18 18:42 ` Sean Christopherson
2022-05-19 6:25 ` Chenyi Qiang
2022-04-21 7:29 ` [PATCH v6 2/3] KVM: selftests: Add a test to get/set triple fault event Chenyi Qiang
2022-05-18 19:20 ` Sean Christopherson
2022-05-23 6:46 ` Chenyi Qiang
2022-05-23 16:23 ` Sean Christopherson
2022-05-24 13:27 ` Chenyi Qiang
2022-04-21 7:29 ` [PATCH v6 3/3] KVM: VMX: Enable Notify VM exit Chenyi Qiang
2022-05-17 0:59 ` Chenyi Qiang
2022-05-18 22:30 ` Sean Christopherson
2022-05-19 10:38 ` Chenyi Qiang [this message]
2022-05-19 15:22 ` Sean Christopherson
2022-05-06 2:43 ` [PATCH v6 0/3] Introduce " Chenyi Qiang
2022-05-23 19:30 ` Paolo Bonzini
2022-05-24 14:00 ` Chenyi Qiang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a3212daa-16d8-71a8-ef65-f73af268c089@intel.com \
--to=chenyi.qiang@intel.com \
--cc=jmattson@google.com \
--cc=joro@8bytes.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pbonzini@redhat.com \
--cc=seanjc@google.com \
--cc=vkuznets@redhat.com \
--cc=wanpengli@tencent.com \
--cc=xiaoyao.li@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).