Re: [PATCH] KVM: x86: Fix vCPUs >= 64 can't be online and hotplugged in some scenarios

[Zelin Deng]

On Mon, Nov 09, 2020 at 07:19:44AM -0600, Brijesh Singh wrote:
> 
> On 9/14/20 2:00 AM, Zelin Deng wrote:
> > pvclock data pointers of vCPUs >= HVC_BOOT_ARRAY_SIZE (64) are stored in
> > hvclock_mem wihch is initialized in kvmclock_init_mem().
> > Here're 3 scenarios in current implementation:
> >     - no-kvmclock is set in cmdline. kvm pv clock driver is disabled,
> >       no impact.
> >     - no-kvmclock-vsyscall is set in cmdline. kvmclock_init_mem() won't
> >       be called. No memory for storing pvclock data of vCPUs >= 64, vCPUs
> >       >= 64 can not be online or hotpluged.
> >     - tsc unstable. kvmclock_init_mem() won't be called. vCPUs >= 64 can
> >       not be online or hotpluged.
> > It's not reasonable that vCPUs hotplug have been impacted by last 2
> > scenarios. Hence move kvmclock_init_mem() to front, in case hvclock_mem
> > can not be initialized unexpectedly.
> >
> > Fixes: 6a1cac56f41f9 (x86/kvm: Use __bss_decrypted attribute in shared variables)
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > Cc: Brijesh Singh <brijesh.singh@xxxxxxx>
> > Cc: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > Signed-off-by: Zelin Deng <zelin.deng@xxxxxxxxxxxxxxxxx>
> > ---
> >  arch/x86/kernel/kvmclock.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> > index 34b18f6eeb2c..1abbda25e037 100644
> > --- a/arch/x86/kernel/kvmclock.c
> > +++ b/arch/x86/kernel/kvmclock.c
> > @@ -271,7 +271,14 @@ static int __init kvm_setup_vsyscall_timeinfo(void)
> >  {
> >  #ifdef CONFIG_X86_64
> >  	u8 flags;
> > +#endif
> > +
> > +	if (!kvmclock)
> > +		return 0;
> 
> 
> Overall, I agree with the fix to move the kvmclock_init_mem() in the
> beginning of the function so that memory hvclock_mem is allocated. But
> curious, why do we need this check? The if (kvmclock) did not exist in
> original function and I don't think kvmclock_init_mem() has any
> dependency with it, am I missing something ?
> 
> 
Per my under standing if "no-kvmclock" is set in cmdline, pvclock will
be disabled in guest kernel kvmclock_init() just returns without doing
anything right? However in this scenarios, this function still will be
executed as it is a early_initcall. To avoid a waste of memory, is it
reasonable to do this check?
> > +
> > +	kvmclock_init_mem();
> >  
> > +#ifdef CONFIG_X86_64
> >  	if (!per_cpu(hv_clock_per_cpu, 0) || !kvmclock_vsyscall)
> >  		return 0;
> >  
> > @@ -282,8 +289,6 @@ static int __init kvm_setup_vsyscall_timeinfo(void)
> >  	kvm_clock.vdso_clock_mode = VDSO_CLOCKMODE_PVCLOCK;
> >  #endif
> >  
> > -	kvmclock_init_mem();
> > -
> >  	return 0;
> >  }
> >  early_initcall(kvm_setup_vsyscall_timeinfo);