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

[Michael S. Tsirkin]

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.