Re: [PATCH RFC 2/2] docs: rstfy vfio-ap documentation

[Peter Maydell]

On Tue, 28 Jan 2020 at 19:39, Cornelia Huck <address@hidden> wrote:
>
> Move to system/, as this is mostly about configuring vfio-ap.
>
> Signed-off-by: Cornelia Huck <address@hidden>

> diff --git a/docs/vfio-ap.txt b/docs/system/vfio-ap.rst
> similarity index 56%
> rename from docs/vfio-ap.txt
> rename to docs/system/vfio-ap.rst
> index b1eb2deeaf2f..c8c5728e0aaa 100644
> --- a/docs/vfio-ap.txt
> +++ b/docs/system/vfio-ap.rst
> @@ -1,17 +1,17 @@
>  Adjunct Processor (AP) Device
>  =============================
>
> -Contents:
> -=========
> -* Introduction
> -* AP Architectural Overview
> -* Start Interpretive Execution (SIE) Instruction
> -* AP Matrix Configuration on Linux Host
> -* Starting a Linux Guest Configured with an AP Matrix
> -* Example: Configure AP Matrices for Three Linux Guests
> -> -Introduction:
> -============
> +Contents
> +--------
> +* `Introduction`_
> +* `AP Architectural Overview`_
> +* `Start Interpretive Execution (SIE) Instruction`_
> +* `AP Matrix Configuration on Linux Host`_
> +* `Starting a Linux Guest Configured with an AP Matrix`_
> +* `Example: Configure AP Matrices for Three Linux Guests`_
> +

You don't need to manually write out a table of contents. You
can just have one line

.. contents::

and Sphinx will produce an autogenerated table of contents
(including a 'Contents' title) for you.

https://docutils.sourceforge.io/docs/ref/rst/directives.html#table-of-contents

An example where we already use this is
docs/interop/live-block-operations.rst:
https://www.qemu.org/docs/master/interop/live-block-operations.html


> +Introduction
> +------------

rST doesn't require it, but I'd recommend a blank line below
the ---- line; I think it makes the source more readable.

>  The IBM Adjunct Processor (AP) Cryptographic Facility is comprised
>  of three AP instructions and from 1 to 256 PCIe cryptographic adapter cards.
>  These AP devices provide cryptographic functions to all CPUs assigned to a
> @@ -21,8 +21,8 @@ On s390x, AP adapter cards are exposed via the AP bus. This 
> document
>  describes how those cards may be made available to KVM guests using the
>  VFIO mediated device framework.
>
> -AP Architectural Overview:
> -=========================
> +AP Architectural Overview
> +-------------------------
>  In order understand the terminology used in the rest of this document, let's
>  start with some definitions:
>
> @@ -75,7 +75,7 @@ start with some definitions:
>    must be one of the control domains.
>
>  Start Interpretive Execution (SIE) Instruction
> -==============================================
> +----------------------------------------------
>  A KVM guest is started by executing the Start Interpretive Execution (SIE)
>  instruction. The SIE state description is a control block that contains the
>  state information for a KVM guest and is supplied as input to the SIE
> @@ -114,246 +114,254 @@ The APQNs can provide secure key functionality - 
> i.e., a private key is stored
>  on the adapter card for each of its domains - so each APQN must be assigned 
> to
>  at most one guest or the linux host.
>
> -   Example 1: Valid configuration:
> -   ------------------------------
> -   Guest1: adapters 1,2  domains 5,6
> -   Guest2: adapter  1,2  domain 7
> +Example 1: Valid configuration
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +Guest1: adapters 1,2  domains 5,6
> +Guest2: adapter  1,2  domain 7

These don't render correctly -- rST thinks the "Example 1..." line
is a subsection heading because of the underlining, and then renders
the next two lines as runon-text:
"Guest1: adapters 1,2 domains 5,6 Guest2: adapter 1,2 domain 7"

Depending on what you want, you could try one of:
 * use a literal block (which gets you fixed-width font, preserved
   whitespace and linebreaks)
 * use a bulleted list
 * use one of rST's table formats

(is it deliberate that line 1 is "adapters" and line 2 is "adapter" ?)

> -   This is valid because both guests have a unique set of APQNs: Guest1 has
> -   APQNs (1,5), (1,6), (2,5) and (2,6); Guest2 has APQNs (1,7) and (2,7).
> +This is valid because both guests have a unique set of APQNs: Guest1 has
> +APQNs (1,5), (1,6), (2,5) and (2,6); Guest2 has APQNs (1,7) and (2,7).
>
> -   Example 2: Valid configuration:
> -   ------------------------------
> -   Guest1: adapters 1,2 domains 5,6
> -   Guest2: adapters 3,4 domains 5,6
> +Example 2: Valid configuration
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +Guest1: adapters 1,2 domains 5,6
> +Guest2: adapters 3,4 domains 5,6
>
> -   This is also valid because both guests have a unique set of APQNs:
> -      Guest1 has APQNs (1,5), (1,6), (2,5), (2,6);
> -      Guest2 has APQNs (3,5), (3,6), (4,5), (4,6)
> +This is also valid because both guests have a unique set of APQNs:
> +   Guest1 has APQNs (1,5), (1,6), (2,5), (2,6);
> +   Guest2 has APQNs (3,5), (3,6), (4,5), (4,6)

This is differently rendered from the equivalent text in example 1,
because of the indent -- rST treats the 2 indented lines as a block
quote, and indents them in the output, but flows the text into
one long line.

I think personally I'd opt to render like this:

This is also valid because both guests have a unique set of APQNs:

* Guest1 has APQNs (1,5), (1,6), (2,5), (2,6)
* Guest2 has APQNs (3,5), (3,6), (4,5), (4,6)

(ie a bulleted list); but anyway consistency between the examples
would be good.

>
> -   Example 3: Invalid configuration:
> -   --------------------------------
> -   Guest1: adapters 1,2  domains 5,6
> -   Guest2: adapter  1    domains 6,7
> +Example 3: Invalid configuration
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +Guest1: adapters 1,2  domains 5,6
> +Guest2: adapter  1    domains 6,7
>
> -   This is an invalid configuration because both guests have access to
> -   APQN (1,6).
> +This is an invalid configuration because both guests have access to
> +APQN (1,6).

> -      * Individual bits in the mask can be switched on and off by specifying
> -        each bit number to be switched in a comma separated list. Each bit
> -        number string must be prepended with a ('+') or minus ('-') to 
> indicate
> -        the corresponding bit is to be switched on ('+') or off ('-'). Some
> -        valid values are:
> +    * Individual bits in the mask can be switched on and off by specifying
> +      each bit number to be switched in a comma separated list. Each bit
> +      number string must be prepended with a ('+') or minus ('-') to indicate
> +      the corresponding bit is to be switched on ('+') or off ('-'). Some
> +      valid values are::
>
>             "+0"    switches bit 0 on
>             "-13"   switches bit 13 off
>             "+0x41" switches bit 65 on
>             "-0xff" switches bit 255 off

Maybe use a definition list here rather than a literal block?

      valid values are:

      ``+0``
          switches bit 0 on
      ``-13``
          switches bit 13 off

etc. The literal block is fine if you think it looks better though.


>
>  assign_adapter

Since these are filenames, you can enclose them in ``  `` and they'll
render as fixed-width text, which I think makes them stand out a
bit better in the HTML. I'm pretty sure this works even in the
term part of a definition list.

Not in this diff, but should the 'No APQN...' para in the assign_domain
docs really be a 2nd para of the preceding bullet point rather than either
its own bullet point or a paragraph outside the bulleted list ?

(ditto for assign_domain)


> @@ -486,20 +494,22 @@ facilities:
>     The AP facilities feature indicates that AP facilities are installed on 
> the
>     guest. This feature will be exposed for use only if the AP facilities
>     are installed on the host system. The feature is s390-specific and is
> -   represented as a parameter of the -cpu option on the QEMU command line:
> +   represented as a parameter of the -cpu option on the QEMU command line::
>
>        qemu-system-s390x -cpu $model,ap=on|off
>
> -      Where:
> +   Where:
>
> -         $model is the CPU model defined for the guest (defaults to the 
> model of
> -                the host system if not specified).
> +      $model

Formatting $model and ap=on|off with `` `` would look nicer I think
(ditto below)

thanks
-- PMM