On Wed, Mar 13, 2024 at 10:40:21AM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <m...@redhat.com> writes:
>
> > On Wed, Mar 13, 2024 at 09:20:08AM +0100, Markus Armbruster wrote:
> >> "Michael S. Tsirkin" <m...@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.
