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.coop...@citrix.com>
This looks functionally correct, so in principle
Reviewed-by: Jan Beulich <jbeul...@suse.com>
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
