On 2024/4/17 13:33, Yan Vugenfirer wrote: > On Wed, Apr 17, 2024 at 6:13 AM Chen, Jiqian <jiqian.c...@amd.com> wrote: >> >> On 2024/4/16 23:45, Igor Mammedov wrote: >>> On Tue, 16 Apr 2024 15:01:27 +0800 >>> Jiqian Chen <jiqian.c...@amd.com> wrote: >>> >>>> In current code, when guest does S3, virtio-gpu 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. >>> >>> Just a high level question. >>> Typically when system goes into S3 all devices (modulo RAM) loose context >>> (aka powered off), and then it's upto device driver to recover whatever >>> was lost. >> No, what you said is just one situation that when system goes into S3 >> devices loose context and fully reinitialized and be D0-uninitialized state. >> The other situation is the context must be maintained so that the devices >> can quickly be D0-active state after resuming. >> There are some descriptions in PCIe specification in Chapter 5.3.1.4 D3 >> state: >> " Functional context is required to be maintained by Functions in the D3Hot >> state if the No_Soft_Reset field in the PMCSR is >> Set. In this case, System Software is not required to re-initialize the >> Function after a transition from D3Hot to D0 (the >> Function will be in the D0active state). If the No_Soft_Reset bit is Clear, >> functional context is not required to be >> maintained by the Function in the D3Hot state, however it is not guaranteed >> that functional context will be cleared and >> software must not depend on such behavior. As a result, in this case System >> Software is required to fully re-initialize the >> Function after a transition to D0 as the Function will be in the >> D0uninitialized state." >> > BTW: according to Windows documentation D3 Hot state implementation is > a must for real HW devices. > https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/device-sleeping-states > "Device power states D1, D2, and D3 are the device low-power states. > Starting with Windows 8, D3 is divided into two substates, D3hot and > D3cold. > > D1 and D2 are intermediate low-power states. Many classes of devices > do not define these states. All devices must define D3hot." Yes, I know, it is also described in PCIe specification. But this is not a conflict. When set No_Soft_Reset didn’t mean the device can't enter D3hot state. It means when device enter D3hot, the context of device must be maintained.
> > Best regards, > Yan. > > >> What's more, not all virtio devices' context can be recovered by driver >> after resuming, like virtio-gpu, we have not enough information to re-create >> all >> display resources, that is discussed in my previous version email thread. >> >>> So why should we implement hw(qemu) workaround for a driver problem? >> I think this is not a workaround, No_Soft_Reset bit is also described in >> PCIe specification, it can be set or not. >> And we can set this bit for the specific device which we want to maintain >> context during S3. >> >>> >>>> >>>> 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. >>>> >>>> No_Soft_Reset bit is implemented for all virtio devices, and was tested >>>> only on virtio-gpu device. Set it false by default for safety. >>>> >>>> Signed-off-by: Jiqian Chen <jiqian.c...@amd.com> >>>> --- >>>> hw/virtio/virtio-pci.c | 37 ++++++++++++++++++++++++++++++++++ >>>> include/hw/virtio/virtio-pci.h | 5 +++++ >>>> 2 files changed, 42 insertions(+) >>>> >>>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c >>>> index a1b61308e7a0..82fa4defe5cd 100644 >>>> --- a/hw/virtio/virtio-pci.c >>>> +++ b/hw/virtio/virtio-pci.c >>>> @@ -2230,6 +2230,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, >>>> @@ -2292,11 +2297,37 @@ static void virtio_pci_reset(DeviceState *qdev) >>>> } >>>> } >>>> >>>> +static bool virtio_pci_no_soft_reset(PCIDevice *dev) >>>> +{ >>>> + uint16_t pmcsr; >>>> + >>>> + if (!pci_is_express(dev) || !dev->exp.pm_cap) { >>>> + return false; >>>> + } >>>> + >>>> + 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 >>>> + */ >>>> + return (pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) && >>>> + (pmcsr & PCI_PM_CTRL_STATE_MASK) == 3; >>>> +} >>>> + >>>> static void virtio_pci_bus_reset_hold(Object *obj) >>>> { >>>> PCIDevice *dev = PCI_DEVICE(obj); >>>> DeviceState *qdev = DEVICE(obj); >>>> >>>> + /* >>>> + * Note that: a proposal to add SUSPEND bit is being discussed, >>>> + * may need to consider the state of SUSPEND bit in future >>>> + */ >>>> + if (virtio_pci_no_soft_reset(dev)) { >>>> + return; >>>> + } >>>> + >>>> virtio_pci_reset(qdev); >>>> >>>> if (pci_is_express(dev)) { >>>> @@ -2336,6 +2367,12 @@ 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), >>>> + /* >>>> + * for safety, set this false by default, if change it to true, >>>> + * need to consider compatible for old machine >>>> + */ >>>> + DEFINE_PROP_BIT("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) >>>> >>> >> >> -- >> Best regards, >> Jiqian Chen. > -- Best regards, Jiqian Chen.