On Fri, Apr 12, 2024 at 1:59 PM Chen, Jiqian <jiqian.c...@amd.com> wrote: > > On 2024/4/7 11:20, 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. > 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.