Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit

[Jason Wang]

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.