Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 17 Feb 2022 21:34:41 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     Dave Hansen <dave.hansen@intel.com>
Cc:     Dave Hansen <dave.hansen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Fenghua Yu <fenghua.yu@intel.com>,
        Rick Edgecombe <rick.p.edgecombe@intel.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH V8 19/44] mm/pkeys: PKS Testing, add pks_mk_*() tests
Message-ID: <Yg8v8XPyDTH4O2rr@iweiny-desk3>
References: <20220127175505.851391-1-ira.weiny@intel.com>
 <20220127175505.851391-20-ira.weiny@intel.com>
 <00b87c5f-b4ed-7593-827c-0e1114b8b456@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <00b87c5f-b4ed-7593-827c-0e1114b8b456@intel.com>
Precedence: bulk

On Tue, Feb 01, 2022 at 09:45:03AM -0800, Dave Hansen wrote:
> On 1/27/22 09:54, ira.weiny@intel.com wrote:
> >  bool pks_test_callback(void)
> >  {
> > -	return false;
> > +	bool armed = (test_armed_key != 0);
> > +
> > +	if (armed) {
> > +		pks_mk_readwrite(test_armed_key);
> > +		fault_cnt++;
> > +	}
> > +
> > +	return armed;
> > +}
> 
> Where's the locking for all this?  I don't think we need anything fancy,
> but is there anything preventing the test from being started from
> multiple threads at the same time?  I think a simple global test mutex
> would probably suffice.

Good idea.  Generally I don't see that happening but it is good to be safe.

> 
> Also, pks_test_callback() needs at least a comment or two about what
> it's doing.

The previous patch which adds this call in the fault handler contains the
following comment which is in the final code:

/*
 * pks_test_callback() is called by the fault handler to indicate it saw a pkey
 * fault.
 *
 * NOTE: The callback is responsible for clearing any condition which would
 * cause the fault to re-trigger.
 */

Would you like more comments within the function?

> 
> Does this work if you have a test armed and then you get an unrelated
> PKS fault on another CPU?  I think this will disarm the test from the
> unrelated thread.

This code will detect a false fault.  But the other unrelated fault will work
correctly.

I've debated if the test code should use a specific fault callback...  :-/
That breaks my test which iterates all keys...  but would fix this problem.

Ira