| Message ID | cover.1708525606.git.yong.huang@smartx.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Adjust the output of x-query-virtio-status | expand |
On Wed, Feb 21, 2024 at 10:28:50PM +0800, Hyman Huang wrote: > v4: > - Rebase on master > - Fix the syntax mistake within the commit message of [PATCH v3 1/3] > - Adjust the linking file in hw/virtio/meson.build suggested by Markus > > Please review, > Yong I'm still not excited about this. For one this will not scale when we add more than 64 feature bits. As long as we are changing this let's address this please. I would also suggest just keeping the name in there, so a decoded feature will be [0, NAME] and a non-decoded will be just [23] will make for a smaller change. > v3: > - Rebase on master > - Use the refined commit message furnished by Markus for [PATCH v2 1/2] > - Drop the [PATCH v2 2/2] > - Add [PATCH v3 2/3] to declare the decoding functions to static > - Add [PATCH v3 3/3] to Define VhostDeviceProtocols and > VirtioDeviceFeatures as plain C types > > v2: > - Changing the hmp_virtio_dump_xxx function signatures to implement > the bitmap decoding, suggested by Philippe. > > This patchset is derived from the series: > https://lore.kernel.org/qemu-devel/cover.1699793550.git.yong.huang@smartx.com/ > Please go to the link to see more background information. > > The following points are what we have done in the patchset: > 1. Take the policy of adding human-readable output just in HMP. > 2. For the HMP output, display the human-readable information and > drop the unknown bits in practice. > 3. For the QMP output, remove the descriptive strings and only > display bits encoded as numbers. > > Hyman Huang (3): > qmp: Switch x-query-virtio-status back to numeric encoding > virtio: Declare the decoding functions to static > qapi: Define VhostDeviceProtocols and VirtioDeviceFeatures as plain C > types > > hw/virtio/meson.build | 4 +- > hw/virtio/virtio-hmp-cmds.c | 702 +++++++++++++++++++++++++++++++++++- > hw/virtio/virtio-qmp.c | 684 +---------------------------------- > hw/virtio/virtio-qmp.h | 3 - > qapi/virtio.json | 231 +----------- > 5 files changed, 724 insertions(+), 900 deletions(-) > > -- > 2.39.3
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Wed, Feb 21, 2024 at 10:28:50PM +0800, Hyman Huang wrote: >> v4: >> - Rebase on master >> - Fix the syntax mistake within the commit message of [PATCH v3 1/3] >> - Adjust the linking file in hw/virtio/meson.build suggested by Markus >> >> Please review, >> Yong > > I'm still not excited about this. > For one this will not scale when we add more than 64 feature bits. x-query-virtio-status is meant to be a low effort, low level debugging aid. Its feature set members correspond 1:1 to uint64_t members of the C struct, which I figure correspond 1:1 to 64-bit words in the binary virtio interface. If we run out of bits in the binary virtio interface, I guess we'd add another 64-bit word. The C struct acquires another uint64_t member, and so does x-query-virtio-status. What's wrong with that? > As long as we are changing this let's address this please. > I would also suggest just keeping the name in there, so > a decoded feature will be > [0, NAME] > and a non-decoded will be just > [23] > > will make for a smaller change. I'm not sure I understand your suggestion. [...]
On Wed, Mar 13, 2024 at 09:20:08AM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > On Wed, Feb 21, 2024 at 10:28:50PM +0800, Hyman Huang wrote: > >> v4: > >> - Rebase on master > >> - Fix the syntax mistake within the commit message of [PATCH v3 1/3] > >> - Adjust the linking file in hw/virtio/meson.build suggested by Markus > >> > >> Please review, > >> Yong > > > > I'm still not excited about this. > > For one this will not scale when we add more than 64 feature bits. > > x-query-virtio-status is meant to be a low effort, low level debugging > aid. Its feature set members correspond 1:1 to uint64_t members of the > C struct, which I figure correspond 1:1 to 64-bit words in the binary > virtio interface. > If we run out of bits in the binary virtio interface, I guess we'd add > another 64-bit word. The C struct acquires another uint64_t member, and > so does x-query-virtio-status. > > What's wrong with that? Nope, that last part about virtio binary interface is wrong. virtio does not have a 64-bit word in it's ABI, it has an array of bits represented, depending on a transport, as a dynamically sized array of 32-bit words (PCI, MMIO) or a dynamically sized array of bytes (CCW). We are beginning to get closer to filling up 64 bits for some devices so I'm already looking at not baking 64 bit in new code. > > > As long as we are changing this let's address this please. > > I would also suggest just keeping the name in there, so > > a decoded feature will be > > [0, NAME] > > and a non-decoded will be just > > [23] > > > > will make for a smaller change. > > I'm not sure I understand your suggestion. > > [...] For example, for the balloon device: instead of e.g. 0x201 as this patch would do, I propose [ [{0, "VIRTIO_BALLOON_F_MUST_TELL_HOST" }, {9, ""}] ].
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Wed, Mar 13, 2024 at 09:20:08AM +0100, Markus Armbruster wrote: >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> >> > On Wed, Feb 21, 2024 at 10:28:50PM +0800, Hyman Huang wrote: >> >> v4: >> >> - Rebase on master >> >> - Fix the syntax mistake within the commit message of [PATCH v3 1/3] >> >> - Adjust the linking file in hw/virtio/meson.build suggested by Markus >> >> >> >> Please review, >> >> Yong >> > >> > I'm still not excited about this. >> > For one this will not scale when we add more than 64 feature bits. >> >> x-query-virtio-status is meant to be a low effort, low level debugging >> aid. Its feature set members correspond 1:1 to uint64_t members of the >> C struct, which I figure correspond 1:1 to 64-bit words in the binary >> virtio interface. >> If we run out of bits in the binary virtio interface, I guess we'd add >> another 64-bit word. The C struct acquires another uint64_t member, and >> so does x-query-virtio-status. >> >> What's wrong with that? > > Nope, that last part about virtio binary interface is wrong. virtio does > not have a 64-bit word in it's ABI, it has an array of bits represented, > depending on a transport, as a dynamically sized array of 32-bit words > (PCI, MMIO) or a dynamically sized array of bytes (CCW). Then have x-query-virtio-status return a suitable array of unsigned numbers. Look ma, no invention! > We are beginning to get closer to filling up 64 bits for some devices > so I'm already looking at not baking 64 bit in new code. > >> >> > As long as we are changing this let's address this please. >> > I would also suggest just keeping the name in there, so >> > a decoded feature will be >> > [0, NAME] >> > and a non-decoded will be just >> > [23] >> > >> > will make for a smaller change. >> >> I'm not sure I understand your suggestion. >> >> [...] > > For example, for the balloon device: > > instead of e.g. 0x201 as this patch would do, > I propose [ [{0, "VIRTIO_BALLOON_F_MUST_TELL_HOST" }, {9, ""}] ]. Syntactially invalid. I guess you mean something like [{"bit": 0, "name": "VIRTIO_BALLOON_F_MUST_TELL_HOST"}, {"bit": 9, "name": ""}] or with optional @name [{"bit": 0, "name": "VIRTIO_BALLOON_F_MUST_TELL_HOST"}, {"bit": 9}] This is an awfully verbose encoding of an n-bit number, even if we omit "VIRTIO_BALLOON_F_" as noise. I could be awkward for the use case described in PATCH 1's commit message: However, we sometimes want to compare features and status bits without caring for their exact meaning. Say we want to verify the correctness of the virtio negotiation between guest, QEMU, and OVS-DPDK. We can use QMP command x-query-virtio-status to retrieve vhost-user net device features, and the "ovs-vsctl list interface" command to retrieve interface features. Without commit f3034ad71fc, we could then simply compare the numbers. With this commit, we first have to map from the strings back to the numeric encoding. It next describes the patch's solution: Revert the decoding for QMP, but keep it for HMP. This makes the QMP command easier to use for use cases where we don't need to decode, like the comparison above. For use cases where we need to decode, we replace parsing undocumented strings by decoding virtio's well-known binary encoding. Since this is not a stable interface, instead of a perfect (and to my subjective self overengineered) solution at some future point, I'd prefer to get in a simple one *now*, even if we may have to evolve it later.
On Wed, Mar 13, 2024 at 10:40:21AM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > On Wed, Mar 13, 2024 at 09:20:08AM +0100, Markus Armbruster wrote: > >> "Michael S. Tsirkin" <mst@redhat.com> writes: > >> > >> > On Wed, Feb 21, 2024 at 10:28:50PM +0800, Hyman Huang wrote: > >> >> v4: > >> >> - Rebase on master > >> >> - Fix the syntax mistake within the commit message of [PATCH v3 1/3] > >> >> - Adjust the linking file in hw/virtio/meson.build suggested by Markus > >> >> > >> >> Please review, > >> >> Yong > >> > > >> > I'm still not excited about this. > >> > For one this will not scale when we add more than 64 feature bits. > >> > >> x-query-virtio-status is meant to be a low effort, low level debugging > >> aid. Its feature set members correspond 1:1 to uint64_t members of the > >> C struct, which I figure correspond 1:1 to 64-bit words in the binary > >> virtio interface. > >> If we run out of bits in the binary virtio interface, I guess we'd add > >> another 64-bit word. The C struct acquires another uint64_t member, and > >> so does x-query-virtio-status. > >> > >> What's wrong with that? > > > > Nope, that last part about virtio binary interface is wrong. virtio does > > not have a 64-bit word in it's ABI, it has an array of bits represented, > > depending on a transport, as a dynamically sized array of 32-bit words > > (PCI, MMIO) or a dynamically sized array of bytes (CCW). > > Then have x-query-virtio-status return a suitable array of unsigned > numbers. Look ma, no invention! Then with bit 53 in spec user has to squint hard and do okay 53/32=1 so array entry 1 and 53%32=21 so bit 21 there. And for ccw it's just completely weird they have a byte array. No one wants these words everyone works with bit #s this is what's in spec. > > We are beginning to get closer to filling up 64 bits for some devices > > so I'm already looking at not baking 64 bit in new code. > > > >> > >> > As long as we are changing this let's address this please. > >> > I would also suggest just keeping the name in there, so > >> > a decoded feature will be > >> > [0, NAME] > >> > and a non-decoded will be just > >> > [23] > >> > > >> > will make for a smaller change. > >> > >> I'm not sure I understand your suggestion. > >> > >> [...] > > > > For example, for the balloon device: > > > > instead of e.g. 0x201 as this patch would do, > > I propose [ [{0, "VIRTIO_BALLOON_F_MUST_TELL_HOST" }, {9, ""}] ]. > > Syntactially invalid. I guess you mean something like > > [{"bit": 0, "name": "VIRTIO_BALLOON_F_MUST_TELL_HOST"}, > {"bit": 9, "name": ""}] > > or with optional @name > > [{"bit": 0, "name": "VIRTIO_BALLOON_F_MUST_TELL_HOST"}, > {"bit": 9}] > > This is an awfully verbose encoding of an n-bit number, even if we omit > "VIRTIO_BALLOON_F_" as noise. > > I could be awkward for the use case described in PATCH 1's commit > message: > > However, we sometimes want to compare features and status bits without > caring for their exact meaning. Say we want to verify the correctness > of the virtio negotiation between guest, QEMU, and OVS-DPDK. We can use > QMP command x-query-virtio-status to retrieve vhost-user net device > features, and the "ovs-vsctl list interface" command to retrieve > interface features. Without commit f3034ad71fc, we could then simply > compare the numbers. With this commit, we first have to map from the > strings back to the numeric encoding. > > It next describes the patch's solution: > > Revert the decoding for QMP, but keep it for HMP. > > This makes the QMP command easier to use for use cases where we > don't need to decode, like the comparison above. For use cases > where we need to decode, we replace parsing undocumented strings by > decoding virtio's well-known binary encoding. > > Since this is not a stable interface, instead of a perfect (and to my > subjective self overengineered) solution at some future point, I'd > prefer to get in a simple one *now*, even if we may have to evolve it > later.
On Wed, Mar 13, 2024 at 10:40:21AM +0100, Markus Armbruster wrote: > I could be awkward for the use case described in PATCH 1's commit > message: > > However, we sometimes want to compare features and status bits without > caring for their exact meaning. Say we want to verify the correctness > of the virtio negotiation between guest, QEMU, and OVS-DPDK. We can use > QMP command x-query-virtio-status to retrieve vhost-user net device > features, and the "ovs-vsctl list interface" command to retrieve > interface features. Without commit f3034ad71fc, we could then simply > compare the numbers. With this commit, we first have to map from the > strings back to the numeric encoding. So, consider how guest kernel presents features then. Do you happen to know? It's actually a binary string: static ssize_t features_show(struct device *_d, struct device_attribute *attr, char *buf) { struct virtio_device *dev = dev_to_virtio(_d); unsigned int i; ssize_t len = 0; /* We actually represent this as a bitstring, as it could be * arbitrary length in future. */ for (i = 0; i < sizeof(dev->features)*8; i++) len += sysfs_emit_at(buf, len, "%c", __virtio_test_bit(dev, i) ? '1' : '0'); len += sysfs_emit_at(buf, len, "\n"); return len; } static DEVICE_ATTR_RO(features);
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Wed, Mar 13, 2024 at 10:40:21AM +0100, Markus Armbruster wrote: >> I could be awkward for the use case described in PATCH 1's commit >> message: >> >> However, we sometimes want to compare features and status bits without >> caring for their exact meaning. Say we want to verify the correctness >> of the virtio negotiation between guest, QEMU, and OVS-DPDK. We can use >> QMP command x-query-virtio-status to retrieve vhost-user net device >> features, and the "ovs-vsctl list interface" command to retrieve >> interface features. Without commit f3034ad71fc, we could then simply >> compare the numbers. With this commit, we first have to map from the >> strings back to the numeric encoding. > > So, consider how guest kernel presents features then. Do you happen to know? > It's actually a binary string: > > static ssize_t features_show(struct device *_d, > struct device_attribute *attr, char *buf) > { > struct virtio_device *dev = dev_to_virtio(_d); > unsigned int i; > ssize_t len = 0; > > /* We actually represent this as a bitstring, as it could be > * arbitrary length in future. */ > for (i = 0; i < sizeof(dev->features)*8; i++) > len += sysfs_emit_at(buf, len, "%c", > __virtio_test_bit(dev, i) ? '1' : '0'); > len += sysfs_emit_at(buf, len, "\n"); > return len; > } > static DEVICE_ATTR_RO(features); I'm willing to accept any reasonably compact representation of the bits that is easy to use for the comparison use case. I strongly prefer integers as long as their width suffices. Since out integers are limited to 64 bits, and you want us to design for more, we need something else. I'm fine with array of integers, but you don't like it. We generally avoid encoding stuff in strings, but using strings for "overwide" integers isn't as bad as using them for structured data. I guess I'd be okay with it. I'd use decimal simply to keep these "overwide" integers as close as possible to regular ones. If using base 2 enables string compare for the comparison use case, that's an argument for base 2. Hyman Huang, can you show us example output of "ovs-vsctl list interface"?
On Fri, Mar 15, 2024 at 3:51 PM Markus Armbruster <armbru@redhat.com> wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > On Wed, Mar 13, 2024 at 10:40:21AM +0100, Markus Armbruster wrote: > >> I could be awkward for the use case described in PATCH 1's commit > >> message: > >> > >> However, we sometimes want to compare features and status bits > without > >> caring for their exact meaning. Say we want to verify the > correctness > >> of the virtio negotiation between guest, QEMU, and OVS-DPDK. We > can use > >> QMP command x-query-virtio-status to retrieve vhost-user net device > >> features, and the "ovs-vsctl list interface" command to retrieve > >> interface features. Without commit f3034ad71fc, we could then > simply > >> compare the numbers. With this commit, we first have to map from > the > >> strings back to the numeric encoding. > > > > So, consider how guest kernel presents features then. Do you happen to > know? > > It's actually a binary string: > > > > static ssize_t features_show(struct device *_d, > > struct device_attribute *attr, char *buf) > > { > > struct virtio_device *dev = dev_to_virtio(_d); > > unsigned int i; > > ssize_t len = 0; > > > > /* We actually represent this as a bitstring, as it could be > > * arbitrary length in future. */ > > for (i = 0; i < sizeof(dev->features)*8; i++) > > len += sysfs_emit_at(buf, len, "%c", > > __virtio_test_bit(dev, i) ? '1' : '0'); > > len += sysfs_emit_at(buf, len, "\n"); > > return len; > > } > > static DEVICE_ATTR_RO(features); > > I'm willing to accept any reasonably compact representation of the bits > that is easy to use for the comparison use case. > > I strongly prefer integers as long as their width suffices. > > Since out integers are limited to 64 bits, and you want us to design for > more, we need something else. > > I'm fine with array of integers, but you don't like it. > > We generally avoid encoding stuff in strings, but using strings for > "overwide" integers isn't as bad as using them for structured data. I > guess I'd be okay with it. > > I'd use decimal simply to keep these "overwide" integers as close as > possible to regular ones. > > If using base 2 enables string compare for the comparison use case, > that's an argument for base 2. > > Hyman Huang, can you show us example output of "ovs-vsctl list > interface"? > > Here is the output of an active interface enable the dpdk feature: # ovs-vsctl list interface port-testvm2pn8 _uuid: xxxx admin_state: up name: port-testvm2pn8 ...... status: {features="0x000000017060a782", mode=client, num_of_vrings="4"......} Yong
v4: - Rebase on master - Fix the syntax mistake within the commit message of [PATCH v3 1/3] - Adjust the linking file in hw/virtio/meson.build suggested by Markus Please review, Yong v3: - Rebase on master - Use the refined commit message furnished by Markus for [PATCH v2 1/2] - Drop the [PATCH v2 2/2] - Add [PATCH v3 2/3] to declare the decoding functions to static - Add [PATCH v3 3/3] to Define VhostDeviceProtocols and VirtioDeviceFeatures as plain C types v2: - Changing the hmp_virtio_dump_xxx function signatures to implement the bitmap decoding, suggested by Philippe. This patchset is derived from the series: https://lore.kernel.org/qemu-devel/cover.1699793550.git.yong.huang@smartx.com/ Please go to the link to see more background information. The following points are what we have done in the patchset: 1. Take the policy of adding human-readable output just in HMP. 2. For the HMP output, display the human-readable information and drop the unknown bits in practice. 3. For the QMP output, remove the descriptive strings and only display bits encoded as numbers. Hyman Huang (3): qmp: Switch x-query-virtio-status back to numeric encoding virtio: Declare the decoding functions to static qapi: Define VhostDeviceProtocols and VirtioDeviceFeatures as plain C types hw/virtio/meson.build | 4 +- hw/virtio/virtio-hmp-cmds.c | 702 +++++++++++++++++++++++++++++++++++- hw/virtio/virtio-qmp.c | 684 +---------------------------------- hw/virtio/virtio-qmp.h | 3 - qapi/virtio.json | 231 +----------- 5 files changed, 724 insertions(+), 900 deletions(-)