Messages in this thread | | | From | Vitaly Kuznetsov <> | Subject | Re: [PATCH 6/7] x86/hyper-v: use hypercall for remove TLB flush | Date | Mon, 10 Apr 2017 19:21:34 +0200 |
| |
Jork Loeser <Jork.Loeser@microsoft.com> writes:
>> -----Original Message----- >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com] >> Sent: Friday, April 7, 2017 04:27 >> To: devel@linuxdriverproject.org; x86@kernel.org >> Cc: linux-kernel@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; >> Haiyang Zhang <haiyangz@microsoft.com>; Stephen Hemminger >> <sthemmin@microsoft.com>; Thomas Gleixner <tglx@linutronix.de>; Ingo >> Molnar <mingo@redhat.com>; H. Peter Anvin <hpa@zytor.com>; Steven >> Rostedt <rostedt@goodmis.org>; Jork Loeser <Jork.Loeser@microsoft.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
| |