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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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.

>> --- a/xen/arch/x86/mm/guest_walk.c
>> +++ b/xen/arch/x86/mm/guest_walk.c
>> @@ -26,7 +26,9 @@
>>  #include <xen/paging.h>
>>  #include <xen/domain_page.h>
>>  #include <xen/sched.h>
>> +
>>  #include <asm/page.h>
>> +#include <asm/prot-key.h>
>>  #include <asm/guest_pt.h>
>>  #include <asm/hvm/emulate.h>
>>  
>> @@ -413,10 +415,11 @@ guest_walk_tables(const struct vcpu *v, struct 
>> p2m_domain *p2m,
>>           guest_pku_enabled(v) )
>>      {
>>          unsigned int pkey = guest_l1e_get_pkey(gw->l1e);
>> -        unsigned int pkru = rdpkru();
>> +        unsigned int pkr = rdpkru();
>> +        unsigned int pk_ar = pkr >> (pkey * PKEY_WIDTH);
> This is correct only because below you only inspect the low two bits.
> Since I don't think masking off the upper bits is really useful here,
> I'd like to suggest to not call the variable "pk_ar". Perhaps
> something as generic as "temp"?

This variable being named pk_ar (or thereabouts) is very important for
clarity below.

I've masked them - seems the compiler is clever enough to undo that even
at -O1.

~Andrew