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

[Jason Wang]

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.