linux-kernel - Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

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: <151002be-33e6-20d6-7699-bc9be7e51f33@intel.com>
Date:   Tue, 6 Aug 2019 09:55:56 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     tglx@...utronix.de, fenghua.yu@...el.com, tony.luck@...el.com,
        kuo-lang.tseng@...el.com, mingo@...hat.com, hpa@...or.com,
        x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower
 level caches

Hi Borislav,

On 8/6/2019 8:57 AM, Borislav Petkov wrote:
> On Mon, Aug 05, 2019 at 10:57:04AM -0700, Reinette Chatre wrote:
>> What do you think?
> 
> Actually, I was thinking about something a lot simpler: something
> along the lines of adding the CPUID check in a helper function which
> rdt_pseudo_lock_init() calls. If the cache is not inclusive - and my
> guess is it would suffice to check any cache but I'd prefer you correct
> me on that - you simply return error and rdt_pseudo_lock_init() returns
> early without doing any futher init.
> 
> How does that sound?

I am a bit cautious about this. When I started this work I initially
added a helper function to resctrl that calls CPUID to determine if the
cache is inclusive. At that time I became aware of a discussion
motivating against scattered CPUID calls and motivating for one instance
of CPUID information:
http://lkml.kernel.org/r/alpine.DEB.2.21.1906162141301.1760@nanos.tec.linutronix.de

My interpretation of the above resulted in a move away from calling
CPUID in resctrl to the patch you are reviewing now.

I do indeed prefer a simple approach and would change to what you
suggest if you find it to be best.

To answer your question about checking any cache: this seems to be
different between L2 and L3. On the Atom systems where L2 pseudo-locking
works well the L2 cache is not inclusive. We are also working on
supporting cache pseudo-locking on L3 cache that is not inclusive.

I could add the single CPUID check during rdt_pseudo_lock_init(),
checking on any CPU should work. I think it would be simpler (reasons
below) to initialize that single system-wide setting in
rdt_pseudo_lock_init() and keep the result locally in the pseudo-locking
code, that can be referred to when the user requests the pseudo-locked
region.

Simpler for two reasons:
* Prevent future platform specific code within rdt_pseudo_lock_init()
trying to pick when L3 is allowed to be inclusive or not.
* rdt_pseudo_lock_init() does not currently make decision whether
platform supports pseudo-locking - this is currently done when user
requests a pseudo-lock region. rdt_pseudo_lock_init() does
initialization of things that should not fail and for which resctrl's
mount should not proceed if it fails. A platform not supporting
pseudo-locking should not prevent resctrl from being mounted and used
for cache allocation.

Thank you very much

Reinette