[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2] xen/arm64: Remove vreg_emulate_sysreg32
Hi Julien, On 06.09.2021 11:07, Julien Grall wrote: > Hi Michal, > > On 29/07/2021 11:42, Michal Orzel wrote: >> According to ARMv8A architecture, AArch64 registers >> are 64bit wide even though in many cases the upper >> 32bit is reserved. Therefore there is no need for >> function vreg_emulate_sysreg32 on arm64. This means >> that we can have just one function vreg_emulate_sysreg >> using new function pointer: >> typedef bool (*vreg_reg_fn_t)(struct cpu_user_regs *regs, >> register_t *r, bool read); >> >> Modify vreg_emulate_cp32 to use the new function pointer >> as well. >> >> This change allows to properly use 64bit registers in AArch64 >> state and in case of AArch32 the upper 32 bits of AArch64 >> registers are inaccessible and are ignored(D1.20.1 ARM DDI 0487A.j). >> >> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx> >> --- >> xen/arch/arm/arm64/vsysreg.c | 2 +- >> xen/arch/arm/vcpreg.c | 16 ++++++++++---- >> xen/arch/arm/vgic-v3.c | 2 +- >> xen/arch/arm/vtimer.c | 11 +++++----- >> xen/include/asm-arm/processor.h | 4 ++-- >> xen/include/asm-arm/vreg.h | 38 ++++++--------------------------- >> 6 files changed, 29 insertions(+), 44 deletions(-) >> >> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c >> index caf17174b8..73fa2ca9ae 100644 >> --- a/xen/arch/arm/arm64/vsysreg.c >> +++ b/xen/arch/arm/arm64/vsysreg.c >> @@ -64,7 +64,7 @@ TVM_REG(CONTEXTIDR_EL1) >> { \ >> bool res; \ >> \ >> - res = vreg_emulate_sysreg64(regs, hsr, vreg_emulate_##reg); \ >> + res = vreg_emulate_sysreg(regs, hsr, vreg_emulate_##reg); \ >> ASSERT(res); \ >> break; \ >> } >> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c >> index e3ce56d875..be1ec08159 100644 >> --- a/xen/arch/arm/vcpreg.c >> +++ b/xen/arch/arm/vcpreg.c >> @@ -57,9 +57,17 @@ >> #define WRITE_SYSREG_SZ(sz, val, sysreg...) WRITE_SYSREG##sz(val, sysreg) >> #endif >> +/* >> + * type32_t is defined as register_t due to the vreg_emulate_cp32 and >> + * vreg_emulate_sysreg taking function pointer with register_t type used for >> + * passing register's value. >> + */ >> +typedef register_t type32_t; >> +typedef uint64_t type64_t ; > > NIT: spurious space before ;. > >> + >> /* The name is passed from the upper macro to workaround macro expansion. >> */ >> #define TVM_REG(sz, func, reg...) >> \ >> -static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) >> \ >> +static bool func(struct cpu_user_regs *regs, type##sz##_t *r, bool read) >> \ >> { >> \ >> struct vcpu *v = current; >> \ >> bool cache_enabled = vcpu_has_cache_enabled(v); >> \ >> @@ -83,7 +91,7 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t >> *r, bool read) \ >> #else /* CONFIG_ARM_64 */ >> #define TVM_REG32_COMBINED(lowreg, hireg, xreg) >> \ >> -static bool vreg_emulate_##xreg(struct cpu_user_regs *regs, uint32_t *r, >> \ >> +static bool vreg_emulate_##xreg(struct cpu_user_regs *regs, register_t *r, >> \ >> bool read, bool hi) >> \ >> { >> \ >> struct vcpu *v = current; >> \ >> @@ -108,13 +116,13 @@ static bool vreg_emulate_##xreg(struct cpu_user_regs >> *regs, uint32_t *r, \ >> return true; >> \ >> } >> \ >> >> \ >> -static bool vreg_emulate_##lowreg(struct cpu_user_regs *regs, uint32_t *r, >> \ >> +static bool vreg_emulate_##lowreg(struct cpu_user_regs *regs, register_t >> *r,\ >> bool read) >> \ >> { >> \ >> return vreg_emulate_##xreg(regs, r, read, false); >> \ >> } >> \ >> >> \ >> -static bool vreg_emulate_##hireg(struct cpu_user_regs *regs, uint32_t *r, >> \ >> +static bool vreg_emulate_##hireg(struct cpu_user_regs *regs, register_t *r, >> \ >> bool read) >> \ >> { >> \ >> return vreg_emulate_##xreg(regs, r, read, true); >> \ >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c >> index 613f37abab..cb5a70c42e 100644 >> --- a/xen/arch/arm/vgic-v3.c >> +++ b/xen/arch/arm/vgic-v3.c >> @@ -1531,7 +1531,7 @@ static bool vgic_v3_emulate_sysreg(struct >> cpu_user_regs *regs, union hsr hsr) >> switch ( hsr.bits & HSR_SYSREG_REGS_MASK ) >> { >> case HSR_SYSREG_ICC_SGI1R_EL1: >> - return vreg_emulate_sysreg64(regs, hsr, vgic_v3_emulate_sgi1r); >> + return vreg_emulate_sysreg(regs, hsr, vgic_v3_emulate_sgi1r); >> default: >> return false; >> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c >> index 167fc6127a..0196951af4 100644 >> --- a/xen/arch/arm/vtimer.c >> +++ b/xen/arch/arm/vtimer.c >> @@ -162,7 +162,8 @@ void virt_timer_restore(struct vcpu *v) >> WRITE_SYSREG(v->arch.virt_timer.ctl, CNTV_CTL_EL0); >> } >> -static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t *r, bool >> read) >> +static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, register_t *r, >> + bool read) >> { >> struct vcpu *v = current; >> s_time_t expires; >> @@ -197,7 +198,7 @@ static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, >> uint32_t *r, bool read) >> return true; >> } >> -static bool vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r, >> +static bool vtimer_cntp_tval(struct cpu_user_regs *regs, register_t *r, >> bool read) >> { >> struct vcpu *v = current; >> @@ -316,11 +317,11 @@ static bool vtimer_emulate_sysreg(struct cpu_user_regs >> *regs, union hsr hsr) >> switch ( hsr.bits & HSR_SYSREG_REGS_MASK ) >> { >> case HSR_SYSREG_CNTP_CTL_EL0: >> - return vreg_emulate_sysreg32(regs, hsr, vtimer_cntp_ctl); >> + return vreg_emulate_sysreg(regs, hsr, vtimer_cntp_ctl); >> case HSR_SYSREG_CNTP_TVAL_EL0: >> - return vreg_emulate_sysreg32(regs, hsr, vtimer_cntp_tval); >> + return vreg_emulate_sysreg(regs, hsr, vtimer_cntp_tval); >> case HSR_SYSREG_CNTP_CVAL_EL0: >> - return vreg_emulate_sysreg64(regs, hsr, vtimer_cntp_cval); >> + return vreg_emulate_sysreg(regs, hsr, vtimer_cntp_cval); >> default: >> return false; >> diff --git a/xen/include/asm-arm/processor.h >> b/xen/include/asm-arm/processor.h >> index 2577e9a244..2058b69447 100644 >> --- a/xen/include/asm-arm/processor.h >> +++ b/xen/include/asm-arm/processor.h >> @@ -484,9 +484,9 @@ extern register_t __cpu_logical_map[]; >> #define CNTKCTL_EL1_EL0PTEN (1u<<9) /* Expose phys timer registers to EL0 >> */ >> /* Timer control registers */ >> -#define CNTx_CTL_ENABLE (1u<<0) /* Enable timer */ >> +#define CNTx_CTL_ENABLE (1ul<<0) /* Enable timer */ >> #define CNTx_CTL_MASK (1ul<<1) /* Mask IRQ */ >> -#define CNTx_CTL_PENDING (1u<<2) /* IRQ pending */ >> +#define CNTx_CTL_PENDING (1ul<<2) /* IRQ pending */ > I would suggest to mention in the commit message why this is necessary. > AFAICT, it is not strictly necessary because you left ctl defined as a > uin32_t. So I am guessing you only keep it for hardening purpose? > > If so how about adding: > > "Take the opportunity to switch CNTx_CTL_* to use UL to avoid any surprise > with the negation of any bits (as used in vtimer_cntp_ctl)". > > The rest of the patch looks fine. So I would be happy to deal with the fixes > on commit: Please do. Thanks. > > Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx> > > Cheers, > Cheers, Michal
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |