[v11] S3 support

[PATCH v11 1/2] virtio-pci: only reset pm state during resetting

Posted by Jiqian Chen 5 months, 3 weeks ago

Fix bug imported by 27ce0f3afc9dd ("fix Power Management Control Register for PCI Express virtio devices"
After this change, observe that QEMU may erroneously clear the power status of the device,
or may erroneously clear non writable registers, such as NO_SOFT_RESET, etc.

Only state of PM_CTRL is writable.
Only when flag VIRTIO_PCI_FLAG_INIT_PM is set, need to reset state.

Fixes: 27ce0f3afc9dd ("fix Power Management Control Register for PCI Express virtio devices"

Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
 hw/virtio/virtio-pci.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index b1d02f4b3de0..1b63bcb3f15c 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -2300,10 +2300,16 @@ static void virtio_pci_bus_reset_hold(Object *obj, ResetType type)
     virtio_pci_reset(qdev);
 
     if (pci_is_express(dev)) {
+        VirtIOPCIProxy *proxy = VIRTIO_PCI(dev);
+
         pcie_cap_deverr_reset(dev);
         pcie_cap_lnkctl_reset(dev);
 
-        pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, 0);
+        if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) {
+            pci_word_test_and_clear_mask(
+                dev->config + dev->exp.pm_cap + PCI_PM_CTRL,
+                PCI_PM_CTRL_STATE_MASK);
+        }
     }
 }
 
-- 
2.34.1

Re: [PATCH v11 1/2] virtio-pci: only reset pm state during resetting

Posted by Chen, Jiqian 5 months ago

Hi MST,

On 2024/6/6 18:22, Jiqian Chen wrote:
> Fix bug imported by 27ce0f3afc9dd ("fix Power Management Control Register for PCI Express virtio devices"
> After this change, observe that QEMU may erroneously clear the power status of the device,
> or may erroneously clear non writable registers, such as NO_SOFT_RESET, etc.
> 
> Only state of PM_CTRL is writable.
> Only when flag VIRTIO_PCI_FLAG_INIT_PM is set, need to reset state.
> 
> Fixes: 27ce0f3afc9dd ("fix Power Management Control Register for PCI Express virtio devices"
> 
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
>  hw/virtio/virtio-pci.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index b1d02f4b3de0..1b63bcb3f15c 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2300,10 +2300,16 @@ static void virtio_pci_bus_reset_hold(Object *obj, ResetType type)
>      virtio_pci_reset(qdev);
>  
>      if (pci_is_express(dev)) {
> +        VirtIOPCIProxy *proxy = VIRTIO_PCI(dev);
> +
>          pcie_cap_deverr_reset(dev);
>          pcie_cap_lnkctl_reset(dev);
>  
> -        pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, 0);
> +        if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) {
> +            pci_word_test_and_clear_mask(
> +                dev->config + dev->exp.pm_cap + PCI_PM_CTRL,
> +                PCI_PM_CTRL_STATE_MASK);
> +        }
>      }
>  }
>  
I noticed that you merged this patch into the staging before, but then reverted it. Do you still have any concerns?

-- 
Best regards,
Jiqian Chen.

Re: [PATCH v11 1/2] virtio-pci: only reset pm state during resetting

Posted by Michael S. Tsirkin 5 months ago

On Fri, Jun 21, 2024 at 09:19:11AM +0000, Chen, Jiqian wrote:
> Hi MST,
> 
> On 2024/6/6 18:22, Jiqian Chen wrote:
> > Fix bug imported by 27ce0f3afc9dd ("fix Power Management Control Register for PCI Express virtio devices"
> > After this change, observe that QEMU may erroneously clear the power status of the device,
> > or may erroneously clear non writable registers, such as NO_SOFT_RESET, etc.
> > 
> > Only state of PM_CTRL is writable.
> > Only when flag VIRTIO_PCI_FLAG_INIT_PM is set, need to reset state.
> > 
> > Fixes: 27ce0f3afc9dd ("fix Power Management Control Register for PCI Express virtio devices"
> > 
> > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> > ---
> >  hw/virtio/virtio-pci.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index b1d02f4b3de0..1b63bcb3f15c 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -2300,10 +2300,16 @@ static void virtio_pci_bus_reset_hold(Object *obj, ResetType type)
> >      virtio_pci_reset(qdev);
> >  
> >      if (pci_is_express(dev)) {
> > +        VirtIOPCIProxy *proxy = VIRTIO_PCI(dev);
> > +
> >          pcie_cap_deverr_reset(dev);
> >          pcie_cap_lnkctl_reset(dev);
> >  
> > -        pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, 0);
> > +        if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) {
> > +            pci_word_test_and_clear_mask(
> > +                dev->config + dev->exp.pm_cap + PCI_PM_CTRL,
> > +                PCI_PM_CTRL_STATE_MASK);
> > +        }
> >      }
> >  }
> >  
> I noticed that you merged this patch into the staging before, but then reverted it. Do you still have any concerns?
> 
> -- 
> Best regards,
> Jiqian Chen.

Sorry I don't remember at this point. Normally it's because of
test failures but I then also notify the authors ... 
I will try to re-merge and see.

-- 
MST