[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-dev] Re: [VIRTIO PCI PATCH v5 1/1] transport-pci: Add freeze_mode to virtio_pci_common_cfg
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.Chen@amd.com> wrote:
> >>
> >> On 2023/9/25 10:52, Jason Wang wrote:
> >>> On Fri, Sep 22, 2023 at 4:07âPM Chen, Jiqian <Jiqian.Chen@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.Chen@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.Chen@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.Chen@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-unsubscribe@lists.oasis-open.org
> >>>>>>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> >>>>>>>
> >>>>>>
> >>>>>> --
> >>>>>> Best regards,
> >>>>>> Jiqian Chen.
> >>>>>
> >>>>
> >>>> --
> >>>> Best regards,
> >>>> Jiqian Chen.
> >>>
> >>>
> >>> ---------------------------------------------------------------------
> >>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> >>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> >>>
> >>
> >> --
> >> Best regards,
> >> Jiqian Chen.
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> >
>
> --
> Best regards,
> Jiqian Chen.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]