PCI: hv: Fix hibernation in case interrupts are not re-created

PATCH Superseded Dexuan Cui 2 participants 6 messages Sep 8, 2020 – Oct 1, 2020

Subsystems (4)

PCI PCI native host bridge and endpoint drivers Hyper-V/Azure core and drivers Microsoft MANA RDMA driver

lore

Share & tools

lore

Dexuan Cui

author maintainer

2020-09-08 23:19 UTC #0 lore raw

PATCH v2 1/1 PCI: hv: Fix hibernation in case interrupts are not re-created

Hyper-V doesn't trap and emulate the accesses to the MSI/MSI-X registers, and we must use hv_compose_msi_msg() to ask Hyper-V to create the IOMMU Interrupt Remapping Table Entries. This is not an issue for a lot of PCI device drivers (e.g. NVMe driver, Mellanox NIC drivers), which destroy and re-create the interrupts across hibernation, so hv_compose_msi_msg() is called automatically. However, some other PCI device drivers (e.g. the Nvidia driver) may not destroy and re-create the interrupts across hibernation, so hv_pci_resume() has to call hv_compose_msi_msg(), otherwise the PCI device drivers can no longer receive MSI/MSI-X interrupts after hibernation.

Fixes ac82fc832708 ("PCI: hv: Add hibernation support")

Signed-off-by Dexuan Cui <[email protected]>

Reviewed-by Jake Oshins <[email protected]>

---

Changes in v2:
Fixed a typo in the comment in hv_irq_unmask. Thanks to Michael! Added Jake's Reviewed-by.

 drivers/pci/controller/pci-hyperv.c | 44 +++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

drivers/pci/controller/pci-hyperv.c +44 -0

index fc4c3a15e570..dd21afb5d62b 100644
@@ -1211,6 +1211,21 @@ static void hv_irq_unmask(struct irq_data *data)
 	pbus = pdev->bus;
 	hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata);
 
+	if (hbus->state == hv_pcibus_removing) {
+		/*
+		 * During hibernation, when a CPU is offlined, the kernel tries
+		 * to move the interrupt to the remaining CPUs that haven't
+		 * been offlined yet. In this case, the below hv_do_hypercall()
+		 * always fails since the vmbus channel has been closed, so we
+		 * should not call the hypercall, but we still need
+		 * pci_msi_unmask_irq() to reset the mask bit in desc->masked:
+		 * see cpu_disable_common() -> fixup_irqs() ->
+		 * irq_migrate_all_off_this_cpu() -> migrate_one_irq().
+		 */
+		pci_msi_unmask_irq(data);
+		return;
+	}
+
 	spin_lock_irqsave(&hbus->retarget_msi_interrupt_lock, flags);
 
 	params = &hbus->retarget_msi_interrupt_params;

@@ -3372,6 +3387,33 @@ static int hv_pci_suspend(struct hv_device *hdev)
 	return 0;
 }
 
+static int hv_pci_restore_msi_msg(struct pci_dev *pdev, void *arg)
+{
+	struct msi_desc *entry;
+	struct irq_data *irq_data;
+
+	for_each_pci_msi_entry(entry, pdev) {
+		irq_data = irq_get_irq_data(entry->irq);
+		if (WARN_ON_ONCE(!irq_data))
+			return -EINVAL;
+
+		hv_compose_msi_msg(irq_data, &entry->msg);
+	}
+
+	return 0;
+}
+
+/*
+ * Upon resume, pci_restore_msi_state() -> ... ->  __pci_write_msi_msg()
+ * re-writes the MSI/MSI-X registers, but since Hyper-V doesn't trap and
+ * emulate the accesses, we have to call hv_compose_msi_msg() to ask
+ * Hyper-V to re-create the IOMMU Interrupt Remapping Table Entries.
+ */
+static void hv_pci_restore_msi_state(struct hv_pcibus_device *hbus)
+{
+	pci_walk_bus(hbus->pci_bus, hv_pci_restore_msi_msg, NULL);
+}
+
 static int hv_pci_resume(struct hv_device *hdev)
 {
 	struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);

@@ -3405,6 +3447,8 @@ static int hv_pci_resume(struct hv_device *hdev)
 
 	prepopulate_bars(hbus);
 
+	hv_pci_restore_msi_state(hbus);
+
 	hbus->state = hv_pcibus_installed;
 	return 0;
 out:

Signature · 1 line

2.19.1

Dexuan Cui

author maintainer

2020-09-21 17:02 UTC #1 lore raw

Quoted 18 lines

From: Dexuan Cui <[email protected]>
Sent: Tuesday, September 8, 2020 4:18 PM
Hyper-V doesn't trap and emulate the accesses to the MSI/MSI-X registers, and we must use hv_compose_msi_msg() to ask Hyper-V to create the IOMMU Interrupt Remapping Table Entries. This is not an issue for a lot of PCI device drivers (e.g. NVMe driver, Mellanox NIC drivers), which destroy and re-create the interrupts across hibernation, so hv_compose_msi_msg() is called automatically. However, some other PCI device drivers (e.g. the Nvidia driver) may not destroy and re-create the interrupts across hibernation, so hv_pci_resume() has to call hv_compose_msi_msg(), otherwise the PCI device drivers can no longer receive MSI/MSI-X interrupts after hibernation.
Fixes ac82fc832708 ("PCI: hv: Add hibernation support")
Signed-off-by Dexuan Cui <[email protected]>
Reviewed-by Jake Oshins <[email protected]>

Quoted 8 lines

---
Changes in v2:
Fixed a typo in the comment in hv_irq_unmask. Thanks to Michael! Added Jake's Reviewed-by.
 drivers/pci/controller/pci-hyperv.c | 44 +++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

Hi Lorenzo, Bjorn,
Can you please take a look at this patch?
I hope it still could have a chance to be in 5.9. :-)

Thanks,
-- Dexuan

Lorenzo Pieralisi

2020-09-28 10:43 UTC #2 lore raw

In reply to Dexuan Cui

[+MarcZ - this patch needs IRQ maintainers vetting]

Quoted 10 lines · Dexuan Cui

Hyper-V doesn't trap and emulate the accesses to the MSI/MSI-X registers, and we must use hv_compose_msi_msg() to ask Hyper-V to create the IOMMU Interrupt Remapping Table Entries. This is not an issue for a lot of PCI device drivers (e.g. NVMe driver, Mellanox NIC drivers), which destroy and re-create the interrupts across hibernation, so hv_compose_msi_msg() is called automatically. However, some other PCI device drivers (e.g. the Nvidia driver) may not destroy and re-create the interrupts across hibernation, so hv_pci_resume() has to call hv_compose_msi_msg(), otherwise the PCI device drivers can no longer receive MSI/MSI-X interrupts after hibernation.

This looks like drivers bugs and I don't think the HV controller driver is where you should fix them. Regardless, this commit log does not provide the information that it should.

Quoted 23 lines

Fixes ac82fc832708 ("PCI: hv: Add hibernation support")
Signed-off-by Dexuan Cui <[email protected]>
Reviewed-by Jake Oshins <[email protected]>
---
Changes in v2:
Fixed a typo in the comment in hv_irq_unmask. Thanks to Michael! Added Jake's Reviewed-by.
 drivers/pci/controller/pci-hyperv.c | 44 +++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
drivers/pci/controller/pci-hyperv.c +2 -0
index fc4c3a15e570..dd21afb5d62b 100644
@@ -1211,6 +1211,21 @@ static void hv_irq_unmask(struct irq_data *data)
 	pbus = pdev->bus;
 	hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata);
 
+	if (hbus->state == hv_pcibus_removing) {
+		/*

drivers/pci/controller/pci-hyperv.c static void hv_irq_unmask(struct irq_data *data) · line 1225

Quoted 10 lines

drivers/pci/controller/pci-hyperv.c +10 -0

+		 * During hibernation, when a CPU is offlined, the kernel tries
+		 * to move the interrupt to the remaining CPUs that haven't
+		 * been offlined yet. In this case, the below hv_do_hypercall()
+		 * always fails since the vmbus channel has been closed, so we
+		 * should not call the hypercall, but we still need
+		 * pci_msi_unmask_irq() to reset the mask bit in desc->masked:
+		 * see cpu_disable_common() -> fixup_irqs() ->
+		 * irq_migrate_all_off_this_cpu() -> migrate_one_irq().
+		 */
+		pci_msi_unmask_irq(data);

This is not appropriate - it looks like a plaster to paper over an issue with hyper-V hibernation code sequence. Fix that issue instead of papering over it here.

Thanks,
Lorenzo

Quoted 52 lines

drivers/pci/controller/pci-hyperv.c +32 -0

+		return;
+	}
+
 	spin_lock_irqsave(&hbus->retarget_msi_interrupt_lock, flags);
 
 	params = &hbus->retarget_msi_interrupt_params;

@@ -3372,6 +3387,33 @@ static int hv_pci_suspend(struct hv_device *hdev)
 	return 0;
 }
 
+static int hv_pci_restore_msi_msg(struct pci_dev *pdev, void *arg)
+{
+	struct msi_desc *entry;
+	struct irq_data *irq_data;
+
+	for_each_pci_msi_entry(entry, pdev) {
+		irq_data = irq_get_irq_data(entry->irq);
+		if (WARN_ON_ONCE(!irq_data))
+			return -EINVAL;
+
+		hv_compose_msi_msg(irq_data, &entry->msg);
+	}
+
+	return 0;
+}
+
+/*
+ * Upon resume, pci_restore_msi_state() -> ... ->  __pci_write_msi_msg()
+ * re-writes the MSI/MSI-X registers, but since Hyper-V doesn't trap and
+ * emulate the accesses, we have to call hv_compose_msi_msg() to ask
+ * Hyper-V to re-create the IOMMU Interrupt Remapping Table Entries.
+ */
+static void hv_pci_restore_msi_state(struct hv_pcibus_device *hbus)
+{
+	pci_walk_bus(hbus->pci_bus, hv_pci_restore_msi_msg, NULL);
+}
+
 static int hv_pci_resume(struct hv_device *hdev)
 {
 	struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);

@@ -3405,6 +3447,8 @@ static int hv_pci_resume(struct hv_device *hdev)
 
 	prepopulate_bars(hbus);
 
+	hv_pci_restore_msi_state(hbus);
+
 	hbus->state = hv_pcibus_installed;
 	return 0;
 out:

Signature · 1 line

2.19.1

Dexuan Cui

author maintainer

2020-09-30 00:38 UTC #3 lore raw

Quoted 4 lines

From: Lorenzo Pieralisi <[email protected]>
Sent: Monday, September 28, 2020 3:43 AM
[+MarcZ - this patch needs IRQ maintainers vetting]

Sure. Hi MarkZ, please also review the patch. Thanks!

Quoted 12 lines · Dexuan Cui

Quoted 10 lines
Hyper-V doesn't trap and emulate the accesses to the MSI/MSI-X registers, and we must use hv_compose_msi_msg() to ask Hyper-V to create the IOMMU Interrupt Remapping Table Entries. This is not an issue for a lot of PCI device drivers (e.g. NVMe driver, Mellanox NIC drivers), which destroy and re-create the interrupts across hibernation, so hv_compose_msi_msg() is called automatically. However, some other PCI device drivers (e.g. the Nvidia driver) may not destroy and re-create the interrupts across hibernation, so hv_pci_resume() has to call hv_compose_msi_msg(), otherwise the PCI device drivers can no longer receive MSI/MSI-X interrupts after hibernation.

Quoted 2 lines

This looks like drivers bugs and I don't think the HV controller driver is where you should fix them.

IMHO this is not a PCI device driver bug, because I think a PCI device driver is allowed to keep and re-use the MSI/MSI-X interrupts across hibernation, otherwise we would not have pci_restore_msi_state() in pci_restore_state().

The in-tree open-source Nvidia GPU driver drivers/gpu/drm/nouveau is such a PCI device driver that re-uses the MSI-X interrupts across hibernation. The out-of-tree proprietary Nvidia GPU driver also does the same thing. It looks some other in-tree PCI device drivers also do the same thing, though I don't remember their names offhand.

IMO it's much better to change the pci-hyperv driver once and for all, than to change every such existing (and future?) PCI device driver.

pci_restore_msi_state() directly writes the MSI/MSI-X related registers in __pci_write_msi_msg() and msix_mask_irq(). On a physical machine, this works perfectly, but for a Linux VM running on a hypervisor, which typically enables IOMMU interrupt remapping, the hypervisor usually should trap and emulate the write accesses to the MSI/MSI-X registers, so the hypervisor is able to create the necessary interrupt remapping table entries in the IOMMU, and the MSI/MSI-X interrupts can work in the VM. Hyper-V is different from other hypervisors in that it does not trap and emulate the write accesses, and instead it uses a para-virtualized method, which requires the VM to call hv_compose_msi_msg() to notify the hypervisor of the info that would be passed to the hypervisor in the case of the trap-and-emulate method.

I mean this is a Hyper-V specific problem, so IMO we should fix the pci-hyperv driver rather than change the PCI device drivers, which work perfectly on a physical machine and on other hypervisors. Also it can be difficult or impossible to ask the authors of the aforementioned PCI device drivers to destry and re-create MSI/MSI-X acorss hibernation, especially for the out-of-tree driver(s).

Quoted 2 lines

Regardless, this commit log does not provide the information that it should.

Hi Lozenzo, I'm happy to add more info. Can you please let me know what extra info I should provide?

Quoted 39 lines

Quoted 6 lines
Fixes ac82fc832708 ("PCI: hv: Add hibernation support")
Signed-off-by Dexuan Cui <[email protected]>
Reviewed-by Jake Oshins <[email protected]>
---
Review
Quoted 8 lines
Changes in v2:
Fixed a typo in the comment in hv_irq_unmask. Thanks to Michael! Added Jake's Reviewed-by.
 drivers/pci/controller/pci-hyperv.c | 44 +++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
(quoted diff) +0 -0
b/drivers/pci/controller/pci-hyperv.c
Quoted 9 lines
index fc4c3a15e570..dd21afb5d62b 100644
drivers/pci/controller/pci-hyperv.c +2 -0
@@ -1211,6 +1211,21 @@ static void hv_irq_unmask(struct irq_data *data)
 	pbus = pdev->bus;
 	hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata);
 
+	if (hbus->state == hv_pcibus_removing) {
+		/*
drivers/pci/controller/pci-hyperv.c static void hv_irq_unmask(struct irq_data *data) · line 1225
Quoted 10 lines
drivers/pci/controller/pci-hyperv.c +10 -0
+		 * During hibernation, when a CPU is offlined, the kernel tries
+		 * to move the interrupt to the remaining CPUs that haven't
+		 * been offlined yet. In this case, the below hv_do_hypercall()
+		 * always fails since the vmbus channel has been closed, so we
+		 * should not call the hypercall, but we still need
+		 * pci_msi_unmask_irq() to reset the mask bit in desc->masked:
+		 * see cpu_disable_common() -> fixup_irqs() ->
+		 * irq_migrate_all_off_this_cpu() -> migrate_one_irq().
+		 */
+		pci_msi_unmask_irqpci_msi_unmask_irq(data);
This is not appropriate - it looks like a plaster to paper over an issue with hyper-V hibernation code sequence. Fix that issue instead of papering over it here.

Quoted 2 lines

Thanks,
Lorenzo

IMO this patch is fixing this Hyper-V specific problem. :-)

The probem is unique to Hyper-V because chip->irq_unmask() may fail in a Linux VM running on Hyper-V.

chip->irq_unmask() can not fail on a physical machine. I guess this is why the return value of irq_unmask() is defined as "void" in include/linux/irq.h:
struct irq_chip {
...

        void            (*irq_mask)(struct irq_data *data);
        void            (*irq_unmask)(struct irq_data *data);

As I described in the comment, in a VM on Hyper-V, chip->irq_unmask() fails during the suspending phase of hibernation because it's called when the non-boot CPUs are being offlined, and at this time all the devices, including Hyper-V VMBus devices, have been "frozen" -- this is part of the standard Linux hibernation workflow. Since Hyper-V thinks the VM has frozen the pci-hyperv VMBus device at this moment (i.e. closed the VMBus channel of the VMBus device), it fails chip->irq_unmask(), i.e. hv_irq_unmask() -> hv_do_hypercall().

On a physical machine, unmasking an MSI/MSI-X register just means an MMIO write, which I think can not fail here.

So I think this patch is the correct fix, considering Hyper-V's unique implementation of the MSI "chip" (i.e. Hyper-V does not trap and emulate the MSI/MSI-X register accesses, and uses a para-virtualized method as I explained above), and the fact that I shouldn't and can't change the standard Linux hibernation workflow.

In hv_irq_unmask(), when I skip the hypercall in the case of hbus->state == hv_pcibus_removing, I still need the pci_msi_unmask_irq(), because of the sequences in kernel/irq/cpuhotplug.c:

static bool migrate_one_irq(struct irq_desc *desc) {
...

        if (maskchip && chip->irq_mask)
                chip->irq_mask(d);
...
        err = irq_do_set_affinity(d, affinity, false);
...
        if (maskchip && chip->irq_unmask)
                chip->irq_unmask(d);

Here if hv_irq_unmask does not call pci_msi_unmask_irq(), the desc->masked remains "true", so later after hibernation, the MSI interrupt line always reamins masked, which is incorrect.

Here the slient failure of hv_irq_unmask() does not matter since all the non-boot CPUs are being offlined (meaning all the devices have been frozen). Note: the correct affinity info is still updated into the irqdata data structure in migrate_one_irq() -> irq_do_set_affinity() -> hv_set_affinity(), so when the VM resumes, hv_pci_resume() -> hv_pci_restore_msi_state() is able to correctly restore the irqs with the correct affinity.

I hope the explanation can help clarify things. I understand this is not as natual as tht case that Linux runs on a physical machine, but due to the unique PCI pass-through implementation of Hyper-V, IMO this is the only viable fix for the problem here. BTW, this patch is only confined to the pci-hyperv driver and I believe it can no cause any regression.

Thanks,
-- Dexuan

Quoted 50 lines

drivers/pci/controller/pci-hyperv.c static int hv_pci_suspend(struct hv_device

Quoted 7 lines

drivers/pci/controller/pci-hyperv.c +3 -0

+		return;
+	}
+
 	spin_lock_irqsave(&hbus->retarget_msi_interrupt_lock, flags);
 
 	params = &hbus->retarget_msi_interrupt_params;

@@ -3372,6 +3387,33 @@ static int hv_pci_suspend(struct hv_device

*hdev)

Quoted 42 lines

drivers/pci/controller/pci-hyperv.c +29 -0

 	return 0;
 }
 
+static int hv_pci_restore_msi_msg(struct pci_dev *pdev, void *arg)
+{
+	struct msi_desc *entry;
+	struct irq_data *irq_data;
+
+	for_each_pci_msi_entry(entry, pdev) {
+		irq_data = irq_get_irq_data(entry->irq);
+		if (WARN_ON_ONCE(!irq_data))
+			return -EINVAL;
+
+		hv_compose_msi_msg(irq_data, &entry->msg);
+	}
+
+	return 0;
+}
+
+/*
+ * Upon resume, pci_restore_msi_state() -> ... ->  __pci_write_msi_msg()
+ * re-writes the MSI/MSI-X registers, but since Hyper-V doesn't trap and
+ * emulate the accesses, we have to call hv_compose_msi_msg() to ask
+ * Hyper-V to re-create the IOMMU Interrupt Remapping Table Entries.
+ */
+static void hv_pci_restore_msi_state(struct hv_pcibus_device *hbus)
+{
+	pci_walk_bus(hbus->pci_bus, hv_pci_restore_msi_msg, NULL);
+}
+
 static int hv_pci_resume(struct hv_device *hdev)
 {
 	struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);

@@ -3405,6 +3447,8 @@ static int hv_pci_resume(struct hv_device *hdev)
 
 	prepopulate_bars(hbus);
 
+	hv_pci_restore_msi_state(hbus);
+
 	hbus->state = hv_pcibus_installed;
 	return 0;
 out:

Lorenzo Pieralisi

2020-10-01 10:13 UTC #4 lore raw

Quoted 48 lines · Dexuan Cui

Quoted 4 lines
From: Lorenzo Pieralisi <[email protected]>
Sent: Monday, September 28, 2020 3:43 AM
[+MarcZ - this patch needs IRQ maintainers vetting]
Sure. Hi MarkZ, please also review the patch. Thanks!
Quoted 12 lines · Dexuan Cui
Quoted 10 lines
Hyper-V doesn't trap and emulate the accesses to the MSI/MSI-X registers, and we must use hv_compose_msi_msg() to ask Hyper-V to create the IOMMU Interrupt Remapping Table Entries. This is not an issue for a lot of PCI device drivers (e.g. NVMe driver, Mellanox NIC drivers), which destroy and re-create the interrupts across hibernation, so hv_compose_msi_msg() is called automatically. However, some other PCI device drivers (e.g. the Nvidia driver) may not destroy and re-create the interrupts across hibernation, so hv_pci_resume() has to call hv_compose_msi_msg(), otherwise the PCI device drivers can no longer receive MSI/MSI-X interrupts after hibernation.
Quoted 2 lines
This looks like drivers bugs and I don't think the HV controller driver is where you should fix them.
IMHO this is not a PCI device driver bug, because I think a PCI device driver is allowed to keep and re-use the MSI/MSI-X interrupts across hibernation, otherwise we would not have pci_restore_msi_state() in pci_restore_state().
The in-tree open-source Nvidia GPU driver drivers/gpu/drm/nouveau is such a PCI device driver that re-uses the MSI-X interrupts across hibernation. The out-of-tree proprietary Nvidia GPU driver also does the same thing. It looks some other in-tree PCI device drivers also do the same thing, though I don't remember their names offhand.
IMO it's much better to change the pci-hyperv driver once and for all, than to change every such existing (and future?) PCI device driver.
pci_restore_msi_state() directly writes the MSI/MSI-X related registers in __pci_write_msi_msg() and msix_mask_irq(). On a physical machine, this works perfectly, but for a Linux VM running on a hypervisor, which typically enables IOMMU interrupt remapping, the hypervisor usually should trap and emulate the write accesses to the MSI/MSI-X registers, so the hypervisor is able to create the necessary interrupt remapping table entries in the IOMMU, and the MSI/MSI-X interrupts can work in the VM. Hyper-V is different from other hypervisors in that it does not trap and emulate the write accesses, and instead it uses a para-virtualized method, which requires the VM to call hv_compose_msi_msg() to notify the hypervisor of the info that would be passed to the hypervisor in the case of the trap-and-emulate method.

Quoted 6 lines

I mean this is a Hyper-V specific problem, so IMO we should fix the pci-hyperv driver rather than change the PCI device drivers, which work perfectly on a physical machine and on other hypervisors. Also it can be difficult or impossible to ask the authors of the aforementioned PCI device drivers to destry and re-create MSI/MSI-X acorss hibernation, especially for the out-of-tree driver(s).

Good, so why did you mention PCI drivers in the commit log if they are not related to the problem you are fixing ?

Quoted 5 lines

Quoted 2 lines
Regardless, this commit log does not provide the information that it should.
Hi Lozenzo, I'm happy to add more info. Can you please let me know what extra info I should provide?

s/Lozenzo/Lorenzo

The info you describe properly below, namely what the _actual_ problem is.

Quoted 98 lines

Quoted 39 lines
Quoted 6 lines
Fixes ac82fc832708 ("PCI: hv: Add hibernation support")
Signed-off-by Dexuan Cui <[email protected]>
Reviewed-by Jake Oshins <[email protected]>
---
Review
Quoted 8 lines
Changes in v2:
Fixed a typo in the comment in hv_irq_unmask. Thanks to Michael! Added Jake's Reviewed-by.
 drivers/pci/controller/pci-hyperv.c | 44 +++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
(quoted diff) +0 -0
b/drivers/pci/controller/pci-hyperv.c
Quoted 9 lines
index fc4c3a15e570..dd21afb5d62b 100644
drivers/pci/controller/pci-hyperv.c +2 -0
@@ -1211,6 +1211,21 @@ static void hv_irq_unmask(struct irq_data *data)
 	pbus = pdev->bus;
 	hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata);
 
+	if (hbus->state == hv_pcibus_removing) {
+		/*
drivers/pci/controller/pci-hyperv.c static void hv_irq_unmask(struct irq_data *data) · line 1225
Quoted 10 lines
drivers/pci/controller/pci-hyperv.c +10 -0
+		 * During hibernation, when a CPU is offlined, the kernel tries
+		 * to move the interrupt to the remaining CPUs that haven't
+		 * been offlined yet. In this case, the below hv_do_hypercall()
+		 * always fails since the vmbus channel has been closed, so we
+		 * should not call the hypercall, but we still need
+		 * pci_msi_unmask_irq() to reset the mask bit in desc->masked:
+		 * see cpu_disable_common() -> fixup_irqs() ->
+		 * irq_migrate_all_off_this_cpu() -> migrate_one_irq().
+		 */
+		pci_msi_unmask_irqpci_msi_unmask_irq(data);
This is not appropriate - it looks like a plaster to paper over an issue with hyper-V hibernation code sequence. Fix that issue instead of papering over it here.
Quoted 2 lines
Thanks,
Lorenzo
IMO this patch is fixing this Hyper-V specific problem. :-) The probem is unique to Hyper-V because chip->irq_unmask() may fail in a Linux VM running on Hyper-V.
chip->irq_unmask() can not fail on a physical machine. I guess this is why the return value of irq_unmask() is defined as "void" in include/linux/irq.h:
struct irq_chip {
...
        void            (*irq_mask)(struct irq_data *data);
        void            (*irq_unmask)(struct irq_data *data);
As I described in the comment, in a VM on Hyper-V, chip->irq_unmask() fails during the suspending phase of hibernation because it's called when the non-boot CPUs are being offlined, and at this time all the devices, including Hyper-V VMBus devices, have been "frozen" -- this is part of the standard Linux hibernation workflow. Since Hyper-V thinks the VM has frozen the pci-hyperv VMBus device at this moment (i.e. closed the VMBus channel of the VMBus device), it fails chip->irq_unmask(), i.e. hv_irq_unmask() -> hv_do_hypercall().
On a physical machine, unmasking an MSI/MSI-X register just means an MMIO write, which I think can not fail here.
So I think this patch is the correct fix, considering Hyper-V's unique implementation of the MSI "chip" (i.e. Hyper-V does not trap and emulate the MSI/MSI-X register accesses, and uses a para-virtualized method as I explained above), and the fact that I shouldn't and can't change the standard Linux hibernation workflow.
In hv_irq_unmask(), when I skip the hypercall in the case of hbus->state == hv_pcibus_removing, I still need the pci_msi_unmask_irq(), because of the sequences in kernel/irq/cpuhotplug.c:
static bool migrate_one_irq(struct irq_desc *desc) {
...
        if (maskchip && chip->irq_mask)
                chip->irq_mask(d);
...
        err = irq_do_set_affinity(d, affinity, false);
...
        if (maskchip && chip->irq_unmask)
                chip->irq_unmask(d);
Here if hv_irq_unmask does not call pci_msi_unmask_irq(), the desc->masked remains "true", so later after hibernation, the MSI interrupt line always reamins masked, which is incorrect.
Here the slient failure of hv_irq_unmask() does not matter since all the non-boot CPUs are being offlined (meaning all the devices have been frozen). Note: the correct affinity info is still updated into the irqdata data structure in migrate_one_irq() -> irq_do_set_affinity() -> hv_set_affinity(), so when the VM resumes, hv_pci_resume() -> hv_pci_restore_msi_state() is able to correctly restore the irqs with the correct affinity.

Quoted 6 lines

I hope the explanation can help clarify things. I understand this is not as natual as tht case that Linux runs on a physical machine, but due to the unique PCI pass-through implementation of Hyper-V, IMO this is the only viable fix for the problem here. BTW, this patch is only confined to the pci-hyperv driver and I believe it can no cause any regression.

Understood, write this in the commit log and I won't nag you any further.

Side note: this issue is there because the hypcall failure triggers an early return from hv_irq_unmask(). Is that early return really correct ? Another possibility is just logging the error and let hv_irq_unmask() continue and call pci_msi_unmask_irq() in the exit path.

Is there a hypcall return value that you can use to detect fatal vs non-fatal (ie hibernation) hypcall failures ?

I was confused by reading the patch since it seemed that you call pci_msi_unmask_irq() _only_ while hibernating, which was certainly a bug.

Thank you for explaining.

Lorenzo

Quoted 55 lines

Thanks,
-- Dexuan

Quoted 50 lines

drivers/pci/controller/pci-hyperv.c static int hv_pci_suspend(struct hv_device

Quoted 7 lines

drivers/pci/controller/pci-hyperv.c +3 -0

+		return;
+	}
+
 	spin_lock_irqsave(&hbus->retarget_msi_interrupt_lock, flags);
 
 	params = &hbus->retarget_msi_interrupt_params;

@@ -3372,6 +3387,33 @@ static int hv_pci_suspend(struct hv_device

*hdev)

Quoted 42 lines

drivers/pci/controller/pci-hyperv.c +29 -0

 	return 0;
 }
 
+static int hv_pci_restore_msi_msg(struct pci_dev *pdev, void *arg)
+{
+	struct msi_desc *entry;
+	struct irq_data *irq_data;
+
+	for_each_pci_msi_entry(entry, pdev) {
+		irq_data = irq_get_irq_data(entry->irq);
+		if (WARN_ON_ONCE(!irq_data))
+			return -EINVAL;
+
+		hv_compose_msi_msg(irq_data, &entry->msg);
+	}
+
+	return 0;
+}
+
+/*
+ * Upon resume, pci_restore_msi_state() -> ... ->  __pci_write_msi_msg()
+ * re-writes the MSI/MSI-X registers, but since Hyper-V doesn't trap and
+ * emulate the accesses, we have to call hv_compose_msi_msg() to ask
+ * Hyper-V to re-create the IOMMU Interrupt Remapping Table Entries.
+ */
+static void hv_pci_restore_msi_state(struct hv_pcibus_device *hbus)
+{
+	pci_walk_bus(hbus->pci_bus, hv_pci_restore_msi_msg, NULL);
+}
+
 static int hv_pci_resume(struct hv_device *hdev)
 {
 	struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);

@@ -3405,6 +3447,8 @@ static int hv_pci_resume(struct hv_device *hdev)
 
 	prepopulate_bars(hbus);
 
+	hv_pci_restore_msi_state(hbus);
+
 	hbus->state = hv_pcibus_installed;
 	return 0;
 out:

Dexuan Cui

author maintainer

2020-10-01 20:53 UTC #5 lore raw

Quoted 10 lines

From: Lorenzo Pieralisi <[email protected]>
Sent: Thursday, October 1, 2020 3:13 AM
Quoted 7 lines
...
I mean this is a Hyper-V specific problem, so IMO we should fix the pci-hyperv driver rather than change the PCI device drivers, which work perfectly on a physical machine and on other hypervisors. Also it can be difficult or impossible to ask the authors of the aforementioned PCI device drivers to destroy and re-create MSI/MSI-X across hibernation, especially for the out-of-tree driver(s).

Quoted 2 lines

Good, so why did you mention PCI drivers in the commit log if they are not related to the problem you are fixing ?

I mentioned the names of the PCI device drivers because IMO people want to know how the issue can reproduce (i.e. which PCI devices are affected and which are not), so they know how to test this patch.

I'll remove the names of the unaffected PCI device drivers from the commit log, and only keep the name of the Nvidia GPU drivers (which are so far the only drivers I have identified that are affected, when Linux VM runs on Hyper-V and hibernates).

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Regardless, this commit log does not provide the information that it should.
Hi Lozenzo, I'm happy to add more info. Can you please let me know what extra info I should provide?
s/Lozenzo/Lorenzo

Sorry! Will fix the typo.

Quoted 2 lines

The info you describe properly below, namely what the _actual_ problem is.

I will send v3 with the below info.

Quoted 12 lines

Quoted 12 lines
Here if hv_irq_unmask does not call pci_msi_unmask_irq(), the desc->masked remains "true", so later after hibernation, the MSI interrupt line always reamins masked, which is incorrect.
Here the slient failure of hv_irq_unmask() does not matter since all the non-boot CPUs are being offlined (meaning all the devices have been frozen). Note: the correct affinity info is still updated into the irqdata data structure in migrate_one_irq() -> irq_do_set_affinity() -> hv_set_affinity(), so when the VM resumes, hv_pci_resume() -> hv_pci_restore_msi_state() is able to correctly restore the irqs with the correct affinity.

Quoted 8 lines

Quoted 6 lines
I hope the explanation can help clarify things. I understand this is not as natual as tht case that Linux runs on a physical machine, but due to the unique PCI pass-through implementation of Hyper-V, IMO this is the only viable fix for the problem here. BTW, this patch is only confined to the pci-hyperv driver and I believe it can no cause any regression.
Understood, write this in the commit log and I won't nag you any further.

Ok. I treat it as an opportunity to practise and improve my writing :-)

Quoted 2 lines

Side note: this issue is there because the hypcall failure triggers an early return from hv_irq_unmask().

Yes.

Quoted 1 line

Is that early return really correct ?

Good question. IMO it's incorrect, because hv_irq_unmask() is called when the interrupt is activated for the first time, and when the interrupt's affinity is being changed. In these cases, we may as well call pci_msi_unmask_irq() unconditionally, even if the hypercall fails.

BTW, AFAIK, in practice the hypercall only fails in 2 cases:
1. The device is removed when Linux VM has not finished the device's initialization.
2. In hibernation, the device has been disabled while the generic hibernation code tries to migrate the interrupt, as I explained.

In the 2 cases, the hypercall returns the same error code HV_STATUS_INVALID_PARAMETER(0x5).

Quoted 3 lines

Another possibility is just logging the error and let hv_irq_unmask() continue and call pci_msi_unmask_irq() in the exit path.

This is a good idea. I'll make this change in v3.

Quoted 2 lines

Is there a hypcall return value that you can use to detect fatal vs non-fatal (ie hibernation) hypcall failures ?

Unluckily IMO there is not. The spec (v6.0b)'s section 10.5.4 (page 106)
https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
does define some return values, but IMO they're not applicable here.

Quoted 7 lines

I was confused by reading the patch since it seemed that you call pci_msi_unmask_irq() _only_ while hibernating, which was certainly a bug.
Thank you for explaining.
Lorenzo

Thanks for reviewing! I'll post v3. Looking forward to your new comments!

Thanks,
-- Dexuan