Re: [PATCH RFC v2 1/1] virtio-msg: Add virtio-msg, a message based virtio transport layer

Hi Andrei,

> On 6 Mar 2026, at 02:33, Andrei Homescu <[email protected]> wrote:
>
> Some minor comments below based on the Trusty implementation of
> virtio-msg, and potential future issues for implementers:
>
>> +  \item \textbf{Number of Feature Bits}: Indicates how many bits (organized in
>> +    32-bit blocks) the device uses for feature negotiation.
> I think v1 required this to be a multiple of 32, and would reject
> invalid values (might be misremembering).
> IMHO it would be simpler for implementers if this was "number of
> feature blocks" instead, like in other places.
> Is there a specific reason it is "bits" here?

Yes definitely and i think this was already pointed out by someone.

It does make sense to clear this up and have this work on 32bit blocks and
have a number of blocks in the header.

I think at some point we had discussion on the implementation impact as giving
a number of blocks here but a size in the header will require to do a conversion
where having this saying number of bits could make implementation a bit simpler.

But looking at this more deeply i think the multiply by 4 and round up saying that
unused/invalid feature bits have to be 0 is making the wording and logic simpler.

Unless someone has arguments for having here number of bits, i will make this
coherent and use 32bit blocks here.

>
>> +The driver \emph{MAY} issue new \msgref{SET_VQUEUE} for virtqueues that have not
>> +yet be setup.  If the VIRTIO_F_RING_RESET feature has been negotiated,
>> +individual virtqueues can be reset and then optionally re-configured.
> This implies that the device should reject SET_VQUEUE for queues that
> have already been setup?
> Or could a vqueue be configured multiple times?

We had several discussions on the subject and there will be an enable added in set_vqueue.
So the idea will be that as along as a queue is not enabled you can reconfigure it.
Once the queue is enabled, you have to reset it to re-configure it.

>
>> +\msgdef{SET_CONFIG}
>> +
>> +This message is sent by the virtio-msg transport driver and requires a
>> +response from the device.
>> +
>> +\begin{tabular}{|l|l|l|l|}
>> +\hline
>> +Type & Offset & Size (bytes) & Content \\
>> +\hline \hline
>> +Request & 0 & 4 & Configuration generation count \\
>> +& 4 & 4 & Configuration offset in bytes \\
>> +& 8 & 4 & Number of bytes \\
>> +& 12 & ... & Configuration data \\
>> +\hline
>> +Answer & 0 & 4 & New Configuration generation count \\
> Is this a copy of the generation count from the request, or the actual
> value from the device?

This is the actual value from the device after the config changes have
been done.

>
>> +& 4 & 4 & Configuration offset in bytes \\
>> +& 8 & 4 &  Number of bytes, or 0 if rejected \\
>> +& 12 & ... & Configuration data \\
> Is this empty in the "if rejected" case, or a copy of the request data?
> My interpretation is "empty", but perhaps this should be specified.

I would say this is empty right now.
It could optimize things and prevent a GET_CONFIG if we return the data
but as the data modified might be outside of the area touched by SET_CONFIG
I think it would make things complex to say that you can get an error and a different
area of data that was modified (and device does not know what is the cache status
on the other side).

So I would say empty unless someone has an other idea.

>
>> +\busdef{GET_DEVICES}
>> +
>> +This message is sent by the virtio-msg driver side bus and requires a response
>> +from the device side bus.
>> +
>> +\begin{tabular}{|l|l|l|l|}
>> +\hline
>> +Type & Offset & Size (bytes) & Content \\
>> +\hline \hline
>> +Request & 0 & 2 & Offset \\
>> +        & 2 & 2 & Number of device numbers requested \\
>> +\hline
>> +Answer & 0 & 2 & Offset \\
>> +       & 2 & 2 & Number of device numbers in the answer \\
> How should a driver handle the case where a misbehaving device sends
> back more devices than requested? (in other words, if this value is
> greater than the device count in the request)

In theory there is nothing wrong in the device deciding to use the full payload
it can transfer. This is not really expected but I do not see a valid argument
to reject that.

> Or is that not misbehaving?

I would say it is a bit weird but still compliant to the spec.

> A driver implementation could ask for e.g. 32 devices at a time and
> store the resulting bitmap in a uint32_t.
> How should it handle getting back more devices than it has space for?

I would say it can either ignore the rest of the device number and do an other GET
or the code could be able to iterate.
At the end the driver side has no idea of how many devices are available at an offset
so to reduce the number of messages i would expect it to ask for as much as it can
in one go and for the device to answer with a smaller set if there are no devices in
a part of the set requested.

>
>> +The offset and number of device numbers requested \emph{MUST} be multiples of 8.
>> +The next offset \emph{MUST} also be a multiple of 8.
> Same question here as for feature bits: why not use bytes (defined as
> 8 bits, or alternatively octets) instead of bits as the unit for
> GET_DEVICES?
> Then the device implementation wouldn't need to check for and reject
> invalid requests.

Good point, we should make things coherent and use the unit directly instead of asking
for multiple when not necessary.

I will do that in v2 and also handle the rest of your comments.

Thanks a lot for the review.

Cheers
Bertrand

>
> Andrei Homescu

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.