linux-kernel - RE: [PATCH v2 8/8] docs: ABI: Add sysfs documentation interface of hardware prefetch control driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <OSBPR01MB203705485512925382FD3E1680119@OSBPR01MB2037.jpnprd01.prod.outlook.com>
Date:   Wed, 16 Mar 2022 12:56:02 +0000
From:   "tarumizu.kohei@...itsu.com" <tarumizu.kohei@...itsu.com>
To:     'Jonathan Cameron' <Jonathan.Cameron@...wei.com>
CC:     "catalin.marinas@....com" <catalin.marinas@....com>,
        "will@...nel.org" <will@...nel.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>, "hpa@...or.com" <hpa@...or.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 8/8] docs: ABI: Add sysfs documentation interface of
 hardware prefetch control driver

> H
> i,
> 
> I'm going to review this with only a fairly basic knowledge of prefetchers and with
> no particular design in mind (though I'll point at ARM docs because they are
> generally good and easy to find ;) Key thing on an ABI like this is to maintain
> flexibility for other implementations.
> 
> It makes me a bit nervous to see an interface for something like this being defined
> with only a couple of implementations.  There are others with public
> documentation such as the ARM N2.
> 
> https://developer.arm.com/documentation/102099/0000/The-Neoverse-N2--co
> re
> 
> As is clear from below, not a lot of this is shared between CPUs so far so it might
> make more sense to document them separately?

> 
> I'd be tempted to have this as multiple blocks.  A lot of the documentation is not
> generic to all of them and that approach tends to give documentation files that are
> easier to add to later.

As you commented, there are no shared attributes between different CPUs.
Therefore, split the document into separate blocks on x86 and arm64.

> No need to say how this is implemented.  If someone wants to do it with a
> mailbox for a particular CPU that would be fine at the ABI level.
> 
> > +		These files exists in every CPU's cache/index[0,2] directory,
> 
> Can see where they are from above.  No need to repeat that bit.

I remove these descriptions in the next version.

> Perhaps
> "Attributes are only present if the particular cache implements the relevant
> prefetcher controls".  Or maybe just "All controls are optional".

The former description is more accurate. I fix it in the next version.

> How likely is it that other prefetcher implementations will allow more than two
> levels for this?  Can we define this ABI more broadly to allow that?  Assuming
> such a scale might exist, this needs renaming to stream_detect_pretcher_strength
> (as no longer on or off)
> 
> Easiest way to do that would probably be to use a separate
> stream_detect_prefetcher_strength_available that lists possible values (or min,
> step, max if that makes more sense).
> Here,
> 
> 0 1
> 
> With my very limited knowledge of the details, a multilevel approach would map
> better to controls like RPF_MODE in the N2
> IMP_CPUECTLR_EL1 register which has 4 levels for example (though I have no
> input on whether those levels could map to 'strength'.
> 

As you commented, the attribute 'strong' may require more than two
levels of type (strong or weak) in the future. In order to allow that,
I will reconsider the interface specification.

> Having two possible ways of representing 'auto' seems likely to cause testing
> complexity for no particular benefit. I'd just not allow one of them.

I modify it so that only 'auto' is allowed and 0 is prohibited in the next version.

> I would not list supported processors in here.  It will get out of sync if this
> becomes popular and it should be easy to see if it is supported by whether the
> sysfs attribute exists or not.

I remove the description about supported processors, because it can be
determined by the sysfs attribute exists or not.

> > +
> > +			- "* Hardware Prefetcher Disable (R/W)"
> > +			    corresponds to the "hardware_prefetcher_enable"
> > +
> > +			- "* Adjacent Cache Line Prefetcher Disable (R/W)"
> > +			    corresponds to the
> "adjacent_cache_line_prefetcher_enable"
> > +
> > +			- "* IP Prefetcher Disable (R/W)"
> > +			    corresponds to the "ip_prefetcher_enable"
> 
> I'm not sure on whether this should be here or not.  It seems like a path to some
> very long documentation once 10+ processor families are supported.
> However, there may be no better place to put this information.

The current MSR specification is defined by a combination of the above
three type of prefetchers. Therefore, it is assumed that the amount of
sentences will not increase any more.

For reference, MSR 0x1A4 are classified into the following three types,
depending on the processor model.

* tyep A (INTEL_FAM6_BROADWELL, etc.)

[0]    L2 Hardware Prefetcher Disable (R/W)
[1]    L2 Adjacent Cache Line Prefetcher Disable (R/W)
[2]    DCU Hardware Prefetcher Disable (R/W)
[3]    DCU IP Prefetcher Disable (R/W)
[63:4] Reserved

* type B (INTEL_FAM6_XEON_PHI_KNL, etc.)

[0]    DCU Hardware Prefetcher Disable (R/W)
[1]    L2 Hardware Prefetcher Disable (R/W)
[63:2] Reserved

* type C (INTEL_FAM6_ATOM_SILVERMONT_D, etc.)

[0]    L2 Hardware Prefetcher Disable (R/W)
[1]    Reserved
[2]    DCU Hardware Prefetcher Disable (R/W)
[63:3] Reserved