DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 7B3237D0DE
From: Vitaly Kuznetsov <vkuznets@redhat.com>
To: Jork Loeser <Jork.Loeser@microsoft.com>
Cc: "devel\@linuxdriverproject.org" <devel@linuxdriverproject.org>,
        "x86\@kernel.org" <x86@kernel.org>,
        "linux-kernel\@vger.kernel.org" <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>
Subject: Re: [PATCH 6/7] x86/hyper-v: use hypercall for remove TLB flush
References: <20170407112701.17157-1-vkuznets@redhat.com>
        <20170407112701.17157-7-vkuznets@redhat.com>
        <MWHPR21MB063911897C6E0286F3055A56F10C0@MWHPR21MB0639.namprd21.prod.outlook.com>
Date: Mon, 10 Apr 2017 19:21:34 +0200
In-Reply-To: <MWHPR21MB063911897C6E0286F3055A56F10C0@MWHPR21MB0639.namprd21.prod.outlook.com>
        (Jork Loeser's message of "Fri, 7 Apr 2017 20:46:13 +0000")
Message-ID: <87a87oqiu9.fsf@vitty.brq.redhat.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3349
Lines: 102

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