On Tue, Sep 26, 2023 at 11:00:28AM +0000, Chen, Jiqian wrote:
> On 2023/9/26 10:51, Jason Wang wrote:
> > On Tue, Sep 26, 2023 at 10:24 AM Chen, Jiqian <jiqian.c...@amd.com> wrote:
> >>
> >> On 2023/9/25 10:52, Jason Wang wrote:
> >>> On Fri, Sep 22, 2023 at 4:07 PM Chen, Jiqian <jiqian.c...@amd.com> wrote:
> >>>>
> >>>> Hi Jason,
> >>>>
> >>>> On 2023/9/22 11:17, Jason Wang wrote:
> >>>>> On Thu, Sep 21, 2023 at 2:28 PM Chen, Jiqian <jiqian.c...@amd.com>
> >>>>> wrote:
> >>>>>>
> >>>>>> Hi Jason,
> >>>>>>
> >>>>>> On 2023/9/21 12:22, Jason Wang wrote:
> >>>>>>> On Tue, Sep 19, 2023 at 7:43 PM Jiqian Chen <jiqian.c...@amd.com>
> >>>>>>> wrote:
> >>>>>>>>
> >>>>>>>> When guest vm does S3, Qemu will reset and clear some things of
> >>>>>>>> virtio
> >>>>>>>> devices, but guest can't aware that, so that may cause some problems.
> >>>>>>>> For excample, Qemu calls virtio_reset->virtio_gpu_gl_reset when guest
> >>>>>>>> resume, that function will destroy render resources of virtio-gpu. As
> >>>>>>>> a result, after guest resume, the display can't come back and we only
> >>>>>>>> saw a black screen. Due to guest can't re-create all the resources,
> >>>>>>>> so
> >>>>>>>> we need to let Qemu not to destroy them when S3.
> >>>>>>>>
> >>>>>>>> For above purpose, we need a mechanism that allows guests and QEMU to
> >>>>>>>> negotiate their reset behavior. So this patch add a new parameter
> >>>>>>>> named freeze_mode to struct virtio_pci_common_cfg. And when guest
> >>>>>>>> suspends, it can write freeze_mode to be FREEZE_S3, and then virtio
> >>>>>>>> devices can change their reset behavior on Qemu side according to
> >>>>>>>> freeze_mode. What's more, freeze_mode can be used for all virtio
> >>>>>>>> devices to affect the behavior of Qemu, not just virtio gpu device.
> >>>>>>>
> >>>>>>> A simple question, why is this issue specific to pci?
> >>>>>> I thought you possibly missed the previous version patches. At the
> >>>>>> beginning, I just wanted to add a new feature flag
> >>>>>> VIRTIO_GPU_F_FREEZE_S3 for virtio-gpu since I encountered virtio-gpu
> >>>>>> issue during guest S3, so that the guest and qemu can negotiate and
> >>>>>> change the reset behavior during S3. But Parav and Mikhail hoped me
> >>>>>> can improve the feature to a pci level, then other virtio devices can
> >>>>>> also benefit from it. Although I am not sure if expanding its
> >>>>>> influence is appropriate, I have not received any feedback from
> >>>>>> others, so I change it to the pci level and made this version.
> >>>>>> If you are interested, please see the previous version:
> >>>>>> https://lists.oasis-open.org/archives/virtio-comment/202307/msg00209.html,
> >>>>>> thank you.
> >>>>>
> >>>>> This is not a good answer. Let me ask you differently, why don't you
> >>>>> see it in other forms of transport like virtio-gpu-mmio?
> >>>> Sorry for not understanding your question before. Did you mean why did I
> >>>> add freeze_mode for virtio-pci type devices instead of virtio-mmio type
> >>>> devices?
> >>>
> >>> Yes.
> >>>
> >>>> If that's the case, it isn't well thought out by me, because there is
> >>>> only virtio-gpu with pci support in my environment, so I, as a matter of
> >>>> course, improve it from virtio-gpu-pci to a virtio-pci level instead of
> >>>> virtio-mmio.
> >>>> Do you think I need to add freeze_mode for virtio-mmio too?
> >>>
> >>> Because the function you describe is to preserve the render resources
> >>> which have nothing specific to PCI. It's a core virtio function.
> >>>
> >>> I'd suggest fixing it at the virtio level instead of PCI level.
> >> Yes, you are right, I will change to describe it to the virtio level in
> >> next version.
> >>
> >>>
> >>>> Or improve it to a higher level, like virtio-bus? I am looking forward
> >>>> to getting your suggestions, thanks.
> >>>
> >>> As pointed out in other thread, reusing the existing status state
> >>> machine is a perfect way for doing this.
> >>>
> >>> We can tweak the state machine to be able to do both migration and S3.
> >> I noticed the other thread you said, it will add a SUSPEND status to into
> >> device_status field, but it seems not enough to meet all scenarios, like
> >> we need to set the unfreeze state of guest,
> >
> > It's just a resume by clearing the suspend bit?
> It is not to trigger resume behavior to device by setting unfreeze in my
> scenario.
> You may misunderstand my intention.
> In current kernel and qemu code, it will trigger reset device behavior by
> setting device_status to 0 during resuming, and I found some operations of
> reset is not reasonable during resuming, like virtio_gpu_reset, it will
> destroy render resources and then caused the display gone after guest
> finishing reusming.
> So, I will set guest to S3 when suspending, and then qemu will notice the S3
> state of guest, and then it will not destroy render resources for virtio-gpu
> during resuming. The unfreeze state will be set after guest completed
> resuming.
> I think other virtio devices can also change the unreasonable behavior of
> resetting during resuming according the S3 state. And this will not affect
> other reset situation, just reset during resuming.
> I think this patch doesn't conflict with the patch in another thread, and
> they have different roles.
> Perhaps I need to use different words to avoid misunderstandings, not freeze
> or suspend, maybe state_s3, state_normal etc.
And then I have to ask - aren't any of the standard power states
appropriate here? I am not a PM expert but I think that it's unusual to
have device get the system state as opposed to device power state. No?
> >
> >>
> >
> >> the S3 state of the guest, the S4 state of the guest etc.
> >
> > Hibernation is different from suspending, it probably requires
> > setting/getting device states. And I don't get how this patch can help
> > with hibernation.
> The same reason as above, guest can set it to S4 state, and then we can
> change some behaviors on qemu side, so that to help guest resume from
> hibernation better.
>
> >
> >> So, one bit is not enough to indicate what status the guest is, since the
> >> freeze mode is a series,
> >
> > We can tweak the existing status state machine, otherwise you need to
> > explain the interaction between the two. For example what happens if
> > we freeze after reset etc.
> >
> > And if you do that, it is actually a single state machine.
> >
> > Thanks
> >
> >> I think add a new field to describe it is more reasonable.
> >>
> >>>
> >>> Thanks
> >>>
> >>>>
> >>>>>
> >>>>> Thanks
> >>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> Thanks
> >>>>>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>> Signed-off-by: Jiqian Chen <jiqian.c...@amd.com>
> >>>>>>>> ---
> >>>>>>>> transport-pci.tex | 7 +++++++
> >>>>>>>> 1 file changed, 7 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/transport-pci.tex b/transport-pci.tex
> >>>>>>>> index a5c6719..2543536 100644
> >>>>>>>> --- a/transport-pci.tex
> >>>>>>>> +++ b/transport-pci.tex
> >>>>>>>> @@ -319,6 +319,7 @@ \subsubsection{Common configuration structure
> >>>>>>>> layout}\label{sec:Virtio Transport
> >>>>>>>> le64 queue_desc; /* read-write */
> >>>>>>>> le64 queue_driver; /* read-write */
> >>>>>>>> le64 queue_device; /* read-write */
> >>>>>>>> + le16 freeze_mode; /* read-write */
> >>>>>>>> le16 queue_notif_config_data; /* read-only for driver */
> >>>>>>>> le16 queue_reset; /* read-write */
> >>>>>>>>
> >>>>>>>> @@ -393,6 +394,12 @@ \subsubsection{Common configuration structure
> >>>>>>>> layout}\label{sec:Virtio Transport
> >>>>>>>> \item[\field{queue_device}]
> >>>>>>>> The driver writes the physical address of Device Area here.
> >>>>>>>> See section \ref{sec:Basic Facilities of a Virtio Device /
> >>>>>>>> Virtqueues}.
> >>>>>>>>
> >>>>>>>> +\item[\field{freeze_mode}]
> >>>>>>>> + The driver writes this to set the freeze mode of virtio pci.
> >>>>>>>> + VIRTIO_PCI_FREEZE_MODE_UNFREEZE - virtio-pci is running;
> >>>>>>>> + VIRTIO_PCI_FREEZE_MODE_FREEZE_S3 - guest vm is doing S3,
> >>>>>>>> and virtio-pci enters S3 suspension;
> >>>>>>>> + Other values are reserved for future use, like S4, etc.
> >>>>>>>> +
> >>>>>>>> \item[\field{queue_notif_config_data}]
> >>>>>>>> This field exists only if VIRTIO_F_NOTIF_CONFIG_DATA has
> >>>>>>>> been negotiated.
> >>>>>>>> The driver will use this value when driver sends available
> >>>>>>>> buffer
> >>>>>>>> --
> >>>>>>>> 2.34.1
> >>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> ---------------------------------------------------------------------
> >>>>>>> To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
> >>>>>>> For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
> >>>>>>>
> >>>>>>
> >>>>>> --
> >>>>>> Best regards,
> >>>>>> Jiqian Chen.
> >>>>>
> >>>>
> >>>> --
> >>>> Best regards,
> >>>> Jiqian Chen.
> >>>
> >>>
> >>> ---------------------------------------------------------------------
> >>> To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
> >>> For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
> >>>
> >>
> >> --
> >> Best regards,
> >> Jiqian Chen.
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
> >
>
> --
> Best regards,
> Jiqian Chen.
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
