Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Thu, 17 Feb 2022 18:22:33 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
Cc:     "hpa@zytor.com" <hpa@zytor.com>,
        "Williams, Dan J" <dan.j.williams@intel.com>,
        "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
        "Yu, Fenghua" <fenghua.yu@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V8 16/44] mm/pkeys: Introduce pks_mk_readwrite()
Message-ID: <Yg8C6UkgfBmQlPSq@iweiny-desk3>
References: <20220127175505.851391-1-ira.weiny@intel.com>
 <20220127175505.851391-17-ira.weiny@intel.com>
 <a1f74401512ee19176397633aa823f9b6e375973.camel@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a1f74401512ee19176397633aa823f9b6e375973.camel@intel.com>
Precedence: bulk

On Mon, Jan 31, 2022 at 03:10:39PM -0800, Edgecombe, Rick P wrote:
> On Thu, 2022-01-27 at 09:54 -0800, ira.weiny@intel.com wrote:
> > +void pks_update_protection(int pkey, u32 protection)
> > +{
> 
> I don't know if this matters too much, but the type of a pkey is either
> int or u16 across this series and PKU. But it's only possibly a 4 bit
> value.

I was settling on 'int' because the PKRU code uses int a lot.

That said, PKRU is a bit more complicated; x86 is 4 bits, powerpc is 5 bits,
and I see 4 different types for pkey [int, u16, u32, s16].

The signed values are used to mean 'key or error' in a couple of places.  Which
leaves 'int' as a convenient choice over 's16' IMO.  The use of u32 and u16
seems arbitrary.  Both should be plenty big for generic core code.

> Seems the smallest that would fit is char. Why use one over the
> other?
> 
> Also, why u32 for protection here? The whole pkrs value containing the
> bits for all keys is 32 bits, but per key there is only room ever for 2
> bits, right?

Correct but I'm not sure anything would be saved by declaring u8.  Regardless
I've changed it.

> 
> It would be nice to be consistent and have a reason, but again, I don't
> know if makes any real difference.

I was consistent in the core code with 'int'.  I'll look at cleaning up some of
the PKRU code but I think that is a separate series from this one.

For this series I'll standardize on u8 because u16 is also too big.  I have
seen one place where it would be nice to have a type of unsigned to check the
bounds of the pkey.  So you have a valid point that following the PKRU code was
less than ideal.

Ira