Re: [PATCH v4 0/3] Adjust the output of x-query-virtio-status

[Michael S. Tsirkin]

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.