| Message ID | 20240328103903.408290-3-Jiqian.Chen@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | S3 support | expand |
On Thu, Mar 28, 2024 at 06:39:03PM +0800, Jiqian Chen wrote: > In current code, when guest does S3, virtio devices are reset due to > the bit No_Soft_Reset is not set. After resetting, the display resources > of virtio-gpu are destroyed, then the display can't come back and only > show blank after resuming. > > Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check > this bit, if this bit is set, the devices resetting will not be done, and > then the display can work after resuming. > > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > --- > hw/virtio/virtio-pci.c | 29 +++++++++++++++++++++++++++++ > include/hw/virtio/virtio-pci.h | 5 +++++ > 2 files changed, 34 insertions(+) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 05dd03758d9f..8d9fab855c7d 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -2378,6 +2378,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) > pcie_cap_lnkctl_init(pci_dev); > } > > + if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) { > + pci_set_word(pci_dev->config + pos + PCI_PM_CTRL, > + PCI_PM_CTRL_NO_SOFT_RESET); > + } > + > if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) { > /* Init Power Management Control Register */ > pci_set_word(pci_dev->wmask + pos + PCI_PM_CTRL, > @@ -2440,11 +2445,33 @@ static void virtio_pci_reset(DeviceState *qdev) > } > } > > +static bool virtio_pci_no_soft_reset(PCIDevice *dev) > +{ > + uint16_t pmcsr; > + > + if (!pci_is_express(dev) || !dev->exp.pm_cap) { > + return false; > + } > + > + pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL); > + > + /* > + * When No_Soft_Reset bit is set and the device > + * is in D3hot state, don't reset device > + */ > + return ((pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) && > + (pmcsr & PCI_PM_CTRL_STATE_MASK) == 3); No need for () around return value. > +} > + > 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), > 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? Or should this be set to true by default and then changed to false for old machine types? > diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h > index 4d57a9c75130..c758eb738234 100644 > --- a/include/hw/virtio/virtio-pci.h > +++ b/include/hw/virtio/virtio-pci.h > @@ -44,6 +44,7 @@ enum { > VIRTIO_PCI_FLAG_AER_BIT, > VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT, > VIRTIO_PCI_FLAG_VDPA_BIT, > + VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, > }; > > /* Need to activate work-arounds for buggy guests at vmstate load. */ > @@ -80,6 +81,10 @@ enum { > /* Init Power Management */ > #define VIRTIO_PCI_FLAG_INIT_PM (1 << VIRTIO_PCI_FLAG_INIT_PM_BIT) > > +/* Init The No_Soft_Reset bit of Power Management */ > +#define VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET \ > + (1 << VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT) > + > /* Init Function Level Reset capability */ > #define VIRTIO_PCI_FLAG_INIT_FLR (1 << VIRTIO_PCI_FLAG_INIT_FLR_BIT) > > -- > 2.34.1
On 2024/3/28 18:57, Michael S. Tsirkin wrote: > On Thu, Mar 28, 2024 at 06:39:03PM +0800, Jiqian Chen wrote: >> In current code, when guest does S3, virtio devices are reset due to >> the bit No_Soft_Reset is not set. After resetting, the display resources >> of virtio-gpu are destroyed, then the display can't come back and only >> show blank after resuming. >> >> Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check >> this bit, if this bit is set, the devices resetting will not be done, and >> then the display can work after resuming. >> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >> --- >> hw/virtio/virtio-pci.c | 29 +++++++++++++++++++++++++++++ >> include/hw/virtio/virtio-pci.h | 5 +++++ >> 2 files changed, 34 insertions(+) >> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c >> index 05dd03758d9f..8d9fab855c7d 100644 >> --- a/hw/virtio/virtio-pci.c >> +++ b/hw/virtio/virtio-pci.c >> @@ -2378,6 +2378,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) >> pcie_cap_lnkctl_init(pci_dev); >> } >> >> + if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) { >> + pci_set_word(pci_dev->config + pos + PCI_PM_CTRL, >> + PCI_PM_CTRL_NO_SOFT_RESET); >> + } >> + >> if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) { >> /* Init Power Management Control Register */ >> pci_set_word(pci_dev->wmask + pos + PCI_PM_CTRL, >> @@ -2440,11 +2445,33 @@ static void virtio_pci_reset(DeviceState *qdev) >> } >> } >> >> +static bool virtio_pci_no_soft_reset(PCIDevice *dev) >> +{ >> + uint16_t pmcsr; >> + >> + if (!pci_is_express(dev) || !dev->exp.pm_cap) { >> + return false; >> + } >> + >> + pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL); >> + >> + /* >> + * When No_Soft_Reset bit is set and the device >> + * is in D3hot state, don't reset device >> + */ >> + return ((pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) && >> + (pmcsr & PCI_PM_CTRL_STATE_MASK) == 3); > > > No need for () around return value. Ok, will delete in next version. > >> +} >> + >> 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), >> 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. > 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? > > >> diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h >> index 4d57a9c75130..c758eb738234 100644 >> --- a/include/hw/virtio/virtio-pci.h >> +++ b/include/hw/virtio/virtio-pci.h >> @@ -44,6 +44,7 @@ enum { >> VIRTIO_PCI_FLAG_AER_BIT, >> VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT, >> VIRTIO_PCI_FLAG_VDPA_BIT, >> + VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, >> }; >> >> /* Need to activate work-arounds for buggy guests at vmstate load. */ >> @@ -80,6 +81,10 @@ enum { >> /* Init Power Management */ >> #define VIRTIO_PCI_FLAG_INIT_PM (1 << VIRTIO_PCI_FLAG_INIT_PM_BIT) >> >> +/* Init The No_Soft_Reset bit of Power Management */ >> +#define VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET \ >> + (1 << VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT) >> + >> /* Init Function Level Reset capability */ >> #define VIRTIO_PCI_FLAG_INIT_FLR (1 << VIRTIO_PCI_FLAG_INIT_FLR_BIT) >> >> -- >> 2.34.1 >
On Thu, Mar 28, 2024 at 11:08:58AM +0000, Chen, Jiqian wrote: > On 2024/3/28 18:57, Michael S. Tsirkin wrote: > > On Thu, Mar 28, 2024 at 06:39:03PM +0800, Jiqian Chen wrote: > >> In current code, when guest does S3, virtio devices are reset due to > >> the bit No_Soft_Reset is not set. After resetting, the display resources > >> of virtio-gpu are destroyed, then the display can't come back and only > >> show blank after resuming. > >> > >> Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check > >> this bit, if this bit is set, the devices resetting will not be done, and > >> then the display can work after resuming. > >> > >> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > >> --- > >> hw/virtio/virtio-pci.c | 29 +++++++++++++++++++++++++++++ > >> include/hw/virtio/virtio-pci.h | 5 +++++ > >> 2 files changed, 34 insertions(+) > >> > >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > >> index 05dd03758d9f..8d9fab855c7d 100644 > >> --- a/hw/virtio/virtio-pci.c > >> +++ b/hw/virtio/virtio-pci.c > >> @@ -2378,6 +2378,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) > >> pcie_cap_lnkctl_init(pci_dev); > >> } > >> > >> + if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) { > >> + pci_set_word(pci_dev->config + pos + PCI_PM_CTRL, > >> + PCI_PM_CTRL_NO_SOFT_RESET); > >> + } > >> + > >> if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) { > >> /* Init Power Management Control Register */ > >> pci_set_word(pci_dev->wmask + pos + PCI_PM_CTRL, > >> @@ -2440,11 +2445,33 @@ static void virtio_pci_reset(DeviceState *qdev) > >> } > >> } > >> > >> +static bool virtio_pci_no_soft_reset(PCIDevice *dev) > >> +{ > >> + uint16_t pmcsr; > >> + > >> + if (!pci_is_express(dev) || !dev->exp.pm_cap) { > >> + return false; > >> + } > >> + > >> + pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL); > >> + > >> + /* > >> + * When No_Soft_Reset bit is set and the device > >> + * is in D3hot state, don't reset device > >> + */ > >> + return ((pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) && > >> + (pmcsr & PCI_PM_CTRL_STATE_MASK) == 3); > > > > > > No need for () around return value. > Ok, will delete in next version. > > > > >> +} > >> + > >> 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), > >> 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. So which virtio devices should and which should not set this bit? > > 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. > > > > > >> diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h > >> index 4d57a9c75130..c758eb738234 100644 > >> --- a/include/hw/virtio/virtio-pci.h > >> +++ b/include/hw/virtio/virtio-pci.h > >> @@ -44,6 +44,7 @@ enum { > >> VIRTIO_PCI_FLAG_AER_BIT, > >> VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT, > >> VIRTIO_PCI_FLAG_VDPA_BIT, > >> + VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, > >> }; > >> > >> /* Need to activate work-arounds for buggy guests at vmstate load. */ > >> @@ -80,6 +81,10 @@ enum { > >> /* Init Power Management */ > >> #define VIRTIO_PCI_FLAG_INIT_PM (1 << VIRTIO_PCI_FLAG_INIT_PM_BIT) > >> > >> +/* Init The No_Soft_Reset bit of Power Management */ > >> +#define VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET \ > >> + (1 << VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT) > >> + > >> /* Init Function Level Reset capability */ > >> #define VIRTIO_PCI_FLAG_INIT_FLR (1 << VIRTIO_PCI_FLAG_INIT_FLR_BIT) > >> > >> -- > >> 2.34.1 > > > > -- > Best regards, > Jiqian Chen.
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), >>>> 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. > > 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. 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. > >>> 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. > >
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. > 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.
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? > >>> >>> 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. > > 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? > >> 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. >
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.
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? > > 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.
On 2024/3/29 18:38, Jason Wang wrote: > 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. Ok, I will set it default to true in next version. If we need compatibility, what's your opinion about which machine types should we compatible with? Does the same as x-pcie-pm-init? > >> >>> >>>>> >>>>> 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/ I looked the detail of this proposal. What the proposal does is to add a new status to trigger device to suspend/resume. But this patch is to add No_Soft_Reset bit, it decides if the device should be reset. This patch doesn't depend on the proposal. If you mean that the proposal says "A device MUST maintain its state while suspended", I think it needs new patch to implement it after the proposal is done. > > 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. >
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. 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. > >>> 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. >
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 > > > >>> 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.
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.
On Sun, Apr 7, 2024 at 7:50 PM Michael S. Tsirkin <mst@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.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? 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. >
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". Back to No_Soft_Reset, do you know which old machines should I consider to compatible with? > 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. >
On 2024/4/7 19:49, Michael S. Tsirkin wrote: >>> 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. Do you know which old machines should I consider to compatible with? Or which guys should I add to "CC" and can get answer from them? I have less knowledge about compatibility. >
On 2024/4/8 12:56, Jason Wang wrote: >>>> 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. I only tested virtio-gpu with my patch, I will mention this in "commit message" in next version. > > It means if we enable it unconditionally, guests may break. Make sense, will keep " No_Soft_Reset bit " false by default. And add some comments in the codes to describe what you considered. > > Thanks > >> What is done here is I feel sane, just add machine compat machinery >> to change to off for old machine types.
On Fri, Apr 12, 2024 at 2:05 PM Chen, Jiqian <Jiqian.Chen@amd.com> wrote: > > On 2024/4/7 19:49, Michael S. Tsirkin wrote: > >>> 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. > Do you know which old machines should I consider to compatible with? > Or which guys should I add to "CC" and can get answer from them? > I have less knowledge about compatibility. If you make it off by default, you don't need otherwise, it's one release before. Thanks > > > > > -- > Best regards, > Jiqian Chen.
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.
On 2024/4/12 14:41, Jason Wang wrote: > On Fri, Apr 12, 2024 at 2:05 PM Chen, Jiqian <Jiqian.Chen@amd.com> wrote: >> >> On 2024/4/7 19:49, Michael S. Tsirkin wrote: >>>>> 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. >> Do you know which old machines should I consider to compatible with? >> Or which guys should I add to "CC" and can get answer from them? >> I have less knowledge about compatibility. > > If you make it off by default, you don't need otherwise, it's one > release before. Ok, thanks. In next version, I will follow the result of discussion and remove "x", while adding some comments in codes. > > Thanks > >> >>> >> >> -- >> Best regards, >> Jiqian Chen. >
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 05dd03758d9f..8d9fab855c7d 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -2378,6 +2378,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) pcie_cap_lnkctl_init(pci_dev); } + if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) { + pci_set_word(pci_dev->config + pos + PCI_PM_CTRL, + PCI_PM_CTRL_NO_SOFT_RESET); + } + if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) { /* Init Power Management Control Register */ pci_set_word(pci_dev->wmask + pos + PCI_PM_CTRL, @@ -2440,11 +2445,33 @@ static void virtio_pci_reset(DeviceState *qdev) } } +static bool virtio_pci_no_soft_reset(PCIDevice *dev) +{ + uint16_t pmcsr; + + if (!pci_is_express(dev) || !dev->exp.pm_cap) { + return false; + } + + pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL); + + /* + * When No_Soft_Reset bit is set and the device + * is in D3hot state, don't reset device + */ + return ((pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) && + (pmcsr & PCI_PM_CTRL_STATE_MASK) == 3); +} + 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), DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_INIT_FLR_BIT, true), DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags, diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h index 4d57a9c75130..c758eb738234 100644 --- a/include/hw/virtio/virtio-pci.h +++ b/include/hw/virtio/virtio-pci.h @@ -44,6 +44,7 @@ enum { VIRTIO_PCI_FLAG_AER_BIT, VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT, VIRTIO_PCI_FLAG_VDPA_BIT, + VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, }; /* Need to activate work-arounds for buggy guests at vmstate load. */ @@ -80,6 +81,10 @@ enum { /* Init Power Management */ #define VIRTIO_PCI_FLAG_INIT_PM (1 << VIRTIO_PCI_FLAG_INIT_PM_BIT) +/* Init The No_Soft_Reset bit of Power Management */ +#define VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET \ + (1 << VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT) + /* Init Function Level Reset capability */ #define VIRTIO_PCI_FLAG_INIT_FLR (1 << VIRTIO_PCI_FLAG_INIT_FLR_BIT)
In current code, when guest does S3, virtio devices are reset due to the bit No_Soft_Reset is not set. After resetting, the display resources of virtio-gpu are destroyed, then the display can't come back and only show blank after resuming. Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check this bit, if this bit is set, the devices resetting will not be done, and then the display can work after resuming. Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> --- hw/virtio/virtio-pci.c | 29 +++++++++++++++++++++++++++++ include/hw/virtio/virtio-pci.h | 5 +++++ 2 files changed, 34 insertions(+)