* Re: [PATCH] hv_balloon: Add the support of hibernation
From: David Hildenbrand @ 2019-09-26 7:19 UTC (permalink / raw)
To: Dexuan Cui, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, Michael Kelley
In-Reply-To: <PU1P153MB0169B05143A68A56740669AFBF870@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM>
On 25.09.19 22:03, Dexuan Cui wrote:
>> From: linux-hyperv-owner@vger.kernel.org
>> [... snipped ...]
>>> Anyhow, just some comments from my side :) I can see how Windows Server
>>> worked around that issue right now by just XOR'ing both features.
>>>
>>> David / dhildenb
>>
>> Thanks for sharing your thoughts!
>>
>> -- Dexuan
>
> Hi David,
> If my explanation sounds good to you, can I have an Acked-by from you?
>
I do ACK the approach but not the patch in it's current state. I don't
like the ifdefs - once you can get rid of the ifdefery - e.g., after the
prerequisite is upstream - you can add my
Acked-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH net v2] vsock: Fix a lockdep warning in __vsock_release()
From: Stefano Garzarella @ 2019-09-26 7:47 UTC (permalink / raw)
To: Dexuan Cui
Cc: davem@davemloft.net, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, sashal@kernel.org, stefanha@redhat.com,
gregkh@linuxfoundation.org, arnd@arndb.de, deepa.kernel@gmail.com,
ytht.net@gmail.com, tglx@linutronix.de, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
Michael Kelley, jhansen@vmware.com
In-Reply-To: <1569460241-57800-1-git-send-email-decui@microsoft.com>
Hi Dexuan,
On Thu, Sep 26, 2019 at 01:11:27AM +0000, Dexuan Cui wrote:
> Lockdep is unhappy if two locks from the same class are held.
>
> Fix the below warning for hyperv and virtio sockets (vmci socket code
> doesn't have the issue) by using lock_sock_nested() when __vsock_release()
> is called recursively:
>
> ============================================
> WARNING: possible recursive locking detected
> 5.3.0+ #1 Not tainted
> --------------------------------------------
> server/1795 is trying to acquire lock:
> ffff8880c5158990 (sk_lock-AF_VSOCK){+.+.}, at: hvs_release+0x10/0x120 [hv_sock]
>
> but task is already holding lock:
> ffff8880c5158150 (sk_lock-AF_VSOCK){+.+.}, at: __vsock_release+0x2e/0xf0 [vsock]
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
>
> CPU0
> ----
> lock(sk_lock-AF_VSOCK);
> lock(sk_lock-AF_VSOCK);
>
> *** DEADLOCK ***
>
> May be due to missing lock nesting notation
>
> 2 locks held by server/1795:
> #0: ffff8880c5d05ff8 (&sb->s_type->i_mutex_key#10){+.+.}, at: __sock_release+0x2d/0xa0
> #1: ffff8880c5158150 (sk_lock-AF_VSOCK){+.+.}, at: __vsock_release+0x2e/0xf0 [vsock]
>
> stack backtrace:
> CPU: 5 PID: 1795 Comm: server Not tainted 5.3.0+ #1
> Call Trace:
> dump_stack+0x67/0x90
> __lock_acquire.cold.67+0xd2/0x20b
> lock_acquire+0xb5/0x1c0
> lock_sock_nested+0x6d/0x90
> hvs_release+0x10/0x120 [hv_sock]
> __vsock_release+0x24/0xf0 [vsock]
> __vsock_release+0xa0/0xf0 [vsock]
> vsock_release+0x12/0x30 [vsock]
> __sock_release+0x37/0xa0
> sock_close+0x14/0x20
> __fput+0xc1/0x250
> task_work_run+0x98/0xc0
> do_exit+0x344/0xc60
> do_group_exit+0x47/0xb0
> get_signal+0x15c/0xc50
> do_signal+0x30/0x720
> exit_to_usermode_loop+0x50/0xa0
> do_syscall_64+0x24e/0x270
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x7f4184e85f31
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>
> NOTE: I only tested the code on Hyper-V. I can not test the code for
> virtio socket, as I don't have a KVM host. :-( Sorry.
>
> @Stefan, @Stefano: please review & test the patch for virtio socket,
> and let me know if the patch breaks anything. Thanks!
Comment below, I'll test it ASAP!
>
> Changes in v2:
> Avoid the duplication of code in v1: https://lkml.org/lkml/2019/8/19/1361
> Also fix virtio socket code.
>
> net/vmw_vsock/af_vsock.c | 19 +++++++++++++++----
> net/vmw_vsock/hyperv_transport.c | 2 +-
> net/vmw_vsock/virtio_transport_common.c | 2 +-
> 3 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index ab47bf3ab66e..dbae4373cbab 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -638,8 +638,10 @@ struct sock *__vsock_create(struct net *net,
> }
> EXPORT_SYMBOL_GPL(__vsock_create);
>
> -static void __vsock_release(struct sock *sk)
> +static void __vsock_release(struct sock *sk, int level)
> {
> + WARN_ON(level != 1 && level != 2);
> +
> if (sk) {
> struct sk_buff *skb;
> struct sock *pending;
> @@ -648,9 +650,18 @@ static void __vsock_release(struct sock *sk)
> vsk = vsock_sk(sk);
> pending = NULL; /* Compiler warning. */
>
> + /* The release call is supposed to use lock_sock_nested()
> + * rather than lock_sock(), if a sock lock should be acquired.
> + */
> transport->release(vsk);
>
> - lock_sock(sk);
> + /* When "level" is 2, use the nested version to avoid the
> + * warning "possible recursive locking detected".
> + */
> + if (level == 1)
> + lock_sock(sk);
Since lock_sock() calls lock_sock_nested(sk, 0), could we use directly
lock_sock_nested(sk, level) with level = 0 in vsock_release() and
level = SINGLE_DEPTH_NESTING here in the while loop?
> + else
> + lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
> sock_orphan(sk);
> sk->sk_shutdown = SHUTDOWN_MASK;
>
> @@ -659,7 +670,7 @@ static void __vsock_release(struct sock *sk)
>
> /* Clean up any sockets that never were accepted. */
> while ((pending = vsock_dequeue_accept(sk)) != NULL) {
> - __vsock_release(pending);
> + __vsock_release(pending, 2);
> sock_put(pending);
> }
>
> @@ -708,7 +719,7 @@ EXPORT_SYMBOL_GPL(vsock_stream_has_space);
>
> static int vsock_release(struct socket *sock)
> {
> - __vsock_release(sock->sk);
> + __vsock_release(sock->sk, 1);
> sock->sk = NULL;
> sock->state = SS_FREE;
>
> diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
> index 261521d286d6..c443db7af8d4 100644
> --- a/net/vmw_vsock/hyperv_transport.c
> +++ b/net/vmw_vsock/hyperv_transport.c
> @@ -559,7 +559,7 @@ static void hvs_release(struct vsock_sock *vsk)
> struct sock *sk = sk_vsock(vsk);
> bool remove_sock;
>
> - lock_sock(sk);
> + lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
> remove_sock = hvs_close_lock_held(vsk);
> release_sock(sk);
> if (remove_sock)
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 5bb70c692b1e..a666ef8fc54e 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -820,7 +820,7 @@ void virtio_transport_release(struct vsock_sock *vsk)
> struct sock *sk = &vsk->sk;
> bool remove_sock = true;
>
> - lock_sock(sk);
> + lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
> if (sk->sk_type == SOCK_STREAM)
> remove_sock = virtio_transport_close(vsk);
>
Thanks,
Stefano
^ permalink raw reply
* Re: [PATCH v5 1/3] x86/hyper-v: Suspend/resume the hypercall page for hibernation
From: Vitaly Kuznetsov @ 2019-09-26 10:44 UTC (permalink / raw)
To: Dexuan Cui
Cc: linux-arch@vger.kernel.org, arnd@arndb.de, bp@alien8.de,
daniel.lezcano@linaro.org, Haiyang Zhang, hpa@zytor.com,
KY Srinivasan, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, mingo@redhat.com, sashal@kernel.org,
Stephen Hemminger, tglx@linutronix.de, x86@kernel.org,
Michael Kelley, Sasha Levin
In-Reply-To: <1567723581-29088-2-git-send-email-decui@microsoft.com>
Dexuan Cui <decui@microsoft.com> writes:
> This is needed for hibernation, e.g. when we resume the old kernel, we need
> to disable the "current" kernel's hypercall page and then resume the old
> kernel's.
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> ---
> arch/x86/hyperv/hv_init.c | 33 +++++++++++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 866dfb3..037b0f3 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -20,6 +20,7 @@
> #include <linux/hyperv.h>
> #include <linux/slab.h>
> #include <linux/cpuhotplug.h>
> +#include <linux/syscore_ops.h>
> #include <clocksource/hyperv_timer.h>
>
> void *hv_hypercall_pg;
> @@ -223,6 +224,34 @@ static int __init hv_pci_init(void)
> return 1;
> }
>
> +static int hv_suspend(void)
> +{
> + union hv_x64_msr_hypercall_contents hypercall_msr;
> +
> + /* Reset the hypercall page */
> + rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> + hypercall_msr.enable = 0;
> + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +
(trying to think out loud, not sure there's a real issue):
When PV IPIs (or PV TLB flush) are enabled we do the following checks:
if (!hv_hypercall_pg)
return false;
or
if (!hv_hypercall_pg)
goto do_native;
which will pass as we're not invalidating the pointer. Can we actually
be sure that the kernel will never try to send an IPI/do TLB flush
before we resume?
--
Vitaly
^ permalink raw reply
* Re: [PATCH v5 3/3] clocksource/drivers: Suspend/resume Hyper-V clocksource for hibernation
From: Daniel Lezcano @ 2019-09-26 13:16 UTC (permalink / raw)
To: Dexuan Cui, tglx@linutronix.de, arnd@arndb.de, bp@alien8.de,
Haiyang Zhang, hpa@zytor.com, KY Srinivasan,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
mingo@redhat.com, sashal@kernel.org, Stephen Hemminger,
x86@kernel.org, Michael Kelley, Sasha Levin
Cc: linux-arch@vger.kernel.org
In-Reply-To: <PU1P153MB0169A28B05A7CDE04A57AA58BF870@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM>
On 26/09/2019 01:35, Dexuan Cui wrote:
>> From: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Sent: Wednesday, September 25, 2019 4:21 PM
>> To: Dexuan Cui <decui@microsoft.com>; arnd@arndb.de; bp@alien8.de;
>> Haiyang Zhang <haiyangz@microsoft.com>; hpa@zytor.com; KY Srinivasan
>> <kys@microsoft.com>; linux-hyperv@vger.kernel.org;
>> linux-kernel@vger.kernel.org; mingo@redhat.com; sashal@kernel.org; Stephen
>> Hemminger <sthemmin@microsoft.com>; tglx@linutronix.de; x86@kernel.org;
>> Michael Kelley <mikelley@microsoft.com>; Sasha Levin
>> <Alexander.Levin@microsoft.com>
>> Cc: linux-arch@vger.kernel.org
>> Subject: Re: [PATCH v5 3/3] clocksource/drivers: Suspend/resume Hyper-V
>> clocksource for hibernation
>>
>> On 06/09/2019 00:47, Dexuan Cui wrote:
>>> This is needed for hibernation, e.g. when we resume the old kernel, we need
>>> to disable the "current" kernel's TSC page and then resume the old kernel's.
>>>
>>> Signed-off-by: Dexuan Cui <decui@microsoft.com>
>>> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>>
>> I can take this patch if needed.
>
> Thanks, Daniel! Usually tglx takes care of the patches, but it looks recently he
> may be too busy to handle the 3 patches.
>
> I guess you can take the patch, if tglx has no objection. :-)
> If you take the patch, please take all the 3 patches.
I maintain drivers/clocksource for the tip/timers/core branch. I don't
want to proxy another tip branch as it is out of my jurisdiction.
So I can take patch 3/3 but will let the other 2 patches to be picked by
the right person. It is your call.
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply
* RE: [PATCH] HID: hyperv: Add the support of hibernation
From: Jiri Kosina @ 2019-09-26 13:22 UTC (permalink / raw)
To: Dexuan Cui
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, benjamin.tissoires@redhat.com,
linux-hyperv@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Michael Kelley
In-Reply-To: <PU1P153MB01695CEE01D65E8CD5CFA4E9BF870@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM>
On Wed, 25 Sep 2019, Dexuan Cui wrote:
> > We need mousevsc_pm_notify() to make sure the pm_wakeup_hard_event()
> > call does not prevent the system from entering hibernation: the
> > hibernation is a relatively long process, which can be aborted by the
> > call pm_wakeup_hard_event(), which is invoked upon mouse events.
> >
> > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > ---
> >
> > This patch is basically a pure Hyper-V specific change and it has a
> > build dependency on the commit 271b2224d42f ("Drivers: hv: vmbus:
> > Implement
> > suspend/resume for VSC drivers for hibernation"), which is on Sasha Levin's
> > Hyper-V tree's hyperv-next branch [ ... snipped ...]
> >
> > I request this patch should go through Sasha's tree rather than the
> > input subsystem's tree.
> >
> > Hi Jiri, Benjamin, can you please Ack?
>
> Hi Jiri, Benjamin,
> Can you please take a look at the patch?
Hi Dexuan,
I am planning to process it once 5.4 merge window is over and thus hid.git
is open again for 5.5 material.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* RE: [PATCH] HID: hyperv: Add the support of hibernation
From: Jiri Kosina @ 2019-09-26 13:23 UTC (permalink / raw)
To: Dexuan Cui
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, benjamin.tissoires@redhat.com,
linux-hyperv@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Michael Kelley
In-Reply-To: <nycvar.YFH.7.76.1909261521410.24354@cbobk.fhfr.pm>
On Thu, 26 Sep 2019, Jiri Kosina wrote:
> > > We need mousevsc_pm_notify() to make sure the pm_wakeup_hard_event()
> > > call does not prevent the system from entering hibernation: the
> > > hibernation is a relatively long process, which can be aborted by the
> > > call pm_wakeup_hard_event(), which is invoked upon mouse events.
> > >
> > > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > > ---
> > >
> > > This patch is basically a pure Hyper-V specific change and it has a
> > > build dependency on the commit 271b2224d42f ("Drivers: hv: vmbus:
> > > Implement
> > > suspend/resume for VSC drivers for hibernation"), which is on Sasha Levin's
> > > Hyper-V tree's hyperv-next branch [ ... snipped ...]
> > >
> > > I request this patch should go through Sasha's tree rather than the
> > > input subsystem's tree.
> > >
> > > Hi Jiri, Benjamin, can you please Ack?
> >
> > Hi Jiri, Benjamin,
> > Can you please take a look at the patch?
>
> Hi Dexuan,
>
> I am planning to process it once 5.4 merge window is over and thus hid.git
> is open again for 5.5 material.
Ah, now I see you asked for this go through hyperv tree. For that, feel
free to add
Acked-by: Jiri Kosina <jkosina@suse.cz>
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 4/4] PCI: hv: Change pci_protocol_version to per-hbus
From: Lorenzo Pieralisi @ 2019-09-26 16:28 UTC (permalink / raw)
To: Dexuan Cui
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, bhelgaas@google.com,
linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, Michael Kelley
In-Reply-To: <1568245086-70601-5-git-send-email-decui@microsoft.com>
On Wed, Sep 11, 2019 at 11:38:23PM +0000, Dexuan Cui wrote:
> A VM can have multiple hbus. It looks incorrect for the second hbus's
> hv_pci_protocol_negotiation() to set the global variable
> 'pci_protocol_version' (which was set by the first hbus), even if the
> same value is written.
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> drivers/pci/controller/pci-hyperv.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
This is a fix that seems unrelated to the rest of the series.
AFAICS the version also affects code paths in the driver, which
means that in case you have busses with different versions the
current code is wrong in this respect.
You have to capture this concept in the commit log, it reads as
an optional change but it looks like a potential bug.
Lorenzo
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 2655df2..55730c5 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -76,11 +76,6 @@ enum pci_protocol_version_t {
> PCI_PROTOCOL_VERSION_1_1,
> };
>
> -/*
> - * Protocol version negotiated by hv_pci_protocol_negotiation().
> - */
> -static enum pci_protocol_version_t pci_protocol_version;
> -
> #define PCI_CONFIG_MMIO_LENGTH 0x2000
> #define CFG_PAGE_OFFSET 0x1000
> #define CFG_PAGE_SIZE (PCI_CONFIG_MMIO_LENGTH - CFG_PAGE_OFFSET)
> @@ -429,6 +424,8 @@ enum hv_pcibus_state {
>
> struct hv_pcibus_device {
> struct pci_sysdata sysdata;
> + /* Protocol version negotiated with the host */
> + enum pci_protocol_version_t protocol_version;
> enum hv_pcibus_state state;
> refcount_t remove_lock;
> struct hv_device *hdev;
> @@ -942,7 +939,7 @@ static void hv_irq_unmask(struct irq_data *data)
> * negative effect (yet?).
> */
>
> - if (pci_protocol_version >= PCI_PROTOCOL_VERSION_1_2) {
> + if (hbus->protocol_version >= PCI_PROTOCOL_VERSION_1_2) {
> /*
> * PCI_PROTOCOL_VERSION_1_2 supports the VP_SET version of the
> * HVCALL_RETARGET_INTERRUPT hypercall, which also coincides
> @@ -1112,7 +1109,7 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> ctxt.pci_pkt.completion_func = hv_pci_compose_compl;
> ctxt.pci_pkt.compl_ctxt = ∁
>
> - switch (pci_protocol_version) {
> + switch (hbus->protocol_version) {
> case PCI_PROTOCOL_VERSION_1_1:
> size = hv_compose_msi_req_v1(&ctxt.int_pkts.v1,
> dest,
> @@ -2116,6 +2113,7 @@ static int hv_pci_protocol_negotiation(struct hv_device *hdev,
> enum pci_protocol_version_t version[],
> int num_version)
> {
> + struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
> struct pci_version_request *version_req;
> struct hv_pci_compl comp_pkt;
> struct pci_packet *pkt;
> @@ -2155,10 +2153,10 @@ static int hv_pci_protocol_negotiation(struct hv_device *hdev,
> }
>
> if (comp_pkt.completion_status >= 0) {
> - pci_protocol_version = version[i];
> + hbus->protocol_version = version[i];
> dev_info(&hdev->device,
> "PCI VMBus probing: Using version %#x\n",
> - pci_protocol_version);
> + hbus->protocol_version);
> goto exit;
> }
>
> @@ -2442,7 +2440,7 @@ static int hv_send_resources_allocated(struct hv_device *hdev)
> u32 wslot;
> int ret;
>
> - size_res = (pci_protocol_version < PCI_PROTOCOL_VERSION_1_2)
> + size_res = (hbus->protocol_version < PCI_PROTOCOL_VERSION_1_2)
> ? sizeof(*res_assigned) : sizeof(*res_assigned2);
>
> pkt = kmalloc(sizeof(*pkt) + size_res, GFP_KERNEL);
> @@ -2461,7 +2459,7 @@ static int hv_send_resources_allocated(struct hv_device *hdev)
> pkt->completion_func = hv_pci_generic_compl;
> pkt->compl_ctxt = &comp_pkt;
>
> - if (pci_protocol_version < PCI_PROTOCOL_VERSION_1_2) {
> + if (hbus->protocol_version < PCI_PROTOCOL_VERSION_1_2) {
> res_assigned =
> (struct pci_resources_assigned *)&pkt->message;
> res_assigned->message_type.type =
> @@ -2812,7 +2810,7 @@ static int hv_pci_resume(struct hv_device *hdev)
> return ret;
>
> /* Only use the version that was in use before hibernation. */
> - version[0] = pci_protocol_version;
> + version[0] = hbus->protocol_version;
> ret = hv_pci_protocol_negotiation(hdev, version, 1);
> if (ret)
> goto out;
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 2/4] PCI: hv: Add the support of hibernation
From: Lorenzo Pieralisi @ 2019-09-26 16:30 UTC (permalink / raw)
To: Dexuan Cui
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, bhelgaas@google.com,
linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, Michael Kelley
In-Reply-To: <1568245086-70601-3-git-send-email-decui@microsoft.com>
On Wed, Sep 11, 2019 at 11:38:20PM +0000, Dexuan Cui wrote:
> Implement the suspend/resume callbacks for hibernation.
>
> hv_pci_suspend() needs to prevent any new work from being queued: a later
> patch will address this issue.
I don't see why you have two separate patches, the second one fixing the
previous (this one). Squash them together and merge them as such, or
there is something I am missing here.
Lorenzo
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> drivers/pci/controller/pci-hyperv.c | 76 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 76 insertions(+)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 03fa039..3b77a3a 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -1398,6 +1398,23 @@ static void prepopulate_bars(struct hv_pcibus_device *hbus)
>
> spin_lock_irqsave(&hbus->device_list_lock, flags);
>
> + /*
> + * Clear the memory enable bit, in case it's already set. This occurs
> + * in the suspend path of hibernation, where the device is suspended,
> + * resumed and suspended again: see hibernation_snapshot() and
> + * hibernation_platform_enter().
> + *
> + * If the memory enable bit is already set, Hyper-V sliently ignores
> + * the below BAR updates, and the related PCI device driver can not
> + * work, because reading from the device register(s) always returns
> + * 0xFFFFFFFF.
> + */
> + list_for_each_entry(hpdev, &hbus->children, list_entry) {
> + _hv_pcifront_read_config(hpdev, PCI_COMMAND, 2, &command);
> + command &= ~PCI_COMMAND_MEMORY;
> + _hv_pcifront_write_config(hpdev, PCI_COMMAND, 2, command);
> + }
> +
> /* Pick addresses for the BARs. */
> do {
> list_for_each_entry(hpdev, &hbus->children, list_entry) {
> @@ -2737,6 +2754,63 @@ static int hv_pci_remove(struct hv_device *hdev)
> return ret;
> }
>
> +static int hv_pci_suspend(struct hv_device *hdev)
> +{
> + struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
> + int ret;
> +
> + /* XXX: Need to prevent any new work from being queued. */
> + flush_workqueue(hbus->wq);
> +
> + ret = hv_pci_bus_exit(hdev, true);
> + if (ret)
> + return ret;
> +
> + vmbus_close(hdev->channel);
> +
> + return 0;
> +}
> +
> +static int hv_pci_resume(struct hv_device *hdev)
> +{
> + struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
> + enum pci_protocol_version_t version[1];
> + int ret;
> +
> + hbus->state = hv_pcibus_init;
> +
> + ret = vmbus_open(hdev->channel, pci_ring_size, pci_ring_size, NULL, 0,
> + hv_pci_onchannelcallback, hbus);
> + if (ret)
> + return ret;
> +
> + /* Only use the version that was in use before hibernation. */
> + version[0] = pci_protocol_version;
> + ret = hv_pci_protocol_negotiation(hdev, version, 1);
> + if (ret)
> + goto out;
> +
> + ret = hv_pci_query_relations(hdev);
> + if (ret)
> + goto out;
> +
> + ret = hv_pci_enter_d0(hdev);
> + if (ret)
> + goto out;
> +
> + ret = hv_send_resources_allocated(hdev);
> + if (ret)
> + goto out;
> +
> + prepopulate_bars(hbus);
> +
> + hbus->state = hv_pcibus_installed;
> + return 0;
> +out:
> + vmbus_close(hdev->channel);
> + return ret;
> +}
> +
> static const struct hv_vmbus_device_id hv_pci_id_table[] = {
> /* PCI Pass-through Class ID */
> /* 44C4F61D-4444-4400-9D52-802E27EDE19F */
> @@ -2751,6 +2825,8 @@ static int hv_pci_remove(struct hv_device *hdev)
> .id_table = hv_pci_id_table,
> .probe = hv_pci_probe,
> .remove = hv_pci_remove,
> + .suspend = hv_pci_suspend,
> + .resume = hv_pci_resume,
> };
>
> static void __exit exit_hv_pci_drv(void)
> --
> 1.8.3.1
>
^ permalink raw reply
* RE: [PATCH] hv_balloon: Add the support of hibernation
From: Dexuan Cui @ 2019-09-26 17:42 UTC (permalink / raw)
To: David Hildenbrand, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, sashal@kernel.org,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
Michael Kelley
In-Reply-To: <2134429f-dc6d-06e0-9e4b-9028f62f6666@redhat.com>
> From: David Hildenbrand <david@redhat.com>
> Sent: Thursday, September 26, 2019 12:20 AM
> To: Dexuan Cui <decui@microsoft.com>; KY Srinivasan <kys@microsoft.com>;
> Haiyang Zhang <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; sashal@kernel.org;
> linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org; Michael Kelley
> <mikelley@microsoft.com>
> Subject: Re: [PATCH] hv_balloon: Add the support of hibernation
>
> On 25.09.19 22:03, Dexuan Cui wrote:
> >> From: linux-hyperv-owner@vger.kernel.org
> >> [... snipped ...]
> >>> Anyhow, just some comments from my side :) I can see how Windows
> Server
> >>> worked around that issue right now by just XOR'ing both features.
> >>>
> >>> David / dhildenb
> >>
> >> Thanks for sharing your thoughts!
> >>
> >> -- Dexuan
> >
> > Hi David,
> > If my explanation sounds good to you, can I have an Acked-by from you?
> >
>
> I do ACK the approach but not the patch in it's current state. I don't
> like the ifdefs - once you can get rid of the ifdefery - e.g., after the
> prerequisite is upstream - you can add my
>
> Acked-by: David Hildenbrand <david@redhat.com>
>
> David / dhildenb
Makes sense. I'll wait for the prerequisite patch (i.e. the patch that implements
hv_is_hibernation_supported(), https://lkml.org/lkml/2019/9/5/1160 ) to be
in upstream first, then I'll be able to get rid of the below "if 0" and post a v2
with your Acked-by. Thanks, David!
+#if 0
+ /*
+ * The patch to implement hv_is_hibernation_supported() is going
+ * through the tip tree. For now, let's hardcode allow_hibernation
+ * to false to keep the current behavior of hv_balloon. If people
+ * want to test hibernation, please blacklist hv_balloon fow now
+ * or do not enable Dynamid Memory and Memory Resizing.
+ *
+ * We'll remove the conditional compilation as soon as
+ * hv_is_hibernation_supported() is available in the mainline tree.
+ */
+ allow_hibernation = hv_is_hibernation_supported();
+#else
+ allow_hibernation = false;
Thanks,
-- Dexuan
^ permalink raw reply
* Re: [PATCH v3 02/26] PCI: hv: Use PCI_STD_NUM_BARS
From: Bjorn Helgaas @ 2019-09-26 22:05 UTC (permalink / raw)
To: Denis Efremov
Cc: linux-kernel, linux-pci, Andrew Murray, linux-hyperv,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin
In-Reply-To: <20190916204158.6889-3-efremov@linux.com>
On Mon, Sep 16, 2019 at 11:41:34PM +0300, Denis Efremov wrote:
> Replace the magic constant (6) with define PCI_STD_NUM_BARS representing
> the number of PCI BARs.
For some reason patches 0 and 1 didn't make it to the list. Can you
resend them?
^ permalink raw reply
* Re: [PATCH v2][PATCH net] hv_netvsc: Add the support of hibernation
From: kbuild test robot @ 2019-09-27 4:17 UTC (permalink / raw)
To: Dexuan Cui
Cc: kbuild-all, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, davem@davemloft.net,
linux-hyperv@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Michael Kelley, Dexuan Cui
In-Reply-To: <1569449034-29924-1-git-send-email-decui@microsoft.com>
[-- Attachment #1: Type: text/plain, Size: 2030 bytes --]
Hi Dexuan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net/master]
url: https://github.com/0day-ci/linux/commits/Dexuan-Cui/hv_netvsc-Add-the-support-of-hibernation/20190926-061258
config: x86_64-rhel-7.6 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
>> drivers/net/hyperv/netvsc_drv.c:2486:3: error: 'struct hv_driver' has no member named 'suspend'
.suspend = netvsc_suspend,
^~~~~~~
>> drivers/net/hyperv/netvsc_drv.c:2486:13: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
.suspend = netvsc_suspend,
^~~~~~~~~~~~~~
drivers/net/hyperv/netvsc_drv.c:2486:13: note: (near initialization for 'netvsc_drv.shutdown')
>> drivers/net/hyperv/netvsc_drv.c:2487:3: error: 'struct hv_driver' has no member named 'resume'; did you mean 'remove'?
.resume = netvsc_resume,
^~~~~~
remove
>> drivers/net/hyperv/netvsc_drv.c:2487:12: warning: excess elements in struct initializer
.resume = netvsc_resume,
^~~~~~~~~~~~~
drivers/net/hyperv/netvsc_drv.c:2487:12: note: (near initialization for 'netvsc_drv')
cc1: some warnings being treated as errors
vim +2486 drivers/net/hyperv/netvsc_drv.c
2479
2480 /* The one and only one */
2481 static struct hv_driver netvsc_drv = {
2482 .name = KBUILD_MODNAME,
2483 .id_table = id_table,
2484 .probe = netvsc_probe,
2485 .remove = netvsc_remove,
> 2486 .suspend = netvsc_suspend,
> 2487 .resume = netvsc_resume,
2488 .driver = {
2489 .probe_type = PROBE_FORCE_SYNCHRONOUS,
2490 },
2491 };
2492
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 48169 bytes --]
^ permalink raw reply
* Re: [PATCH v2] hv_sock: Add the support of hibernation
From: kbuild test robot @ 2019-09-27 4:18 UTC (permalink / raw)
To: Dexuan Cui
Cc: kbuild-all, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, davem@davemloft.net,
linux-hyperv@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Michael Kelley, Dexuan Cui
In-Reply-To: <1569447243-27433-1-git-send-email-decui@microsoft.com>
[-- Attachment #1: Type: text/plain, Size: 2189 bytes --]
Hi Dexuan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
[cannot apply to v5.3 next-20190925]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Dexuan-Cui/hv_sock-Add-the-support-of-hibernation/20190926-053950
config: x86_64-rhel-7.6 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
>> net//vmw_vsock/hyperv_transport.c:970:3: error: 'struct hv_driver' has no member named 'suspend'
.suspend = hvs_suspend,
^~~~~~~
>> net//vmw_vsock/hyperv_transport.c:970:13: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
.suspend = hvs_suspend,
^~~~~~~~~~~
net//vmw_vsock/hyperv_transport.c:970:13: note: (near initialization for 'hvs_drv.shutdown')
>> net//vmw_vsock/hyperv_transport.c:971:3: error: 'struct hv_driver' has no member named 'resume'; did you mean 'remove'?
.resume = hvs_resume,
^~~~~~
remove
>> net//vmw_vsock/hyperv_transport.c:971:13: warning: excess elements in struct initializer
.resume = hvs_resume,
^~~~~~~~~~
net//vmw_vsock/hyperv_transport.c:971:13: note: (near initialization for 'hvs_drv')
cc1: some warnings being treated as errors
vim +970 net//vmw_vsock/hyperv_transport.c
963
964 static struct hv_driver hvs_drv = {
965 .name = "hv_sock",
966 .hvsock = true,
967 .id_table = id_table,
968 .probe = hvs_probe,
969 .remove = hvs_remove,
> 970 .suspend = hvs_suspend,
> 971 .resume = hvs_resume,
972 };
973
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 48067 bytes --]
^ permalink raw reply
* RE: [PATCH v2][PATCH net] hv_netvsc: Add the support of hibernation
From: Dexuan Cui @ 2019-09-27 4:39 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all@01.org, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, sashal@kernel.org, davem@davemloft.net,
linux-hyperv@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Michael Kelley
In-Reply-To: <201909271207.245jsWr2%lkp@intel.com>
> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of kbuild test robot
> Sent: Thursday, September 26, 2019 9:18 PM
>
> Hi Dexuan,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on net/master]
>
> 'netvsc_drv.shutdown')
> >> drivers/net/hyperv/netvsc_drv.c:2487:3: error: 'struct hv_driver' has no
> member named 'resume'; did you mean 'remove'?
> .resume = netvsc_resume,
> ^~~~~~
This is a false alarm. Your code base needs to be merged with the latest
Linus's tree, which has the prerequisite patch:
271b2224d42f ("Drivers: hv: vmbus: Implement suspend/resume for VSC drivers for hibernation")
Thanks,
-- Dexuan
^ permalink raw reply
* RE: [PATCH v2] hv_sock: Add the support of hibernation
From: Dexuan Cui @ 2019-09-27 4:40 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all@01.org, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, sashal@kernel.org, davem@davemloft.net,
linux-hyperv@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Michael Kelley
In-Reply-To: <201909271231.iD9JBAQs%lkp@intel.com>
> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of kbuild test robot
> Sent: Thursday, September 26, 2019 9:19 PM
>
> Hi Dexuan,
>
> Thank you for the patch! Yet something to improve:
>
> >> net//vmw_vsock/hyperv_transport.c:970:3: error: 'struct hv_driver' has no
> member named 'suspend'
> .suspend = hvs_suspend,
> ^~~~~~~
This is a false alarm. Your code base needs to be merged with the latest
Linus's tree, which has the prerequisite patch:
271b2224d42f ("Drivers: hv: vmbus: Implement suspend/resume for VSC drivers for hibernation")
Thanks,
-- Dexuan
^ permalink raw reply
* RE: [PATCH net v2] vsock: Fix a lockdep warning in __vsock_release()
From: Dexuan Cui @ 2019-09-27 5:37 UTC (permalink / raw)
To: Stefano Garzarella
Cc: davem@davemloft.net, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, sashal@kernel.org, stefanha@redhat.com,
gregkh@linuxfoundation.org, arnd@arndb.de, deepa.kernel@gmail.com,
ytht.net@gmail.com, tglx@linutronix.de, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
Michael Kelley, jhansen@vmware.com
In-Reply-To: <20190926074749.sltehhkcgfduu7n2@steredhat.homenet.telecomitalia.it>
> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Stefano Garzarella
> Sent: Thursday, September 26, 2019 12:48 AM
>
> Hi Dexuan,
>
> On Thu, Sep 26, 2019 at 01:11:27AM +0000, Dexuan Cui wrote:
> > ...
> > NOTE: I only tested the code on Hyper-V. I can not test the code for
> > virtio socket, as I don't have a KVM host. :-( Sorry.
> >
> > @Stefan, @Stefano: please review & test the patch for virtio socket,
> > and let me know if the patch breaks anything. Thanks!
>
> Comment below, I'll test it ASAP!
Stefano, Thank you!
BTW, this is how I tested the patch:
1. write a socket server program in the guest. The program calls listen()
and then calls sleep(10000 seconds). Note: accept() is not called.
2. create some connections to the server program in the guest.
3. kill the server program by Ctrl+C, and "dmesg" will show the scary
call-trace, if the kernel is built with
CONFIG_LOCKDEP=y
CONFIG_LOCKDEP_SUPPORT=y
4. Apply the patch, do the same test and we should no longer see the call-trace.
> > - lock_sock(sk);
> > + /* When "level" is 2, use the nested version to avoid the
> > + * warning "possible recursive locking detected".
> > + */
> > + if (level == 1)
> > + lock_sock(sk);
>
> Since lock_sock() calls lock_sock_nested(sk, 0), could we use directly
> lock_sock_nested(sk, level) with level = 0 in vsock_release() and
> level = SINGLE_DEPTH_NESTING here in the while loop?
>
> Thanks,
> Stefano
IMHO it's better to make the lock usage more explicit, as the patch does.
lock_sock_nested(sk, level) or lock_sock_nested(sk, 0) seems a little
odd to me. But I'm open to your suggestion: if any of the network
maintainers, e.g. davem, also agrees with you, I'll change the code
as you suggested. :-)
Thanks,
-- Dexuan
^ permalink raw reply
* RE: [PATCH] HID: hyperv: Add the support of hibernation
From: Dexuan Cui @ 2019-09-27 5:42 UTC (permalink / raw)
To: Jiri Kosina
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, benjamin.tissoires@redhat.com,
linux-hyperv@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Michael Kelley
In-Reply-To: <nycvar.YFH.7.76.1909261522380.24354@cbobk.fhfr.pm>
> From: Jiri Kosina <jikos@kernel.org>
> Sent: Thursday, September 26, 2019 6:23 AM
> To: Dexuan Cui <decui@microsoft.com>
>
> On Thu, 26 Sep 2019, Jiri Kosina wrote:
>
> > > > This patch is basically a pure Hyper-V specific change and it has a
> > > > build dependency on the commit 271b2224d42f ("Drivers: hv: vmbus:
> > > > Implement
> > > > suspend/resume for VSC drivers for hibernation"), which is on Sasha
> Levin's
> > > > Hyper-V tree's hyperv-next branch [ ... snipped ...]
> > > >
> > > > I request this patch should go through Sasha's tree rather than the
> > > > input subsystem's tree.
> > > >
> > > > Hi Jiri, Benjamin, can you please Ack?
> > >
> > > Hi Jiri, Benjamin,
> > > Can you please take a look at the patch?
> >
> > Hi Dexuan,
> >
> > I am planning to process it once 5.4 merge window is over and thus hid.git
> > is open again for 5.5 material.
>
> Ah, now I see you asked for this go through hyperv tree. For that, feel
> free to add
> Acked-by: Jiri Kosina <jkosina@suse.cz>
> Jiri Kosina
Thanks for the Ack, Jiri!
I have a bunch of patches, including this one, to support Linux VM's hibernation
when the VM runs on Hyper-V. I just feel it would be better for all of them to
go through the Hyper-V tree. :-)
Thanks,
-- Dexuan
^ permalink raw reply
* RE: [PATCH v5 3/3] clocksource/drivers: Suspend/resume Hyper-V clocksource for hibernation
From: Dexuan Cui @ 2019-09-27 5:57 UTC (permalink / raw)
To: Daniel Lezcano, tglx@linutronix.de, arnd@arndb.de, bp@alien8.de,
Haiyang Zhang, hpa@zytor.com, KY Srinivasan,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
mingo@redhat.com, sashal@kernel.org, Stephen Hemminger,
x86@kernel.org, Michael Kelley, Sasha Levin
Cc: linux-arch@vger.kernel.org
In-Reply-To: <17e5a535-8597-4780-7cd0-e8c4d2aa8f0f@linaro.org>
> From: Daniel Lezcano <daniel.lezcano@linaro.org>
> Sent: Thursday, September 26, 2019 6:17 AM
> >>
> >> I can take this patch if needed.
> >
> > Thanks, Daniel! Usually tglx takes care of the patches, but it looks recently he
> > may be too busy to handle the 3 patches.
> >
> > I guess you can take the patch, if tglx has no objection. :-)
> > If you take the patch, please take all the 3 patches.
>
> I maintain drivers/clocksource for the tip/timers/core branch. I don't
> want to proxy another tip branch as it is out of my jurisdiction.
I see. Thanks for the explanation! I learned more about the tip tree. :-)
> So I can take patch 3/3 but will let the other 2 patches to be picked by
> the right person. It is your call.
Sounds good. Daniel, then please take this patch, e.g. patch 3/3.
Patch 2/3 may as well go through Sasha's hyper-v tree, since it's required by
other changes to the drivers hv_balloon, hv_utils and hv_vmbus.
I suppose tglx is the best person to take patch 1/3, but if he's too busy to
handle it, it can also go through the hyper-v tree, since the related other
patches have been in the mainline now.
Thanks,
-- Dexuan
^ permalink raw reply
* RE: [PATCH v5 1/3] x86/hyper-v: Suspend/resume the hypercall page for hibernation
From: Dexuan Cui @ 2019-09-27 6:48 UTC (permalink / raw)
To: vkuznets
Cc: linux-arch@vger.kernel.org, arnd@arndb.de, bp@alien8.de,
daniel.lezcano@linaro.org, Haiyang Zhang, hpa@zytor.com,
KY Srinivasan, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, mingo@redhat.com, sashal@kernel.org,
Stephen Hemminger, tglx@linutronix.de, x86@kernel.org,
Michael Kelley, Sasha Levin
In-Reply-To: <87ef0372wv.fsf@vitty.brq.redhat.com>
> From: Vitaly Kuznetsov <vkuznets@redhat.com>
> Sent: Thursday, September 26, 2019 3:44 AM
> > [...]
> > +static int hv_suspend(void)
> > +{
> > + union hv_x64_msr_hypercall_contents hypercall_msr;
> > +
> > + /* Reset the hypercall page */
> > + rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> > + hypercall_msr.enable = 0;
> > + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> > +
>
> (trying to think out loud, not sure there's a real issue):
>
> When PV IPIs (or PV TLB flush) are enabled we do the following checks:
>
> if (!hv_hypercall_pg)
> return false;
>
> or
> if (!hv_hypercall_pg)
> goto do_native;
>
> which will pass as we're not invalidating the pointer. Can we actually
> be sure that the kernel will never try to send an IPI/do TLB flush
> before we resume?
>
> Vitaly
When hv_suspend() and hv_resume() are called by syscore_suspend()
and syscore_resume(), respectively, all the non-boot CPUs are disabled and
only CPU0 is active and interrupts are disabled, e.g. see
hibernate() ->
hibernation_snapshot() ->
create_image() ->
suspend_disable_secondary_cpus()
local_irq_disable()
syscore_suspend()
swsusp_arch_suspend
syscore_resume
local_irq_enable
enable_nonboot_cpus
So, I'm pretty sure no IPI can happen between hv_suspend() and hv_resume().
self-IPI is not supposed to happen either, since interrupts are disabled.
IMO TLB flush should not be an issue either, unless the kernel changes page
tables between hv_suspend() and hv_resume(), which is not the case as I
checked the related code, but it looks in theory that might happen, say, in
the future, so if you insist we should save the variable "hv_hypercall_pg"
to a temporary variable and set the "hv_hypercall_pg" to NULL before we
disable the hypercall page, I would be happy to post a new version of this
patch, or we can keep this patch as is and I can make an extra patch.
Thanks,
-- Dexuan
^ permalink raw reply
* RE: [PATCH 2/4] PCI: hv: Add the support of hibernation
From: Dexuan Cui @ 2019-09-27 7:00 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, bhelgaas@google.com,
linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, Michael Kelley
In-Reply-To: <20190926163049.GB7827@e121166-lin.cambridge.arm.com>
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Sent: Thursday, September 26, 2019 9:31 AM
>
> On Wed, Sep 11, 2019 at 11:38:20PM +0000, Dexuan Cui wrote:
> > Implement the suspend/resume callbacks for hibernation.
> >
> > hv_pci_suspend() needs to prevent any new work from being queued: a later
> > patch will address this issue.
>
> I don't see why you have two separate patches, the second one fixing the
> previous (this one). Squash them together and merge them as such, or
> there is something I am missing here.
>
> Lorenzo
I was trying to make it easier to review the changes by spliting the changes
into 2 smaller patches. :-)
I'll merge patch 2/4 and patch 3/4 into one patch, and post a v2.
Thanks,
-- Dexuan
^ permalink raw reply
* RE: [PATCH 4/4] PCI: hv: Change pci_protocol_version to per-hbus
From: Dexuan Cui @ 2019-09-27 7:01 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, bhelgaas@google.com,
linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, Michael Kelley
In-Reply-To: <20190926162856.GA7827@e121166-lin.cambridge.arm.com>
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Sent: Thursday, September 26, 2019 9:29 AM
>
> On Wed, Sep 11, 2019 at 11:38:23PM +0000, Dexuan Cui wrote:
> > A VM can have multiple hbus. It looks incorrect for the second hbus's
> > hv_pci_protocol_negotiation() to set the global variable
> > 'pci_protocol_version' (which was set by the first hbus), even if the
> > same value is written.
> >
> > Signed-off-by: Dexuan Cui <decui@microsoft.com>
>
> This is a fix that seems unrelated to the rest of the series.
Correct.
> AFAICS the version also affects code paths in the driver, which
> means that in case you have busses with different versions the
> current code is wrong in this respect.
Correct.
> You have to capture this concept in the commit log, it reads as
> an optional change but it looks like a potential bug.
>
> Lorenzo
Agreed. Let me improve the commit log in v2.
Thanks,
-- Dexuan
^ permalink raw reply
* Re: [PATCH net v2] vsock: Fix a lockdep warning in __vsock_release()
From: Stefano Garzarella @ 2019-09-27 8:32 UTC (permalink / raw)
To: Dexuan Cui
Cc: davem@davemloft.net, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, sashal@kernel.org, stefanha@redhat.com,
gregkh@linuxfoundation.org, arnd@arndb.de, deepa.kernel@gmail.com,
ytht.net@gmail.com, tglx@linutronix.de, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
Michael Kelley, jhansen@vmware.com
In-Reply-To: <PU1P153MB01698C46C9348B9762D5E122BF810@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM>
On Fri, Sep 27, 2019 at 05:37:20AM +0000, Dexuan Cui wrote:
> > From: linux-hyperv-owner@vger.kernel.org
> > <linux-hyperv-owner@vger.kernel.org> On Behalf Of Stefano Garzarella
> > Sent: Thursday, September 26, 2019 12:48 AM
> >
> > Hi Dexuan,
> >
> > On Thu, Sep 26, 2019 at 01:11:27AM +0000, Dexuan Cui wrote:
> > > ...
> > > NOTE: I only tested the code on Hyper-V. I can not test the code for
> > > virtio socket, as I don't have a KVM host. :-( Sorry.
> > >
> > > @Stefan, @Stefano: please review & test the patch for virtio socket,
> > > and let me know if the patch breaks anything. Thanks!
> >
> > Comment below, I'll test it ASAP!
>
> Stefano, Thank you!
>
> BTW, this is how I tested the patch:
> 1. write a socket server program in the guest. The program calls listen()
> and then calls sleep(10000 seconds). Note: accept() is not called.
>
> 2. create some connections to the server program in the guest.
>
> 3. kill the server program by Ctrl+C, and "dmesg" will show the scary
> call-trace, if the kernel is built with
> CONFIG_LOCKDEP=y
> CONFIG_LOCKDEP_SUPPORT=y
>
> 4. Apply the patch, do the same test and we should no longer see the call-trace.
Thanks very useful! I'll follow these steps!
>
> > > - lock_sock(sk);
> > > + /* When "level" is 2, use the nested version to avoid the
> > > + * warning "possible recursive locking detected".
> > > + */
> > > + if (level == 1)
> > > + lock_sock(sk);
> >
> > Since lock_sock() calls lock_sock_nested(sk, 0), could we use directly
> > lock_sock_nested(sk, level) with level = 0 in vsock_release() and
> > level = SINGLE_DEPTH_NESTING here in the while loop?
> >
> > Thanks,
> > Stefano
>
> IMHO it's better to make the lock usage more explicit, as the patch does.
>
> lock_sock_nested(sk, level) or lock_sock_nested(sk, 0) seems a little
> odd to me. But I'm open to your suggestion: if any of the network
> maintainers, e.g. davem, also agrees with you, I'll change the code
> as you suggested. :-)
Sure!
Just to be clear, I'm proposing this (plus the changes in the transports
and yours useful comments):
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -638,7 +638,7 @@ struct sock *__vsock_create(struct net *net,
}
EXPORT_SYMBOL_GPL(__vsock_create);
-static void __vsock_release(struct sock *sk)
+static void __vsock_release(struct sock *sk, int level)
{
if (sk) {
struct sk_buff *skb;
@@ -650,7 +650,7 @@ static void __vsock_release(struct sock *sk)
transport->release(vsk);
- lock_sock(sk);
+ lock_sock_nested(sk, level);
sock_orphan(sk);
sk->sk_shutdown = SHUTDOWN_MASK;
@@ -659,7 +659,7 @@ static void __vsock_release(struct sock *sk)
/* Clean up any sockets that never were accepted. */
while ((pending = vsock_dequeue_accept(sk)) != NULL) {
- __vsock_release(pending);
+ __vsock_release(pending, SINGLE_DEPTH_NESTING);
sock_put(pending);
}
@@ -708,7 +708,7 @@ EXPORT_SYMBOL_GPL(vsock_stream_has_space);
static int vsock_release(struct socket *sock)
{
- __vsock_release(sock->sk);
+ __vsock_release(sock->sk, 0);
sock->sk = NULL;
sock->state = SS_FREE;
Thanks,
Stefano
^ permalink raw reply
* RE: [PATCH v5 1/3] x86/hyper-v: Suspend/resume the hypercall page for hibernation
From: Vitaly Kuznetsov @ 2019-09-27 9:05 UTC (permalink / raw)
To: Dexuan Cui
Cc: linux-arch@vger.kernel.org, arnd@arndb.de, bp@alien8.de,
daniel.lezcano@linaro.org, Haiyang Zhang, hpa@zytor.com,
KY Srinivasan, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, mingo@redhat.com, sashal@kernel.org,
Stephen Hemminger, tglx@linutronix.de, x86@kernel.org,
Michael Kelley, Sasha Levin
In-Reply-To: <PU1P153MB01695A159E9843B4E1EC13AEBF810@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM>
Dexuan Cui <decui@microsoft.com> writes:
>> From: Vitaly Kuznetsov <vkuznets@redhat.com>
>> Sent: Thursday, September 26, 2019 3:44 AM
>> > [...]
>> > +static int hv_suspend(void)
>> > +{
>> > + union hv_x64_msr_hypercall_contents hypercall_msr;
>> > +
>> > + /* Reset the hypercall page */
>> > + rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>> > + hypercall_msr.enable = 0;
>> > + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>> > +
>>
>> (trying to think out loud, not sure there's a real issue):
>>
>> When PV IPIs (or PV TLB flush) are enabled we do the following checks:
>>
>> if (!hv_hypercall_pg)
>> return false;
>>
>> or
>> if (!hv_hypercall_pg)
>> goto do_native;
>>
>> which will pass as we're not invalidating the pointer. Can we actually
>> be sure that the kernel will never try to send an IPI/do TLB flush
>> before we resume?
>>
>> Vitaly
>
> When hv_suspend() and hv_resume() are called by syscore_suspend()
> and syscore_resume(), respectively, all the non-boot CPUs are disabled and
> only CPU0 is active and interrupts are disabled, e.g. see
>
> hibernate() ->
> hibernation_snapshot() ->
> create_image() ->
> suspend_disable_secondary_cpus()
> local_irq_disable()
>
> syscore_suspend()
> swsusp_arch_suspend
> syscore_resume
>
> local_irq_enable
> enable_nonboot_cpus
>
>
> So, I'm pretty sure no IPI can happen between hv_suspend() and hv_resume().
> self-IPI is not supposed to happen either, since interrupts are disabled.
>
> IMO TLB flush should not be an issue either, unless the kernel changes page
> tables between hv_suspend() and hv_resume(), which is not the case as I
> checked the related code, but it looks in theory that might happen, say, in
> the future, so if you insist we should save the variable "hv_hypercall_pg"
> to a temporary variable and set the "hv_hypercall_pg" to NULL before we
> disable the hypercall page
Let's do it as a future proof so we can keep relying on !hv_hypercall_pg
everywhere we need. No need to change this patch IMO, a follow-up would
do.
--
Vitaly
^ permalink raw reply
* [RFC PATCH 00/13] vsock: add multi-transports support
From: Stefano Garzarella @ 2019-09-27 11:26 UTC (permalink / raw)
To: netdev
Cc: linux-hyperv, K. Y. Srinivasan, Stefan Hajnoczi, Sasha Levin,
linux-kernel, kvm, David S. Miller, virtualization,
Stephen Hemminger, Jason Wang, Michael S. Tsirkin, Haiyang Zhang,
Dexuan Cui, Jorgen Hansen
Hi all,
this series adds the multi-transports support to vsock, following
this proposal:
https://www.spinics.net/lists/netdev/msg575792.html
With the multi-transports support, we can use vsock with nested VMs
(using also different hypervisors) loading both guest->host and
host->guest transports at the same time.
Before this series, vmci-transport supported this behavior but only
using VMware hypervisor on L0, L1, etc.
The first 8 patches are cleanups and preparations, maybe some of
these can go regardless of this series.
Patch 9 changes the hvs_remote_addr_init(). setting the
VMADDR_CID_HOST as remote CID instead of VMADDR_CID_ANY to make
the choice of transport to be used work properly.
@Dexuan Could this change break anything?
Patch 10 adds multi-transports support.
RFC:
- I'd like to move MODULE_ALIAS_NETPROTO(PF_VSOCK) to af_vsock.c.
@Jorgen could this break the VMware products?
- DGRAM sockets are handled as before, I don't know if make sense
work on it now, or when another transport will support DGRAM.
The big issues here is that we cannot link 1-1 a socket to
transport as for stream sockets since DGRAM is not
connection-oriented.
Patches 11 and 12 maybe can be merged with patch 10.
Patch 11 maybe is tricky, but it allows to have vmci_transport and
vhost_vsock loaded at the same time and it also alleviates the
problem of having MODULE_ALIAS_NETPROTO(PF_VSOCK) in vmci_transport.c
Patch 12 prevents the transport modules unloading while sockets are
assigned to them.
Patch 13 fixes an issue in the bind() logic discoverable only with
the new multi-transport support.
I've tested this series with nested KVM (vsock-transport [L0,L1],
virtio-transport[L1,L2]) and with VMware (L0) + KVM (L1)
(vmci-transport [L0,L1], vhost-transport [L1], virtio-transport[L2]).
@Dexuan please can you test on HyperV that I didn't break anything
even without nested VMs?
I'll try to setup a Windows host where to test the nested VMs.
Thanks in advance for your comments and suggestions,
Stefano
Stefano Garzarella (13):
vsock/vmci: remove unused VSOCK_DEFAULT_CONNECT_TIMEOUT
vsock: remove vm_sockets_get_local_cid()
vsock: remove include/linux/vm_sockets.h file
vsock: add 'transport' member in the struct vsock_sock
vsock/virtio: add transport parameter to the
virtio_transport_reset_no_sock()
vsock: add 'struct vsock_sock *' param to vsock_core_get_transport()
vsock: handle buffer_size sockopts in the core
vsock: move vsock_insert_unbound() in the vsock_create()
hv_sock: set VMADDR_CID_HOST in the hvs_remote_addr_init()
vsock: add multi-transports support
vsock: add 'transport_hg' to handle g2h\h2g transports
vsock: prevent transport modules unloading
vsock: fix bind() behaviour taking care of CID
drivers/vhost/vsock.c | 96 +++---
include/linux/virtio_vsock.h | 18 +-
include/linux/vm_sockets.h | 15 -
include/net/af_vsock.h | 35 ++-
include/net/vsock_addr.h | 2 +-
net/vmw_vsock/af_vsock.c | 374 ++++++++++++++++++------
net/vmw_vsock/hyperv_transport.c | 68 ++---
net/vmw_vsock/virtio_transport.c | 177 ++++++-----
net/vmw_vsock/virtio_transport_common.c | 127 +++-----
net/vmw_vsock/vmci_transport.c | 123 +++-----
net/vmw_vsock/vmci_transport.h | 3 -
net/vmw_vsock/vmci_transport_notify.h | 1 -
12 files changed, 555 insertions(+), 484 deletions(-)
delete mode 100644 include/linux/vm_sockets.h
--
2.21.0
^ permalink raw reply
* [RFC PATCH 02/13] vsock: remove vm_sockets_get_local_cid()
From: Stefano Garzarella @ 2019-09-27 11:26 UTC (permalink / raw)
To: netdev
Cc: linux-hyperv, K. Y. Srinivasan, Stefan Hajnoczi, Sasha Levin,
linux-kernel, kvm, David S. Miller, virtualization,
Stephen Hemminger, Jason Wang, Michael S. Tsirkin, Haiyang Zhang,
Dexuan Cui, Jorgen Hansen
In-Reply-To: <20190927112703.17745-1-sgarzare@redhat.com>
vm_sockets_get_local_cid() is only used in virtio_transport_common.c.
We can replace it calling the virtio_transport_get_ops() and
using the get_local_cid() callback registered by the transport.
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
include/linux/vm_sockets.h | 2 --
net/vmw_vsock/af_vsock.c | 10 ----------
net/vmw_vsock/virtio_transport_common.c | 2 +-
3 files changed, 1 insertion(+), 13 deletions(-)
diff --git a/include/linux/vm_sockets.h b/include/linux/vm_sockets.h
index 33f1a2ecd905..7dd899ccb920 100644
--- a/include/linux/vm_sockets.h
+++ b/include/linux/vm_sockets.h
@@ -10,6 +10,4 @@
#include <uapi/linux/vm_sockets.h>
-int vm_sockets_get_local_cid(void);
-
#endif /* _VM_SOCKETS_H */
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index ab47bf3ab66e..f609434b2794 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -129,16 +129,6 @@ static struct proto vsock_proto = {
static const struct vsock_transport *transport;
static DEFINE_MUTEX(vsock_register_mutex);
-/**** EXPORTS ****/
-
-/* Get the ID of the local context. This is transport dependent. */
-
-int vm_sockets_get_local_cid(void)
-{
- return transport->get_local_cid();
-}
-EXPORT_SYMBOL_GPL(vm_sockets_get_local_cid);
-
/**** UTILS ****/
/* Each bound VSocket is stored in the bind hash table and each connected
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 5bb70c692b1e..ed1ad5289164 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -168,7 +168,7 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
struct virtio_vsock_pkt *pkt;
u32 pkt_len = info->pkt_len;
- src_cid = vm_sockets_get_local_cid();
+ src_cid = virtio_transport_get_ops()->transport.get_local_cid();
src_port = vsk->local_addr.svm_port;
if (!info->remote_cid) {
dst_cid = vsk->remote_addr.svm_cid;
--
2.21.0
^ permalink raw reply related
* [RFC PATCH 03/13] vsock: remove include/linux/vm_sockets.h file
From: Stefano Garzarella @ 2019-09-27 11:26 UTC (permalink / raw)
To: netdev
Cc: linux-hyperv, K. Y. Srinivasan, Stefan Hajnoczi, Sasha Levin,
linux-kernel, kvm, David S. Miller, virtualization,
Stephen Hemminger, Jason Wang, Michael S. Tsirkin, Haiyang Zhang,
Dexuan Cui, Jorgen Hansen
In-Reply-To: <20190927112703.17745-1-sgarzare@redhat.com>
This header file now only includes the "uapi/linux/vm_sockets.h".
We can include directly it when needed.
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
include/linux/vm_sockets.h | 13 -------------
include/net/af_vsock.h | 2 +-
include/net/vsock_addr.h | 2 +-
net/vmw_vsock/vmci_transport_notify.h | 1 -
4 files changed, 2 insertions(+), 16 deletions(-)
delete mode 100644 include/linux/vm_sockets.h
diff --git a/include/linux/vm_sockets.h b/include/linux/vm_sockets.h
deleted file mode 100644
index 7dd899ccb920..000000000000
--- a/include/linux/vm_sockets.h
+++ /dev/null
@@ -1,13 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * VMware vSockets Driver
- *
- * Copyright (C) 2007-2013 VMware, Inc. All rights reserved.
- */
-
-#ifndef _VM_SOCKETS_H
-#define _VM_SOCKETS_H
-
-#include <uapi/linux/vm_sockets.h>
-
-#endif /* _VM_SOCKETS_H */
diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
index 80ea0f93d3f7..c660402b10f2 100644
--- a/include/net/af_vsock.h
+++ b/include/net/af_vsock.h
@@ -10,7 +10,7 @@
#include <linux/kernel.h>
#include <linux/workqueue.h>
-#include <linux/vm_sockets.h>
+#include <uapi/linux/vm_sockets.h>
#include "vsock_addr.h"
diff --git a/include/net/vsock_addr.h b/include/net/vsock_addr.h
index 57d2db5c4bdf..cf8cc140d68d 100644
--- a/include/net/vsock_addr.h
+++ b/include/net/vsock_addr.h
@@ -8,7 +8,7 @@
#ifndef _VSOCK_ADDR_H_
#define _VSOCK_ADDR_H_
-#include <linux/vm_sockets.h>
+#include <uapi/linux/vm_sockets.h>
void vsock_addr_init(struct sockaddr_vm *addr, u32 cid, u32 port);
int vsock_addr_validate(const struct sockaddr_vm *addr);
diff --git a/net/vmw_vsock/vmci_transport_notify.h b/net/vmw_vsock/vmci_transport_notify.h
index 7843f08d4290..a1aa5a998c0e 100644
--- a/net/vmw_vsock/vmci_transport_notify.h
+++ b/net/vmw_vsock/vmci_transport_notify.h
@@ -11,7 +11,6 @@
#include <linux/types.h>
#include <linux/vmw_vmci_defs.h>
#include <linux/vmw_vmci_api.h>
-#include <linux/vm_sockets.h>
#include "vmci_transport.h"
--
2.21.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox