On 18-09-13 13:24:07, Thomas Gleixner wrote: > On Thu, 13 Sep 2018, Yi Sun wrote: > > +++ b/arch/x86/hyperv/hv_spinlock.c > > @@ -0,0 +1,99 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +/* > > + * Hyper-V specific spinlock code. > > + * > > + * Copyright (C) 2018, Intel, Inc. > > + * > > + * Author : Yi Sun <yi.y....@linux.intel.com> > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms of the GNU General Public License version 2 as published > > + * by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, but > > + * WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or > > + * NON INFRINGEMENT. See the GNU General Public License for more > > + * details. > > Please remove the boilerplate. The SPDX identifier is enough. If in doubt > talk to Intel legal. > I will check this. Thanks!
> > +static bool hv_pvspin = true; > > __initdata please. > > > +static u32 spin_wait_info = 0; > > No need for 0 initialization. > Will modify them. > > + > > +static void hv_notify_long_spin_wait(void) > > +{ > > + u64 input = spin_wait_info | 0x00000000ffffffff; > > What? The result is always 0x00000000ffffffff ..... > Oh, sorry for such error. > > + spin_wait_info++; > > + hv_do_fast_hypercall8(HVCALL_NOTIFY_LONG_SPIN_WAIT, input); > > +} > > + > > +static void hv_qlock_kick(int cpu) > > +{ > > + spin_wait_info--; > > Looking at the above this is completely pointless and I have no idea what > that variable is supposed to do. > Per Microsoft Hypervisor Top Level Functional Specification, the input parameter of HVCALL_NOTIFY_LONG_SPIN_WAIT is defined as below: SpinwaitInfo – Specifies the accumulated count the guest was spinning. So, it is increased when guest is spinning and reduced when spinlock is released. I may add comments to explain it. > > + apic->send_IPI(cpu, X86_PLATFORM_IPI_VECTOR); > > +} > > + > > +/* > > + * Halt the current CPU & release it back to the host > > + */ > > +static void hv_qlock_wait(u8 *byte, u8 val) > > +{ > > + unsigned long msr_val; > > + > > + if (READ_ONCE(*byte) != val) > > + return; > > + > > + hv_notify_long_spin_wait(); > > + > > + rdmsrl(HV_X64_MSR_GUEST_IDLE, msr_val); > > Magic rdmsrl(). That wants a comment what this is for. > Per above spec, reading HV_X64_MSR_GUEST_IDLE can make guest idle. I will add comment here. > > +/* > > + * Hyper-V does not support this so far. > > + */ > > +bool hv_vcpu_is_preempted(int vcpu) > > static ? > PV_CALLEE_SAVE_REGS_THUNK definition is below. #define PV_CALLEE_SAVE_REGS_THUNK(func) \ extern typeof(func) __raw_callee_save_##func; \ ...... So, the hv_vcpu_is_preempted cannot be declared as 'static'. Otherwise, the make fails with below info. arch/x86/hyperv/hv_spinlock.o: In function `__raw_callee_save_hv_vcpu_is_preempted': hv_spinlock.c:(.text+0xd): undefined reference to `hv_vcpu_is_preempted' > > +{ > > + return false; > > +} > > +PV_CALLEE_SAVE_REGS_THUNK(hv_vcpu_is_preempted); > > + > > +void __init hv_init_spinlocks(void) > > +{ > > + if (!hv_pvspin || > > + !apic || > > + !(ms_hyperv.hints & HV_X64_CLUSTER_IPI_RECOMMENDED) || > > + !(ms_hyperv.features & HV_X64_MSR_GUEST_IDLE_AVAILABLE)) { > > + pr_warn("hv: PV spinlocks disabled\n"); > > Why does this need to be pr_warn? This is purely informational. Also please > use pr_fmt instead of the 'hv:' prefix. > Got it. Thanks! > > +static __init int hv_parse_nopvspin(char *arg) > > +{ > > + hv_pvspin = false; > > + return 0; > > +} > > +early_param("hv_nopvspin", hv_parse_nopvspin); > > That lacks Documentation of the command line parameter. Wants to be in the > same patch. > Will merge patch 3 into 2. Thanks! > Thanks, > > tglx