On Sun, Apr 7, 2024 at 7:50 PM Michael S. Tsirkin <m...@redhat.com> wrote:
>
> 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.c...@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.c...@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?
If I understand the suspending status proposal correctly, there are at
least 1 device that is not safe. And this series does not mention
which device it has tested.
It means if we enable it unconditionally, guests may break.
Thanks
> 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.
>
