All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [RFC QEMU PATCH v8 0/1] S3 support
@ 2024-03-28 10:39 Jiqian Chen
  2024-03-28 10:39 ` [RFC QEMU PATCH v8 1/2] virtio-pci: only reset pm state during resetting Jiqian Chen
  2024-03-28 10:39 ` [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit Jiqian Chen
  0 siblings, 2 replies; 22+ messages in thread
From: Jiqian Chen @ 2024-03-28 10:39 UTC (permalink / raw)
  To: Michael S . Tsirkin, qemu-devel; +Cc: Huang Rui, Jiqian Chen

Hi all,
This is the v8 patch to support S3.
v8 makes below changes:
* Add a new patch#1 to fix a problem import by 27ce0f3afc9dd25d21b43bbce505157afd93d111,
  the right action is that only the state of PM_CTRL can be clear when resetting.
* patch#2 is the original patch to implement No_Soft_Reset bit, and in this version, I
  rename function and change some condition sequence.

Best regards,
Jiqian Chen


v7 makes below changes:
* Tested this patch with Qemu on Xen hypervisor. Depending on kernel
  patch (virtio: Add support for no-reset virtio PCI PM:
  https://lore.kernel.org/lkml/20231208070754.3132339-1-stevensd@chromium.org/)
* Changed the default value of flag VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT to false
* Fixed coding style violation
* Modified the content of the comments.
* Removed useless flag PCI_PM_CTRL_DATA_SCALE_MASK.


V6:
In current code, when guest does S3, virtio devices are reset during that process, that
causes the display resources of virtio-gpu are destroyed, then the display can't come
back after resuming.
This v6 patch implement the No_Soft_Reset bit of PCI_PM_CTRL register, when this bit is
set, the resetting will not be done, so that the display can work after resuming.
This version abandons all previous version implementations and is a new different
solution according to the outcome of the discussion and suggestions in the mailing
thread of virtio-spec.
(https://lists.oasis-open.org/archives/virtio-comment/202401/msg00077.html)


V5:
v5 makes below changes:
* Since this series patches add a new mechanism that let virtgpu and Qemu can negotiate
  their reset behavior, and other guys hope me can improve this mechanism to virtio pci
  level, so that other virtio devices can also benefit from it. So instead of adding
  new feature flag VIRTIO_GPU_F_FREEZE_S3 only serves for virtgpu, v5 add a new parameter
  named freeze_mode to struct VirtIODevice, when guest begin suspending, set freeze_mode
  to VIRTIO_PCI_FREEZE_MODE_FREEZE_S3, and then all virtio devices can get this status,
  and notice that guest is suspending, then they can change their reset behavior . See
  the new commit "virtio-pci: Add freeze_mode case for virtio pci"
* The second commit is just for virtgpu, when freeze_mode is VIRTIO_PCI_FREEZE_MODE_FREEZE_S3,
  prevent Qemu destroying render resources, so that the display can come back after resuming.
V5 of kernel patch:
https://lore.kernel.org/lkml/20230919104607.2282248-1-Jiqian.Chen@amd.com/T/#t
The link to trace this issue:
https://gitlab.com/qemu-project/qemu/-/issues/1860


v4:
Thanks for Gerd Hoffmann's advice. V4 makes below changes:
* Use enum for freeze mode, so this can be extended with more
  modes in the future.
* Rename functions and paratemers with "_S3" postfix.
And no functional changes.
Link:
https://lore.kernel.org/qemu-devel/20230720120816.8751-1-Jiqian.Chen@amd.com/
No v4 patch on kernel side.


v3:
Thanks for Michael S. Tsirkin's advice. V3 makes below changes:
* Remove changes in file include/standard-headers/linux/virtio_gpu.h
  I am not supposed to edit this file and it will be imported after
  the patches of linux kernel was merged.
Link:
https://lore.kernel.org/qemu-devel/20230719074726.1613088-1-Jiqian.Chen@amd.com/T/#t
V3 of kernel patch:
https://lore.kernel.org/lkml/20230720115805.8206-1-Jiqian.Chen@amd.com/T/#t


v2:
makes below changes:
* Change VIRTIO_CPU_CMD_STATUS_FREEZING to 0x0400 (<0x1000)
* Add virtio_gpu_device_unrealize to destroy resources to solve
  potential memory leak problem. This also needs hot-plug support.
* Add a new feature flag VIRTIO_GPU_F_FREEZING, so that guest and
  host can negotiate whenever freezing is supported or not.
Link:
https://lore.kernel.org/qemu-devel/20230630070016.841459-1-Jiqian.Chen@amd.com/T/#t
V2 of kernel patch:
https://lore.kernel.org/lkml/20230630073448.842767-1-Jiqian.Chen@amd.com/T/#t


v1:
Hi all,
I am working to implement virtgpu S3 function on Xen.

Currently on Xen, if we start a guest who enables virtgpu, and then run
"echo mem > /sys/power/state" to suspend guest. And run "sudo xl trigger <guest id> s3resume"
to resume guest. We can find that the guest kernel comes back, but the display doesn't.
It just shown a black screen.

Through reading codes, I founded that when guest was during suspending, it called into Qemu
to call virtio_gpu_gl_reset. In virtio_gpu_gl_reset, it destroyed all resources and reset
renderer. This made the display gone after guest resumed.

I think we should keep resources or prevent they being destroyed when guest is suspending.
So, I add a new status named freezing to virtgpu, and add a new ctrl message
VIRTIO_GPU_CMD_STATUS_FREEZING to get notification from guest. If freezing is set to true,
and then Qemu will realize that guest is suspending, it will not destroy resources and will
not reset renderer. If freezing is set to false, Qemu will do its origin actions, and has no
other impaction.

And now, display can come back and applications can continue their status after guest resumes.
Link:
https://lore.kernel.org/qemu-devel/20230608025655.1674357-1-Jiqian.Chen@amd.com/
V1 of kernel patch:
https://lore.kernel.org/lkml/20230608063857.1677973-1-Jiqian.Chen@amd.com/


Jiqian Chen (2):
  virtio-pci: only reset pm state during resetting
  virtio-pci: implement No_Soft_Reset bit

 hw/virtio/virtio-pci.c         | 37 +++++++++++++++++++++++++++++++++-
 include/hw/virtio/virtio-pci.h |  5 +++++
 2 files changed, 41 insertions(+), 1 deletion(-)

-- 
2.34.1



^ permalink raw reply	[flat|nested] 22+ messages in thread

* [RFC QEMU PATCH v8 1/2] virtio-pci: only reset pm state during resetting
  2024-03-28 10:39 [RFC QEMU PATCH v8 0/1] S3 support Jiqian Chen
@ 2024-03-28 10:39 ` Jiqian Chen
  2024-03-28 10:39 ` [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit Jiqian Chen
  1 sibling, 0 replies; 22+ messages in thread
From: Jiqian Chen @ 2024-03-28 10:39 UTC (permalink / raw)
  To: Michael S . Tsirkin, qemu-devel; +Cc: Huang Rui, Jiqian Chen

Fix bug imported by 27ce0f3afc9dd25d21b43bbce505157afd93d111
(fix Power Management Control Register for PCI Express virtio devices)

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

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 eaaf86402cfa..05dd03758d9f 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -2448,10 +2448,16 @@ static void virtio_pci_bus_reset_hold(Object *obj)
     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



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-03-28 10:39 [RFC QEMU PATCH v8 0/1] S3 support Jiqian Chen
  2024-03-28 10:39 ` [RFC QEMU PATCH v8 1/2] virtio-pci: only reset pm state during resetting Jiqian Chen
@ 2024-03-28 10:39 ` Jiqian Chen
  2024-03-28 10:57   ` Michael S. Tsirkin
  1 sibling, 1 reply; 22+ messages in thread
From: Jiqian Chen @ 2024-03-28 10:39 UTC (permalink / raw)
  To: Michael S . Tsirkin, qemu-devel; +Cc: Huang Rui, Jiqian Chen

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         | 29 +++++++++++++++++++++++++++++
 include/hw/virtio/virtio-pci.h |  5 +++++
 2 files changed, 34 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 05dd03758d9f..8d9fab855c7d 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -2378,6 +2378,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,
@@ -2440,11 +2445,33 @@ 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);
 
+    if (virtio_pci_no_soft_reset(dev)) {
+        return;
+    }
+
     virtio_pci_reset(qdev);
 
     if (pci_is_express(dev)) {
@@ -2484,6 +2511,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 4d57a9c75130..c758eb738234 100644
--- a/include/hw/virtio/virtio-pci.h
+++ b/include/hw/virtio/virtio-pci.h
@@ -44,6 +44,7 @@ enum {
     VIRTIO_PCI_FLAG_AER_BIT,
     VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT,
     VIRTIO_PCI_FLAG_VDPA_BIT,
+    VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT,
 };
 
 /* Need to activate work-arounds for buggy guests at vmstate load. */
@@ -80,6 +81,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



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-03-28 10:39 ` [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit Jiqian Chen
@ 2024-03-28 10:57   ` Michael S. Tsirkin
  2024-03-28 11:08     ` Chen, Jiqian
  0 siblings, 1 reply; 22+ messages in thread
From: Michael S. Tsirkin @ 2024-03-28 10:57 UTC (permalink / raw)
  To: Jiqian Chen; +Cc: qemu-devel, Huang Rui

On Thu, Mar 28, 2024 at 06:39:03PM +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         | 29 +++++++++++++++++++++++++++++
>  include/hw/virtio/virtio-pci.h |  5 +++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 05dd03758d9f..8d9fab855c7d 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2378,6 +2378,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,
> @@ -2440,11 +2445,33 @@ 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);


No need for () around return value.

> +}
> +
>  static void virtio_pci_bus_reset_hold(Object *obj)
>  {
>      PCIDevice *dev = PCI_DEVICE(obj);
>      DeviceState *qdev = DEVICE(obj);
>  
> +    if (virtio_pci_no_soft_reset(dev)) {
> +        return;
> +    }
> +
>      virtio_pci_reset(qdev);
>  
>      if (pci_is_express(dev)) {
> @@ -2484,6 +2511,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,

I am a bit confused about this part.
Do you want to make this software controllable?
Or should this be set to true by default and then
changed to false for old machine types?


> diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
> index 4d57a9c75130..c758eb738234 100644
> --- a/include/hw/virtio/virtio-pci.h
> +++ b/include/hw/virtio/virtio-pci.h
> @@ -44,6 +44,7 @@ enum {
>      VIRTIO_PCI_FLAG_AER_BIT,
>      VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT,
>      VIRTIO_PCI_FLAG_VDPA_BIT,
> +    VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT,
>  };
>  
>  /* Need to activate work-arounds for buggy guests at vmstate load. */
> @@ -80,6 +81,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



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-03-28 10:57   ` Michael S. Tsirkin
@ 2024-03-28 11:08     ` Chen, Jiqian
  2024-03-28 12:36       ` Michael S. Tsirkin
  0 siblings, 1 reply; 22+ messages in thread
From: Chen, Jiqian @ 2024-03-28 11:08 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: qemu-devel@nongnu.org, Huang, Ray, Chen, Jiqian

On 2024/3/28 18:57, Michael S. Tsirkin wrote:
> On Thu, Mar 28, 2024 at 06:39:03PM +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         | 29 +++++++++++++++++++++++++++++
>>  include/hw/virtio/virtio-pci.h |  5 +++++
>>  2 files changed, 34 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 05dd03758d9f..8d9fab855c7d 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -2378,6 +2378,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,
>> @@ -2440,11 +2445,33 @@ 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);
> 
> 
> No need for () around return value.
Ok, will delete in next version.

> 
>> +}
>> +
>>  static void virtio_pci_bus_reset_hold(Object *obj)
>>  {
>>      PCIDevice *dev = PCI_DEVICE(obj);
>>      DeviceState *qdev = DEVICE(obj);
>>  
>> +    if (virtio_pci_no_soft_reset(dev)) {
>> +        return;
>> +    }
>> +
>>      virtio_pci_reset(qdev);
>>  
>>      if (pci_is_express(dev)) {
>> @@ -2484,6 +2511,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,
> 
> I am a bit confused about this part.
> Do you want to make this software controllable?
Yes, because even the real hardware, this bit is not always set.

> Or should this be set to true by default and then
> changed to false for old machine types?
How can I do so?
Do you mean set this to true by default, and if old machine types don't need this bit, they can pass false config to qemu when running qemu?

> 
> 
>> diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
>> index 4d57a9c75130..c758eb738234 100644
>> --- a/include/hw/virtio/virtio-pci.h
>> +++ b/include/hw/virtio/virtio-pci.h
>> @@ -44,6 +44,7 @@ enum {
>>      VIRTIO_PCI_FLAG_AER_BIT,
>>      VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT,
>>      VIRTIO_PCI_FLAG_VDPA_BIT,
>> +    VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT,
>>  };
>>  
>>  /* Need to activate work-arounds for buggy guests at vmstate load. */
>> @@ -80,6 +81,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
> 

-- 
Best regards,
Jiqian Chen.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-03-28 11:08     ` Chen, Jiqian
@ 2024-03-28 12:36       ` Michael S. Tsirkin
  2024-03-29  7:00         ` Chen, Jiqian
  0 siblings, 1 reply; 22+ messages in thread
From: Michael S. Tsirkin @ 2024-03-28 12:36 UTC (permalink / raw)
  To: Chen, Jiqian; +Cc: qemu-devel@nongnu.org, Huang, Ray

On Thu, Mar 28, 2024 at 11:08:58AM +0000, Chen, Jiqian wrote:
> On 2024/3/28 18:57, Michael S. Tsirkin wrote:
> > On Thu, Mar 28, 2024 at 06:39:03PM +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         | 29 +++++++++++++++++++++++++++++
> >>  include/hw/virtio/virtio-pci.h |  5 +++++
> >>  2 files changed, 34 insertions(+)
> >>
> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >> index 05dd03758d9f..8d9fab855c7d 100644
> >> --- a/hw/virtio/virtio-pci.c
> >> +++ b/hw/virtio/virtio-pci.c
> >> @@ -2378,6 +2378,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,
> >> @@ -2440,11 +2445,33 @@ 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);
> > 
> > 
> > No need for () around return value.
> Ok, will delete in next version.
> 
> > 
> >> +}
> >> +
> >>  static void virtio_pci_bus_reset_hold(Object *obj)
> >>  {
> >>      PCIDevice *dev = PCI_DEVICE(obj);
> >>      DeviceState *qdev = DEVICE(obj);
> >>  
> >> +    if (virtio_pci_no_soft_reset(dev)) {
> >> +        return;
> >> +    }
> >> +
> >>      virtio_pci_reset(qdev);
> >>  
> >>      if (pci_is_express(dev)) {
> >> @@ -2484,6 +2511,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,
> > 
> > I am a bit confused about this part.
> > Do you want to make this software controllable?
> Yes, because even the real hardware, this bit is not always set.

So which virtio devices should and which should not set this bit?

> > Or should this be set to true by default and then
> > changed to false for old machine types?
> How can I do so?
> Do you mean set this to true by default, and if old machine types don't need this bit, they can pass false config to qemu when running qemu?

No, you would use compat machinery. See how is x-pcie-flr-init handled.


> > 
> > 
> >> diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
> >> index 4d57a9c75130..c758eb738234 100644
> >> --- a/include/hw/virtio/virtio-pci.h
> >> +++ b/include/hw/virtio/virtio-pci.h
> >> @@ -44,6 +44,7 @@ enum {
> >>      VIRTIO_PCI_FLAG_AER_BIT,
> >>      VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT,
> >>      VIRTIO_PCI_FLAG_VDPA_BIT,
> >> +    VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT,
> >>  };
> >>  
> >>  /* Need to activate work-arounds for buggy guests at vmstate load. */
> >> @@ -80,6 +81,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
> > 
> 
> -- 
> Best regards,
> Jiqian Chen.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-03-28 12:36       ` Michael S. Tsirkin
@ 2024-03-29  7:00         ` Chen, Jiqian
  2024-03-29  7:20           ` Jason Wang
  0 siblings, 1 reply; 22+ messages in thread
From: Chen, Jiqian @ 2024-03-29  7:00 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: qemu-devel@nongnu.org, Huang, Ray, Chen, Jiqian

On 2024/3/28 20:36, Michael S. Tsirkin wrote:
>>>> +}
>>>> +
>>>>  static void virtio_pci_bus_reset_hold(Object *obj)
>>>>  {
>>>>      PCIDevice *dev = PCI_DEVICE(obj);
>>>>      DeviceState *qdev = DEVICE(obj);
>>>>  
>>>> +    if (virtio_pci_no_soft_reset(dev)) {
>>>> +        return;
>>>> +    }
>>>> +
>>>>      virtio_pci_reset(qdev);
>>>>  
>>>>      if (pci_is_express(dev)) {
>>>> @@ -2484,6 +2511,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,
>>>
>>> I am a bit confused about this part.
>>> Do you want to make this software controllable?
>> Yes, because even the real hardware, this bit is not always set.
> 
> So which virtio devices should and which should not set this bit?
This depends on the scenario the virtio-device is used, if we want to trigger an internal soft reset for the virtio-device during S3, this bit shouldn't be set.
In my use case on my environment, I don't want to reset virtio-gpu during S3,
because once the display resources are destroyed, there are not enough information to re-create them, so this bit should be set.
Making this bit software controllable is convenient for users to take their own choices.

> 
>>> Or should this be set to true by default and then
>>> changed to false for old machine types?
>> How can I do so?
>> Do you mean set this to true by default, and if old machine types don't need this bit, they can pass false config to qemu when running qemu?
> 
> No, you would use compat machinery. See how is x-pcie-flr-init handled.
> 
> 

-- 
Best regards,
Jiqian Chen.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-03-29  7:00         ` Chen, Jiqian
@ 2024-03-29  7:20           ` Jason Wang
  2024-03-29  8:00             ` Chen, Jiqian
  2024-03-29 10:44             ` Michael S. Tsirkin
  0 siblings, 2 replies; 22+ messages in thread
From: Jason Wang @ 2024-03-29  7:20 UTC (permalink / raw)
  To: Chen, Jiqian; +Cc: Michael S. Tsirkin, qemu-devel@nongnu.org, Huang, Ray

On Fri, Mar 29, 2024 at 3:07 PM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
>
> On 2024/3/28 20:36, Michael S. Tsirkin wrote:
> >>>> +}
> >>>> +
> >>>>  static void virtio_pci_bus_reset_hold(Object *obj)
> >>>>  {
> >>>>      PCIDevice *dev = PCI_DEVICE(obj);
> >>>>      DeviceState *qdev = DEVICE(obj);
> >>>>
> >>>> +    if (virtio_pci_no_soft_reset(dev)) {
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>>      virtio_pci_reset(qdev);
> >>>>
> >>>>      if (pci_is_express(dev)) {
> >>>> @@ -2484,6 +2511,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),

Why does it come with an x prefix?

> >>>>      DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
> >>>>                      VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
> >>>>      DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
> >>>
> >>> I am a bit confused about this part.
> >>> Do you want to make this software controllable?
> >> Yes, because even the real hardware, this bit is not always set.

We are talking about emulated devices here.

> >
> > So which virtio devices should and which should not set this bit?
> This depends on the scenario the virtio-device is used, if we want to trigger an internal soft reset for the virtio-device during S3, this bit shouldn't be set.

If the device doesn't need reset, why bother the driver for this?

Btw, no_soft_reset is insufficient for some cases, there's a proposal
for the virtio-spec. I think we need to wait until it is done.

> In my use case on my environment, I don't want to reset virtio-gpu during S3,
> because once the display resources are destroyed, there are not enough information to re-create them, so this bit should be set.
> Making this bit software controllable is convenient for users to take their own choices.

Thanks

>
> >
> >>> Or should this be set to true by default and then
> >>> changed to false for old machine types?
> >> How can I do so?
> >> Do you mean set this to true by default, and if old machine types don't need this bit, they can pass false config to qemu when running qemu?
> >
> > No, you would use compat machinery. See how is x-pcie-flr-init handled.
> >
> >
>
> --
> Best regards,
> Jiqian Chen.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-03-29  7:20           ` Jason Wang
@ 2024-03-29  8:00             ` Chen, Jiqian
  2024-03-29 10:38               ` Jason Wang
  2024-03-29 10:44             ` Michael S. Tsirkin
  1 sibling, 1 reply; 22+ messages in thread
From: Chen, Jiqian @ 2024-03-29  8:00 UTC (permalink / raw)
  To: Jason Wang
  Cc: Michael S. Tsirkin, qemu-devel@nongnu.org, Huang, Ray,
	Chen, Jiqian

On 2024/3/29 15:20, Jason Wang wrote:
> On Fri, Mar 29, 2024 at 3:07 PM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
>>
>> On 2024/3/28 20:36, Michael S. Tsirkin wrote:
>>>>>> +}
>>>>>> +
>>>>>>  static void virtio_pci_bus_reset_hold(Object *obj)
>>>>>>  {
>>>>>>      PCIDevice *dev = PCI_DEVICE(obj);
>>>>>>      DeviceState *qdev = DEVICE(obj);
>>>>>>
>>>>>> +    if (virtio_pci_no_soft_reset(dev)) {
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>>      virtio_pci_reset(qdev);
>>>>>>
>>>>>>      if (pci_is_express(dev)) {
>>>>>> @@ -2484,6 +2511,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),
> 
> Why does it come with an x prefix?
Sorry, it's my misunderstanding of this prefix, if No_Soft_Reset doesn't need this prefix, I will delete it in next version.
Does x prefix means compat machinery? Or other meanings?

> 
>>>>>>      DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
>>>>>>                      VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
>>>>>>      DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
>>>>>
>>>>> I am a bit confused about this part.
>>>>> Do you want to make this software controllable?
>>>> Yes, because even the real hardware, this bit is not always set.
> 
> We are talking about emulated devices here.
Yes, I just gave an example. It actually this bit is not always set. What's your opinion about when to set this bit or which virtio-device should set this bit?

> 
>>>
>>> So which virtio devices should and which should not set this bit?
>> This depends on the scenario the virtio-device is used, if we want to trigger an internal soft reset for the virtio-device during S3, this bit shouldn't be set.
> 
> If the device doesn't need reset, why bother the driver for this?
I don't know what you mean.
If the device doesn't need reset, we can config true to set this bit, then on the driver side, driver finds this bit is set, then driver will not trigger a soft reset.

> 
> Btw, no_soft_reset is insufficient for some cases, 
May I know which cases?

> there's a proposal for the virtio-spec. I think we need to wait until it is done.
Can you share the proposal?

> 
>> In my use case on my environment, I don't want to reset virtio-gpu during S3,
>> because once the display resources are destroyed, there are not enough information to re-create them, so this bit should be set.
>> Making this bit software controllable is convenient for users to take their own choices.
> 
> Thanks
> 
>>
>>>
>>>>> Or should this be set to true by default and then
>>>>> changed to false for old machine types?
>>>> How can I do so?
>>>> Do you mean set this to true by default, and if old machine types don't need this bit, they can pass false config to qemu when running qemu?
>>>
>>> No, you would use compat machinery. See how is x-pcie-flr-init handled.
>>>
>>>
>>
>> --
>> Best regards,
>> Jiqian Chen.
> 

-- 
Best regards,
Jiqian Chen.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-03-29  8:00             ` Chen, Jiqian
@ 2024-03-29 10:38               ` Jason Wang
  2024-04-02  2:56                 ` Chen, Jiqian
  0 siblings, 1 reply; 22+ messages in thread
From: Jason Wang @ 2024-03-29 10:38 UTC (permalink / raw)
  To: Chen, Jiqian; +Cc: Michael S. Tsirkin, qemu-devel@nongnu.org, Huang, Ray

On Fri, Mar 29, 2024 at 4:00 PM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
>
> On 2024/3/29 15:20, Jason Wang wrote:
> > On Fri, Mar 29, 2024 at 3:07 PM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
> >>
> >> On 2024/3/28 20:36, Michael S. Tsirkin wrote:
> >>>>>> +}
> >>>>>> +
> >>>>>>  static void virtio_pci_bus_reset_hold(Object *obj)
> >>>>>>  {
> >>>>>>      PCIDevice *dev = PCI_DEVICE(obj);
> >>>>>>      DeviceState *qdev = DEVICE(obj);
> >>>>>>
> >>>>>> +    if (virtio_pci_no_soft_reset(dev)) {
> >>>>>> +        return;
> >>>>>> +    }
> >>>>>> +
> >>>>>>      virtio_pci_reset(qdev);
> >>>>>>
> >>>>>>      if (pci_is_express(dev)) {
> >>>>>> @@ -2484,6 +2511,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),
> >
> > Why does it come with an x prefix?
> Sorry, it's my misunderstanding of this prefix, if No_Soft_Reset doesn't need this prefix, I will delete it in next version.
> Does x prefix means compat machinery? Or other meanings?
>
> >
> >>>>>>      DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
> >>>>>>                      VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
> >>>>>>      DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
> >>>>>
> >>>>> I am a bit confused about this part.
> >>>>> Do you want to make this software controllable?
> >>>> Yes, because even the real hardware, this bit is not always set.
> >
> > We are talking about emulated devices here.
> Yes, I just gave an example. It actually this bit is not always set. What's your opinion about when to set this bit or which virtio-device should set this bit?

If the implementation of Qemu is correct, we should set it unless we
need compatibility.

>
> >
> >>>
> >>> So which virtio devices should and which should not set this bit?
> >> This depends on the scenario the virtio-device is used, if we want to trigger an internal soft reset for the virtio-device during S3, this bit shouldn't be set.
> >
> > If the device doesn't need reset, why bother the driver for this?
> I don't know what you mean.
> If the device doesn't need reset, we can config true to set this bit, then on the driver side, driver finds this bit is set, then driver will not trigger a soft reset.

I mean if the device can suspend without reset, we don't need to
bother the driver to save and load states.

>
> >
> > Btw, no_soft_reset is insufficient for some cases,
> May I know which cases?
>
> > there's a proposal for the virtio-spec. I think we need to wait until it is done.
> Can you share the proposal?

See this

https://lore.kernel.org/all/20240227015345.3614965-1-stevensd@chromium.org/T/

Thanks

>
> >
> >> In my use case on my environment, I don't want to reset virtio-gpu during S3,
> >> because once the display resources are destroyed, there are not enough information to re-create them, so this bit should be set.
> >> Making this bit software controllable is convenient for users to take their own choices.
> >
> > Thanks
> >
> >>
> >>>
> >>>>> Or should this be set to true by default and then
> >>>>> changed to false for old machine types?
> >>>> How can I do so?
> >>>> Do you mean set this to true by default, and if old machine types don't need this bit, they can pass false config to qemu when running qemu?
> >>>
> >>> No, you would use compat machinery. See how is x-pcie-flr-init handled.
> >>>
> >>>
> >>
> >> --
> >> Best regards,
> >> Jiqian Chen.
> >
>
> --
> Best regards,
> Jiqian Chen.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-03-29  7:20           ` Jason Wang
  2024-03-29  8:00             ` Chen, Jiqian
@ 2024-03-29 10:44             ` Michael S. Tsirkin
  2024-04-02  3:03               ` Chen, Jiqian
  1 sibling, 1 reply; 22+ messages in thread
From: Michael S. Tsirkin @ 2024-03-29 10:44 UTC (permalink / raw)
  To: Jason Wang; +Cc: Chen, Jiqian, qemu-devel@nongnu.org, Huang, Ray

On Fri, Mar 29, 2024 at 03:20:59PM +0800, Jason Wang wrote:
> On Fri, Mar 29, 2024 at 3:07 PM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
> >
> > On 2024/3/28 20:36, Michael S. Tsirkin wrote:
> > >>>> +}
> > >>>> +
> > >>>>  static void virtio_pci_bus_reset_hold(Object *obj)
> > >>>>  {
> > >>>>      PCIDevice *dev = PCI_DEVICE(obj);
> > >>>>      DeviceState *qdev = DEVICE(obj);
> > >>>>
> > >>>> +    if (virtio_pci_no_soft_reset(dev)) {
> > >>>> +        return;
> > >>>> +    }
> > >>>> +
> > >>>>      virtio_pci_reset(qdev);
> > >>>>
> > >>>>      if (pci_is_express(dev)) {
> > >>>> @@ -2484,6 +2511,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),
> 
> Why does it come with an x prefix?
> 
> > >>>>      DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
> > >>>>                      VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
> > >>>>      DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
> > >>>
> > >>> I am a bit confused about this part.
> > >>> Do you want to make this software controllable?
> > >> Yes, because even the real hardware, this bit is not always set.
> 
> We are talking about emulated devices here.
> 
> > >
> > > So which virtio devices should and which should not set this bit?
> > This depends on the scenario the virtio-device is used, if we want to trigger an internal soft reset for the virtio-device during S3, this bit shouldn't be set.
> 
> If the device doesn't need reset, why bother the driver for this?
> 
> Btw, no_soft_reset is insufficient for some cases, there's a proposal
> for the virtio-spec. I think we need to wait until it is done.

That seems orthogonal or did I miss something?

> > In my use case on my environment, I don't want to reset virtio-gpu during S3,
> > because once the display resources are destroyed, there are not enough information to re-create them, so this bit should be set.
> > Making this bit software controllable is convenient for users to take their own choices.
> 
> Thanks
> 
> >
> > >
> > >>> Or should this be set to true by default and then
> > >>> changed to false for old machine types?
> > >> How can I do so?
> > >> Do you mean set this to true by default, and if old machine types don't need this bit, they can pass false config to qemu when running qemu?
> > >
> > > No, you would use compat machinery. See how is x-pcie-flr-init handled.
> > >
> > >
> >
> > --
> > Best regards,
> > Jiqian Chen.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-03-29 10:38               ` Jason Wang
@ 2024-04-02  2:56                 ` Chen, Jiqian
  0 siblings, 0 replies; 22+ messages in thread
From: Chen, Jiqian @ 2024-04-02  2:56 UTC (permalink / raw)
  To: Jason Wang
  Cc: Michael S. Tsirkin, qemu-devel@nongnu.org, Huang, Ray,
	Chen, Jiqian

On 2024/3/29 18:38, Jason Wang wrote:
> On Fri, Mar 29, 2024 at 4:00 PM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
>>
>> On 2024/3/29 15:20, Jason Wang wrote:
>>> On Fri, Mar 29, 2024 at 3:07 PM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
>>>>
>>>> On 2024/3/28 20:36, Michael S. Tsirkin wrote:
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>  static void virtio_pci_bus_reset_hold(Object *obj)
>>>>>>>>  {
>>>>>>>>      PCIDevice *dev = PCI_DEVICE(obj);
>>>>>>>>      DeviceState *qdev = DEVICE(obj);
>>>>>>>>
>>>>>>>> +    if (virtio_pci_no_soft_reset(dev)) {
>>>>>>>> +        return;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>>      virtio_pci_reset(qdev);
>>>>>>>>
>>>>>>>>      if (pci_is_express(dev)) {
>>>>>>>> @@ -2484,6 +2511,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),
>>>
>>> Why does it come with an x prefix?
>> Sorry, it's my misunderstanding of this prefix, if No_Soft_Reset doesn't need this prefix, I will delete it in next version.
>> Does x prefix means compat machinery? Or other meanings?
>>
>>>
>>>>>>>>      DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
>>>>>>>>                      VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
>>>>>>>>      DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
>>>>>>>
>>>>>>> I am a bit confused about this part.
>>>>>>> Do you want to make this software controllable?
>>>>>> Yes, because even the real hardware, this bit is not always set.
>>>
>>> We are talking about emulated devices here.
>> Yes, I just gave an example. It actually this bit is not always set. What's your opinion about when to set this bit or which virtio-device should set this bit?
> 
> If the implementation of Qemu is correct, we should set it unless we
> need compatibility.
Ok, I will set it default to true in next version.
If we need compatibility, what's your opinion about which machine types should we compatible with? Does the same as x-pcie-pm-init?
> 
>>
>>>
>>>>>
>>>>> So which virtio devices should and which should not set this bit?
>>>> This depends on the scenario the virtio-device is used, if we want to trigger an internal soft reset for the virtio-device during S3, this bit shouldn't be set.
>>>
>>> If the device doesn't need reset, why bother the driver for this?
>> I don't know what you mean.
>> If the device doesn't need reset, we can config true to set this bit, then on the driver side, driver finds this bit is set, then driver will not trigger a soft reset.
> 
> I mean if the device can suspend without reset, we don't need to
> bother the driver to save and load states.
> 
>>
>>>
>>> Btw, no_soft_reset is insufficient for some cases,
>> May I know which cases?
>>
>>> there's a proposal for the virtio-spec. I think we need to wait until it is done.
>> Can you share the proposal?
> 
> See this
> 
> https://lore.kernel.org/all/20240227015345.3614965-1-stevensd@chromium.org/T/
I looked the detail of this proposal.
What the proposal does is to add a new status to trigger device to suspend/resume.
But this patch is to add No_Soft_Reset bit, it decides if the device should be reset. This patch doesn't depend on the proposal.
If you mean that the proposal says "A device MUST maintain its state while suspended", I think it needs new patch to implement it after the proposal is done.

> 
> Thanks
> 
>>
>>>
>>>> In my use case on my environment, I don't want to reset virtio-gpu during S3,
>>>> because once the display resources are destroyed, there are not enough information to re-create them, so this bit should be set.
>>>> Making this bit software controllable is convenient for users to take their own choices.
>>>
>>> Thanks
>>>
>>>>
>>>>>
>>>>>>> Or should this be set to true by default and then
>>>>>>> changed to false for old machine types?
>>>>>> How can I do so?
>>>>>> Do you mean set this to true by default, and if old machine types don't need this bit, they can pass false config to qemu when running qemu?
>>>>>
>>>>> No, you would use compat machinery. See how is x-pcie-flr-init handled.
>>>>>
>>>>>
>>>>
>>>> --
>>>> Best regards,
>>>> Jiqian Chen.
>>>
>>
>> --
>> Best regards,
>> Jiqian Chen.
> 

-- 
Best regards,
Jiqian Chen.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-03-29 10:44             ` Michael S. Tsirkin
@ 2024-04-02  3:03               ` Chen, Jiqian
  2024-04-07  3:20                 ` Jason Wang
  0 siblings, 1 reply; 22+ messages in thread
From: Chen, Jiqian @ 2024-04-02  3:03 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Jason Wang, qemu-devel@nongnu.org, Huang, Ray, Chen, Jiqian

On 2024/3/29 18:44, Michael S. Tsirkin wrote:
> On Fri, Mar 29, 2024 at 03:20:59PM +0800, Jason Wang wrote:
>> On Fri, Mar 29, 2024 at 3:07 PM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
>>>
>>> On 2024/3/28 20:36, Michael S. Tsirkin wrote:
>>>>>>> +}
>>>>>>> +
>>>>>>>  static void virtio_pci_bus_reset_hold(Object *obj)
>>>>>>>  {
>>>>>>>      PCIDevice *dev = PCI_DEVICE(obj);
>>>>>>>      DeviceState *qdev = DEVICE(obj);
>>>>>>>
>>>>>>> +    if (virtio_pci_no_soft_reset(dev)) {
>>>>>>> +        return;
>>>>>>> +    }
>>>>>>> +
>>>>>>>      virtio_pci_reset(qdev);
>>>>>>>
>>>>>>>      if (pci_is_express(dev)) {
>>>>>>> @@ -2484,6 +2511,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),
>>
>> Why does it come with an x prefix?
>>
>>>>>>>      DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
>>>>>>>                      VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
>>>>>>>      DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
>>>>>>
>>>>>> I am a bit confused about this part.
>>>>>> Do you want to make this software controllable?
>>>>> Yes, because even the real hardware, this bit is not always set.
>>
>> We are talking about emulated devices here.
>>
>>>>
>>>> So which virtio devices should and which should not set this bit?
>>> This depends on the scenario the virtio-device is used, if we want to trigger an internal soft reset for the virtio-device during S3, this bit shouldn't be set.
>>
>> If the device doesn't need reset, why bother the driver for this?
>>
>> Btw, no_soft_reset is insufficient for some cases, there's a proposal
>> for the virtio-spec. I think we need to wait until it is done.
> 
> That seems orthogonal or did I miss something?
Yes, I looked the detail of the proposal, I also think they are unrelated.
I will set the default value of No_Soft_Reset bit to true in next version according to your opinion.
About the compatibility of old machine types, which types should I consider? Does the same as x-pcie-pm-init(hw_compat_2_8)?
Forgive me for not knowing much about compatibility.
> 
>>> In my use case on my environment, I don't want to reset virtio-gpu during S3,
>>> because once the display resources are destroyed, there are not enough information to re-create them, so this bit should be set.
>>> Making this bit software controllable is convenient for users to take their own choices.
>>
>> Thanks
>>
>>>
>>>>
>>>>>> Or should this be set to true by default and then
>>>>>> changed to false for old machine types?
>>>>> How can I do so?
>>>>> Do you mean set this to true by default, and if old machine types don't need this bit, they can pass false config to qemu when running qemu?
>>>>
>>>> No, you would use compat machinery. See how is x-pcie-flr-init handled.
>>>>
>>>>
>>>
>>> --
>>> Best regards,
>>> Jiqian Chen.
> 

-- 
Best regards,
Jiqian Chen.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-04-02  3:03               ` Chen, Jiqian
@ 2024-04-07  3:20                 ` Jason Wang
  2024-04-07 11:49                   ` Michael S. Tsirkin
  2024-04-12  5:59                   ` Chen, Jiqian
  0 siblings, 2 replies; 22+ messages in thread
From: Jason Wang @ 2024-04-07  3:20 UTC (permalink / raw)
  To: Chen, Jiqian; +Cc: Michael S. Tsirkin, qemu-devel@nongnu.org, Huang, Ray

On Tue, Apr 2, 2024 at 11:03 AM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
>
> On 2024/3/29 18:44, Michael S. Tsirkin wrote:
> > On Fri, Mar 29, 2024 at 03:20:59PM +0800, Jason Wang wrote:
> >> On Fri, Mar 29, 2024 at 3:07 PM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
> >>>
> >>> On 2024/3/28 20:36, Michael S. Tsirkin wrote:
> >>>>>>> +}
> >>>>>>> +
> >>>>>>>  static void virtio_pci_bus_reset_hold(Object *obj)
> >>>>>>>  {
> >>>>>>>      PCIDevice *dev = PCI_DEVICE(obj);
> >>>>>>>      DeviceState *qdev = DEVICE(obj);
> >>>>>>>
> >>>>>>> +    if (virtio_pci_no_soft_reset(dev)) {
> >>>>>>> +        return;
> >>>>>>> +    }
> >>>>>>> +
> >>>>>>>      virtio_pci_reset(qdev);
> >>>>>>>
> >>>>>>>      if (pci_is_express(dev)) {
> >>>>>>> @@ -2484,6 +2511,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),
> >>
> >> Why does it come with an x prefix?
> >>
> >>>>>>>      DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
> >>>>>>>                      VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
> >>>>>>>      DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
> >>>>>>
> >>>>>> I am a bit confused about this part.
> >>>>>> Do you want to make this software controllable?
> >>>>> Yes, because even the real hardware, this bit is not always set.
> >>
> >> We are talking about emulated devices here.
> >>
> >>>>
> >>>> So which virtio devices should and which should not set this bit?
> >>> This depends on the scenario the virtio-device is used, if we want to trigger an internal soft reset for the virtio-device during S3, this bit shouldn't be set.
> >>
> >> If the device doesn't need reset, why bother the driver for this?
> >>
> >> Btw, no_soft_reset is insufficient for some cases, there's a proposal
> >> for the virtio-spec. I think we need to wait until it is done.
> >
> > That seems orthogonal or did I miss something?
> Yes, I looked the detail of the proposal, I also think they are unrelated.

The point is the proposal said

"""
Without a mechanism to
suspend/resume virtio devices when the driver is suspended/resumed in
the early phase of suspend/late phase of resume, there is a window where
interrupts can be lost.
"""

It looks safe to enable it with the suspend bit. Or if you think it's
wrong, please comment on the virtio spec patch.

> I will set the default value of No_Soft_Reset bit to true in next version according to your opinion.
> About the compatibility of old machine types, which types should I consider? Does the same as x-pcie-pm-init(hw_compat_2_8)?
> Forgive me for not knowing much about compatibility.

"x" means no compatibility at all, please drop the "x" prefix. And it
looks more safe to start as "false" by default.

Thanks

> >
> >>> In my use case on my environment, I don't want to reset virtio-gpu during S3,
> >>> because once the display resources are destroyed, there are not enough information to re-create them, so this bit should be set.
> >>> Making this bit software controllable is convenient for users to take their own choices.
> >>
> >> Thanks
> >>
> >>>
> >>>>
> >>>>>> Or should this be set to true by default and then
> >>>>>> changed to false for old machine types?
> >>>>> How can I do so?
> >>>>> Do you mean set this to true by default, and if old machine types don't need this bit, they can pass false config to qemu when running qemu?
> >>>>
> >>>> No, you would use compat machinery. See how is x-pcie-flr-init handled.
> >>>>
> >>>>
> >>>
> >>> --
> >>> Best regards,
> >>> Jiqian Chen.
> >
>
> --
> Best regards,
> Jiqian Chen.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-04-07  3:20                 ` Jason Wang
@ 2024-04-07 11:49                   ` Michael S. Tsirkin
  2024-04-08  4:56                     ` Jason Wang
  2024-04-12  6:04                     ` Chen, Jiqian
  2024-04-12  5:59                   ` Chen, Jiqian
  1 sibling, 2 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2024-04-07 11:49 UTC (permalink / raw)
  To: Jason Wang; +Cc: Chen, Jiqian, qemu-devel@nongnu.org, Huang, Ray

On Sun, Apr 07, 2024 at 11:20:57AM +0800, Jason Wang wrote:
> On Tue, Apr 2, 2024 at 11:03 AM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
> >
> > On 2024/3/29 18:44, Michael S. Tsirkin wrote:
> > > On Fri, Mar 29, 2024 at 03:20:59PM +0800, Jason Wang wrote:
> > >> On Fri, Mar 29, 2024 at 3:07 PM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
> > >>>
> > >>> On 2024/3/28 20:36, Michael S. Tsirkin wrote:
> > >>>>>>> +}
> > >>>>>>> +
> > >>>>>>>  static void virtio_pci_bus_reset_hold(Object *obj)
> > >>>>>>>  {
> > >>>>>>>      PCIDevice *dev = PCI_DEVICE(obj);
> > >>>>>>>      DeviceState *qdev = DEVICE(obj);
> > >>>>>>>
> > >>>>>>> +    if (virtio_pci_no_soft_reset(dev)) {
> > >>>>>>> +        return;
> > >>>>>>> +    }
> > >>>>>>> +
> > >>>>>>>      virtio_pci_reset(qdev);
> > >>>>>>>
> > >>>>>>>      if (pci_is_express(dev)) {
> > >>>>>>> @@ -2484,6 +2511,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),
> > >>
> > >> Why does it come with an x prefix?
> > >>
> > >>>>>>>      DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
> > >>>>>>>                      VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
> > >>>>>>>      DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
> > >>>>>>
> > >>>>>> I am a bit confused about this part.
> > >>>>>> Do you want to make this software controllable?
> > >>>>> Yes, because even the real hardware, this bit is not always set.
> > >>
> > >> We are talking about emulated devices here.
> > >>
> > >>>>
> > >>>> So which virtio devices should and which should not set this bit?
> > >>> This depends on the scenario the virtio-device is used, if we want to trigger an internal soft reset for the virtio-device during S3, this bit shouldn't be set.
> > >>
> > >> If the device doesn't need reset, why bother the driver for this?
> > >>
> > >> Btw, no_soft_reset is insufficient for some cases, there's a proposal
> > >> for the virtio-spec. I think we need to wait until it is done.
> > >
> > > That seems orthogonal or did I miss something?
> > Yes, I looked the detail of the proposal, I also think they are unrelated.
> 
> The point is the proposal said
> 
> """
> Without a mechanism to
> suspend/resume virtio devices when the driver is suspended/resumed in
> the early phase of suspend/late phase of resume, there is a window where
> interrupts can be lost.
> """
> 
> It looks safe to enable it with the suspend bit. Or if you think it's
> wrong, please comment on the virtio spec patch.
> 
> > I will set the default value of No_Soft_Reset bit to true in next version according to your opinion.
> > About the compatibility of old machine types, which types should I consider? Does the same as x-pcie-pm-init(hw_compat_2_8)?
> > Forgive me for not knowing much about compatibility.
> 
> "x" means no compatibility at all, please drop the "x" prefix. And it
> looks more safe to start as "false" by default.
> 
> Thanks


Not sure I agree. External flags are for when users want to tweak them.
When would users want it to be off?
What is done here is I feel sane, just add machine compat machinery
to change to off for old machine types.


> > >
> > >>> In my use case on my environment, I don't want to reset virtio-gpu during S3,
> > >>> because once the display resources are destroyed, there are not enough information to re-create them, so this bit should be set.
> > >>> Making this bit software controllable is convenient for users to take their own choices.
> > >>
> > >> Thanks
> > >>
> > >>>
> > >>>>
> > >>>>>> Or should this be set to true by default and then
> > >>>>>> changed to false for old machine types?
> > >>>>> How can I do so?
> > >>>>> Do you mean set this to true by default, and if old machine types don't need this bit, they can pass false config to qemu when running qemu?
> > >>>>
> > >>>> No, you would use compat machinery. See how is x-pcie-flr-init handled.
> > >>>>
> > >>>>
> > >>>
> > >>> --
> > >>> Best regards,
> > >>> Jiqian Chen.
> > >
> >
> > --
> > Best regards,
> > Jiqian Chen.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-04-07 11:49                   ` Michael S. Tsirkin
@ 2024-04-08  4:56                     ` Jason Wang
  2024-04-12  6:10                       ` Chen, Jiqian
  2024-04-12  6:04                     ` Chen, Jiqian
  1 sibling, 1 reply; 22+ messages in thread
From: Jason Wang @ 2024-04-08  4:56 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Chen, Jiqian, qemu-devel@nongnu.org, Huang, Ray

On Sun, Apr 7, 2024 at 7:50 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sun, Apr 07, 2024 at 11:20:57AM +0800, Jason Wang wrote:
> > On Tue, Apr 2, 2024 at 11:03 AM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
> > >
> > > On 2024/3/29 18:44, Michael S. Tsirkin wrote:
> > > > On Fri, Mar 29, 2024 at 03:20:59PM +0800, Jason Wang wrote:
> > > >> On Fri, Mar 29, 2024 at 3:07 PM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
> > > >>>
> > > >>> On 2024/3/28 20:36, Michael S. Tsirkin wrote:
> > > >>>>>>> +}
> > > >>>>>>> +
> > > >>>>>>>  static void virtio_pci_bus_reset_hold(Object *obj)
> > > >>>>>>>  {
> > > >>>>>>>      PCIDevice *dev = PCI_DEVICE(obj);
> > > >>>>>>>      DeviceState *qdev = DEVICE(obj);
> > > >>>>>>>
> > > >>>>>>> +    if (virtio_pci_no_soft_reset(dev)) {
> > > >>>>>>> +        return;
> > > >>>>>>> +    }
> > > >>>>>>> +
> > > >>>>>>>      virtio_pci_reset(qdev);
> > > >>>>>>>
> > > >>>>>>>      if (pci_is_express(dev)) {
> > > >>>>>>> @@ -2484,6 +2511,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),
> > > >>
> > > >> Why does it come with an x prefix?
> > > >>
> > > >>>>>>>      DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
> > > >>>>>>>                      VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
> > > >>>>>>>      DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
> > > >>>>>>
> > > >>>>>> I am a bit confused about this part.
> > > >>>>>> Do you want to make this software controllable?
> > > >>>>> Yes, because even the real hardware, this bit is not always set.
> > > >>
> > > >> We are talking about emulated devices here.
> > > >>
> > > >>>>
> > > >>>> So which virtio devices should and which should not set this bit?
> > > >>> This depends on the scenario the virtio-device is used, if we want to trigger an internal soft reset for the virtio-device during S3, this bit shouldn't be set.
> > > >>
> > > >> If the device doesn't need reset, why bother the driver for this?
> > > >>
> > > >> Btw, no_soft_reset is insufficient for some cases, there's a proposal
> > > >> for the virtio-spec. I think we need to wait until it is done.
> > > >
> > > > That seems orthogonal or did I miss something?
> > > Yes, I looked the detail of the proposal, I also think they are unrelated.
> >
> > The point is the proposal said
> >
> > """
> > Without a mechanism to
> > suspend/resume virtio devices when the driver is suspended/resumed in
> > the early phase of suspend/late phase of resume, there is a window where
> > interrupts can be lost.
> > """
> >
> > It looks safe to enable it with the suspend bit. Or if you think it's
> > wrong, please comment on the virtio spec patch.
> >
> > > I will set the default value of No_Soft_Reset bit to true in next version according to your opinion.
> > > About the compatibility of old machine types, which types should I consider? Does the same as x-pcie-pm-init(hw_compat_2_8)?
> > > Forgive me for not knowing much about compatibility.
> >
> > "x" means no compatibility at all, please drop the "x" prefix. And it
> > looks more safe to start as "false" by default.
> >
> > Thanks
>
>
> Not sure I agree. External flags are for when users want to tweak them.
> When would users want it to be off?

If I understand the suspending status proposal correctly, there are at
least 1 device that is not safe. And this series does not mention
which device it has tested.

It means if we enable it unconditionally, guests may break.

Thanks

> What is done here is I feel sane, just add machine compat machinery
> to change to off for old machine types.
>
>
> > > >
> > > >>> In my use case on my environment, I don't want to reset virtio-gpu during S3,
> > > >>> because once the display resources are destroyed, there are not enough information to re-create them, so this bit should be set.
> > > >>> Making this bit software controllable is convenient for users to take their own choices.
> > > >>
> > > >> Thanks
> > > >>
> > > >>>
> > > >>>>
> > > >>>>>> Or should this be set to true by default and then
> > > >>>>>> changed to false for old machine types?
> > > >>>>> How can I do so?
> > > >>>>> Do you mean set this to true by default, and if old machine types don't need this bit, they can pass false config to qemu when running qemu?
> > > >>>>
> > > >>>> No, you would use compat machinery. See how is x-pcie-flr-init handled.
> > > >>>>
> > > >>>>
> > > >>>
> > > >>> --
> > > >>> Best regards,
> > > >>> Jiqian Chen.
> > > >
> > >
> > > --
> > > Best regards,
> > > Jiqian Chen.
>



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-04-07  3:20                 ` Jason Wang
  2024-04-07 11:49                   ` Michael S. Tsirkin
@ 2024-04-12  5:59                   ` Chen, Jiqian
  2024-04-12  6:42                     ` Jason Wang
  1 sibling, 1 reply; 22+ messages in thread
From: Chen, Jiqian @ 2024-04-12  5:59 UTC (permalink / raw)
  To: Jason Wang
  Cc: Michael S. Tsirkin, qemu-devel@nongnu.org, Huang, Ray,
	Chen, Jiqian

On 2024/4/7 11:20, Jason Wang wrote:
> On Tue, Apr 2, 2024 at 11:03 AM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
>>
>> On 2024/3/29 18:44, Michael S. Tsirkin wrote:
>>> On Fri, Mar 29, 2024 at 03:20:59PM +0800, Jason Wang wrote:
>>>> On Fri, Mar 29, 2024 at 3:07 PM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
>>>>>
>>>>> On 2024/3/28 20:36, Michael S. Tsirkin wrote:
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>  static void virtio_pci_bus_reset_hold(Object *obj)
>>>>>>>>>  {
>>>>>>>>>      PCIDevice *dev = PCI_DEVICE(obj);
>>>>>>>>>      DeviceState *qdev = DEVICE(obj);
>>>>>>>>>
>>>>>>>>> +    if (virtio_pci_no_soft_reset(dev)) {
>>>>>>>>> +        return;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>>      virtio_pci_reset(qdev);
>>>>>>>>>
>>>>>>>>>      if (pci_is_express(dev)) {
>>>>>>>>> @@ -2484,6 +2511,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),
>>>>
>>>> Why does it come with an x prefix?
>>>>
>>>>>>>>>      DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
>>>>>>>>>                      VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
>>>>>>>>>      DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
>>>>>>>>
>>>>>>>> I am a bit confused about this part.
>>>>>>>> Do you want to make this software controllable?
>>>>>>> Yes, because even the real hardware, this bit is not always set.
>>>>
>>>> We are talking about emulated devices here.
>>>>
>>>>>>
>>>>>> So which virtio devices should and which should not set this bit?
>>>>> This depends on the scenario the virtio-device is used, if we want to trigger an internal soft reset for the virtio-device during S3, this bit shouldn't be set.
>>>>
>>>> If the device doesn't need reset, why bother the driver for this?
>>>>
>>>> Btw, no_soft_reset is insufficient for some cases, there's a proposal
>>>> for the virtio-spec. I think we need to wait until it is done.
>>>
>>> That seems orthogonal or did I miss something?
>> Yes, I looked the detail of the proposal, I also think they are unrelated.
> 
> The point is the proposal said
> 
> """
> Without a mechanism to
> suspend/resume virtio devices when the driver is suspended/resumed in
> the early phase of suspend/late phase of resume, there is a window where
> interrupts can be lost.
> """
> 
> It looks safe to enable it with the suspend bit. Or if you think it's
> wrong, please comment on the virtio spec patch.
If I understand the proposal correctly.
Only need to check the SUSPEND bit when virtio_pci_bus_reset_hold is called.
It seems the proposal won't block this patch to upstream.
In next version, I will add comments to note the SUSPEND bit that need to be considered once it is accepted.

> 
>> I will set the default value of No_Soft_Reset bit to true in next version according to your opinion.
>> About the compatibility of old machine types, which types should I consider? Does the same as x-pcie-pm-init(hw_compat_2_8)?
>> Forgive me for not knowing much about compatibility.
> 
> "x" means no compatibility at all, please drop the "x" prefix. And it
Thanks to explain.
So it seems the prefix "x" of "x-pcie-pm-init" is also wrong? Because it considered the hw_compat_2_8. Also "x-pcie-flr-init".
Back to No_Soft_Reset, do you know which old machines should I consider to compatible with?

> looks more safe to start as "false" by default.
> 
> Thanks
> 
>>>
>>>>> In my use case on my environment, I don't want to reset virtio-gpu during S3,
>>>>> because once the display resources are destroyed, there are not enough information to re-create them, so this bit should be set.
>>>>> Making this bit software controllable is convenient for users to take their own choices.
>>>>
>>>> Thanks
>>>>
>>>>>
>>>>>>
>>>>>>>> Or should this be set to true by default and then
>>>>>>>> changed to false for old machine types?
>>>>>>> How can I do so?
>>>>>>> Do you mean set this to true by default, and if old machine types don't need this bit, they can pass false config to qemu when running qemu?
>>>>>>
>>>>>> No, you would use compat machinery. See how is x-pcie-flr-init handled.
>>>>>>
>>>>>>
>>>>>
>>>>> --
>>>>> Best regards,
>>>>> Jiqian Chen.
>>>
>>
>> --
>> Best regards,
>> Jiqian Chen.
> 

-- 
Best regards,
Jiqian Chen.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-04-07 11:49                   ` Michael S. Tsirkin
  2024-04-08  4:56                     ` Jason Wang
@ 2024-04-12  6:04                     ` Chen, Jiqian
  2024-04-12  6:41                       ` Jason Wang
  1 sibling, 1 reply; 22+ messages in thread
From: Chen, Jiqian @ 2024-04-12  6:04 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Jason Wang, Chen, Jiqian, qemu-devel@nongnu.org, Huang, Ray

On 2024/4/7 19:49, Michael S. Tsirkin wrote:
>>> I will set the default value of No_Soft_Reset bit to true in next version according to your opinion.
>>> About the compatibility of old machine types, which types should I consider? Does the same as x-pcie-pm-init(hw_compat_2_8)?
>>> Forgive me for not knowing much about compatibility.
>>
>> "x" means no compatibility at all, please drop the "x" prefix. And it
>> looks more safe to start as "false" by default.
>>
>> Thanks
> 
> 
> Not sure I agree. External flags are for when users want to tweak them.
> When would users want it to be off?
> What is done here is I feel sane, just add machine compat machinery
> to change to off for old machine types.
Do you know which old machines should I consider to compatible with?
Or which guys should I add to "CC" and can get answer from them?
I have less knowledge about compatibility.

> 

-- 
Best regards,
Jiqian Chen.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-04-08  4:56                     ` Jason Wang
@ 2024-04-12  6:10                       ` Chen, Jiqian
  0 siblings, 0 replies; 22+ messages in thread
From: Chen, Jiqian @ 2024-04-12  6:10 UTC (permalink / raw)
  To: Jason Wang, Michael S. Tsirkin
  Cc: Chen, Jiqian, qemu-devel@nongnu.org, Huang, Ray

On 2024/4/8 12:56, Jason Wang wrote:
>>>> I will set the default value of No_Soft_Reset bit to true in next version according to your opinion.
>>>> About the compatibility of old machine types, which types should I consider? Does the same as x-pcie-pm-init(hw_compat_2_8)?
>>>> Forgive me for not knowing much about compatibility.
>>>
>>> "x" means no compatibility at all, please drop the "x" prefix. And it
>>> looks more safe to start as "false" by default.
>>>
>>> Thanks
>>
>>
>> Not sure I agree. External flags are for when users want to tweak them.
>> When would users want it to be off?
> 
> If I understand the suspending status proposal correctly, there are at
> least 1 device that is not safe. And this series does not mention
> which device it has tested.
I only tested virtio-gpu with my patch, I will mention this in "commit message" in next version.

> 
> It means if we enable it unconditionally, guests may break.
Make sense, will keep " No_Soft_Reset bit " false by default. And add some comments in the codes to describe what you considered.

> 
> Thanks
> 
>> What is done here is I feel sane, just add machine compat machinery
>> to change to off for old machine types.

-- 
Best regards,
Jiqian Chen.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-04-12  6:04                     ` Chen, Jiqian
@ 2024-04-12  6:41                       ` Jason Wang
  2024-04-12  6:49                         ` Chen, Jiqian
  0 siblings, 1 reply; 22+ messages in thread
From: Jason Wang @ 2024-04-12  6:41 UTC (permalink / raw)
  To: Chen, Jiqian; +Cc: Michael S. Tsirkin, qemu-devel@nongnu.org, Huang, Ray

On Fri, Apr 12, 2024 at 2:05 PM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
>
> On 2024/4/7 19:49, Michael S. Tsirkin wrote:
> >>> I will set the default value of No_Soft_Reset bit to true in next version according to your opinion.
> >>> About the compatibility of old machine types, which types should I consider? Does the same as x-pcie-pm-init(hw_compat_2_8)?
> >>> Forgive me for not knowing much about compatibility.
> >>
> >> "x" means no compatibility at all, please drop the "x" prefix. And it
> >> looks more safe to start as "false" by default.
> >>
> >> Thanks
> >
> >
> > Not sure I agree. External flags are for when users want to tweak them.
> > When would users want it to be off?
> > What is done here is I feel sane, just add machine compat machinery
> > to change to off for old machine types.
> Do you know which old machines should I consider to compatible with?
> Or which guys should I add to "CC" and can get answer from them?
> I have less knowledge about compatibility.

If you make it off by default, you don't need otherwise, it's one
release before.

Thanks

>
> >
>
> --
> Best regards,
> Jiqian Chen.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-04-12  5:59                   ` Chen, Jiqian
@ 2024-04-12  6:42                     ` Jason Wang
  0 siblings, 0 replies; 22+ messages in thread
From: Jason Wang @ 2024-04-12  6:42 UTC (permalink / raw)
  To: Chen, Jiqian; +Cc: Michael S. Tsirkin, qemu-devel@nongnu.org, Huang, Ray

On Fri, Apr 12, 2024 at 1:59 PM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
>
> On 2024/4/7 11:20, Jason Wang wrote:
> > On Tue, Apr 2, 2024 at 11:03 AM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
> >>
> >> On 2024/3/29 18:44, Michael S. Tsirkin wrote:
> >>> On Fri, Mar 29, 2024 at 03:20:59PM +0800, Jason Wang wrote:
> >>>> On Fri, Mar 29, 2024 at 3:07 PM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
> >>>>>
> >>>>> On 2024/3/28 20:36, Michael S. Tsirkin wrote:
> >>>>>>>>> +}
> >>>>>>>>> +
> >>>>>>>>>  static void virtio_pci_bus_reset_hold(Object *obj)
> >>>>>>>>>  {
> >>>>>>>>>      PCIDevice *dev = PCI_DEVICE(obj);
> >>>>>>>>>      DeviceState *qdev = DEVICE(obj);
> >>>>>>>>>
> >>>>>>>>> +    if (virtio_pci_no_soft_reset(dev)) {
> >>>>>>>>> +        return;
> >>>>>>>>> +    }
> >>>>>>>>> +
> >>>>>>>>>      virtio_pci_reset(qdev);
> >>>>>>>>>
> >>>>>>>>>      if (pci_is_express(dev)) {
> >>>>>>>>> @@ -2484,6 +2511,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),
> >>>>
> >>>> Why does it come with an x prefix?
> >>>>
> >>>>>>>>>      DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
> >>>>>>>>>                      VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
> >>>>>>>>>      DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
> >>>>>>>>
> >>>>>>>> I am a bit confused about this part.
> >>>>>>>> Do you want to make this software controllable?
> >>>>>>> Yes, because even the real hardware, this bit is not always set.
> >>>>
> >>>> We are talking about emulated devices here.
> >>>>
> >>>>>>
> >>>>>> So which virtio devices should and which should not set this bit?
> >>>>> This depends on the scenario the virtio-device is used, if we want to trigger an internal soft reset for the virtio-device during S3, this bit shouldn't be set.
> >>>>
> >>>> If the device doesn't need reset, why bother the driver for this?
> >>>>
> >>>> Btw, no_soft_reset is insufficient for some cases, there's a proposal
> >>>> for the virtio-spec. I think we need to wait until it is done.
> >>>
> >>> That seems orthogonal or did I miss something?
> >> Yes, I looked the detail of the proposal, I also think they are unrelated.
> >
> > The point is the proposal said
> >
> > """
> > Without a mechanism to
> > suspend/resume virtio devices when the driver is suspended/resumed in
> > the early phase of suspend/late phase of resume, there is a window where
> > interrupts can be lost.
> > """
> >
> > It looks safe to enable it with the suspend bit. Or if you think it's
> > wrong, please comment on the virtio spec patch.
> If I understand the proposal correctly.
> Only need to check the SUSPEND bit when virtio_pci_bus_reset_hold is called.
> It seems the proposal won't block this patch to upstream.
> In next version, I will add comments to note the SUSPEND bit that need to be considered once it is accepted.
>
> >
> >> I will set the default value of No_Soft_Reset bit to true in next version according to your opinion.
> >> About the compatibility of old machine types, which types should I consider? Does the same as x-pcie-pm-init(hw_compat_2_8)?
> >> Forgive me for not knowing much about compatibility.
> >
> > "x" means no compatibility at all, please drop the "x" prefix. And it
> Thanks to explain.
> So it seems the prefix "x" of "x-pcie-pm-init" is also wrong? Because it considered the hw_compat_2_8. Also "x-pcie-flr-init".

Probably but too late to fix.

> Back to No_Soft_Reset, do you know which old machines should I consider to compatible with?

Replied in another mail.

Thanks

>
> > looks more safe to start as "false" by default.
> >
> > Thanks
> >
> >>>
> >>>>> In my use case on my environment, I don't want to reset virtio-gpu during S3,
> >>>>> because once the display resources are destroyed, there are not enough information to re-create them, so this bit should be set.
> >>>>> Making this bit software controllable is convenient for users to take their own choices.
> >>>>
> >>>> Thanks
> >>>>
> >>>>>
> >>>>>>
> >>>>>>>> Or should this be set to true by default and then
> >>>>>>>> changed to false for old machine types?
> >>>>>>> How can I do so?
> >>>>>>> Do you mean set this to true by default, and if old machine types don't need this bit, they can pass false config to qemu when running qemu?
> >>>>>>
> >>>>>> No, you would use compat machinery. See how is x-pcie-flr-init handled.
> >>>>>>
> >>>>>>
> >>>>>
> >>>>> --
> >>>>> Best regards,
> >>>>> Jiqian Chen.
> >>>
> >>
> >> --
> >> Best regards,
> >> Jiqian Chen.
> >
>
> --
> Best regards,
> Jiqian Chen.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
  2024-04-12  6:41                       ` Jason Wang
@ 2024-04-12  6:49                         ` Chen, Jiqian
  0 siblings, 0 replies; 22+ messages in thread
From: Chen, Jiqian @ 2024-04-12  6:49 UTC (permalink / raw)
  To: Jason Wang
  Cc: Michael S. Tsirkin, qemu-devel@nongnu.org, Huang, Ray,
	Chen, Jiqian

On 2024/4/12 14:41, Jason Wang wrote:
> On Fri, Apr 12, 2024 at 2:05 PM Chen, Jiqian <Jiqian.Chen@amd.com> wrote:
>>
>> On 2024/4/7 19:49, Michael S. Tsirkin wrote:
>>>>> I will set the default value of No_Soft_Reset bit to true in next version according to your opinion.
>>>>> About the compatibility of old machine types, which types should I consider? Does the same as x-pcie-pm-init(hw_compat_2_8)?
>>>>> Forgive me for not knowing much about compatibility.
>>>>
>>>> "x" means no compatibility at all, please drop the "x" prefix. And it
>>>> looks more safe to start as "false" by default.
>>>>
>>>> Thanks
>>>
>>>
>>> Not sure I agree. External flags are for when users want to tweak them.
>>> When would users want it to be off?
>>> What is done here is I feel sane, just add machine compat machinery
>>> to change to off for old machine types.
>> Do you know which old machines should I consider to compatible with?
>> Or which guys should I add to "CC" and can get answer from them?
>> I have less knowledge about compatibility.
> 
> If you make it off by default, you don't need otherwise, it's one
> release before.
Ok, thanks. In next version, I will follow the result of discussion and remove "x", while adding some comments in codes.

> 
> Thanks
> 
>>
>>>
>>
>> --
>> Best regards,
>> Jiqian Chen.
> 

-- 
Best regards,
Jiqian Chen.

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2024-04-12  6:49 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-28 10:39 [RFC QEMU PATCH v8 0/1] S3 support Jiqian Chen
2024-03-28 10:39 ` [RFC QEMU PATCH v8 1/2] virtio-pci: only reset pm state during resetting Jiqian Chen
2024-03-28 10:39 ` [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit Jiqian Chen
2024-03-28 10:57   ` Michael S. Tsirkin
2024-03-28 11:08     ` Chen, Jiqian
2024-03-28 12:36       ` Michael S. Tsirkin
2024-03-29  7:00         ` Chen, Jiqian
2024-03-29  7:20           ` Jason Wang
2024-03-29  8:00             ` Chen, Jiqian
2024-03-29 10:38               ` Jason Wang
2024-04-02  2:56                 ` Chen, Jiqian
2024-03-29 10:44             ` Michael S. Tsirkin
2024-04-02  3:03               ` Chen, Jiqian
2024-04-07  3:20                 ` Jason Wang
2024-04-07 11:49                   ` Michael S. Tsirkin
2024-04-08  4:56                     ` Jason Wang
2024-04-12  6:10                       ` Chen, Jiqian
2024-04-12  6:04                     ` Chen, Jiqian
2024-04-12  6:41                       ` Jason Wang
2024-04-12  6:49                         ` Chen, Jiqian
2024-04-12  5:59                   ` Chen, Jiqian
2024-04-12  6:42                     ` Jason Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.