Messages in this thread | | | From | Vitaly Kuznetsov <> | Subject | Re: [PATCH v2 3/7] KVM: x86: hyper-v: Move the remote TLB flush logic out of vmx | Date | Tue, 20 Apr 2021 17:57:19 +0200 |
| |
Vineeth Pillai <viremana@linux.microsoft.com> writes:
> On 4/16/2021 4:36 AM, Vitaly Kuznetsov wrote: >> >>> 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.
> > >>> @@ -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
| |