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);
