linux-kernel - Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 14 Sep 2018 17:04:40 +0800
From:   Yi Sun <yi.y.sun@...ux.intel.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        chao.p.peng@...el.com, chao.gao@...el.com,
        isaku.yamahata@...el.com, michael.h.kelly@...rosoft.com,
        tianyu.lan@...rosoft.com, "K. Y. Srinivasan" <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>
Subject: Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for
 Hyper-V

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.sun@...ux.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