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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.12.2021 10:54, Andrew Cooper wrote:
> asm/processor.h is in desperate need of splitting up, and protection key
> functionality in only used in the emulator and pagewalk.  Introduce a new
> asm/prot-key.h and move the relevant content over.
> 
> Rename the PKRU_* constants to drop the user part and to use the architectural
> terminology.
> 
> Drop the read_pkru_{ad,wd}() helpers entirely.  The pkru infix is about to
> become wrong, and the sole user is shorter and easier to follow without the
> helpers.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

This looks functionally correct, so in principle
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
But I have two remarks:

> --- /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.

> +    asm volatile ( ".byte 0x0f,0x01,0xee"
> +                   : "=a" (pkru) : "c" (0) : "dx" );
> +
> +    return pkru;
> +}
> +
> +static inline void wrpkru(uint32_t pkru)

(To avoid an intermediate local variable, I can agree with using
uint32_t for the parameter type directly here.)

> --- 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"?

Jan