linux-kernel - Re: [PATCH 6/7] x86/hyper-v: use hypercall for remove TLB flush

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]

Message-ID: <87a87oqiu9.fsf@vitty.brq.redhat.com>
Date:   Mon, 10 Apr 2017 19:21:34 +0200
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Jork Loeser <Jork.Loeser@...rosoft.com>
Cc:     "devel\@linuxdriverproject.org" <devel@...uxdriverproject.org>,
        "x86\@kernel.org" <x86@...nel.org>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
        "KY Srinivasan" <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH 6/7] x86/hyper-v: use hypercall for remove TLB flush

Jork Loeser <Jork.Loeser@...rosoft.com> writes:

>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:vkuznets@...hat.com]
>> Sent: Friday, April 7, 2017 04:27
>> To: devel@...uxdriverproject.org; x86@...nel.org
>> Cc: linux-kernel@...r.kernel.org; KY Srinivasan <kys@...rosoft.com>;
>> Haiyang Zhang <haiyangz@...rosoft.com>; Stephen Hemminger
>> <sthemmin@...rosoft.com>; Thomas Gleixner <tglx@...utronix.de>; Ingo
>> Molnar <mingo@...hat.com>; H. Peter Anvin <hpa@...or.com>; Steven
>> Rostedt <rostedt@...dmis.org>; Jork Loeser <Jork.Loeser@...rosoft.com>
>> Subject: [PATCH 6/7] x86/hyper-v: use hypercall for remove TLB flush
>
>> diff --git a/arch/x86/hyperv/mmu.c b/arch/x86/hyperv/mmu.c new file
>> mode 100644 index 0000000..fb487cb
>> --- /dev/null
>> +++ b/arch/x86/hyperv/mmu.c
>> @@ -0,0 +1,128 @@
>> +#include <linux/types.h>
>> +#include <linux/hyperv.h>
>> +#include <linux/slab.h>
>> +#include <asm/mshyperv.h>
>> +#include <asm/tlbflush.h>
>> +#include <asm/msr.h>
>> +#include <asm/fpu/api.h>
>> +
>> +/*
>> + * Arbitrary number; we need to pre-allocate per-cpu struct for doing
>> +TLB
>> + * flush hypercalls and we need to pick a size. '16' means we'll be
>> +able
>> + * to flush 16 * 4096 pages (256MB) with one hypercall.
>> + */
>> +#define HV_MMU_MAX_GVAS 16
>> +
>> +/* HvFlushVirtualAddressSpace*, HvFlushVirtualAddressList hypercalls */
>> +struct hv_flush_pcpu {
>> +	struct {
>> +		__u64 address_space;
>> +		__u64 flags;
>> +		__u64 processor_mask;
>> +		__u64 gva_list[HV_MMU_MAX_GVAS];
>> +	} flush;
>> +
>> +	spinlock_t lock;
>> +};
> Does this need an alignment declaration, so that the flush portion never crosses a page boundary when allocated with alloc_percpu()?
>

Thanks for pointing this out! I would slightly prefer we use
__alloc_percpu() and specify something like roundup_pow_of_two()
alignment.

>> +
>> +static struct hv_flush_pcpu __percpu *pcpu_flush;
>> +
>> +static void hyperv_flush_tlb_others(const struct cpumask *cpus,
>> +				    struct mm_struct *mm, unsigned long
>> start,
>> +				    unsigned long end)
>> +{
>> +	struct hv_flush_pcpu *flush;
>> +	unsigned long cur, flags;
>> +	u64 status = -1ULL;
>> +	int cpu, vcpu, gva_n;
>> +
>> +	if (!pcpu_flush || !hv_hypercall_pg)
>> +		goto do_native;
>> +
>> +	if (cpumask_empty(cpus))
>> +		return;
>> +
>> +	flush = this_cpu_ptr(pcpu_flush);
>> +	spin_lock_irqsave(&flush->lock, flags);
>
> What purpose does the spinlock on the CPU-local struct serve? Would a
> local_irq_save() do?

Now I'm not sure why I put it here in the first place :-) Yes, it would
probably do.

> Could this be called from NMI context, such as from the debugger?
>

NMI - I don't think so, native function does smp_call_function_many()
which WARNs even if it's called with interrupts disabled.

> Could this be a long-running loop, e.g. due to a large start/end
> range? If so, consider disabling interrupts only in the inner loop /
> flush the entire space?

The decision for flushing the entire space should probably be done
elsewhere as it is not implementation-specific (and I think it's done
somewhere as I never see requests to flush more than 4096 pages in my
testing).

I can disable interrupts in the inner loop but we'll have to stash flags
and calculated cpu_mask to some local variables. This is not supposed to
be expensive.

-- 
  Vitaly