diff mbox series

[v3] xen/public: move xenstore related doc into 9pfs.h

Message ID 20230130090937.31623-1-jgross@suse.com (mailing list archive)
State New, archived
Headers show
Series [v3] xen/public: move xenstore related doc into 9pfs.h | expand

Commit Message

Jürgen Groß Jan. 30, 2023, 9:09 a.m. UTC
The Xenstore related documentation is currently to be found in
docs/misc/9pfs.pandoc, instead of the related header file
xen/include/public/io/9pfs.h like for most other paravirtualized
device protocols.

There is a comment in the header pointing at the document, but the
given file name is wrong. Additionally such headers are meant to be
copied into consuming projects (Linux kernel, qemu, etc.), so pointing
at a doc file in the Xen git repository isn't really helpful for the
consumers of the header.

This situation is far from ideal, which is already being proved by the
fact that neither qemu nor the Linux kernel are implementing the
device attach/detach protocol correctly. Additionally the documented
Xenstore entries are not matching the reality, as the "tag" Xenstore
entry is on the frontend side, not on the backend one.

There is another bug in the connection sequence: the frontend needs to
wait for the backend to have published its features before being able
to allocate its rings and event-channels.

Change that by moving the Xenstore related 9pfs documentation from
docs/misc/9pfs.pandoc into xen/include/public/io/9pfs.h while fixing
the wrong Xenstore entry detail and the connection sequence.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- add reference to header in the pandoc document (Jan Beulich)
V3:
- fix flaw in the connection sequence
---
 docs/misc/9pfs.pandoc        | 153 +-----------------------------
 xen/include/public/io/9pfs.h | 178 ++++++++++++++++++++++++++++++++++-
 2 files changed, 181 insertions(+), 150 deletions(-)

Comments

Julien Grall Jan. 30, 2023, 9:18 a.m. UTC | #1
Hi Juergen,

On 30/01/2023 09:09, Juergen Gross wrote:
> The Xenstore related documentation is currently to be found in
> docs/misc/9pfs.pandoc, instead of the related header file
> xen/include/public/io/9pfs.h like for most other paravirtualized
> device protocols.
> 
> There is a comment in the header pointing at the document, but the
> given file name is wrong. Additionally such headers are meant to be
> copied into consuming projects (Linux kernel, qemu, etc.), so pointing
> at a doc file in the Xen git repository isn't really helpful for the
> consumers of the header.
> 
> This situation is far from ideal, which is already being proved by the
> fact that neither qemu nor the Linux kernel are implementing the
> device attach/detach protocol correctly. Additionally the documented
> Xenstore entries are not matching the reality, as the "tag" Xenstore
> entry is on the frontend side, not on the backend one.
> 
> There is another bug in the connection sequence: the frontend needs to
> wait for the backend to have published its features before being able
> to allocate its rings and event-channels.
> 
> Change that by moving the Xenstore related 9pfs documentation from
> docs/misc/9pfs.pandoc into xen/include/public/io/9pfs.h while fixing
> the wrong Xenstore entry detail and the connection sequence.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V2:
> - add reference to header in the pandoc document (Jan Beulich)
> V3:
> - fix flaw in the connection sequence

Please don't do that in the same patch. This makes a lot more difficult 
to confirm the doc movement was correct.

Cheers,
Jürgen Groß Jan. 30, 2023, 9:23 a.m. UTC | #2
On 30.01.23 10:18, Julien Grall wrote:
> Hi Juergen,
> 
> On 30/01/2023 09:09, Juergen Gross wrote:
>> The Xenstore related documentation is currently to be found in
>> docs/misc/9pfs.pandoc, instead of the related header file
>> xen/include/public/io/9pfs.h like for most other paravirtualized
>> device protocols.
>>
>> There is a comment in the header pointing at the document, but the
>> given file name is wrong. Additionally such headers are meant to be
>> copied into consuming projects (Linux kernel, qemu, etc.), so pointing
>> at a doc file in the Xen git repository isn't really helpful for the
>> consumers of the header.
>>
>> This situation is far from ideal, which is already being proved by the
>> fact that neither qemu nor the Linux kernel are implementing the
>> device attach/detach protocol correctly. Additionally the documented
>> Xenstore entries are not matching the reality, as the "tag" Xenstore
>> entry is on the frontend side, not on the backend one.
>>
>> There is another bug in the connection sequence: the frontend needs to
>> wait for the backend to have published its features before being able
>> to allocate its rings and event-channels.
>>
>> Change that by moving the Xenstore related 9pfs documentation from
>> docs/misc/9pfs.pandoc into xen/include/public/io/9pfs.h while fixing
>> the wrong Xenstore entry detail and the connection sequence.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V2:
>> - add reference to header in the pandoc document (Jan Beulich)
>> V3:
>> - fix flaw in the connection sequence
> 
> Please don't do that in the same patch. This makes a lot more difficult to 
> confirm the doc movement was correct.

You have to check it manually anyway, as the comment format is prefixing
" * " to every line.

Its not as if the changes would be THAT complicated, and the commit message
is quite clear WHAT is changed.

I can do the split, of course, if you really need that (in this case I'd
end up with 3 patches, one for the move, one for the "tag" entry, and one
for the fix of the connection sequence).


Juergen
Julien Grall Jan. 30, 2023, 9:37 a.m. UTC | #3
Hi,

On 30/01/2023 09:23, Juergen Gross wrote:
> On 30.01.23 10:18, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 30/01/2023 09:09, Juergen Gross wrote:
>>> The Xenstore related documentation is currently to be found in
>>> docs/misc/9pfs.pandoc, instead of the related header file
>>> xen/include/public/io/9pfs.h like for most other paravirtualized
>>> device protocols.
>>>
>>> There is a comment in the header pointing at the document, but the
>>> given file name is wrong. Additionally such headers are meant to be
>>> copied into consuming projects (Linux kernel, qemu, etc.), so pointing
>>> at a doc file in the Xen git repository isn't really helpful for the
>>> consumers of the header.
>>>
>>> This situation is far from ideal, which is already being proved by the
>>> fact that neither qemu nor the Linux kernel are implementing the
>>> device attach/detach protocol correctly. Additionally the documented
>>> Xenstore entries are not matching the reality, as the "tag" Xenstore
>>> entry is on the frontend side, not on the backend one.
>>>
>>> There is another bug in the connection sequence: the frontend needs to
>>> wait for the backend to have published its features before being able
>>> to allocate its rings and event-channels.
>>>
>>> Change that by moving the Xenstore related 9pfs documentation from
>>> docs/misc/9pfs.pandoc into xen/include/public/io/9pfs.h while fixing
>>> the wrong Xenstore entry detail and the connection sequence.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> V2:
>>> - add reference to header in the pandoc document (Jan Beulich)
>>> V3:
>>> - fix flaw in the connection sequence
>>
>> Please don't do that in the same patch. This makes a lot more 
>> difficult to confirm the doc movement was correct.
> 
> You have to check it manually anyway, as the comment format is prefixing
> " * " to every line.
> 
> Its not as if the changes would be THAT complicated, and the commit message
> is quite clear WHAT is changed.

You are breaking two conventional rules [1]:
   1. Patch should only be doing one logical thing
   2. Don't mix code clean-up and code changes

While there are a couple of cases we are tolerating it, I think doing 
code clean-up and code movement at the same time should always be avoided.

> 
> I can do the split, of course, if you really need that (in this case I'd
> end up with 3 patches, one for the move, one for the "tag" entry, and one
> for the fix of the connection sequence).

That would be my preference because the simpler the patches are the 
easier they are to review.

Cheers,

[1] 
https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches_appropriately
Stefano Stabellini Jan. 30, 2023, 9:01 p.m. UTC | #4
On Mon, 30 Jan 2023, Juergen Gross wrote:
> On 30.01.23 10:18, Julien Grall wrote:
> > Hi Juergen,
> > 
> > On 30/01/2023 09:09, Juergen Gross wrote:
> > > The Xenstore related documentation is currently to be found in
> > > docs/misc/9pfs.pandoc, instead of the related header file
> > > xen/include/public/io/9pfs.h like for most other paravirtualized
> > > device protocols.
> > > 
> > > There is a comment in the header pointing at the document, but the
> > > given file name is wrong. Additionally such headers are meant to be
> > > copied into consuming projects (Linux kernel, qemu, etc.), so pointing
> > > at a doc file in the Xen git repository isn't really helpful for the
> > > consumers of the header.
> > > 
> > > This situation is far from ideal, which is already being proved by the
> > > fact that neither qemu nor the Linux kernel are implementing the
> > > device attach/detach protocol correctly. Additionally the documented
> > > Xenstore entries are not matching the reality, as the "tag" Xenstore
> > > entry is on the frontend side, not on the backend one.
> > > 
> > > There is another bug in the connection sequence: the frontend needs to
> > > wait for the backend to have published its features before being able
> > > to allocate its rings and event-channels.
> > > 
> > > Change that by moving the Xenstore related 9pfs documentation from
> > > docs/misc/9pfs.pandoc into xen/include/public/io/9pfs.h while fixing
> > > the wrong Xenstore entry detail and the connection sequence.
> > > 
> > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > > ---
> > > V2:
> > > - add reference to header in the pandoc document (Jan Beulich)
> > > V3:
> > > - fix flaw in the connection sequence
> > 
> > Please don't do that in the same patch. This makes a lot more difficult to
> > confirm the doc movement was correct.
> 
> You have to check it manually anyway, as the comment format is prefixing
> " * " to every line.

Just to give an example, in the past I used vim to remove the " * "
prefix on every line then used diff or wdiff to check that the content
of the old and the new files are identical.
diff mbox series

Patch

diff --git a/docs/misc/9pfs.pandoc b/docs/misc/9pfs.pandoc
index b034fb5fa6..5c82625040 100644
--- a/docs/misc/9pfs.pandoc
+++ b/docs/misc/9pfs.pandoc
@@ -59,155 +59,10 @@  This document does not cover the 9pfs client/server design or
 implementation, only the transport for it.
 
 
-## Xenstore
+## Configuration
 
-The frontend and the backend connect via xenstore to exchange
-information. The toolstack creates front and back nodes with state
-[XenbusStateInitialising]. The protocol node name is **9pfs**.
-
-Multiple rings are supported for each frontend and backend connection.
-
-### Backend XenBus Nodes
-
-Backend specific properties, written by the backend, read by the
-frontend:
-
-    versions
-         Values:         <string>
-
-         List of comma separated protocol versions supported by the backend.
-         For example "1,2,3". Currently the value is just "1", as there is
-         only one version. N.B.: this is the version of the Xen trasport
-         protocol, not the version of 9pfs supported by the server.
-
-    max-rings
-         Values:         <uint32_t>
-
-         The maximum supported number of rings per frontend.
-
-    max-ring-page-order
-         Values:         <uint32_t>
-
-         The maximum supported size of a memory allocation in units of
-         log2n(machine pages), e.g. 1 = 2 pages, 2 == 4 pages, etc. It
-         must be at least 1.
-
-Backend configuration nodes, written by the toolstack, read by the
-backend:
-
-    path
-         Values:         <string>
-
-         Host filesystem path to share.
-
-    tag
-         Values:         <string>
-
-         Alphanumeric tag that identifies the 9pfs share. The client needs
-         to know the tag to be able to mount it.
-
-    security-model
-         Values:         "none"
-
-         *none*: files are stored using the same credentials as they are
-                 created on the guest (no user ownership squash or remap)
-         Only "none" is supported in this version of the protocol.
-
-### Frontend XenBus Nodes
-
-    version
-         Values:         <string>
-
-         Protocol version, chosen among the ones supported by the backend
-         (see **versions** under [Backend XenBus Nodes]). Currently the
-         value must be "1".
-
-    num-rings
-         Values:         <uint32_t>
-
-         Number of rings. It needs to be lower or equal to max-rings.
-
-    event-channel-<num> (event-channel-0, event-channel-1, etc)
-         Values:         <uint32_t>
-
-         The identifier of the Xen event channel used to signal activity
-         in the ring buffer. One for each ring.
-
-    ring-ref<num> (ring-ref0, ring-ref1, etc)
-         Values:         <uint32_t>
-
-         The Xen grant reference granting permission for the backend to
-         map a page with information to setup a share ring. One for each
-         ring.
-
-### State Machine
-
-Initialization:
-
-    *Front*                               *Back*
-    XenbusStateInitialising               XenbusStateInitialising
-    - Query virtual device                - Query backend device
-      properties.                           identification data.
-    - Setup OS device instance.           - Publish backend features
-    - Allocate and initialize the           and transport parameters
-      request ring.                                      |
-    - Publish transport parameters                       |
-      that will be in effect during                      V
-      this connection.                            XenbusStateInitWait
-                 |
-                 |
-                 V
-       XenbusStateInitialised
-
-                                          - Query frontend transport parameters.
-                                          - Connect to the request ring and
-                                            event channel.
-                                                         |
-                                                         |
-                                                         V
-                                                 XenbusStateConnected
-
-     - Query backend device properties.
-     - Finalize OS virtual device
-       instance.
-                 |
-                 |
-                 V
-        XenbusStateConnected
-
-Once frontend and backend are connected, they have a shared page per
-ring, which are used to setup the rings, and an event channel per ring,
-which are used to send notifications.
-
-Shutdown:
-
-    *Front*                            *Back*
-    XenbusStateConnected               XenbusStateConnected
-                |
-                |
-                V
-       XenbusStateClosing
-
-                                       - Unmap grants
-                                       - Unbind evtchns
-                                                 |
-                                                 |
-                                                 V
-                                         XenbusStateClosing
-
-    - Unbind evtchns
-    - Free rings
-    - Free data structures
-               |
-               |
-               V
-       XenbusStateClosed
-
-                                       - Free remaining data structures
-                                                 |
-                                                 |
-                                                 V
-                                         XenbusStateClosed
+The frontend and backend are configured via Xenstore. See [header] for
+the detailed Xenstore entries and the connection protocol.
 
 
 ## Ring Setup
@@ -415,5 +270,5 @@  the *size* field of the 9pfs header.
 
 [paper]: https://www.usenix.org/legacy/event/usenix05/tech/freenix/full_papers/hensbergen/hensbergen.pdf
 [website]: https://github.com/chaos/diod/blob/master/protocol.md
-[XenbusStateInitialising]: https://xenbits.xen.org/docs/unstable/hypercall/x86_64/include,public,io,xenbus.h.html
+[header]: https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/io/9pfs.h;hb=HEAD
 [ring.h]: https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/io/ring.h;hb=HEAD
diff --git a/xen/include/public/io/9pfs.h b/xen/include/public/io/9pfs.h
index ad26bd69eb..88c131820b 100644
--- a/xen/include/public/io/9pfs.h
+++ b/xen/include/public/io/9pfs.h
@@ -14,9 +14,185 @@ 
 #include "ring.h"
 
 /*
- * See docs/misc/9pfs.markdown in xen.git for the full specification:
+ * See docs/misc/9pfs.pandoc in xen.git for the full specification:
  * https://xenbits.xen.org/docs/unstable/misc/9pfs.html
  */
+
+/*
+ ******************************************************************************
+ *                                  Xenstore
+ ******************************************************************************
+ *
+ * The frontend and the backend connect via xenstore to exchange
+ * information. The toolstack creates front and back nodes with state
+ * XenbusStateInitialising. The protocol node name is **9pfs**.
+ *
+ * Multiple rings are supported for each frontend and backend connection.
+ *
+ ******************************************************************************
+ *                            Backend XenBus Nodes
+ ******************************************************************************
+ *
+ * Backend specific properties, written by the backend, read by the
+ * frontend:
+ *
+ *    versions
+ *         Values:         <string>
+ *
+ *         List of comma separated protocol versions supported by the backend.
+ *         For example "1,2,3". Currently the value is just "1", as there is
+ *         only one version. N.B.: this is the version of the Xen transport
+ *         protocol, not the version of 9pfs supported by the server.
+ *
+ *    max-rings
+ *         Values:         <uint32_t>
+ *
+ *         The maximum supported number of rings per frontend.
+ *
+ *    max-ring-page-order
+ *         Values:         <uint32_t>
+ *
+ *         The maximum supported size of a memory allocation in units of
+ *         log2n(machine pages), e.g. 1 = 2 pages, 2 == 4 pages, etc. It
+ *         must be at least 1.
+ *
+ * Backend configuration nodes, written by the toolstack, read by the
+ * backend:
+ *
+ *    path
+ *         Values:         <string>
+ *
+ *         Host filesystem path to share.
+ *
+ *    security-model
+ *         Values:         "none"
+ *
+ *         *none*: files are stored using the same credentials as they are
+ *                 created on the guest (no user ownership squash or remap)
+ *         Only "none" is supported in this version of the protocol.
+ *
+ ******************************************************************************
+ *                            Frontend XenBus Nodes
+ ******************************************************************************
+ *
+ *    version
+ *         Values:         <string>
+ *
+ *         Protocol version, chosen among the ones supported by the backend
+ *         (see **versions** under [Backend XenBus Nodes]). Currently the
+ *         value must be "1".
+ *
+ *    num-rings
+ *         Values:         <uint32_t>
+ *
+ *         Number of rings. It needs to be lower or equal to max-rings.
+ *
+ *    event-channel-<num> (event-channel-0, event-channel-1, etc)
+ *         Values:         <uint32_t>
+ *
+ *         The identifier of the Xen event channel used to signal activity
+ *         in the ring buffer. One for each ring.
+ *
+ *    ring-ref<num> (ring-ref0, ring-ref1, etc)
+ *         Values:         <uint32_t>
+ *
+ *         The Xen grant reference granting permission for the backend to
+ *         map a page with information to setup a share ring. One for each
+ *         ring.
+ *
+ *    tag
+ *         Values:         <string>
+ *
+ *         Alphanumeric tag that identifies the 9pfs share. The client needs
+ *         to know the tag to be able to mount it.
+ *
+ ******************************************************************************
+ *                              State Machine
+ ******************************************************************************
+ *
+ * Initialization:
+ *
+ *    *Front*                               *Back*
+ *    XenbusStateInitialising               XenbusStateInitialising
+ *                                          - Query backend device
+ *                                            identification data.
+ *                                          - Publish backend features
+ *                                            and transport parameters.
+ *                                                         |
+ *                                                         |
+ *                                                         V
+ *                                                  XenbusStateInitWait
+ *
+ *    - Query virtual device
+ *      properties.
+ *    - Query backend features and
+ *      transport parameters.
+ *    - Setup OS device instance.
+ *    - Allocate and initialize the
+ *      request ring(s) and
+ *      event-channel(s).
+ *    - Publish transport parameters
+ *      that will be in effect during
+ *      this connection.
+ *                 |
+ *                 |
+ *                 V
+ *       XenbusStateInitialised
+ *
+ *                                          - Query frontend transport
+ *                                            parameters.
+ *                                          - Connect to the request ring(s)
+ *                                            and event channel(s).
+ *                                                         |
+ *                                                         |
+ *                                                         V
+ *                                                 XenbusStateConnected
+ *
+ *     - Query backend device properties.
+ *     - Finalize OS virtual device
+ *       instance.
+ *                 |
+ *                 |
+ *                 V
+ *        XenbusStateConnected
+ *
+ * Once frontend and backend are connected, they have a shared page per
+ * ring, which are used to setup the rings, and an event channel per ring,
+ * which are used to send notifications.
+ *
+ * Shutdown:
+ *
+ *    *Front*                            *Back*
+ *    XenbusStateConnected               XenbusStateConnected
+ *                |
+ *                |
+ *                V
+ *       XenbusStateClosing
+ *
+ *                                       - Unmap grants
+ *                                       - Unbind evtchns
+ *                                                 |
+ *                                                 |
+ *                                                 V
+ *                                         XenbusStateClosing
+ *
+ *    - Unbind evtchns
+ *    - Free rings
+ *    - Free data structures
+ *               |
+ *               |
+ *               V
+ *       XenbusStateClosed
+ *
+ *                                       - Free remaining data structures
+ *                                                 |
+ *                                                 |
+ *                                                 V
+ *                                         XenbusStateClosed
+ *
+ ******************************************************************************
+ */
+
 DEFINE_XEN_FLEX_RING_AND_INTF(xen_9pfs);
 
 #endif