Thread (22 messages) flat view 22 messages, 4 authors, 2021-04-21

Re: [PATCH v2 3/7] KVM: x86: hyper-v: Move the remote TLB flush logic out of vmx

From: Vitaly Kuznetsov <vkuznets@redhat.com>
Date: 2021-04-20 15:57:27
Also in: kvm, lkml

Vineeth Pillai [off-list ref] writes:
On 4/16/2021 4:36 AM, Vitaly Kuznetsov wrote:
quoted
quoted
  struct kvm_vm_stat {
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 58fa8c029867..614b4448a028 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
I still think that using arch/x86/kvm/hyperv.[ch] for KVM-on-Hyper-V is
misleading. Currently, these are dedicated to emulating Hyper-V
interface to KVM guests and this is orthogonal to nesting KVM on
Hyper-V. As a solution, I'd suggest you either:
- Put the stuff in x86.c
- Create a dedicated set of files, e.g. 'kvmonhyperv.[ch]' (I also
thought about 'hyperv_host.[ch]' but then I realized it's equally
misleading as one can read this as 'KVM is acting as Hyper-V host').

Personally, I'd vote for the later. Besides eliminating confusion, the
benefit of having dedicated files is that we can avoid compiling them
completely when !IS_ENABLED(CONFIG_HYPERV) (#ifdefs in C are ugly).
Makes sense, creating new set of files looks good to me. The default 
hyperv.c
for hyperv emulation also seems misleading - probably we should rename it
to hyperv_host_emul.[ch] or similar. That way, probably I can use 
hyperv.[ch]
for kvm on hyperv code. If you feel, thats too big of a churn, I shall use
kvm_on_hyperv.[ch] (to avoid reading the file differently). What do you 
think?
I agree that 'hyperv.[ch]' is not ideal but I'm on the fence whether
renaming it is worth it. If we were to rename it, I'd suggest just
'hyperv_emul.[ch]' to indicate that here we're emulating Hyper-V.

I don't think reusing 'hyperv.[ch]' for KVM-on-Hyper-V is a good idea,
it would be doubly misleading and not friendly to backporters. Let's not
do that.
quoted
quoted
@@ -10470,7 +10474,6 @@ void kvm_arch_free_vm(struct kvm *kvm)
  	vfree(kvm);
  }
  
-
Stray change?
It was kinda leftover, but I thought I'd keep it as it removes and 
unnecessary line.
The idea is to have meaninful patches as concise as possible splitting
off cleanup / preparatory patches which don't actually change anything;
this way big series are much easier to review.
Thanks,
Vineeth
-- 
Vitaly
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help