[PATCH 0/4] Enhance pci-hyperv to support hibernation

[PATCH 0/4] Enhance pci-hyperv to support hibernation

[Dexuan Cui]

This patchset 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:
https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/log/?h=hyperv-next

I request this patch should go through Sasha's tree rather than the
pci tree.

Dexuan Cui (4):
  PCI: hv: Reorganize the code in preparation of hibernation
  PCI: hv: Add the support of hibernation
  PCI: hv: Do not queue new work items on hibernation
  PCI: hv: Change pci_protocol_version to per-hbus

 drivers/pci/controller/pci-hyperv.c | 166 ++++++++++++++++++++++++++++++------
 1 file changed, 140 insertions(+), 26 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] PCI: hv: Reorganize the code in preparation of hibernation

[Dexuan Cui]

There is no functional change. This is just preparatory to a later
patch which adds the hibernation support for the pci-hyperv driver.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/pci/controller/pci-hyperv.c | 43 ++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 40b6254..03fa039 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -2080,7 +2080,9 @@ static void hv_pci_onchannelcallback(void *context)
  * failing if the host doesn't support the necessary protocol
  * level.
  */
-static int hv_pci_protocol_negotiation(struct hv_device *hdev)
+static int hv_pci_protocol_negotiation(struct hv_device *hdev,
+				       enum pci_protocol_version_t version[],
+				       int num_version)
 {
 	struct pci_version_request *version_req;
 	struct hv_pci_compl comp_pkt;
@@ -2104,8 +2106,8 @@ static int hv_pci_protocol_negotiation(struct hv_device *hdev)
 	version_req = (struct pci_version_request *)&pkt->message;
 	version_req->message_type.type = PCI_QUERY_PROTOCOL_VERSION;
 
-	for (i = 0; i < ARRAY_SIZE(pci_protocol_versions); i++) {
-		version_req->protocol_version = pci_protocol_versions[i];
+	for (i = 0; i < num_version; i++) {
+		version_req->protocol_version = version[i];
 		ret = vmbus_sendpacket(hdev->channel, version_req,
 				sizeof(struct pci_version_request),
 				(unsigned long)pkt, VM_PKT_DATA_INBAND,
@@ -2121,7 +2123,7 @@ static int hv_pci_protocol_negotiation(struct hv_device *hdev)
 		}
 
 		if (comp_pkt.completion_status >= 0) {
-			pci_protocol_version = pci_protocol_versions[i];
+			pci_protocol_version = version[i];
 			dev_info(&hdev->device,
 				"PCI VMBus probing: Using version %#x\n",
 				pci_protocol_version);
@@ -2572,7 +2574,8 @@ static int hv_pci_probe(struct hv_device *hdev,
 
 	hv_set_drvdata(hdev, hbus);
 
-	ret = hv_pci_protocol_negotiation(hdev);
+	ret = hv_pci_protocol_negotiation(hdev, pci_protocol_versions,
+					  ARRAY_SIZE(pci_protocol_versions));
 	if (ret)
 		goto close;
 
@@ -2644,7 +2647,7 @@ static int hv_pci_probe(struct hv_device *hdev,
 	return ret;
 }
 
-static void hv_pci_bus_exit(struct hv_device *hdev)
+static int hv_pci_bus_exit(struct hv_device *hdev, bool hibernating)
 {
 	struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
 	struct {
@@ -2660,16 +2663,20 @@ static void hv_pci_bus_exit(struct hv_device *hdev)
 	 * access the per-channel ringbuffer any longer.
 	 */
 	if (hdev->channel->rescind)
-		return;
+		return 0;
 
-	/* Delete any children which might still exist. */
-	memset(&relations, 0, sizeof(relations));
-	hv_pci_devices_present(hbus, &relations);
+	if (!hibernating) {
+		/* Delete any children which might still exist. */
+		memset(&relations, 0, sizeof(relations));
+		hv_pci_devices_present(hbus, &relations);
+	}
 
 	ret = hv_send_resources_released(hdev);
-	if (ret)
+	if (ret) {
 		dev_err(&hdev->device,
 			"Couldn't send resources released packet(s)\n");
+		return ret;
+	}
 
 	memset(&pkt.teardown_packet, 0, sizeof(pkt.teardown_packet));
 	init_completion(&comp_pkt.host_event);
@@ -2682,8 +2689,13 @@ static void hv_pci_bus_exit(struct hv_device *hdev)
 			       (unsigned long)&pkt.teardown_packet,
 			       VM_PKT_DATA_INBAND,
 			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
-	if (!ret)
-		wait_for_completion_timeout(&comp_pkt.host_event, 10 * HZ);
+	if (ret)
+		return ret;
+
+	if (wait_for_completion_timeout(&comp_pkt.host_event, 10 * HZ) == 0)
+		return -ETIMEDOUT;
+
+	return 0;
 }
 
 /**
@@ -2695,6 +2707,7 @@ static void hv_pci_bus_exit(struct hv_device *hdev)
 static int hv_pci_remove(struct hv_device *hdev)
 {
 	struct hv_pcibus_device *hbus;
+	int ret;
 
 	hbus = hv_get_drvdata(hdev);
 	if (hbus->state == hv_pcibus_installed) {
@@ -2707,7 +2720,7 @@ static int hv_pci_remove(struct hv_device *hdev)
 		hbus->state = hv_pcibus_removed;
 	}
 
-	hv_pci_bus_exit(hdev);
+	ret = hv_pci_bus_exit(hdev, false);
 
 	vmbus_close(hdev->channel);
 
@@ -2721,7 +2734,7 @@ static int hv_pci_remove(struct hv_device *hdev)
 	wait_for_completion(&hbus->remove_event);
 	destroy_workqueue(hbus->wq);
 	free_page((unsigned long)hbus);
-	return 0;
+	return ret;
 }
 
 static const struct hv_vmbus_device_id hv_pci_id_table[] = {
-- 
1.8.3.1

[PATCH 2/4] PCI: hv: Add the support of hibernation

[Dexuan Cui]

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.

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

[PATCH 3/4] PCI: hv: Do not queue new work items on hibernation

[Dexuan Cui]

We must make sure there is no pending work items before we call
vmbus_close().

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/pci/controller/pci-hyperv.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 3b77a3a..2655df2 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -422,6 +422,7 @@ enum hv_pcibus_state {
 	hv_pcibus_init = 0,
 	hv_pcibus_probed,
 	hv_pcibus_installed,
+	hv_pcibus_removing,
 	hv_pcibus_removed,
 	hv_pcibus_maximum
 };
@@ -1841,6 +1842,12 @@ static void hv_pci_devices_present(struct hv_pcibus_device *hbus,
 	unsigned long flags;
 	bool pending_dr;
 
+	if (hbus->state == hv_pcibus_removing) {
+		dev_info(&hbus->hdev->device,
+			 "PCI VMBus BUS_RELATIONS: ignored\n");
+		return;
+	}
+
 	dr_wrk = kzalloc(sizeof(*dr_wrk), GFP_NOWAIT);
 	if (!dr_wrk)
 		return;
@@ -1957,11 +1964,19 @@ static void hv_eject_device_work(struct work_struct *work)
  */
 static void hv_pci_eject_device(struct hv_pci_dev *hpdev)
 {
+	struct hv_pcibus_device *hbus = hpdev->hbus;
+	struct hv_device *hdev = hbus->hdev;
+
+	if (hbus->state == hv_pcibus_removing) {
+		dev_info(&hdev->device, "PCI VMBus EJECT: ignored\n");
+		return;
+	}
+
 	hpdev->state = hv_pcichild_ejecting;
 	get_pcichild(hpdev);
 	INIT_WORK(&hpdev->wrk, hv_eject_device_work);
-	get_hvpcibus(hpdev->hbus);
-	queue_work(hpdev->hbus->wq, &hpdev->wrk);
+	get_hvpcibus(hbus);
+	queue_work(hbus->wq, &hpdev->wrk);
 }
 
 /**
@@ -2757,9 +2772,21 @@ static int hv_pci_remove(struct hv_device *hdev)
 static int hv_pci_suspend(struct hv_device *hdev)
 {
 	struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
+	enum hv_pcibus_state old_state;
 	int ret;
 
-	/* XXX: Need to prevent any new work from being queued. */
+	tasklet_disable(&hdev->channel->callback_event);
+
+	/* Change the hbus state to prevent new work items. */
+	old_state = hbus->state;
+	if (hbus->state == hv_pcibus_installed)
+		hbus->state = hv_pcibus_removing;
+
+	tasklet_enable(&hdev->channel->callback_event);
+
+	if (old_state != hv_pcibus_installed)
+		return -EINVAL;
+
 	flush_workqueue(hbus->wq);
 
 	ret = hv_pci_bus_exit(hdev, true);
-- 
1.8.3.1

[PATCH 4/4] PCI: hv: Change pci_protocol_version to per-hbus

[Dexuan Cui]

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(-)

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 = &comp;
 
-	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

RE: [PATCH 0/4] Enhance pci-hyperv to support hibernation

[Dexuan Cui]

> From: Dexuan Cui <decui@microsoft.com>
> Sent: Wednesday, September 11, 2019 4:38 PM
> To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; sashal@kernel.org; lorenzo.pieralisi@arm.com;
> bhelgaas@google.com; linux-hyperv@vger.kernel.org;
> linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; Michael Kelley
> <mikelley@microsoft.com>
> Cc: Dexuan Cui <decui@microsoft.com>
> Subject: [PATCH 0/4] Enhance pci-hyperv to support hibernation
> 
> This patchset 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
> pci tree.
> 
> Dexuan Cui (4):
>   PCI: hv: Reorganize the code in preparation of hibernation
>   PCI: hv: Add the support of hibernation
>   PCI: hv: Do not queue new work items on hibernation
>   PCI: hv: Change pci_protocol_version to per-hbus
> 
>  drivers/pci/controller/pci-hyperv.c | 166
> ++++++++++++++++++++++++++++++------
>  1 file changed, 140 insertions(+), 26 deletions(-)

Hi Lorenzo, Bjorn, and all,

Can you please take a look at the patchset (4 patches in total)?

Thanks,
-- Dexuan

Re: [PATCH 4/4] PCI: hv: Change pci_protocol_version to per-hbus

[Lorenzo Pieralisi]

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 = &comp;
>  
> -	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
> 

Re: [PATCH 2/4] PCI: hv: Add the support of hibernation

[Lorenzo Pieralisi]

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
> 

RE: [PATCH 2/4] PCI: hv: Add the support of hibernation

[Dexuan Cui]

> 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

RE: [PATCH 4/4] PCI: hv: Change pci_protocol_version to per-hbus

[Dexuan Cui]

> 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