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

[Markus Armbruster]

"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.