| Message ID | 20231006202045.1161543-5-vsementsov@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | vhost-user-blk: live resize additional APIs | expand |
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes: > Send a new event when guest reads virtio-pci config after > virtio_notify_config() call. > > That's useful to check that guest fetched modified config, for example > after resizing disk backend. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > --- > hw/virtio/virtio-pci.c | 9 +++++++++ > include/monitor/qdev.h | 1 + > monitor/monitor.c | 1 + > qapi/qdev.json | 22 ++++++++++++++++++++++ > softmmu/qdev-monitor.c | 5 +++++ > 5 files changed, 38 insertions(+) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index dd4620462b..f24f8ff03d 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -23,6 +23,7 @@ > #include "hw/boards.h" > #include "hw/virtio/virtio.h" > #include "migration/qemu-file-types.h" > +#include "monitor/qdev.h" > #include "hw/pci/pci.h" > #include "hw/pci/pci_bus.h" > #include "hw/qdev-properties.h" > @@ -541,6 +542,10 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr, > } > addr -= config; > > + if (vdev->generation > 0) { > + qdev_config_read_event(DEVICE(proxy)); > + } > + > switch (size) { > case 1: > val = virtio_config_readb(vdev, addr); > @@ -1728,6 +1733,10 @@ static uint64_t virtio_pci_device_read(void *opaque, hwaddr addr, > return UINT64_MAX; > } > > + if (vdev->generation > 0) { > + qdev_config_read_event(DEVICE(proxy)); > + } > + > switch (size) { > case 1: > val = virtio_config_modern_readb(vdev, addr); > diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h > index 949a3672cb..f0b0eab07e 100644 > --- a/include/monitor/qdev.h > +++ b/include/monitor/qdev.h > @@ -39,6 +39,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, > const char *qdev_set_id(DeviceState *dev, char *id, Error **errp); > > void qdev_hotplug_device_on_event(DeviceState *dev); > +void qdev_config_read_event(DeviceState *dev); > > DeviceAndPath *qdev_new_device_and_path(DeviceState *dev); > > diff --git a/monitor/monitor.c b/monitor/monitor.c > index 941f87815a..f8aa91b190 100644 > --- a/monitor/monitor.c > +++ b/monitor/monitor.c > @@ -315,6 +315,7 @@ static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = { > [QAPI_EVENT_QUORUM_FAILURE] = { 1000 * SCALE_MS }, > [QAPI_EVENT_VSERPORT_CHANGE] = { 1000 * SCALE_MS }, > [QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE] = { 1000 * SCALE_MS }, > + [QAPI_EVENT_X_CONFIG_READ] = { 300 * SCALE_MS }, > }; > > /* > diff --git a/qapi/qdev.json b/qapi/qdev.json > index 2468f8bddf..37a8785b81 100644 > --- a/qapi/qdev.json > +++ b/qapi/qdev.json > @@ -329,3 +329,25 @@ > # Since: 8.2 > ## > { 'command': 'x-device-sync-config', 'data': {'id': 'str'} } > + > +## > +# @X_CONFIG_READ: > +# > +# Emitted whenever guest reads virtio device config after config change. > +# > +# @device: device name > +# > +# @path: device path > +# > +# Since: 5.0.1-24 > +# > +# Example: > +# > +# <- { "event": "X_CONFIG_READ", > +# "data": { "device": "virtio-net-pci-0", > +# "path": "/machine/peripheral/virtio-net-pci-0" }, > +# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } > +# > +## > +{ 'event': 'X_CONFIG_READ', > + 'data': { '*device': 'str', 'path': 'str' } } The commit message talks about event CONFIG_READ, but you actually name it x-device-sync-config. I figure you use x- to signify "unstable". Please use feature flag 'unstable' for that. See docs/devel/qapi-code-gen.rst section "Features", in particular "Special features", and also the note on x- in section "Naming rules and reserved names". The name CONFIG_READ feels overly generic for something that makes sense only with virtio devices. > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c > index b485375049..d0f022e925 100644 > --- a/softmmu/qdev-monitor.c > +++ b/softmmu/qdev-monitor.c > @@ -1252,3 +1252,8 @@ void qdev_hotplug_device_on_event(DeviceState *dev) > dev->device_on_event_sent = true; > qapi_event_send_x_device_on(dev->id, dev->canonical_path); > } > + > +void qdev_config_read_event(DeviceState *dev) > +{ > + qapi_event_send_x_config_read(dev->id, dev->canonical_path); > +}
On 17.10.23 18:00, Markus Armbruster wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes: > >> Send a new event when guest reads virtio-pci config after >> virtio_notify_config() call. >> >> That's useful to check that guest fetched modified config, for example >> after resizing disk backend. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> --- >> hw/virtio/virtio-pci.c | 9 +++++++++ >> include/monitor/qdev.h | 1 + >> monitor/monitor.c | 1 + >> qapi/qdev.json | 22 ++++++++++++++++++++++ >> softmmu/qdev-monitor.c | 5 +++++ >> 5 files changed, 38 insertions(+) >> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c >> index dd4620462b..f24f8ff03d 100644 >> --- a/hw/virtio/virtio-pci.c >> +++ b/hw/virtio/virtio-pci.c >> @@ -23,6 +23,7 @@ >> #include "hw/boards.h" >> #include "hw/virtio/virtio.h" >> #include "migration/qemu-file-types.h" >> +#include "monitor/qdev.h" >> #include "hw/pci/pci.h" >> #include "hw/pci/pci_bus.h" >> #include "hw/qdev-properties.h" >> @@ -541,6 +542,10 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr, >> } >> addr -= config; >> >> + if (vdev->generation > 0) { >> + qdev_config_read_event(DEVICE(proxy)); >> + } >> + >> switch (size) { >> case 1: >> val = virtio_config_readb(vdev, addr); >> @@ -1728,6 +1733,10 @@ static uint64_t virtio_pci_device_read(void *opaque, hwaddr addr, >> return UINT64_MAX; >> } >> >> + if (vdev->generation > 0) { >> + qdev_config_read_event(DEVICE(proxy)); >> + } >> + >> switch (size) { >> case 1: >> val = virtio_config_modern_readb(vdev, addr); >> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h >> index 949a3672cb..f0b0eab07e 100644 >> --- a/include/monitor/qdev.h >> +++ b/include/monitor/qdev.h >> @@ -39,6 +39,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, >> const char *qdev_set_id(DeviceState *dev, char *id, Error **errp); >> >> void qdev_hotplug_device_on_event(DeviceState *dev); >> +void qdev_config_read_event(DeviceState *dev); >> >> DeviceAndPath *qdev_new_device_and_path(DeviceState *dev); >> >> diff --git a/monitor/monitor.c b/monitor/monitor.c >> index 941f87815a..f8aa91b190 100644 >> --- a/monitor/monitor.c >> +++ b/monitor/monitor.c >> @@ -315,6 +315,7 @@ static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = { >> [QAPI_EVENT_QUORUM_FAILURE] = { 1000 * SCALE_MS }, >> [QAPI_EVENT_VSERPORT_CHANGE] = { 1000 * SCALE_MS }, >> [QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE] = { 1000 * SCALE_MS }, >> + [QAPI_EVENT_X_CONFIG_READ] = { 300 * SCALE_MS }, >> }; >> >> /* >> diff --git a/qapi/qdev.json b/qapi/qdev.json >> index 2468f8bddf..37a8785b81 100644 >> --- a/qapi/qdev.json >> +++ b/qapi/qdev.json >> @@ -329,3 +329,25 @@ >> # Since: 8.2 >> ## >> { 'command': 'x-device-sync-config', 'data': {'id': 'str'} } >> + >> +## >> +# @X_CONFIG_READ: >> +# >> +# Emitted whenever guest reads virtio device config after config change. >> +# >> +# @device: device name >> +# >> +# @path: device path >> +# >> +# Since: 5.0.1-24 >> +# >> +# Example: >> +# >> +# <- { "event": "X_CONFIG_READ", >> +# "data": { "device": "virtio-net-pci-0", >> +# "path": "/machine/peripheral/virtio-net-pci-0" }, >> +# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } >> +# >> +## >> +{ 'event': 'X_CONFIG_READ', >> + 'data': { '*device': 'str', 'path': 'str' } } > > The commit message talks about event CONFIG_READ, but you actually name > it x-device-sync-config. will fix > > I figure you use x- to signify "unstable". Please use feature flag > 'unstable' for that. See docs/devel/qapi-code-gen.rst section > "Features", in particular "Special features", and also the note on x- in > section "Naming rules and reserved names". OK, will do. Hmm, it say Names beginning with ``x-`` used to signify "experimental". This convention has been replaced by special feature "unstable". "replaced".. So, I should use "unstable" flag without "x-" prefix? Can't find an example. Seems "unstable" always used together with "x-". Also, nothing said about events. Is using "X_" wrong idea? Should it be x-SOME_EVENT instead? > > The name CONFIG_READ feels overly generic for something that makes sense > only with virtio devices. Hmm, right.. I think, we can say same thing about DEVICE_UNPLUG_GUEST_ERROR. So, what about DEVICE_GUEST_READ_CONFIG ? > >> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >> index b485375049..d0f022e925 100644 >> --- a/softmmu/qdev-monitor.c >> +++ b/softmmu/qdev-monitor.c >> @@ -1252,3 +1252,8 @@ void qdev_hotplug_device_on_event(DeviceState *dev) >> dev->device_on_event_sent = true; >> qapi_event_send_x_device_on(dev->id, dev->canonical_path); >> } >> + >> +void qdev_config_read_event(DeviceState *dev) >> +{ >> + qapi_event_send_x_config_read(dev->id, dev->canonical_path); >> +} >
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes: > On 17.10.23 18:00, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes: >> >>> Send a new event when guest reads virtio-pci config after >>> virtio_notify_config() call. >>> >>> That's useful to check that guest fetched modified config, for example >>> after resizing disk backend. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> [...] >>> diff --git a/qapi/qdev.json b/qapi/qdev.json >>> index 2468f8bddf..37a8785b81 100644 >>> --- a/qapi/qdev.json >>> +++ b/qapi/qdev.json >>> @@ -329,3 +329,25 @@ >>> # Since: 8.2 >>> ## >>> { 'command': 'x-device-sync-config', 'data': {'id': 'str'} } >>> + >>> +## >>> +# @X_CONFIG_READ: >>> +# >>> +# Emitted whenever guest reads virtio device config after config change. >>> +# >>> +# @device: device name >>> +# >>> +# @path: device path >>> +# >>> +# Since: 5.0.1-24 >>> +# >>> +# Example: >>> +# >>> +# <- { "event": "X_CONFIG_READ", >>> +# "data": { "device": "virtio-net-pci-0", >>> +# "path": "/machine/peripheral/virtio-net-pci-0" }, >>> +# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } >>> +# >>> +## >>> +{ 'event': 'X_CONFIG_READ', >>> + 'data': { '*device': 'str', 'path': 'str' } } >> >> The commit message talks about event CONFIG_READ, but you actually name >> it x-device-sync-config. > > will fix > >> I figure you use x- to signify "unstable". Please use feature flag >> 'unstable' for that. See docs/devel/qapi-code-gen.rst section >> "Features", in particular "Special features", and also the note on x- in >> section "Naming rules and reserved names". > > OK, will do. > > Hmm, it say > > Names beginning with ``x-`` used to signify "experimental". This > convention has been replaced by special feature "unstable". > > "replaced".. So, I should use "unstable" flag without "x-" prefix? Can't find an example. Seems "unstable" always used together with "x-". True. The "x-" prefix originated with qdev properties. First use might be commit f0c07c7c7b4. The convention wasn't documented then, but QOM/qdev properties have always been a documentation wasteland. It then spread to other places, and eventually to the QAPI schema. Where we try pretty hard to document things properly. We documented the "x-" prefix in commit e790e666518: Any name (command, event, type, field, or enum value) beginning with "x-" is marked experimental, and may be withdrawn or changed incompatibly in a future release. Minor pain point: when things grow up from experimental to stable, we have to rename. The convention didn't stop us from naming non-experimental things x-FOO, e.g. QOM property "x-origin" in commit 6105683da35. Made it to the QAPI schema in commit 8825587b53c. Point is: the prefix isn't a reliable marker for "unstable". Since I needed a reliable marker for my "set policy for unstable interfaces" feature (see CLI option -compat), I created special feature flag "unstable", and dropped the "x-" convention for the QAPI schema. Renaming existing "x-" names felt like pointless churn, so I didn't. I'm not objecting to new names starting with "x-". Nor am I objecting to feature 'unstable' on names that don't start with "x-". I guess "x-" remains just fine for things we don't intend to make stable at some point. The "x-" can remind humans "this is unstable" better than a feature flag can (for machines, it's the other way round). For things we do intend (hope?) to make stable, I wouldn't bother with the "x-". Clearer now? > Also, nothing said about events. Is using "X_" wrong idea? Should it be x-SOME_EVENT instead? Since this is the first unstable event, there is no precedent. Let's use no prefix, and move on. >> The name CONFIG_READ feels overly generic for something that makes sense >> only with virtio devices. > > Hmm, right.. I think, we can say same thing about DEVICE_UNPLUG_GUEST_ERROR. That one came to be as a generalization of existing MEM_UNPLUG_ERROR and a concrete need to signal CPU unplug errors. Demonstrates "unplug guest errors" can happen for different kinds of devices. So we went with a generic event we can use for all of them. This doesn't seem to be the case for this patch's event. Thoughts? > So, what about DEVICE_GUEST_READ_CONFIG ? > >> >>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >>> index b485375049..d0f022e925 100644 >>> --- a/softmmu/qdev-monitor.c >>> +++ b/softmmu/qdev-monitor.c >>> @@ -1252,3 +1252,8 @@ void qdev_hotplug_device_on_event(DeviceState *dev) >>> dev->device_on_event_sent = true; >>> qapi_event_send_x_device_on(dev->id, dev->canonical_path); >>> } >>> + >>> +void qdev_config_read_event(DeviceState *dev) >>> +{ >>> + qapi_event_send_x_config_read(dev->id, dev->canonical_path); >>> +} >>
On 18.10.23 09:47, Markus Armbruster wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes: > >> On 17.10.23 18:00, Markus Armbruster wrote: >>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes: >>> >>>> Send a new event when guest reads virtio-pci config after >>>> virtio_notify_config() call. >>>> >>>> That's useful to check that guest fetched modified config, for example >>>> after resizing disk backend. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > [...] > >>>> diff --git a/qapi/qdev.json b/qapi/qdev.json >>>> index 2468f8bddf..37a8785b81 100644 >>>> --- a/qapi/qdev.json >>>> +++ b/qapi/qdev.json >>>> @@ -329,3 +329,25 @@ >>>> # Since: 8.2 >>>> ## >>>> { 'command': 'x-device-sync-config', 'data': {'id': 'str'} } >>>> + >>>> +## >>>> +# @X_CONFIG_READ: >>>> +# >>>> +# Emitted whenever guest reads virtio device config after config change. >>>> +# >>>> +# @device: device name >>>> +# >>>> +# @path: device path >>>> +# >>>> +# Since: 5.0.1-24 >>>> +# >>>> +# Example: >>>> +# >>>> +# <- { "event": "X_CONFIG_READ", >>>> +# "data": { "device": "virtio-net-pci-0", >>>> +# "path": "/machine/peripheral/virtio-net-pci-0" }, >>>> +# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } >>>> +# >>>> +## >>>> +{ 'event': 'X_CONFIG_READ', >>>> + 'data': { '*device': 'str', 'path': 'str' } } >>> >>> The commit message talks about event CONFIG_READ, but you actually name >>> it x-device-sync-config. >> >> will fix >> >>> I figure you use x- to signify "unstable". Please use feature flag >>> 'unstable' for that. See docs/devel/qapi-code-gen.rst section >>> "Features", in particular "Special features", and also the note on x- in >>> section "Naming rules and reserved names". >> >> OK, will do. >> >> Hmm, it say >> >> Names beginning with ``x-`` used to signify "experimental". This >> convention has been replaced by special feature "unstable". >> >> "replaced".. So, I should use "unstable" flag without "x-" prefix? Can't find an example. Seems "unstable" always used together with "x-". > > True. > > The "x-" prefix originated with qdev properties. First use might be > commit f0c07c7c7b4. The convention wasn't documented then, but QOM/qdev > properties have always been a documentation wasteland. It then spread > to other places, and eventually to the QAPI schema. Where we try pretty > hard to document things properly. We documented the "x-" prefix in > commit e790e666518: > > Any name (command, event, type, field, or enum value) beginning with > "x-" is marked experimental, and may be withdrawn or changed > incompatibly in a future release. > > Minor pain point: when things grow up from experimental to stable, we > have to rename. > > The convention didn't stop us from naming non-experimental things x-FOO, > e.g. QOM property "x-origin" in commit 6105683da35. Made it to the QAPI > schema in commit 8825587b53c. Point is: the prefix isn't a reliable > marker for "unstable". > > Since I needed a reliable marker for my "set policy for unstable > interfaces" feature (see CLI option -compat), I created special feature > flag "unstable", and dropped the "x-" convention for the QAPI schema. > > Renaming existing "x-" names felt like pointless churn, so I didn't. > > I'm not objecting to new names starting with "x-". Nor am I objecting > to feature 'unstable' on names that don't start with "x-". > > I guess "x-" remains just fine for things we don't intend to make stable > at some point. The "x-" can remind humans "this is unstable" better > than a feature flag can (for machines, it's the other way round). > > For things we do intend (hope?) to make stable, I wouldn't bother with > the "x-". > > Clearer now? Yes, thanks. x- seems safer for management tool that doesn't know about "unstable" properties.. But on the other hand, changing from x- to no-prefix is already done when the feature is stable, and thouse who use it already use the latest version of interface, so, removing the prefix is just extra work. So, I think, I'd go without prefix. > >> Also, nothing said about events. Is using "X_" wrong idea? Should it be x-SOME_EVENT instead? > > Since this is the first unstable event, there is no precedent. Let's > use no prefix, and move on. > >>> The name CONFIG_READ feels overly generic for something that makes sense >>> only with virtio devices. >> >> Hmm, right.. I think, we can say same thing about DEVICE_UNPLUG_GUEST_ERROR. > > That one came to be as a generalization of existing MEM_UNPLUG_ERROR and > a concrete need to signal CPU unplug errors. Demonstrates "unplug guest > errors" can happen for different kinds of devices. So we went with a > generic event we can use for all of them. > > This doesn't seem to be the case for this patch's event. Thoughts? Right.. VIRTIO_CONFIG_READ maybe? > >> So, what about DEVICE_GUEST_READ_CONFIG ? >> >>> >>>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >>>> index b485375049..d0f022e925 100644 >>>> --- a/softmmu/qdev-monitor.c >>>> +++ b/softmmu/qdev-monitor.c >>>> @@ -1252,3 +1252,8 @@ void qdev_hotplug_device_on_event(DeviceState *dev) >>>> dev->device_on_event_sent = true; >>>> qapi_event_send_x_device_on(dev->id, dev->canonical_path); >>>> } >>>> + >>>> +void qdev_config_read_event(DeviceState *dev) >>>> +{ >>>> + qapi_event_send_x_config_read(dev->id, dev->canonical_path); >>>> +} >>> >
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes: > On 18.10.23 09:47, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes: >> >>> On 17.10.23 18:00, Markus Armbruster wrote: >>>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes: >>>> >>>>> Send a new event when guest reads virtio-pci config after >>>>> virtio_notify_config() call. >>>>> >>>>> That's useful to check that guest fetched modified config, for example >>>>> after resizing disk backend. >>>>> >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> [...] >> >>>>> diff --git a/qapi/qdev.json b/qapi/qdev.json >>>>> index 2468f8bddf..37a8785b81 100644 >>>>> --- a/qapi/qdev.json >>>>> +++ b/qapi/qdev.json >>>>> @@ -329,3 +329,25 @@ >>>>> # Since: 8.2 >>>>> ## >>>>> { 'command': 'x-device-sync-config', 'data': {'id': 'str'} } >>>>> + >>>>> +## >>>>> +# @X_CONFIG_READ: >>>>> +# >>>>> +# Emitted whenever guest reads virtio device config after config change. >>>>> +# >>>>> +# @device: device name >>>>> +# >>>>> +# @path: device path >>>>> +# >>>>> +# Since: 5.0.1-24 >>>>> +# >>>>> +# Example: >>>>> +# >>>>> +# <- { "event": "X_CONFIG_READ", >>>>> +# "data": { "device": "virtio-net-pci-0", >>>>> +# "path": "/machine/peripheral/virtio-net-pci-0" }, >>>>> +# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } >>>>> +# >>>>> +## >>>>> +{ 'event': 'X_CONFIG_READ', >>>>> + 'data': { '*device': 'str', 'path': 'str' } } >>>> >>>> The commit message talks about event CONFIG_READ, but you actually name >>>> it x-device-sync-config. >>> >>> will fix >>> >>>> I figure you use x- to signify "unstable". Please use feature flag >>>> 'unstable' for that. See docs/devel/qapi-code-gen.rst section >>>> "Features", in particular "Special features", and also the note on x- in >>>> section "Naming rules and reserved names". >>> >>> OK, will do. >>> >>> Hmm, it say >>> >>> Names beginning with ``x-`` used to signify "experimental". This >>> convention has been replaced by special feature "unstable". >>> >>> "replaced".. So, I should use "unstable" flag without "x-" prefix? Can't find an example. Seems "unstable" always used together with "x-". >> >> True. >> >> The "x-" prefix originated with qdev properties. First use might be >> commit f0c07c7c7b4. The convention wasn't documented then, but QOM/qdev >> properties have always been a documentation wasteland. It then spread >> to other places, and eventually to the QAPI schema. Where we try pretty >> hard to document things properly. We documented the "x-" prefix in >> commit e790e666518: >> >> Any name (command, event, type, field, or enum value) beginning with >> "x-" is marked experimental, and may be withdrawn or changed >> incompatibly in a future release. >> >> Minor pain point: when things grow up from experimental to stable, we >> have to rename. >> >> The convention didn't stop us from naming non-experimental things x-FOO, >> e.g. QOM property "x-origin" in commit 6105683da35. Made it to the QAPI >> schema in commit 8825587b53c. Point is: the prefix isn't a reliable >> marker for "unstable". >> >> Since I needed a reliable marker for my "set policy for unstable >> interfaces" feature (see CLI option -compat), I created special feature >> flag "unstable", and dropped the "x-" convention for the QAPI schema. >> >> Renaming existing "x-" names felt like pointless churn, so I didn't. >> >> I'm not objecting to new names starting with "x-". Nor am I objecting >> to feature 'unstable' on names that don't start with "x-". >> >> I guess "x-" remains just fine for things we don't intend to make stable >> at some point. The "x-" can remind humans "this is unstable" better >> than a feature flag can (for machines, it's the other way round). >> >> For things we do intend (hope?) to make stable, I wouldn't bother with >> the "x-". >> >> Clearer now? > > Yes, thanks. > > x- seems safer for management tool that doesn't know about "unstable" properties.. Easy, traditional, and unreliable :) > But on the other hand, changing from x- to no-prefix is already done when the feature is stable, and thouse who use it already use the latest version of interface, so, removing the prefix is just extra work. Exactly. > So, I think, I'd go without prefix. Makes sense. >>> Also, nothing said about events. Is using "X_" wrong idea? Should it be x-SOME_EVENT instead? >> >> Since this is the first unstable event, there is no precedent. Let's >> use no prefix, and move on. >> >>>> The name CONFIG_READ feels overly generic for something that makes sense >>>> only with virtio devices. >>> >>> Hmm, right.. I think, we can say same thing about DEVICE_UNPLUG_GUEST_ERROR. >> >> That one came to be as a generalization of existing MEM_UNPLUG_ERROR and >> a concrete need to signal CPU unplug errors. Demonstrates "unplug guest >> errors" can happen for different kinds of devices. So we went with a >> generic event we can use for all of them. >> This doesn't seem to be the case for this patch's event. Thoughts? > > Right.. VIRTIO_CONFIG_READ maybe? Works for me. [...]
On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote: > > x- seems safer for management tool that doesn't know about "unstable" properties.. > > Easy, traditional, and unreliable :) > > But on the other hand, changing from x- to no-prefix is already done when the feature is stable, and thouse who use it already use the latest version of interface, so, removing the prefix is just extra work. > > Exactly. > I think "x-" is still better for command line use of properties - we don't have an API to mark things unstable there, do we?
On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote: > On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote: > > > x- seems safer for management tool that doesn't know about "unstable" properties.. > > > > Easy, traditional, and unreliable :) > > > > But on the other hand, changing from x- to no-prefix is already > > > done when the feature is stable, and thouse who use it already > > > use the latest version of interface, so, removing the prefix is > > > just extra work. > > > > Exactly. > > > > I think "x-" is still better for command line use of properties - we > don't have an API to mark things unstable there, do we? Personally I like to see "x-" prefix present *everywhere* there is an unstable feature, and consider the need to rename when declaring it stable to be good thing as it sets an easily identifiable line in the sand and is self-evident to outside observers. The self-documenting nature of the "x-" prefer is what makes it most compelling to me. A patch submission, or command line invokation or an example QMP command, or a bug report, that exhibit an 'x-' prefix are an immediate red flag to anyone who sees them. If someone sees a QMP comamnd / a typical giant QEMU command line, they are never going to go look at the QAPI schema to check whether any feature used had an 'unstable' marker. The 'unstable' marker might as well not exist in most cases. IOW, having the 'unstable' flag in the QAPI schema is great for machine introspection, but it isn't a substitute for having an 'x-' prefix used for the benefit of humans IMHO. With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote: >> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote: >> > > x- seems safer for management tool that doesn't know about "unstable" properties.. >> > >> > Easy, traditional, and unreliable :) >> >> > > But on the other hand, changing from x- to no-prefix is already >> > > done when the feature is stable, and thouse who use it already >> > > use the latest version of interface, so, removing the prefix is >> > > just extra work. >> > >> > Exactly. >> > >> >> I think "x-" is still better for command line use of properties - we >> don't have an API to mark things unstable there, do we? > > Personally I like to see "x-" prefix present *everywhere* there is > an unstable feature, and consider the need to rename when declaring > it stable to be good thing as it sets an easily identifiable line > in the sand and is self-evident to outside observers. > > The self-documenting nature of the "x-" prefer is what makes it most > compelling to me. A patch submission, or command line invokation or > an example QMP command, or a bug report, that exhibit an 'x-' prefix > are an immediate red flag to anyone who sees them. Except when it isn't, like in "x-origin". > If someone sees a QMP comamnd / a typical giant QEMU command line, > they are never going to go look at the QAPI schema to check whether > any feature used had an 'unstable' marker. The 'unstable' marker > might as well not exist in most cases. > > IOW, having the 'unstable' flag in the QAPI schema is great for machine > introspection, but it isn't a substitute for having an 'x-' prefix used > for the benefit of humans IMHO. I'm not sure there's disagreement. Quoting myself: The "x-" can remind humans "this is unstable" better than a feature flag can (for machines, it's the other way round). CLI and HMP are for humans. We continue to use "x-" there. QMP is for machines. The feature flag is the sole source of truth. Additional use of "x-" is fine, but not required.
On Wed, Oct 18, 2023 at 02:02:08PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote: > >> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote: > >> > > x- seems safer for management tool that doesn't know about "unstable" properties.. > >> > > >> > Easy, traditional, and unreliable :) > >> > >> > > But on the other hand, changing from x- to no-prefix is already > >> > > done when the feature is stable, and thouse who use it already > >> > > use the latest version of interface, so, removing the prefix is > >> > > just extra work. > >> > > >> > Exactly. > >> > > >> > >> I think "x-" is still better for command line use of properties - we > >> don't have an API to mark things unstable there, do we? > > > > Personally I like to see "x-" prefix present *everywhere* there is > > an unstable feature, and consider the need to rename when declaring > > it stable to be good thing as it sets an easily identifiable line > > in the sand and is self-evident to outside observers. > > > > The self-documenting nature of the "x-" prefer is what makes it most > > compelling to me. A patch submission, or command line invokation or > > an example QMP command, or a bug report, that exhibit an 'x-' prefix > > are an immediate red flag to anyone who sees them. > > Except when it isn't, like in "x-origin". > > > If someone sees a QMP comamnd / a typical giant QEMU command line, > > they are never going to go look at the QAPI schema to check whether > > any feature used had an 'unstable' marker. The 'unstable' marker > > might as well not exist in most cases. > > > > IOW, having the 'unstable' flag in the QAPI schema is great for machine > > introspection, but it isn't a substitute for having an 'x-' prefix used > > for the benefit of humans IMHO. > > I'm not sure there's disagreement. Quoting myself: > > The "x-" can remind humans "this is unstable" better than a feature > flag can (for machines, it's the other way round). > > CLI and HMP are for humans. We continue to use "x-" there. > > QMP is for machines. The feature flag is the sole source of truth. > Additional use of "x-" is fine, but not required. I guess we have different defintions of "for humans" in this context. I consider QMP data still relevant for humans, because humans are reviewing patches to libvirt that add usage of QMP features, or triaging bug reports that include examples of usage, and in both cases it is pretty relevant to make unstable features stand out to the human via the x- prefix IMHO. With regards, Daniel
On 18.10.23 15:02, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > >> On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote: >>> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote: >>>>> x- seems safer for management tool that doesn't know about "unstable" properties.. >>>> >>>> Easy, traditional, and unreliable :) >>> >>>>> But on the other hand, changing from x- to no-prefix is already >>>>> done when the feature is stable, and thouse who use it already >>>>> use the latest version of interface, so, removing the prefix is >>>>> just extra work. >>>> >>>> Exactly. >>>> >>> >>> I think "x-" is still better for command line use of properties - we >>> don't have an API to mark things unstable there, do we? >> >> Personally I like to see "x-" prefix present *everywhere* there is >> an unstable feature, and consider the need to rename when declaring >> it stable to be good thing as it sets an easily identifiable line >> in the sand and is self-evident to outside observers. >> >> The self-documenting nature of the "x-" prefer is what makes it most >> compelling to me. A patch submission, or command line invokation or >> an example QMP command, or a bug report, that exhibit an 'x-' prefix >> are an immediate red flag to anyone who sees them. > > Except when it isn't, like in "x-origin". > Interesting how many such stable x-FOO things we have? Probably we could deprecate and than rename them?
* Daniel P. Berrangé (berrange@redhat.com) wrote: > On Wed, Oct 18, 2023 at 02:02:08PM +0200, Markus Armbruster wrote: > > Daniel P. Berrangé <berrange@redhat.com> writes: > > > > > On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote: > > >> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote: > > >> > > x- seems safer for management tool that doesn't know about "unstable" properties.. > > >> > > > >> > Easy, traditional, and unreliable :) > > >> > > >> > > But on the other hand, changing from x- to no-prefix is already > > >> > > done when the feature is stable, and thouse who use it already > > >> > > use the latest version of interface, so, removing the prefix is > > >> > > just extra work. > > >> > > > >> > Exactly. > > >> > > > >> > > >> I think "x-" is still better for command line use of properties - we > > >> don't have an API to mark things unstable there, do we? > > > > > > Personally I like to see "x-" prefix present *everywhere* there is > > > an unstable feature, and consider the need to rename when declaring > > > it stable to be good thing as it sets an easily identifiable line > > > in the sand and is self-evident to outside observers. > > > > > > The self-documenting nature of the "x-" prefer is what makes it most > > > compelling to me. A patch submission, or command line invokation or > > > an example QMP command, or a bug report, that exhibit an 'x-' prefix > > > are an immediate red flag to anyone who sees them. > > > > Except when it isn't, like in "x-origin". > > > > > If someone sees a QMP comamnd / a typical giant QEMU command line, > > > they are never going to go look at the QAPI schema to check whether > > > any feature used had an 'unstable' marker. The 'unstable' marker > > > might as well not exist in most cases. > > > > > > IOW, having the 'unstable' flag in the QAPI schema is great for machine > > > introspection, but it isn't a substitute for having an 'x-' prefix used > > > for the benefit of humans IMHO. > > > > I'm not sure there's disagreement. Quoting myself: > > > > The "x-" can remind humans "this is unstable" better than a feature > > flag can (for machines, it's the other way round). > > > > CLI and HMP are for humans. We continue to use "x-" there. > > > > QMP is for machines. The feature flag is the sole source of truth. > > Additional use of "x-" is fine, but not required. > > I guess we have different defintions of "for humans" in this context. > I consider QMP data still relevant for humans, because humans are > reviewing patches to libvirt that add usage of QMP features, or > triaging bug reports that include examples of usage, and in both > cases it is pretty relevant to make unstable features stand out to > the human via the x- prefix IMHO. Using x- for events makes sense to me; the semantics of events can be quite subtle; often you don't find out how broken they are until you wire them through libvirt and up the stack; so it's not impossible you might need to change it - but then without the x- the semantics (rather than existence) of the event is carved in stone. Dave > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes: > On 18.10.23 15:02, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >>> On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote: >>>> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote: >>>>>> x- seems safer for management tool that doesn't know about "unstable" properties.. >>>>> >>>>> Easy, traditional, and unreliable :) >>>> >>>>>> But on the other hand, changing from x- to no-prefix is already >>>>>> done when the feature is stable, and thouse who use it already >>>>>> use the latest version of interface, so, removing the prefix is >>>>>> just extra work. >>>>> >>>>> Exactly. >>>>> >>>> >>>> I think "x-" is still better for command line use of properties - we >>>> don't have an API to mark things unstable there, do we? >>> >>> Personally I like to see "x-" prefix present *everywhere* there is >>> an unstable feature, and consider the need to rename when declaring >>> it stable to be good thing as it sets an easily identifiable line >>> in the sand and is self-evident to outside observers. >>> >>> The self-documenting nature of the "x-" prefer is what makes it most >>> compelling to me. A patch submission, or command line invokation or >>> an example QMP command, or a bug report, that exhibit an 'x-' prefix >>> are an immediate red flag to anyone who sees them. >> Except when it isn't, like in "x-origin". >> > > Interesting how many such stable x-FOO things we have? I don't know. I think the more interesting question is how many *unstable* things we have that aren't named x-FOO. I'm thinking of stuff that was never intended to be exposed externally. QOM/qdev properties, mostly. For extra spiciness, throw in qom-get and especially qom-set. > Probably we could deprecate and than rename them? I guess we can if we care :)
"Dr. David Alan Gilbert" <dave@treblig.org> writes: > Using x- for events makes sense to me; the semantics of events can be > quite subtle; often you don't find out how broken they are until you > wire them through libvirt and up the stack; so it's not impossible > you might need to change it - but then without the x- the semantics > (rather than existence) of the event is carved in stone. It is carved in stone unless feature 'unstable' is declared. The name gets no vote. We may elect to name unstable QAPI things 'x-FOO' to help humans. [...]
Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Oct 18, 2023 at 02:02:08PM +0200, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> > On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote: >> >> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote: >> >> > > x- seems safer for management tool that doesn't know about "unstable" properties.. >> >> > >> >> > Easy, traditional, and unreliable :) >> >> >> >> > > But on the other hand, changing from x- to no-prefix is already >> >> > > done when the feature is stable, and thouse who use it already >> >> > > use the latest version of interface, so, removing the prefix is >> >> > > just extra work. >> >> > >> >> > Exactly. >> >> > >> >> >> >> I think "x-" is still better for command line use of properties - we >> >> don't have an API to mark things unstable there, do we? >> > >> > Personally I like to see "x-" prefix present *everywhere* there is >> > an unstable feature, and consider the need to rename when declaring >> > it stable to be good thing as it sets an easily identifiable line >> > in the sand and is self-evident to outside observers. >> > >> > The self-documenting nature of the "x-" prefer is what makes it most >> > compelling to me. A patch submission, or command line invokation or >> > an example QMP command, or a bug report, that exhibit an 'x-' prefix >> > are an immediate red flag to anyone who sees them. >> >> Except when it isn't, like in "x-origin". >> >> > If someone sees a QMP comamnd / a typical giant QEMU command line, >> > they are never going to go look at the QAPI schema to check whether >> > any feature used had an 'unstable' marker. The 'unstable' marker >> > might as well not exist in most cases. >> > >> > IOW, having the 'unstable' flag in the QAPI schema is great for machine >> > introspection, but it isn't a substitute for having an 'x-' prefix used >> > for the benefit of humans IMHO. >> >> I'm not sure there's disagreement. Quoting myself: >> >> The "x-" can remind humans "this is unstable" better than a feature >> flag can (for machines, it's the other way round). >> >> CLI and HMP are for humans. We continue to use "x-" there. >> >> QMP is for machines. The feature flag is the sole source of truth. >> Additional use of "x-" is fine, but not required. > > I guess we have different defintions of "for humans" in this context. > I consider QMP data still relevant for humans, because humans are > reviewing patches to libvirt that add usage of QMP features, or There's a much more reliable way: 1. Require tests 2. Run them with -compat unstable-input=reject,unstable-output=hide Or unstable-input=crash if you don't trust or don't want to rely on your test harness. > triaging bug reports that include examples of usage, Here you can run the reproducer with -compat. > and in both > cases it is pretty relevant to make unstable features stand out to > the human via the x- prefix IMHO. Your point remains at least somewhat valid all the same.
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index dd4620462b..f24f8ff03d 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -23,6 +23,7 @@ #include "hw/boards.h" #include "hw/virtio/virtio.h" #include "migration/qemu-file-types.h" +#include "monitor/qdev.h" #include "hw/pci/pci.h" #include "hw/pci/pci_bus.h" #include "hw/qdev-properties.h" @@ -541,6 +542,10 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr, } addr -= config; + if (vdev->generation > 0) { + qdev_config_read_event(DEVICE(proxy)); + } + switch (size) { case 1: val = virtio_config_readb(vdev, addr); @@ -1728,6 +1733,10 @@ static uint64_t virtio_pci_device_read(void *opaque, hwaddr addr, return UINT64_MAX; } + if (vdev->generation > 0) { + qdev_config_read_event(DEVICE(proxy)); + } + switch (size) { case 1: val = virtio_config_modern_readb(vdev, addr); diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h index 949a3672cb..f0b0eab07e 100644 --- a/include/monitor/qdev.h +++ b/include/monitor/qdev.h @@ -39,6 +39,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, const char *qdev_set_id(DeviceState *dev, char *id, Error **errp); void qdev_hotplug_device_on_event(DeviceState *dev); +void qdev_config_read_event(DeviceState *dev); DeviceAndPath *qdev_new_device_and_path(DeviceState *dev); diff --git a/monitor/monitor.c b/monitor/monitor.c index 941f87815a..f8aa91b190 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -315,6 +315,7 @@ static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = { [QAPI_EVENT_QUORUM_FAILURE] = { 1000 * SCALE_MS }, [QAPI_EVENT_VSERPORT_CHANGE] = { 1000 * SCALE_MS }, [QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE] = { 1000 * SCALE_MS }, + [QAPI_EVENT_X_CONFIG_READ] = { 300 * SCALE_MS }, }; /* diff --git a/qapi/qdev.json b/qapi/qdev.json index 2468f8bddf..37a8785b81 100644 --- a/qapi/qdev.json +++ b/qapi/qdev.json @@ -329,3 +329,25 @@ # Since: 8.2 ## { 'command': 'x-device-sync-config', 'data': {'id': 'str'} } + +## +# @X_CONFIG_READ: +# +# Emitted whenever guest reads virtio device config after config change. +# +# @device: device name +# +# @path: device path +# +# Since: 5.0.1-24 +# +# Example: +# +# <- { "event": "X_CONFIG_READ", +# "data": { "device": "virtio-net-pci-0", +# "path": "/machine/peripheral/virtio-net-pci-0" }, +# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } +# +## +{ 'event': 'X_CONFIG_READ', + 'data': { '*device': 'str', 'path': 'str' } } diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index b485375049..d0f022e925 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -1252,3 +1252,8 @@ void qdev_hotplug_device_on_event(DeviceState *dev) dev->device_on_event_sent = true; qapi_event_send_x_device_on(dev->id, dev->canonical_path); } + +void qdev_config_read_event(DeviceState *dev) +{ + qapi_event_send_x_config_read(dev->id, dev->canonical_path); +}
Send a new event when guest reads virtio-pci config after virtio_notify_config() call. That's useful to check that guest fetched modified config, for example after resizing disk backend. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- hw/virtio/virtio-pci.c | 9 +++++++++ include/monitor/qdev.h | 1 + monitor/monitor.c | 1 + qapi/qdev.json | 22 ++++++++++++++++++++++ softmmu/qdev-monitor.c | 5 +++++ 5 files changed, 38 insertions(+)