virtio: add man page for virtqueue(9) functions

[freebsd_igalic.co]

Summary

• alloc functions
• describe virtqueue intr functions
• add virtqueue paddr functions
• add full, empty, index, size and nfree/nused function descriptions
• add dump and notify function description
• enqueue, dequeue, notify and poll

PR: 272716
Sponsored by: The FreeBSD Foundation

[freebsd_igalic.co]

I'd like to first hear some feedback on language, grammar, consistency, before opening this up to the people who know the code.
There's every chance that this man page needs to be broken up into multiple pages, but I don't know yet which ones.

[pstef]

share/man/man9/virtqueue.9

149

the the

159

buses?

219

is is

221

surplus newline?

245

is used as?

[freebsd_igalic.co]

thanks for the review @pstef .
This update should fix it.

share/man/man9/virtqueue.9

159

we got 67 "buses" and 17 "busses".
my dictionary said busses is the American spelling, which I thought we preferred.

[freebsd_igalic.co]

add more reviewers

[fernape]

Just my two cents here.

Thanks for working on this!

share/man/man9/virtqueue.9

161

I would move this up before virtqueue_free paragraph to respect the order set before.

178

I would respect the previous order and say ...enable, ...postpone and ...disable... or sort them alphabetically in the top description.

208

Should probably go before the virtqueue_full paragraph.

229

The function virtqueue_enqueue... --> The virtqueue_enqueue function...

233

This might be removed? It does not add much

235

integers in plural referring to readable and writable, but the latter is too far in the sentence.
I would rephrase as:

uses readable..., and writable... dropping the integers. It is already in the function signature.

259

If not exhausted, what does it return?

263

.F vq --> .Fa vq

Damn! it took me a while to catch this!

265

What's the len parameter for in this case?

[freebsd_igalic.co]

i believe to now have addressed all of your remarks, @fernape.

[fernape]

LGTM but I would like to know the opinion of an src committer before this lands.

[mhorne]

Thanks for working on this, here's my comments after a first pass.

You are missing the additions to share/man/man9/Makefile to install the page and its MLINKS.

share/man/man9/virtqueue.9

18

The C-style comments are strange for a man page. IMO just the text would be fine if you want to split up the prototypes.

31

Here and below.

136

This first paragraph should briefly state what the virtqueue abstraction is: what is the purpose and function of a virtqueue object? Knowing this helps the reader determine if they are looking at the correct page.

141

.Ft is for function return types only. For general C data types, you should use .Vt.

Although, I think several instances of the word 'virtqueue' in this document do not really need markup (including this one). You are either talking about the conceptual 'virtqueue' object (no markup), or the struct virtqueue C type (.Vt).

174

182–193

Here you are trying to explain way too much with one sentence.

How about this:

Several helper functions are provided for obtaining the physical address (vm_paddr_t) of the ring buffer and its components.
The virtqueue_paddr() function returns the address of the underlying ring buffer.
The virtqueue_desc_paddr() function returns the address of the first descriptor in the ring.
The virtqueue_avail_paddr() and virtqueue_used_paddr() functions return the address of the "available" and "used" rings, respectively.
These helper functions are wrappers around vtophys().

This description might feel more natural if preceded by a more general description of the ring buffer, such as the difference between the available and used rings. If most consumers of the virtqueue(9) interface don't need to mess around with these paddrs (i.e. it is an implementation detail), then we might just shorten it and say:

The
.Fn virtqueue_paddr ,
.Fn virtqueue_desc_paddr ,
.Fn virtqueue_avail_paddr ,
and
.Fn virtqueue_used_paddr
helper functions return the physical address of the virtqueue's ring buffer, and the ring's individual components.

280

Still todo :)

282

Please sort alphabetically.

[freebsd_igalic.co]

In D41852#957094, @mhorne wrote:

Thanks for working on this, here's my comments after a first pass.

thank you for reviewing this!

You are missing the additions to share/man/man9/Makefile to install the page and its MLINKS.

the main reason i left this off is because I'm not sure if this page should stay as-is, or needs to be split up.

[freebsd_igalic.co]

share/man/man9/virtqueue.9

18

I think this is kinda part of not knowing whether this page needs to remain as-is, or if it needs to be split up.

136

what is the purpose and function of a virtqueue object?

I haven't quite figured that out fully yet ;)

[mhorne]

In D41852#957104, @freebsd_igalic.co wrote:

In D41852#957094, @mhorne wrote:

Thanks for working on this, here's my comments after a first pass.

thank you for reviewing this!

No problem, I intend to look at the others, but I have quite a few reviews to catch up on so it will take a bit of time.

You are missing the additions to share/man/man9/Makefile to install the page and its MLINKS.

the main reason i left this off is because I'm not sure if this page should stay as-is, or needs to be split up.

I see. IMO this page is fine in its current length and is appropriately split from other virtio-related content. So whatever you think going forward.

[bryanv]

I'm very hesitant to consider these - or other virtio methods from other pending reviews - as man "Section 9 - Kernel Interfaces". The header files are not installed and I really don't think should be, and would require clean up before being suitable. Typically these kind methods are not documented in man pages; I think the only existing analogous man(9) would be usbdi(9) and despite the numerous MLINKs only documents a handful of methods around data transfer, that I presume are most relevant for peripheral drivers, and goes into great detail of how those 5 methods are used and related with each other. I think these proposed man pages require a lot more fleshing out before providing the level of detail to code against the "interface".

share/man/man9/virtqueue.9

143

This is an implementation detail (and it uses more than just malloc) and I don't see why this is relevant.

144

It doesn't associate with the device. That done by a transport specific method later.

168

Thsese don't "attach" - it is really just doing a callback.

218

When things go wrong, this can be the most useful :)

229

This doesn't send data - it is really just sending buffers, that may or may not have any data in them depending how they are being used.

243

The cookie does not imply verifying anything. It is just an opaque pointer that's returned on deq and drain.

[freebsd_igalic.co]

In D41852#957171, @bryanv wrote:

I'm very hesitant to consider these - or other virtio methods from other pending reviews - as man "Section 9 - Kernel Interfaces".

i would happily just put the useful parts of the text from this review as comments into the code

I think these proposed man pages require a lot more fleshing out before providing the level of detail to code against the "interface".

and that's why i haven't written much code yet, because i was trying to understand the interface

share/man/man9/virtqueue.9

136

maybe i can reuse some words from the spec? https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html#x1-230005