| Message ID | 1574234218-49195-3-git-send-email-decui@microsoft.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Enhance pci-hyperv to support hibernation, and 2 misc fixes | expand |
On Tue, Nov 19, 2019 at 11:16:56PM -0800, Dexuan Cui wrote: > Implement the suspend/resume callbacks. > > We must make sure there is no pending work items before we call > vmbus_close(). Where ? Why ? Imagine a developer reading this log to try to understand why you made this change, do you really think this commit log is informative in its current form ? I am not asking a book but this is a significant feature please make an effort to explain it (I can update the log for you but please write one and I shall do it). Lorenzo > Signed-off-by: Dexuan Cui <decui@microsoft.com> > --- > > Changes in v2: this patch is a simple merge of 2 previous smaller patches, > accordign to the suggestion of Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>. > > drivers/pci/controller/pci-hyperv.c | 107 +++++++++++++++++++++++++++- > 1 file changed, 105 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > index 65f18f840ce9..e71eb6e0bfdd 100644 > --- a/drivers/pci/controller/pci-hyperv.c > +++ b/drivers/pci/controller/pci-hyperv.c > @@ -455,6 +455,7 @@ enum hv_pcibus_state { > hv_pcibus_init = 0, > hv_pcibus_probed, > hv_pcibus_installed, > + hv_pcibus_removing, > hv_pcibus_removed, > hv_pcibus_maximum > }; > @@ -1681,6 +1682,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) { > @@ -2107,6 +2125,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; > @@ -2223,11 +2247,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); > } > > /** > @@ -3107,6 +3139,75 @@ 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); > + enum hv_pcibus_state old_state; > + int ret; > + > + 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); > + 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 */ > @@ -3121,6 +3222,8 @@ static struct hv_driver hv_pci_drv = { > .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) > -- > 2.19.1 >
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Sent: Wednesday, November 20, 2019 9:20 AM > > On Tue, Nov 19, 2019 at 11:16:56PM -0800, Dexuan Cui wrote: > > Implement the suspend/resume callbacks. > > > > We must make sure there is no pending work items before we call > > vmbus_close(). > > Where ? Why ? Imagine a developer reading this log to try to understand > why you made this change, do you really think this commit log is > informative in its current form ? > > I am not asking a book but this is a significant feature please make > an effort to explain it (I can update the log for you but please > write one and I shall do it). > > Lorenzo Sorry for being sloppy on this patch's changelog! Can you please use the below? I can also post v3 with the new changelog if that's better. PCI: hv: Add the support of hibernation hv_pci_suspend() runs in a process context as a callback in dpm_suspend(). When it starts to run, the channel callback hv_pci_onchannelcallback(), which runs in a tasklet context, can be still running concurrently and scheduling new work items onto hbus->wq in hv_pci_devices_present() and hv_pci_eject_device(), and the work item handlers can access the vmbus channel, which can be being closed by hv_pci_suspend(), e.g. the work item handler pci_devices_present_work() -> new_pcichild_device() writes to the vmbus channel. To eliminate the race, hv_pci_suspend() disables the channel callback tasklet, sets hbus->state to hv_pcibus_removing, and re-enables the tasklet. This way, when hv_pci_suspend() proceeds, it knows that no new work item can be scheduled, and then it flushes hbus->wq and safely closes the vmbus channel. Thanks, -- Dexuan
On Thu, Nov 21, 2019 at 12:50:17AM +0000, Dexuan Cui wrote: > > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Sent: Wednesday, November 20, 2019 9:20 AM > > > > On Tue, Nov 19, 2019 at 11:16:56PM -0800, Dexuan Cui wrote: > > > Implement the suspend/resume callbacks. > > > > > > We must make sure there is no pending work items before we call > > > vmbus_close(). > > > > Where ? Why ? Imagine a developer reading this log to try to understand > > why you made this change, do you really think this commit log is > > informative in its current form ? > > > > I am not asking a book but this is a significant feature please make > > an effort to explain it (I can update the log for you but please > > write one and I shall do it). > > > > Lorenzo > > Sorry for being sloppy on this patch's changelog! Can you please use the > below? I can also post v3 with the new changelog if that's better. As you wish but more importantly get hyper-V maintainers to ACK these changes since time is running out for v5.5. Lorenzo > PCI: hv: Add the support of hibernation > > hv_pci_suspend() runs in a process context as a callback in dpm_suspend(). > When it starts to run, the channel callback hv_pci_onchannelcallback(), > which runs in a tasklet context, can be still running concurrently and > scheduling new work items onto hbus->wq in hv_pci_devices_present() and > hv_pci_eject_device(), and the work item handlers can access the vmbus > channel, which can be being closed by hv_pci_suspend(), e.g. the work item > handler pci_devices_present_work() -> new_pcichild_device() writes to > the vmbus channel. > > To eliminate the race, hv_pci_suspend() disables the channel callback > tasklet, sets hbus->state to hv_pcibus_removing, and re-enables the tasklet. > > This way, when hv_pci_suspend() proceeds, it knows that no new work item > can be scheduled, and then it flushes hbus->wq and safely closes the vmbus > channel. > > Thanks, > -- Dexuan
From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Sent: Thursday, November 21, 2019 3:44 AM > > On Thu, Nov 21, 2019 at 12:50:17AM +0000, Dexuan Cui wrote: > > > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > Sent: Wednesday, November 20, 2019 9:20 AM > > > > > > On Tue, Nov 19, 2019 at 11:16:56PM -0800, Dexuan Cui wrote: > > > > Implement the suspend/resume callbacks. > > > > > > > > We must make sure there is no pending work items before we call > > > > vmbus_close(). > > > > > > Where ? Why ? Imagine a developer reading this log to try to understand > > > why you made this change, do you really think this commit log is > > > informative in its current form ? > > > > > > I am not asking a book but this is a significant feature please make > > > an effort to explain it (I can update the log for you but please > > > write one and I shall do it). > > > > > > Lorenzo > > > > Sorry for being sloppy on this patch's changelog! Can you please use the > > below? I can also post v3 with the new changelog if that's better. > > As you wish but more importantly get hyper-V maintainers to ACK these > changes since time is running out for v5.5. > > Lorenzo > > > PCI: hv: Add the support of hibernation > > > > hv_pci_suspend() runs in a process context as a callback in dpm_suspend(). > > When it starts to run, the channel callback hv_pci_onchannelcallback(), > > which runs in a tasklet context, can be still running concurrently and > > scheduling new work items onto hbus->wq in hv_pci_devices_present() and > > hv_pci_eject_device(), and the work item handlers can access the vmbus > > channel, which can be being closed by hv_pci_suspend(), e.g. the work item > > handler pci_devices_present_work() -> new_pcichild_device() writes to > > the vmbus channel. > > > > To eliminate the race, hv_pci_suspend() disables the channel callback > > tasklet, sets hbus->state to hv_pcibus_removing, and re-enables the tasklet. > > > > This way, when hv_pci_suspend() proceeds, it knows that no new work item > > can be scheduled, and then it flushes hbus->wq and safely closes the vmbus > > channel. > > > > Thanks, > > -- Dexuan FWIW, I'd like to see the above level of detail also as comments in the code Itself so that whoever next looks at the code sees the explanation directly without having to review the commit logs. Also, the commit message doesn't say what the commit actually does and why. I'd suggest the commit message along these lines: Add suspend() and resume() functions so that Hyper-V virtual PCI devices are handled properly when the VM hibernates and resumes from hibernation. Note that the suspend() function must make sure there are no pending work items before calling vmbus_close(), since it runs in a process context as a callback in dpm_suspend(). When it starts to run, the channel callback hv_pci_onchannelcallback(), which runs in a tasklet context, can be still running concurrently and scheduling new work items onto hbus->wq in hv_pci_devices_present() and hv_pci_eject_device(), and the work item handlers can access the vmbus channel, which can be being closed by hv_pci_suspend(), e.g. the work item handler pci_devices_present_work() -> new_pcichild_device() writes to the vmbus channel. To eliminate the race, hv_pci_suspend() disables the channel callback tasklet, sets hbus->state to hv_pcibus_removing, and re-enables the tasklet. This way, when hv_pci_suspend() proceeds, it knows that no new work item can be scheduled, and then it flushes hbus->wq and safely closes the vmbus channel. Michael
On Sun, Nov 24, 2019 at 10:19:46PM +0000, Michael Kelley wrote: > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Sent: Thursday, November 21, 2019 3:44 AM > > > > On Thu, Nov 21, 2019 at 12:50:17AM +0000, Dexuan Cui wrote: > > > > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > > Sent: Wednesday, November 20, 2019 9:20 AM > > > > > > > > On Tue, Nov 19, 2019 at 11:16:56PM -0800, Dexuan Cui wrote: > > > > > Implement the suspend/resume callbacks. > > > > > > > > > > We must make sure there is no pending work items before we call > > > > > vmbus_close(). > > > > > > > > Where ? Why ? Imagine a developer reading this log to try to understand > > > > why you made this change, do you really think this commit log is > > > > informative in its current form ? > > > > > > > > I am not asking a book but this is a significant feature please make > > > > an effort to explain it (I can update the log for you but please > > > > write one and I shall do it). > > > > > > > > Lorenzo > > > > > > Sorry for being sloppy on this patch's changelog! Can you please use the > > > below? I can also post v3 with the new changelog if that's better. > > > > As you wish but more importantly get hyper-V maintainers to ACK these > > changes since time is running out for v5.5. > > > > Lorenzo > > > > > PCI: hv: Add the support of hibernation > > > > > > hv_pci_suspend() runs in a process context as a callback in dpm_suspend(). > > > When it starts to run, the channel callback hv_pci_onchannelcallback(), > > > which runs in a tasklet context, can be still running concurrently and > > > scheduling new work items onto hbus->wq in hv_pci_devices_present() and > > > hv_pci_eject_device(), and the work item handlers can access the vmbus > > > channel, which can be being closed by hv_pci_suspend(), e.g. the work item > > > handler pci_devices_present_work() -> new_pcichild_device() writes to > > > the vmbus channel. > > > > > > To eliminate the race, hv_pci_suspend() disables the channel callback > > > tasklet, sets hbus->state to hv_pcibus_removing, and re-enables the tasklet. > > > > > > This way, when hv_pci_suspend() proceeds, it knows that no new work item > > > can be scheduled, and then it flushes hbus->wq and safely closes the vmbus > > > channel. > > > > > > Thanks, > > > -- Dexuan > > FWIW, I'd like to see the above level of detail also as comments in the code > Itself so that whoever next looks at the code sees the explanation directly > without having to review the commit logs. > > Also, the commit message doesn't say what the commit actually does and > why. I'd suggest the commit message along these lines: > > Add suspend() and resume() functions so that Hyper-V virtual PCI devices are > handled properly when the VM hibernates and resumes from hibernation. > > Note that the suspend() function must make sure there are no pending work > items before calling vmbus_close(), since it runs in a process context as a > callback in dpm_suspend(). When it starts to run, the channel callback > hv_pci_onchannelcallback(), which runs in a tasklet context, can be still running > concurrently and scheduling new work items onto hbus->wq in > hv_pci_devices_present() and hv_pci_eject_device(), and the work item > handlers can access the vmbus channel, which can be being closed by > hv_pci_suspend(), e.g. the work item handler pci_devices_present_work() -> > new_pcichild_device() writes to the vmbus channel. > > To eliminate the race, hv_pci_suspend() disables the channel callback > tasklet, sets hbus->state to hv_pcibus_removing, and re-enables the tasklet. > This way, when hv_pci_suspend() proceeds, it knows that no new work item > can be scheduled, and then it flushes hbus->wq and safely closes the vmbus > channel. This is much better, thank you, if you are happy with the patches please add your tags so that I can pull the series asap, hopefully we can merge it in v5.5. Thanks, Lorenzo
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c index 65f18f840ce9..e71eb6e0bfdd 100644 --- a/drivers/pci/controller/pci-hyperv.c +++ b/drivers/pci/controller/pci-hyperv.c @@ -455,6 +455,7 @@ enum hv_pcibus_state { hv_pcibus_init = 0, hv_pcibus_probed, hv_pcibus_installed, + hv_pcibus_removing, hv_pcibus_removed, hv_pcibus_maximum }; @@ -1681,6 +1682,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) { @@ -2107,6 +2125,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; @@ -2223,11 +2247,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); } /** @@ -3107,6 +3139,75 @@ 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); + enum hv_pcibus_state old_state; + int ret; + + 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); + 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 */ @@ -3121,6 +3222,8 @@ static struct hv_driver hv_pci_drv = { .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)
Implement the suspend/resume callbacks. We must make sure there is no pending work items before we call vmbus_close(). Signed-off-by: Dexuan Cui <decui@microsoft.com> --- Changes in v2: this patch is a simple merge of 2 previous smaller patches, accordign to the suggestion of Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>. drivers/pci/controller/pci-hyperv.c | 107 +++++++++++++++++++++++++++- 1 file changed, 105 insertions(+), 2 deletions(-)