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

[Cornelia Huck]

On Fri, 7 Feb 2020 12:43:03 +0000
Peter Maydell <address@hidden> wrote:

> 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

Oh, nice, will do. (Not sure how useful this was in the text document,
anyway.)

> 
> 
> > +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

Hm... I think this is supposed to be:
- header ("Example 1: ...")
- config
- explanation why this is a valid config

Maybe a table? Tony, any preferences?

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

I don't think so.

> 
> > -   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.

Nod. The indentation in the document was a bit inconsistent, it just
did not matter before.

> 
> >
> > -   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.

Not sure if a definition list for something that is basically a list of
examples makes sense. Will check how it renders.

> 
> 
> >
> >  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.

That's probably a good idea, will check.

> 
> 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 ?

Will also check, I mostly converted this document via a whack-a-mole
approach.

> 
> (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)

Yep, I'll check if there is anything else that should be fixed-width.

> 
> thanks
> -- PMM
> 

Thanks for looking!