| Message ID | 20211216095421.12871-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Support PKS | expand |
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@citrix.com> This looks functionally correct, so in principle Reviewed-by: Jan Beulich <jbeulich@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
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
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
diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h index 400b4fac5ed4..eb1687d0795c 100644 --- a/xen/arch/x86/include/asm/processor.h +++ b/xen/arch/x86/include/asm/processor.h @@ -367,44 +367,6 @@ static always_inline void set_in_cr4 (unsigned long mask) write_cr4(read_cr4() | mask); } -static inline unsigned int rdpkru(void) -{ - unsigned int pkru; - - asm volatile (".byte 0x0f,0x01,0xee" - : "=a" (pkru) : "c" (0) : "dx"); - - return pkru; -} - -static inline void wrpkru(unsigned int pkru) -{ - asm volatile ( ".byte 0x0f, 0x01, 0xef" - :: "a" (pkru), "d" (0), "c" (0) ); -} - -/* Macros for PKRU domain */ -#define PKRU_READ (0) -#define PKRU_WRITE (1) -#define PKRU_ATTRS (2) - -/* - * PKRU defines 32 bits, there are 16 domains and 2 attribute bits per - * domain in pkru, pkeys is index to a defined domain, so the value of - * pte_pkeys * PKRU_ATTRS + R/W is offset of a defined domain attribute. - */ -static inline bool_t read_pkru_ad(uint32_t pkru, unsigned int pkey) -{ - ASSERT(pkey < 16); - return (pkru >> (pkey * PKRU_ATTRS + PKRU_READ)) & 1; -} - -static inline bool_t read_pkru_wd(uint32_t pkru, unsigned int pkey) -{ - ASSERT(pkey < 16); - return (pkru >> (pkey * PKRU_ATTRS + PKRU_WRITE)) & 1; -} - static always_inline void __monitor(const void *eax, unsigned long ecx, unsigned long edx) { diff --git a/xen/arch/x86/include/asm/prot-key.h b/xen/arch/x86/include/asm/prot-key.h new file mode 100644 index 000000000000..084b248d81a5 --- /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; + + asm volatile ( ".byte 0x0f,0x01,0xee" + : "=a" (pkru) : "c" (0) : "dx" ); + + return pkru; +} + +static inline void wrpkru(uint32_t pkru) +{ + asm volatile ( ".byte 0x0f, 0x01, 0xef" + :: "a" (pkru), "d" (0), "c" (0) ); +} + +#endif /* ASM_PROT_KEY_H */ diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c index b9f607272c39..dc8fdde0212e 100644 --- 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); - if ( read_pkru_ad(pkru, pkey) || - ((walk & PFEC_write_access) && read_pkru_wd(pkru, pkey) && + if ( (pk_ar & PKEY_AD) || + ((walk & PFEC_write_access) && (pk_ar & PKEY_WD) && ((walk & PFEC_user_mode) || guest_wp_enabled(v))) ) { gw->pfec |= PFEC_prot_key; diff --git a/xen/arch/x86/x86_emulate.c b/xen/arch/x86/x86_emulate.c index 1e082e6f3b2d..551ad0f7b303 100644 --- a/xen/arch/x86/x86_emulate.c +++ b/xen/arch/x86/x86_emulate.c @@ -12,8 +12,10 @@ #include <xen/domain_page.h> #include <xen/err.h> #include <xen/event.h> + #include <asm/x86_emulate.h> #include <asm/processor.h> /* current_cpu_info */ +#include <asm/prot-key.h> #include <asm/xstate.h> #include <asm/amd.h> /* cpu_has_amd_erratum() */ #include <asm/debugreg.h>
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@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> --- xen/arch/x86/include/asm/processor.h | 38 ------------------------------ xen/arch/x86/include/asm/prot-key.h | 45 ++++++++++++++++++++++++++++++++++++ xen/arch/x86/mm/guest_walk.c | 9 +++++--- xen/arch/x86/x86_emulate.c | 2 ++ 4 files changed, 53 insertions(+), 41 deletions(-) create mode 100644 xen/arch/x86/include/asm/prot-key.h