Skip to content

Comments

Review v1/v0#31

Open
bertrand-marquis wants to merge 36 commits intoLinaro:virtio-msg-patch1from
bertrand-marquis:review-v1/v0
Open

Review v1/v0#31
bertrand-marquis wants to merge 36 commits intoLinaro:virtio-msg-patch1from
bertrand-marquis:review-v1/v0

Conversation

@bertrand-marquis
Copy link
Collaborator

Hi Everyone,

This pull request include a number of fixes or changes to handle the review comment we got on virtio mailing list.

Some points are not handled because they need a decision between us first (following summary was generated by chatgpt):

Ordering / Out‑of‑Order Feature Discussion

  • Parav Pandit proposes relaxing the strict request/response ordering because it is too constraining for high‑performance data paths. He suggests that the bus may need to deliver responses out of
    order, and the spec should support that for performance.
  • Demi Marie Obenour raises reset and out‑of‑order race risks if ordering is relaxed. She suggests a generation/stream number or bus‑level reset semantics to avoid stale or mis‑matched responses.
  • Michael S. Tsirkin recommends keeping v1 simple and introducing performance features via optional feature bits. This aligns with making relaxed ordering a negotiated capability rather than a
    baseline requirement.

Configuration Generation Optional Feature Discussion

  • Peter Hilber suggests making the SET_CONFIG generation count optional or dropping it, because Linux drivers assume config writes are not rejected and do not retry. His rationale is avoiding
    incompatibility with existing driver behavior.

VQ Enabled Flag Discussion

  • Peter Hilber suggests adding an explicit “queue enabled” indication in SET_VQUEUE (and also reporting it in GET_VQUEUE) so drivers can avoid an extra GET_VQUEUE round‑trip just to confirm
    enablement. This is aimed at reducing overhead in the initialization flow.

Device Number Size Discussion

  • Parav Pandit argues the 16‑bit device number is too small; he suggests 32‑bit to cover >64k devices and PCI BDF domain usage.
  • Demi Marie Obenour argues device identifiers should be 64‑bit or even 128‑bit to ensure long‑term uniqueness.
  • Peter Hilber highlights that reuse of device numbers after removal can cause races, and suggests adding a reuse‑limiting policy. He ties this to the broader question of expanding the device number
    space.

EVENT_CONFIG Data Mandatory Discussion

  • Demi Marie Obenour proposes making EVENT_CONFIG always include configuration data (MUST), to avoid a round trip. If data is mandatory, she notes that offset/length‑only signaling becomes
    unnecessary.
  • There is a noted contradiction with earlier guidance (Peter Hilber) and the current spec change ( I already updated the spec to require offset/length even when data is omitted, keeping data optional as I think this makes sense and i am not quite sure we can mandate the data without breaking compatibility with other transports or existing devices). This needs a clear decision.

Peter noted inconsistent use of “device number” vs “device ID”. I did not
find other ambiguous occurrences in transport-msg, so I added a sentence
explicitly distinguishing the bus-assigned device number from the virtio
device ID returned by GET_DEVICE_INFO.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Peter noted minor editorial issues (“Implement”/“respond”).
Update the Device bullet to “Implements” and “responds”.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Peter noted inconsistent naming. Update the virtio-msg revision wording
and paragraph title to consistently use “transport revision”.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Peter requested active-voice wording for the reserved header bits
requirement. Rephrase to make the bus implementation the subject.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Peter noted this requirement was already stated in the Initialization
Flow driver requirements. Remove the duplicate from the Device
Information driver requirements to avoid repetition.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Peter questioned “reset” on DEVICE_BUS_STATE_REMOVED. Rephrase the
intended-usage text to avoid implying a device reset after removal and
to focus on driver-side teardown.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Peter noted the spec implied “one or more” devices. Update the device
numbering section to allow zero devices, matching the earlier bus-layer
statement and hotplug use cases.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Demi suggested a non-normative note discouraging configuration space for
bulk data when a virtqueue can be used. Add a short explanatory note in
Device Configuration.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Parav requested symmetric naming for device hotplug events. Rename the
READY state to ADDED throughout the EVENT_DEVICE definition and related
text; values remain unchanged.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Peter asked whether transport feature bits are negotiated. Clarify that
the bus presents only the common subset and add a bus requirement to
advertise only mutually supported bits.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Peter asked whether devices may reset the configuration generation count.
Allow reset (including on device reset) and require drivers to avoid
assuming monotonic or persistent values.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Rephrase driver and device notification text to focus on which messages
are used, avoiding timing-specific requirements for EVENT_AVAIL,
EVENT_CONFIG, and EVENT_USED.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Strengthen EVENT_USED requirements to MUST, with explicit exceptions
for polling or other agreed-upon completion mechanisms.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Remove the runtime requirement to issue RESET_VQUEUE before
reprogramming a queue, since it does not avoid races.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Use le32 for admin_vq_start and admin_vq_count to match max_virtqueues.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
PROPOSAL: Clarify that echoed request parameters in responses are
informational and do not change the ordering-based correlation model.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Make device requirements explicit: if features are unsupported, the
device MUST clear FEATURES_OK in the status response.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Use le64 for shared memory length and address, and add a reserved
padding field aligned with existing message layouts.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Require EVENT_CONFIG to carry the affected offset/length even when
data is omitted, and update related guidance.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Clarify that DEVICE_NEEDS_RESET is reported via status updates without
repeating EVENT_CONFIG-specific requirements.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Require the bus to discard invalid bus msg_ids and leave transport
msg_id validation to the transport endpoints.

NOTE: I am unsure whether the explicit "MUST NOT validate transport msg_id"
line is necessary; please comment if a softer requirement is preferred.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Remove the redundant sentence in SET_DRIVER_FEATURES device requirements.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Add a non-normative description of bus-level rejection for forbidden
requests and how it can be surfaced to the driver.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Rephrase the Message Ordering requirements so the device, not the bus,
MUST send exactly one response per request. The bus ordering guarantee
remains unchanged.

Signed-off-by: Bertrand Marquis <Bertrand.Marquis@arm.com>
Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Add an explicit note that virtqueue descriptors and buffer data are
exchanged via shared memory or other DMA-accessible memory. Transport
messages remain control-plane only.

Signed-off-by: Bertrand Marquis <Bertrand.Marquis@arm.com>
Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Clarify that event notifications are independent and may be delivered
out of order with respect to other events, while request/response
ordering remains unchanged.

Signed-off-by: Bertrand Marquis <Bertrand.Marquis@arm.com>
Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Align virtqueue and shared-memory address wording with mmio/pci by
consistently referring to physical addresses throughout the virtio-msg
transport text and message definitions.

Signed-off-by: Bertrand Marquis <Bertrand.Marquis@arm.com>
Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
The configuration generation count is now allowed to reset, but the
normative text still required unique values for EVENT_CONFIG across the
entire device lifetime. Scope the uniqueness requirement to the period
since the last generation-count reset to remove the contradiction while
preserving the intended sequencing guarantee.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
EVENT_CONFIG is used for both configuration changes and status updates,
but the offset/length fields only describe configuration ranges. Define
that status-only updates set offset and length to zero, removing the
ambiguity while keeping range reporting for configuration updates.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
The Device Notifications section only allows EVENT_USED omission when
polling or another completion mechanism is used. Drop the extra
"driver explicitly disabled" clause from the message definition to
match the normative text and keep the rules consistent.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
The common header section implied the bus must sanitize reserved bits
when forwarding transport messages. Rephrase the rule to apply to the
message originator and receivers, preserving forward-compatibility
requirements without conflicting with bus forwarding behavior.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Mandatory messages such as GET_VQUEUE/SET_VQUEUE are 40-byte payloads
plus the 8-byte header. Update the revision table and the normative
minimum to 48 bytes so the minimum advertised size can carry all
mandatory messages.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
The bus is not responsible for validating device numbers. Drop the
"MUST NOT forward" clause from device-number assignment and clarify
that routing failures may be dropped or surfaced as a transport error.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
GET_DEVICE_INFO does not report shared memory segments. Update the
Device Information description to mention admin virtqueues only and
remove the shared-memory reference.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
The revision table shows the protocol-defined maximum message size,
while the text recommends advertising no more than 264 bytes. Add a
sentence to distinguish the protocol maximum from the recommended
advertised maximum.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Update the label path for "Bus Specific Messages" to match the Bus
Messages section hierarchy, keeping labels consistent with the document
structure.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant