Re: [PATCH 2/6] x86/prot-key: Split PKRU infrastructure out of asm/processor.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.01.2023 15:57, Andrew Cooper wrote:
> On 21/12/2021 11:28 am, Jan Beulich wrote:
>> On 16.12.2021 10:54, Andrew Cooper wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/x86/include/asm/prot-key.h
>>> @@ -0,0 +1,45 @@
>>> +/******************************************************************************
>>> + * arch/x86/include/asm/spec_ctrl.h
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
>>> + *
>>> + * Copyright (c) 2021 Citrix Systems Ltd.
>>> + */
>>> +#ifndef ASM_PROT_KEY_H
>>> +#define ASM_PROT_KEY_H
>>> +
>>> +#include <xen/types.h>
>>> +
>>> +#define PKEY_AD 1 /* Access Disable */
>>> +#define PKEY_WD 2 /* Write Disable */
>>> +
>>> +#define PKEY_WIDTH 2 /* Two bits per protection key */
>>> +
>>> +static inline uint32_t rdpkru(void)
>>> +{
>>> +    uint32_t pkru;
>> I agree this wants to be uint32_t (i.e. unlike the original function),
>> but I don't see why the function's return type needs to be, the more
>> that the sole caller also uses unsigned int for the variable to store
>> the result in.
> 
> This is thinnest-possible wrapper around an instruction which
> architecturally returns exactly 32 bits of data.
> 
> It is literally the example that CODING_STYLE uses to demonstrate when
> fixed width types should be used.

I don't read it like that, but I guess we're simply drawing the line in
different places (and agreeing on one place would be nice). To me using
uint32_t for a variable accessed by an asm() is what is meant. But that
then (to me) doesn't extend to the function return type here.

But no, I don't mean to block this change just because of this aspect.
We've got many far worse uses of types, which bother me more. It merely
would be nice if new code (regardless of the contributor) ended up all
consistent in this regard.

Jan