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

Fenghua Yu
2020-04-27T20:14:57Z

On Sun, Apr 26, 2020 at 01:02:12PM +0200, Thomas Gleixner wrote:
> Fenghua Yu <fenghua.yu@intel.com> writes:
>
> s/Add a documentation/Add documentation/
>
> > From: Ashok Raj <ashok.raj@intel.com>
> >
> > 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@intel.com>
> > Co-developed-by: Fenghua Yu <fenghua.yu@intel.com>
> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > Reviewed-by: Tony Luck <tony.luck@intel.com>
> > ---
> > 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