On Wed, 2017-01-18 at 10:02 +0800, Yi Sun wrote:
> This patch creates L2 CAT feature document in doc/features/.
> It describes details of L2 CAT.
> 
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> ---
>
Hey,

it is very very useful to put _RIGHT_HERE_ a summary of what changed,
within, this patch, wrt the previous version.

That helps reviewers quite a bit, especially in making sure that you
hav considered and addressed comments made during the previous
iterations.

>  docs/features/intel_psr_l2_cat.pandoc | 347
> ++++++++++++++++++++++++++++++++++
>  1 file changed, 347 insertions(+)
>  create mode 100644 docs/features/intel_psr_l2_cat.pandoc

> --- /dev/null
> +++ b/docs/features/intel_psr_l2_cat.pandoc
> @@ -0,0 +1,347 @@
> +% Intel L2 Cache Allocation Technology (L2 CAT) Feature
> +% Revision 1.0
> +
> +\clearpage
> +
> +# Basics
> +
> +---------------- -------------------------------------------------
> ---
> +         Status: **Tech Preview**
> +
> +Architecture(s): Intel x86
> +
> +   Component(s): Hypervisor, toolstack
> +
> +       Hardware: Atom codename Goldmont and beyond CPUs
> +---------------- -------------------------------------------------
> ---
> +
> +# Overview
> +
> +L2 CAT allows an OS or Hypervisor/VMM to control allocation of a
> +CPU's shared L2 cache based on application priority or Class of
> Service
> +(COS). Each CLOS is configured using capacity bitmasks (CBM) which
> +represent cache capacity and indicate the degree of overlap and
> +isolation between classes. Once L2 CAT is configured, the processor
> +allows access to portions of L2 cache according to the established
> +class of service.
> +
Well, considering that even here in the overview, acronyms are used,
that are then defined in the section below, it would look better to me
to move the Terminology section up (and let it be the first one).

> +## Terminology
> +
> +* CAT         Cache Allocation Technology
> +* CBM         Capacity BitMasks
> +* CDP         Code and Data Prioritization
> +* COS/CLOS    Class of Service
> +* MSRs        Machine Specific Registers
> +* PSR         Intel Platform Shared Resource
> +* VMM         Virtual Machine Monitor
>
So, the term 'hypervisor' is (must be!) quite a well known one, for
anyone reading docs in this directory. And, in the docs we already have
around in here, it is far more common, AFAICT, to call an hypervisor
'hypervisor', rather than 'Virtual Machine Monitor'.

Therefore, I don't think there is the need to define the term 'VMM' in
this section, and, at the same time, I'd replace occurrences of
'Hypervisor/VMM' to just 'hypervisor' (the only one of which appear to
be the one above, in the Overview).

> +# User details
> +
> +* Feature Enabling:
> +
> +  Add "psr=cat" to boot line parameter to enable all supported level
> CAT
> +  features.
> +
> +* xl interfaces:
> +
> +  1. `psr-cat-show [OPTIONS] domain-id`:
> +
> +     Show domain L2 or L3 CAT CBM.
> +
> +     New option `-l` is added.
> +     `-l2`: Show cbm for L2 cache.
> +     `-l3`: Show cbm for L3 cache.
> +
> +     If neither `-l2` nor `-l3` is given, show both of them. If any
> one
> +     is not supported, will print error info.
> +
I actually think the best behavior would be:
 - if -lX is specified, and LX is not supported ==> print error;
 - if no -l is specified ==> print info about the supported levels.

(See comment on patch 21.)

> +## The relationship between L2 CAT and L3 CAT/CDP
> +
> +L2 CAT is independent of L3 CAT/CDP, which means L2 CAT would be
> enabled
> +while L3 CAT/CDP is disabled, or L2 CAT and L3 CAT/CDP are all
> enabled.
> +

I find 'would be enabled' and 'are all enabled' a bit confusing.

Maybe: "which means L2 cat can be enabled while L3 CAT/CDP is disabled,
or L2 CAT and L3 CAT/CDP can be all enabled"

?

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)