Re: [PATCH 1/7] docs: x86: Add a documentation for ENQCMD

[Fenghua Yu]

On Sun, Apr 26, 2020 at 01:02:12PM +0200, Thomas Gleixner wrote:
> Fenghua Yu <fenghua.yu@xxxxxxxxx> writes:
> 
> s/Add a documentation/Add documentation/
> 
> > From: Ashok Raj <ashok.raj@xxxxxxxxx>
> >
> > ENQCMD and Data Streaming Accelerator (DSA) and all of their associated
> > features are a complicated stack with lots of interconnected pieces.
> > This documentation provides a big picture overview for all of the
> > features.
> >
> > Signed-off-by: Ashok Raj <ashok.raj@xxxxxxxxx>
> > Co-developed-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> > Signed-off-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> > Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx>
> > ---
> >  Documentation/x86/enqcmd.rst | 185 +++++++++++++++++++++++++++++++++++
> 
> How is that hooked up into the Documentation index?
> 
>  Documentation/x86/enqcmd.rst: WARNING: document isn't included in any toctree
> 
> > +++ b/Documentation/x86/enqcmd.rst
> > @@ -0,0 +1,185 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +Improved Device Interaction Overview
> 
> So the document is about ENQCMD, right? Can you please make that in some
> way consistently named?
> 
> > +
> > +== Background ==
> 
> This lacks any docbook formatting.... The resulting HTML looks like ...
> 
> > +
> > +Shared Virtual Addressing (SVA) allows the processor and device to use the
> > +same virtual addresses avoiding the need for software to translate virtual
> > +addresses to physical addresses. ENQCMD is a new instruction on Intel
> > +platforms that allows user applications to directly notify hardware of new
> > +work, much like doorbells are used in some hardware, but carries a payload
> > +that carries the PASID and some additional device specific commands
> > +along with it.
> 
> Sorry that's not background information, that's an agglomeration of
> words.
> 
> Can you please explain properly what's the background of SVA, how it
> differs from regular device addressing and what kind of requirements it
> has?
> 
> ENQCMD is not related to background. It's part of the new technology.
> 
> > +== Address Space Tagging ==
> > +
> > +A new MSR (MSR_IA32_PASID) allows an application address space to be
> > +associated with what the PCIe spec calls a Process Address Space ID
> > +(PASID). This PASID tag is carried along with all requests between
> > +applications and devices and allows devices to interact with the process
> > +address space.
> 
> Sigh. The important part here is not the MSR. The important part is to
> explain what PASID is and where it comes from. Documentation has similar
> rules as changelogs:
> 
>       1) Provide context
> 
>       2) Explain requirements
>       
>       3) Explain implementation
> 
> The pile you provided is completely backwards and unstructured.

Ok. Will address all of the comments.

Thanks.

-Fenghua