| Message ID | 20240325070724.574508-2-Jiqian.Chen@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | S3 support | expand |
Hi MST, I modified this patch according to your comments. Do you have any other suggestions? On 2024/3/25 15:07, Jiqian Chen wrote: > In current code, when guest does S3, virtio devices are reset due to > the bit No_Soft_Reset is not set. After resetting, the display resources > of virtio-gpu are destroyed, then the display can't come back and only > show blank after resuming. > > Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check > this bit, if this bit is set, the devices resetting will not be done, and > then the display can work after resuming. > > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > --- > hw/virtio/virtio-pci.c | 38 +++++++++++++++++++++++++++++++++- > include/hw/virtio/virtio-pci.h | 5 +++++ > 2 files changed, 42 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 1a7039fb0c68..daafda315f8c 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -2197,6 +2197,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) > pcie_cap_lnkctl_init(pci_dev); > } > > + if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) { > + pci_set_word(pci_dev->config + pos + PCI_PM_CTRL, > + PCI_PM_CTRL_NO_SOFT_RESET); > + } > + > if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) { > /* Init Power Management Control Register */ > pci_set_word(pci_dev->wmask + pos + PCI_PM_CTRL, > @@ -2259,18 +2264,47 @@ static void virtio_pci_reset(DeviceState *qdev) > } > } > > +static bool device_no_need_reset(PCIDevice *dev) > +{ > + if (pci_is_express(dev)) { > + uint16_t pmcsr; > + > + pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL); > + /* > + * When No_Soft_Reset bit is set and the device > + * is in D3hot state, don't reset device > + */ > + if ((pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) && > + (pmcsr & PCI_PM_CTRL_STATE_MASK) == 3) { > + return true; > + } > + } > + > + return false; > +} > + > static void virtio_pci_bus_reset_hold(Object *obj) > { > PCIDevice *dev = PCI_DEVICE(obj); > DeviceState *qdev = DEVICE(obj); > > + if (device_no_need_reset(dev)) { > + return; > + } > + > virtio_pci_reset(qdev); > > if (pci_is_express(dev)) { > + uint16_t val = 0; > + 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_PM_NO_SOFT_RESET) { > + val |= PCI_PM_CTRL_NO_SOFT_RESET; > + } > + pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, val); > } > } > > @@ -2297,6 +2331,8 @@ static Property virtio_pci_properties[] = { > VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true), > DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags, > VIRTIO_PCI_FLAG_INIT_PM_BIT, true), > + DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy, flags, > + VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, false), > DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags, > VIRTIO_PCI_FLAG_INIT_FLR_BIT, true), > DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags, > diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h > index 59d88018c16a..9e67ba38c748 100644 > --- a/include/hw/virtio/virtio-pci.h > +++ b/include/hw/virtio/virtio-pci.h > @@ -43,6 +43,7 @@ enum { > VIRTIO_PCI_FLAG_INIT_FLR_BIT, > VIRTIO_PCI_FLAG_AER_BIT, > VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT, > + VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, > }; > > /* Need to activate work-arounds for buggy guests at vmstate load. */ > @@ -79,6 +80,10 @@ enum { > /* Init Power Management */ > #define VIRTIO_PCI_FLAG_INIT_PM (1 << VIRTIO_PCI_FLAG_INIT_PM_BIT) > > +/* Init The No_Soft_Reset bit of Power Management */ > +#define VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET \ > + (1 << VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT) > + > /* Init Function Level Reset capability */ > #define VIRTIO_PCI_FLAG_INIT_FLR (1 << VIRTIO_PCI_FLAG_INIT_FLR_BIT) >
On Mon, Mar 25, 2024 at 03:07:24PM +0800, Jiqian Chen wrote: > In current code, when guest does S3, virtio devices are reset due to > the bit No_Soft_Reset is not set. After resetting, the display resources > of virtio-gpu are destroyed, then the display can't come back and only > show blank after resuming. > > Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check > this bit, if this bit is set, the devices resetting will not be done, and > then the display can work after resuming. > > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > --- > hw/virtio/virtio-pci.c | 38 +++++++++++++++++++++++++++++++++- > include/hw/virtio/virtio-pci.h | 5 +++++ > 2 files changed, 42 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 1a7039fb0c68..daafda315f8c 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -2197,6 +2197,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) > pcie_cap_lnkctl_init(pci_dev); > } > > + if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) { > + pci_set_word(pci_dev->config + pos + PCI_PM_CTRL, > + PCI_PM_CTRL_NO_SOFT_RESET); > + } > + > if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) { > /* Init Power Management Control Register */ > pci_set_word(pci_dev->wmask + pos + PCI_PM_CTRL, > @@ -2259,18 +2264,47 @@ static void virtio_pci_reset(DeviceState *qdev) > } > } > > +static bool device_no_need_reset(PCIDevice *dev) I'd just call it virtio_pci_no_soft_reset() . > +{ > + if (pci_is_express(dev)) { A cleaner way to structure this is by reversing the test: if (!pci_is_express(dev)) { return false; } I would also check that pm_cap is actually set here. > + uint16_t pmcsr; > + > + pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL); > + /* > + * When No_Soft_Reset bit is set and the device > + * is in D3hot state, don't reset device > + */ > + if ((pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) && > + (pmcsr & PCI_PM_CTRL_STATE_MASK) == 3) { > + return true; And then here it will be return (pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) && (pmcsr & PCI_PM_CTRL_STATE_MASK) == 3; > + } > + } > + > + return false; > +} > + > static void virtio_pci_bus_reset_hold(Object *obj) > { > PCIDevice *dev = PCI_DEVICE(obj); > DeviceState *qdev = DEVICE(obj); > > + if (device_no_need_reset(dev)) { > + return; > + } > + > virtio_pci_reset(qdev); > > if (pci_is_express(dev)) { > + uint16_t val = 0; call it pm_ctrl > + 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_PM_NO_SOFT_RESET) { > + val |= PCI_PM_CTRL_NO_SOFT_RESET; > + } > + pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, val); There is no need to do it like this - only state is writeable anyway. So simply pci_word_test_and_clear_mask(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, PCI_PM_CTRL_STATE_MASK) maybe we should actually check here: if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) there's a chance commit 27ce0f3afc9 broke things for old machines and we never noticed. If so that should be a separate bugfix patch though. > } > } > > @@ -2297,6 +2331,8 @@ static Property virtio_pci_properties[] = { > VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true), > DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags, > VIRTIO_PCI_FLAG_INIT_PM_BIT, true), > + DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy, flags, > + VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, false), > DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags, > VIRTIO_PCI_FLAG_INIT_FLR_BIT, true), > DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags, > diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h > index 59d88018c16a..9e67ba38c748 100644 > --- a/include/hw/virtio/virtio-pci.h > +++ b/include/hw/virtio/virtio-pci.h > @@ -43,6 +43,7 @@ enum { > VIRTIO_PCI_FLAG_INIT_FLR_BIT, > VIRTIO_PCI_FLAG_AER_BIT, > VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT, > + VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, > }; > > /* Need to activate work-arounds for buggy guests at vmstate load. */ > @@ -79,6 +80,10 @@ enum { > /* Init Power Management */ > #define VIRTIO_PCI_FLAG_INIT_PM (1 << VIRTIO_PCI_FLAG_INIT_PM_BIT) > > +/* Init The No_Soft_Reset bit of Power Management */ > +#define VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET \ > + (1 << VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT) > + > /* Init Function Level Reset capability */ > #define VIRTIO_PCI_FLAG_INIT_FLR (1 << VIRTIO_PCI_FLAG_INIT_FLR_BIT) > > -- > 2.34.1
On 2024/3/28 16:11, Michael S. Tsirkin wrote: > >> + 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_PM_NO_SOFT_RESET) { >> + val |= PCI_PM_CTRL_NO_SOFT_RESET; >> + } >> + pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, val); > > > There is no need to do it like this - only state is writeable > anyway. So simply > pci_word_test_and_clear_mask(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, PCI_PM_CTRL_STATE_MASK) > > > maybe we should actually check here: > if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) > there's a chance commit 27ce0f3afc9 broke things for old machines > and we never noticed. If so that should be a separate bugfix patch though. Make sense. It is actually a bug imported by 27ce0f3afc9. According to your comments, I think here should be a separate patch, like: if (pci_is_express(dev)) { VirtIOPCIProxy *proxy = VIRTIO_PCI(dev); pcie_cap_deverr_reset(dev); pcie_cap_lnkctl_reset(dev); 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); } } Right? > >
On Thu, Mar 28, 2024 at 09:02:28AM +0000, Chen, Jiqian wrote: > > On 2024/3/28 16:11, Michael S. Tsirkin wrote: > > > >> + 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_PM_NO_SOFT_RESET) { > >> + val |= PCI_PM_CTRL_NO_SOFT_RESET; > >> + } > >> + pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, val); > > > > > > There is no need to do it like this - only state is writeable > > anyway. So simply > > pci_word_test_and_clear_mask(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, PCI_PM_CTRL_STATE_MASK) > > > > > > maybe we should actually check here: > > if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) > > there's a chance commit 27ce0f3afc9 broke things for old machines > > and we never noticed. If so that should be a separate bugfix patch though. > Make sense. It is actually a bug imported by 27ce0f3afc9. > According to your comments, I think here should be a separate patch, like: > if (pci_is_express(dev)) { > VirtIOPCIProxy *proxy = VIRTIO_PCI(dev); > > pcie_cap_deverr_reset(dev); > pcie_cap_lnkctl_reset(dev); > > 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); > } > } > Right? Works for me. > > > > > > -- > Best regards, > Jiqian Chen.
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 1a7039fb0c68..daafda315f8c 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -2197,6 +2197,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) pcie_cap_lnkctl_init(pci_dev); } + if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) { + pci_set_word(pci_dev->config + pos + PCI_PM_CTRL, + PCI_PM_CTRL_NO_SOFT_RESET); + } + if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) { /* Init Power Management Control Register */ pci_set_word(pci_dev->wmask + pos + PCI_PM_CTRL, @@ -2259,18 +2264,47 @@ static void virtio_pci_reset(DeviceState *qdev) } } +static bool device_no_need_reset(PCIDevice *dev) +{ + if (pci_is_express(dev)) { + uint16_t pmcsr; + + pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL); + /* + * When No_Soft_Reset bit is set and the device + * is in D3hot state, don't reset device + */ + if ((pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) && + (pmcsr & PCI_PM_CTRL_STATE_MASK) == 3) { + return true; + } + } + + return false; +} + static void virtio_pci_bus_reset_hold(Object *obj) { PCIDevice *dev = PCI_DEVICE(obj); DeviceState *qdev = DEVICE(obj); + if (device_no_need_reset(dev)) { + return; + } + virtio_pci_reset(qdev); if (pci_is_express(dev)) { + uint16_t val = 0; + 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_PM_NO_SOFT_RESET) { + val |= PCI_PM_CTRL_NO_SOFT_RESET; + } + pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, val); } } @@ -2297,6 +2331,8 @@ static Property virtio_pci_properties[] = { VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true), DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_INIT_PM_BIT, true), + DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy, flags, + VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, false), DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_INIT_FLR_BIT, true), DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags, diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h index 59d88018c16a..9e67ba38c748 100644 --- a/include/hw/virtio/virtio-pci.h +++ b/include/hw/virtio/virtio-pci.h @@ -43,6 +43,7 @@ enum { VIRTIO_PCI_FLAG_INIT_FLR_BIT, VIRTIO_PCI_FLAG_AER_BIT, VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT, + VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, }; /* Need to activate work-arounds for buggy guests at vmstate load. */ @@ -79,6 +80,10 @@ enum { /* Init Power Management */ #define VIRTIO_PCI_FLAG_INIT_PM (1 << VIRTIO_PCI_FLAG_INIT_PM_BIT) +/* Init The No_Soft_Reset bit of Power Management */ +#define VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET \ + (1 << VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT) + /* Init Function Level Reset capability */ #define VIRTIO_PCI_FLAG_INIT_FLR (1 << VIRTIO_PCI_FLAG_INIT_FLR_BIT)
In current code, when guest does S3, virtio devices are reset due to the bit No_Soft_Reset is not set. After resetting, the display resources of virtio-gpu are destroyed, then the display can't come back and only show blank after resuming. Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check this bit, if this bit is set, the devices resetting will not be done, and then the display can work after resuming. Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> --- hw/virtio/virtio-pci.c | 38 +++++++++++++++++++++++++++++++++- include/hw/virtio/virtio-pci.h | 5 +++++ 2 files changed, 42 insertions(+), 1 deletion(-)