* [PATCH 1/9] arm64/vfp: Get rid of READ/WRITE_SYSREG32
2021-04-20 7:08 [PATCH 0/9] xen/arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
@ 2021-04-20 7:08 ` Michal Orzel
2021-04-20 13:04 ` Julien Grall
2021-04-20 7:08 ` [PATCH 2/9] arm/domain: " Michal Orzel
` (7 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Michal Orzel @ 2021-04-20 7:08 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk, bertrand.marquis
AArch64 system registers are 64bit whereas AArch32 ones
are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
we should get rid of helpers READ/WRITE_SYSREG32
in favour of using READ/WRITE_SYSREG.
We should also use register_t type when reading sysregs
which can correspond to uint64_t or uint32_t.
Even though many AArch64 sysregs have upper 32bit reserved
it does not mean that they can't be widen in the future.
Modify type of FPCR, FPSR, FPEXC32_EL2 to register_t.
Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
xen/arch/arm/arm64/vfp.c | 12 ++++++------
xen/include/asm-arm/arm64/vfp.h | 6 +++---
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/xen/arch/arm/arm64/vfp.c b/xen/arch/arm/arm64/vfp.c
index 999a0d58a5..47885e76ba 100644
--- a/xen/arch/arm/arm64/vfp.c
+++ b/xen/arch/arm/arm64/vfp.c
@@ -26,10 +26,10 @@ void vfp_save_state(struct vcpu *v)
"stp q30, q31, [%1, #16 * 30]\n\t"
: "=Q" (*v->arch.vfp.fpregs) : "r" (v->arch.vfp.fpregs));
- v->arch.vfp.fpsr = READ_SYSREG32(FPSR);
- v->arch.vfp.fpcr = READ_SYSREG32(FPCR);
+ v->arch.vfp.fpsr = READ_SYSREG(FPSR);
+ v->arch.vfp.fpcr = READ_SYSREG(FPCR);
if ( is_32bit_domain(v->domain) )
- v->arch.vfp.fpexc32_el2 = READ_SYSREG32(FPEXC32_EL2);
+ v->arch.vfp.fpexc32_el2 = READ_SYSREG(FPEXC32_EL2);
}
void vfp_restore_state(struct vcpu *v)
@@ -55,8 +55,8 @@ void vfp_restore_state(struct vcpu *v)
"ldp q30, q31, [%1, #16 * 30]\n\t"
: : "Q" (*v->arch.vfp.fpregs), "r" (v->arch.vfp.fpregs));
- WRITE_SYSREG32(v->arch.vfp.fpsr, FPSR);
- WRITE_SYSREG32(v->arch.vfp.fpcr, FPCR);
+ WRITE_SYSREG(v->arch.vfp.fpsr, FPSR);
+ WRITE_SYSREG(v->arch.vfp.fpcr, FPCR);
if ( is_32bit_domain(v->domain) )
- WRITE_SYSREG32(v->arch.vfp.fpexc32_el2, FPEXC32_EL2);
+ WRITE_SYSREG(v->arch.vfp.fpexc32_el2, FPEXC32_EL2);
}
diff --git a/xen/include/asm-arm/arm64/vfp.h b/xen/include/asm-arm/arm64/vfp.h
index 6ab5d36c6c..e6e8c363bc 100644
--- a/xen/include/asm-arm/arm64/vfp.h
+++ b/xen/include/asm-arm/arm64/vfp.h
@@ -7,9 +7,9 @@
struct vfp_state
{
uint64_t fpregs[64] __vfp_aligned;
- uint32_t fpcr;
- uint32_t fpexc32_el2;
- uint32_t fpsr;
+ register_t fpcr;
+ register_t fpexc32_el2;
+ register_t fpsr;
};
#endif /* _ARM_ARM64_VFP_H */
--
2.29.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 1/9] arm64/vfp: Get rid of READ/WRITE_SYSREG32
2021-04-20 7:08 ` [PATCH 1/9] arm64/vfp: " Michal Orzel
@ 2021-04-20 13:04 ` Julien Grall
0 siblings, 0 replies; 27+ messages in thread
From: Julien Grall @ 2021-04-20 13:04 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis
Hi Michal,
On 20/04/2021 08:08, Michal Orzel wrote:
> AArch64 system registers are 64bit whereas AArch32 ones
> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
> we should get rid of helpers READ/WRITE_SYSREG32
> in favour of using READ/WRITE_SYSREG.
> We should also use register_t type when reading sysregs
> which can correspond to uint64_t or uint32_t.
> Even though many AArch64 sysregs have upper 32bit reserved
> it does not mean that they can't be widen in the future.
>
> Modify type of FPCR, FPSR, FPEXC32_EL2 to register_t.
>
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 2/9] arm/domain: Get rid of READ/WRITE_SYSREG32
2021-04-20 7:08 [PATCH 0/9] xen/arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
2021-04-20 7:08 ` [PATCH 1/9] arm64/vfp: " Michal Orzel
@ 2021-04-20 7:08 ` Michal Orzel
2021-04-20 13:12 ` Julien Grall
2021-04-20 7:08 ` [PATCH 3/9] arm/gic: " Michal Orzel
` (6 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Michal Orzel @ 2021-04-20 7:08 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk, bertrand.marquis
AArch64 system registers are 64bit whereas AArch32 ones
are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
we should get rid of helpers READ/WRITE_SYSREG32
in favour of using READ/WRITE_SYSREG.
We should also use register_t type when reading sysregs
which can correspond to uint64_t or uint32_t.
Even though many AArch64 sysregs have upper 32bit reserved
it does not mean that they can't be widen in the future.
Modify type of registers: actlr, cntkctl to register_t.
Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
xen/arch/arm/domain.c | 20 ++++++++++----------
xen/include/asm-arm/domain.h | 4 ++--
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index bdd3d3e5b5..c021a03c61 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -113,13 +113,13 @@ static void ctxt_switch_from(struct vcpu *p)
p->arch.tpidr_el1 = READ_SYSREG(TPIDR_EL1);
/* Arch timer */
- p->arch.cntkctl = READ_SYSREG32(CNTKCTL_EL1);
+ p->arch.cntkctl = READ_SYSREG(CNTKCTL_EL1);
virt_timer_save(p);
if ( is_32bit_domain(p->domain) && cpu_has_thumbee )
{
- p->arch.teecr = READ_SYSREG32(TEECR32_EL1);
- p->arch.teehbr = READ_SYSREG32(TEEHBR32_EL1);
+ p->arch.teecr = READ_SYSREG(TEECR32_EL1);
+ p->arch.teehbr = READ_SYSREG(TEEHBR32_EL1);
}
#ifdef CONFIG_ARM_32
@@ -175,7 +175,7 @@ static void ctxt_switch_from(struct vcpu *p)
static void ctxt_switch_to(struct vcpu *n)
{
- uint32_t vpidr;
+ register_t vpidr;
/* When the idle VCPU is running, Xen will always stay in hypervisor
* mode. Therefore we don't need to restore the context of an idle VCPU.
@@ -183,8 +183,8 @@ static void ctxt_switch_to(struct vcpu *n)
if ( is_idle_vcpu(n) )
return;
- vpidr = READ_SYSREG32(MIDR_EL1);
- WRITE_SYSREG32(vpidr, VPIDR_EL2);
+ vpidr = READ_SYSREG(MIDR_EL1);
+ WRITE_SYSREG(vpidr, VPIDR_EL2);
WRITE_SYSREG(n->arch.vmpidr, VMPIDR_EL2);
/* VGIC */
@@ -257,8 +257,8 @@ static void ctxt_switch_to(struct vcpu *n)
if ( is_32bit_domain(n->domain) && cpu_has_thumbee )
{
- WRITE_SYSREG32(n->arch.teecr, TEECR32_EL1);
- WRITE_SYSREG32(n->arch.teehbr, TEEHBR32_EL1);
+ WRITE_SYSREG(n->arch.teecr, TEECR32_EL1);
+ WRITE_SYSREG(n->arch.teehbr, TEEHBR32_EL1);
}
#ifdef CONFIG_ARM_32
@@ -274,7 +274,7 @@ static void ctxt_switch_to(struct vcpu *n)
/* This is could trigger an hardware interrupt from the virtual
* timer. The interrupt needs to be injected into the guest. */
- WRITE_SYSREG32(n->arch.cntkctl, CNTKCTL_EL1);
+ WRITE_SYSREG(n->arch.cntkctl, CNTKCTL_EL1);
virt_timer_restore(n);
}
@@ -330,7 +330,7 @@ static void schedule_tail(struct vcpu *prev)
static void continue_new_vcpu(struct vcpu *prev)
{
- current->arch.actlr = READ_SYSREG32(ACTLR_EL1);
+ current->arch.actlr = READ_SYSREG(ACTLR_EL1);
processor_vcpu_initialise(current);
schedule_tail(prev);
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 0a74df9931..2d4f38c669 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -156,7 +156,7 @@ struct arch_vcpu
/* Control Registers */
register_t sctlr;
- uint32_t actlr;
+ register_t actlr;
uint32_t cpacr;
uint32_t contextidr;
@@ -190,7 +190,7 @@ struct arch_vcpu
struct vgic_cpu vgic;
/* Timer registers */
- uint32_t cntkctl;
+ register_t cntkctl;
struct vtimer phys_timer;
struct vtimer virt_timer;
--
2.29.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 2/9] arm/domain: Get rid of READ/WRITE_SYSREG32
2021-04-20 7:08 ` [PATCH 2/9] arm/domain: " Michal Orzel
@ 2021-04-20 13:12 ` Julien Grall
2021-04-21 7:36 ` Michal Orzel
0 siblings, 1 reply; 27+ messages in thread
From: Julien Grall @ 2021-04-20 13:12 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis
Hi Michal,
On 20/04/2021 08:08, Michal Orzel wrote:
> AArch64 system registers are 64bit whereas AArch32 ones
> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
> we should get rid of helpers READ/WRITE_SYSREG32
> in favour of using READ/WRITE_SYSREG.
> We should also use register_t type when reading sysregs
> which can correspond to uint64_t or uint32_t.
> Even though many AArch64 sysregs have upper 32bit reserved
> it does not mean that they can't be widen in the future.
>
> Modify type of registers: actlr, cntkctl to register_t.
ACTLR is implementation defined, so in theory there might already bits
already defined in the range [32:63]. So I would consider to split it
from the patch so we can backport it.
>
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> ---
> xen/arch/arm/domain.c | 20 ++++++++++----------
> xen/include/asm-arm/domain.h | 4 ++--
> 2 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index bdd3d3e5b5..c021a03c61 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -113,13 +113,13 @@ static void ctxt_switch_from(struct vcpu *p)
> p->arch.tpidr_el1 = READ_SYSREG(TPIDR_EL1);
>
> /* Arch timer */
> - p->arch.cntkctl = READ_SYSREG32(CNTKCTL_EL1);
> + p->arch.cntkctl = READ_SYSREG(CNTKCTL_EL1);
> virt_timer_save(p);
>
> if ( is_32bit_domain(p->domain) && cpu_has_thumbee )
> {
> - p->arch.teecr = READ_SYSREG32(TEECR32_EL1);
> - p->arch.teehbr = READ_SYSREG32(TEEHBR32_EL1);
> + p->arch.teecr = READ_SYSREG(TEECR32_EL1);
> + p->arch.teehbr = READ_SYSREG(TEEHBR32_EL1);
It feels strange you converted cntkctl and actlr to use register_t but
not teecr and teehbr. Can you explain why?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2/9] arm/domain: Get rid of READ/WRITE_SYSREG32
2021-04-20 13:12 ` Julien Grall
@ 2021-04-21 7:36 ` Michal Orzel
2021-04-21 10:20 ` Julien Grall
0 siblings, 1 reply; 27+ messages in thread
From: Michal Orzel @ 2021-04-21 7:36 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis
Hi Julien,
On 20.04.2021 15:12, Julien Grall wrote:
> Hi Michal,
>
> On 20/04/2021 08:08, Michal Orzel wrote:
>> AArch64 system registers are 64bit whereas AArch32 ones
>> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
>> we should get rid of helpers READ/WRITE_SYSREG32
>> in favour of using READ/WRITE_SYSREG.
>> We should also use register_t type when reading sysregs
>> which can correspond to uint64_t or uint32_t.
>> Even though many AArch64 sysregs have upper 32bit reserved
>> it does not mean that they can't be widen in the future.
>>
>> Modify type of registers: actlr, cntkctl to register_t.
>
> ACTLR is implementation defined, so in theory there might already bits already defined in the range [32:63]. So I would consider to split it from the patch so we can backport it.
>
You mean not to touch it at all in this series or to create a seperate patch only for ACTLR?
>>
>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>> ---
>> xen/arch/arm/domain.c | 20 ++++++++++----------
>> xen/include/asm-arm/domain.h | 4 ++--
>> 2 files changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index bdd3d3e5b5..c021a03c61 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -113,13 +113,13 @@ static void ctxt_switch_from(struct vcpu *p)
>> p->arch.tpidr_el1 = READ_SYSREG(TPIDR_EL1);
>> /* Arch timer */
>> - p->arch.cntkctl = READ_SYSREG32(CNTKCTL_EL1);
>> + p->arch.cntkctl = READ_SYSREG(CNTKCTL_EL1);
>> virt_timer_save(p);
>> if ( is_32bit_domain(p->domain) && cpu_has_thumbee )
>> {
>> - p->arch.teecr = READ_SYSREG32(TEECR32_EL1);
>> - p->arch.teehbr = READ_SYSREG32(TEEHBR32_EL1);
>> + p->arch.teecr = READ_SYSREG(TEECR32_EL1);
>> + p->arch.teehbr = READ_SYSREG(TEEHBR32_EL1);
>
> It feels strange you converted cntkctl and actlr to use register_t but not teecr and teehbr. Can you explain why?
>
Because thumbee and its registers do not appear in any of ARMv8 ARM version.
This means that this code could be protected even with #ifdef CONFIG_ARM_32 as AArch64 do not have them.
> Cheers,
>
Cheers,
Michal
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2/9] arm/domain: Get rid of READ/WRITE_SYSREG32
2021-04-21 7:36 ` Michal Orzel
@ 2021-04-21 10:20 ` Julien Grall
0 siblings, 0 replies; 27+ messages in thread
From: Julien Grall @ 2021-04-21 10:20 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis
On 21/04/2021 08:36, Michal Orzel wrote:
> Hi Julien,
Hi Michal,
>
> On 20.04.2021 15:12, Julien Grall wrote:
>> Hi Michal,
>>
>> On 20/04/2021 08:08, Michal Orzel wrote:
>>> AArch64 system registers are 64bit whereas AArch32 ones
>>> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
>>> we should get rid of helpers READ/WRITE_SYSREG32
>>> in favour of using READ/WRITE_SYSREG.
>>> We should also use register_t type when reading sysregs
>>> which can correspond to uint64_t or uint32_t.
>>> Even though many AArch64 sysregs have upper 32bit reserved
>>> it does not mean that they can't be widen in the future.
>>>
>>> Modify type of registers: actlr, cntkctl to register_t.
>>
>> ACTLR is implementation defined, so in theory there might already bits already defined in the range [32:63]. So I would consider to split it from the patch so we can backport it.
>>
> You mean not to touch it at all in this series or to create a seperate patch only for ACTLR?
I meant creating a separate patch for ACTLR as part of this series.
>>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>>> ---
>>> xen/arch/arm/domain.c | 20 ++++++++++----------
>>> xen/include/asm-arm/domain.h | 4 ++--
>>> 2 files changed, 12 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>>> index bdd3d3e5b5..c021a03c61 100644
>>> --- a/xen/arch/arm/domain.c
>>> +++ b/xen/arch/arm/domain.c
>>> @@ -113,13 +113,13 @@ static void ctxt_switch_from(struct vcpu *p)
>>> p->arch.tpidr_el1 = READ_SYSREG(TPIDR_EL1);
>>> /* Arch timer */
>>> - p->arch.cntkctl = READ_SYSREG32(CNTKCTL_EL1);
>>> + p->arch.cntkctl = READ_SYSREG(CNTKCTL_EL1);
>>> virt_timer_save(p);
>>> if ( is_32bit_domain(p->domain) && cpu_has_thumbee )
>>> {
>>> - p->arch.teecr = READ_SYSREG32(TEECR32_EL1);
>>> - p->arch.teehbr = READ_SYSREG32(TEEHBR32_EL1);
>>> + p->arch.teecr = READ_SYSREG(TEECR32_EL1);
>>> + p->arch.teehbr = READ_SYSREG(TEEHBR32_EL1);
>>
>> It feels strange you converted cntkctl and actlr to use register_t but not teecr and teehbr. Can you explain why?
>>
> Because thumbee and its registers do not appear in any of ARMv8 ARM version.
This was defined on the very first version of the ARMv8-A spec (issue
a.a) but was dropped on subsequent versinos. Hence why the compiler
doesn't shout at you when finding TEECR32_EL1 and TEEHBR32_EL1.
> This means that this code could be protected even with #ifdef CONFIG_ARM_32 as AArch64 do not have them.
+1. The feature was completely dropped for Armv8 (even if it appeared in
earlier version of the spec). I have checked AMD Seattle, QEMU and the
Foundation model to confirm the feature is not exposed.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 3/9] arm/gic: Get rid of READ/WRITE_SYSREG32
2021-04-20 7:08 [PATCH 0/9] xen/arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
2021-04-20 7:08 ` [PATCH 1/9] arm64/vfp: " Michal Orzel
2021-04-20 7:08 ` [PATCH 2/9] arm/domain: " Michal Orzel
@ 2021-04-20 7:08 ` Michal Orzel
2021-04-20 13:28 ` Julien Grall
2021-04-20 7:08 ` [PATCH 4/9] arm/p2m: " Michal Orzel
` (5 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Michal Orzel @ 2021-04-20 7:08 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk, bertrand.marquis
AArch64 system registers are 64bit whereas AArch32 ones
are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
we should get rid of helpers READ/WRITE_SYSREG32
in favour of using READ/WRITE_SYSREG.
We should also use register_t type when reading sysregs
which can correspond to uint64_t or uint32_t.
Even though many AArch64 sysregs have upper 32bit reserved
it does not mean that they can't be widen in the future.
Modify types of following members of struct gic_v3 to register_t:
-hcr(not used at all in Xen)
-vmcr
-sre_el1
-apr0
-apr1
Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
xen/arch/arm/gic-v3-lpi.c | 2 +-
xen/arch/arm/gic-v3.c | 96 ++++++++++++++++++++-------------------
xen/include/asm-arm/gic.h | 6 +--
3 files changed, 54 insertions(+), 50 deletions(-)
diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
index 869bc97fa1..e1594dd20e 100644
--- a/xen/arch/arm/gic-v3-lpi.c
+++ b/xen/arch/arm/gic-v3-lpi.c
@@ -178,7 +178,7 @@ void gicv3_do_LPI(unsigned int lpi)
irq_enter();
/* EOI the LPI already. */
- WRITE_SYSREG32(lpi, ICC_EOIR1_EL1);
+ WRITE_SYSREG(lpi, ICC_EOIR1_EL1);
/* Find out if a guest mapped something to this physical LPI. */
hlpip = gic_get_host_lpi(lpi);
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index ac28013c19..0634013a67 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -246,12 +246,12 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
*/
static void gicv3_enable_sre(void)
{
- uint32_t val;
+ register_t val;
- val = READ_SYSREG32(ICC_SRE_EL2);
+ val = READ_SYSREG(ICC_SRE_EL2);
val |= GICC_SRE_EL2_SRE;
- WRITE_SYSREG32(val, ICC_SRE_EL2);
+ WRITE_SYSREG(val, ICC_SRE_EL2);
isb();
}
@@ -315,16 +315,16 @@ static void restore_aprn_regs(const union gic_state_data *d)
switch ( gicv3.nr_priorities )
{
case 7:
- WRITE_SYSREG32(d->v3.apr0[2], ICH_AP0R2_EL2);
- WRITE_SYSREG32(d->v3.apr1[2], ICH_AP1R2_EL2);
+ WRITE_SYSREG(d->v3.apr0[2], ICH_AP0R2_EL2);
+ WRITE_SYSREG(d->v3.apr1[2], ICH_AP1R2_EL2);
/* Fall through */
case 6:
- WRITE_SYSREG32(d->v3.apr0[1], ICH_AP0R1_EL2);
- WRITE_SYSREG32(d->v3.apr1[1], ICH_AP1R1_EL2);
+ WRITE_SYSREG(d->v3.apr0[1], ICH_AP0R1_EL2);
+ WRITE_SYSREG(d->v3.apr1[1], ICH_AP1R1_EL2);
/* Fall through */
case 5:
- WRITE_SYSREG32(d->v3.apr0[0], ICH_AP0R0_EL2);
- WRITE_SYSREG32(d->v3.apr1[0], ICH_AP1R0_EL2);
+ WRITE_SYSREG(d->v3.apr0[0], ICH_AP0R0_EL2);
+ WRITE_SYSREG(d->v3.apr1[0], ICH_AP1R0_EL2);
break;
default:
BUG();
@@ -338,16 +338,16 @@ static void save_aprn_regs(union gic_state_data *d)
switch ( gicv3.nr_priorities )
{
case 7:
- d->v3.apr0[2] = READ_SYSREG32(ICH_AP0R2_EL2);
- d->v3.apr1[2] = READ_SYSREG32(ICH_AP1R2_EL2);
+ d->v3.apr0[2] = READ_SYSREG(ICH_AP0R2_EL2);
+ d->v3.apr1[2] = READ_SYSREG(ICH_AP1R2_EL2);
/* Fall through */
case 6:
- d->v3.apr0[1] = READ_SYSREG32(ICH_AP0R1_EL2);
- d->v3.apr1[1] = READ_SYSREG32(ICH_AP1R1_EL2);
+ d->v3.apr0[1] = READ_SYSREG(ICH_AP0R1_EL2);
+ d->v3.apr1[1] = READ_SYSREG(ICH_AP1R1_EL2);
/* Fall through */
case 5:
- d->v3.apr0[0] = READ_SYSREG32(ICH_AP0R0_EL2);
- d->v3.apr1[0] = READ_SYSREG32(ICH_AP1R0_EL2);
+ d->v3.apr0[0] = READ_SYSREG(ICH_AP0R0_EL2);
+ d->v3.apr1[0] = READ_SYSREG(ICH_AP1R0_EL2);
break;
default:
BUG();
@@ -371,15 +371,15 @@ static void gicv3_save_state(struct vcpu *v)
dsb(sy);
gicv3_save_lrs(v);
save_aprn_regs(&v->arch.gic);
- v->arch.gic.v3.vmcr = READ_SYSREG32(ICH_VMCR_EL2);
- v->arch.gic.v3.sre_el1 = READ_SYSREG32(ICC_SRE_EL1);
+ v->arch.gic.v3.vmcr = READ_SYSREG(ICH_VMCR_EL2);
+ v->arch.gic.v3.sre_el1 = READ_SYSREG(ICC_SRE_EL1);
}
static void gicv3_restore_state(const struct vcpu *v)
{
- uint32_t val;
+ register_t val;
- val = READ_SYSREG32(ICC_SRE_EL2);
+ val = READ_SYSREG(ICC_SRE_EL2);
/*
* Don't give access to system registers when the guest is using
* GICv2
@@ -388,7 +388,7 @@ static void gicv3_restore_state(const struct vcpu *v)
val &= ~GICC_SRE_EL2_ENEL1;
else
val |= GICC_SRE_EL2_ENEL1;
- WRITE_SYSREG32(val, ICC_SRE_EL2);
+ WRITE_SYSREG(val, ICC_SRE_EL2);
/*
* VFIQEn is RES1 if ICC_SRE_EL1.SRE is 1. This causes a Group0
@@ -398,9 +398,9 @@ static void gicv3_restore_state(const struct vcpu *v)
* want before starting to mess with the rest of the GIC, and
* VMCR_EL1 in particular.
*/
- WRITE_SYSREG32(v->arch.gic.v3.sre_el1, ICC_SRE_EL1);
+ WRITE_SYSREG(v->arch.gic.v3.sre_el1, ICC_SRE_EL1);
isb();
- WRITE_SYSREG32(v->arch.gic.v3.vmcr, ICH_VMCR_EL2);
+ WRITE_SYSREG(v->arch.gic.v3.vmcr, ICH_VMCR_EL2);
restore_aprn_regs(&v->arch.gic);
gicv3_restore_lrs(v);
@@ -468,24 +468,25 @@ static void gicv3_mask_irq(struct irq_desc *irqd)
static void gicv3_eoi_irq(struct irq_desc *irqd)
{
/* Lower the priority */
- WRITE_SYSREG32(irqd->irq, ICC_EOIR1_EL1);
+ WRITE_SYSREG(irqd->irq, ICC_EOIR1_EL1);
isb();
}
static void gicv3_dir_irq(struct irq_desc *irqd)
{
/* Deactivate */
- WRITE_SYSREG32(irqd->irq, ICC_DIR_EL1);
+ WRITE_SYSREG(irqd->irq, ICC_DIR_EL1);
isb();
}
static unsigned int gicv3_read_irq(void)
{
- unsigned int irq = READ_SYSREG32(ICC_IAR1_EL1);
+ register_t irq = READ_SYSREG(ICC_IAR1_EL1);
dsb(sy);
- return irq;
+ /* Number of IRQs do not exceed 32bit. */
+ return (unsigned int)irq;
}
/*
@@ -857,16 +858,16 @@ static int gicv3_cpu_init(void)
gicv3_enable_sre();
/* No priority grouping */
- WRITE_SYSREG32(0, ICC_BPR1_EL1);
+ WRITE_SYSREG(0, ICC_BPR1_EL1);
/* Set priority mask register */
- WRITE_SYSREG32(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
+ WRITE_SYSREG(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
/* EOI drops priority, DIR deactivates the interrupt (mode 1) */
- WRITE_SYSREG32(GICC_CTLR_EL1_EOImode_drop, ICC_CTLR_EL1);
+ WRITE_SYSREG(GICC_CTLR_EL1_EOImode_drop, ICC_CTLR_EL1);
/* Enable Group1 interrupts */
- WRITE_SYSREG32(1, ICC_IGRPEN1_EL1);
+ WRITE_SYSREG(1, ICC_IGRPEN1_EL1);
/* Sync at once at the end of cpu interface configuration */
isb();
@@ -876,15 +877,15 @@ static int gicv3_cpu_init(void)
static void gicv3_cpu_disable(void)
{
- WRITE_SYSREG32(0, ICC_CTLR_EL1);
+ WRITE_SYSREG(0, ICC_CTLR_EL1);
isb();
}
static void gicv3_hyp_init(void)
{
- uint32_t vtr;
+ register_t vtr;
- vtr = READ_SYSREG32(ICH_VTR_EL2);
+ vtr = READ_SYSREG(ICH_VTR_EL2);
gicv3_info.nr_lrs = (vtr & ICH_VTR_NRLRGS) + 1;
gicv3.nr_priorities = ((vtr >> ICH_VTR_PRIBITS_SHIFT) &
ICH_VTR_PRIBITS_MASK) + 1;
@@ -892,8 +893,8 @@ static void gicv3_hyp_init(void)
if ( !((gicv3.nr_priorities > 4) && (gicv3.nr_priorities < 8)) )
panic("GICv3: Invalid number of priority bits\n");
- WRITE_SYSREG32(ICH_VMCR_EOI | ICH_VMCR_VENG1, ICH_VMCR_EL2);
- WRITE_SYSREG32(GICH_HCR_EN, ICH_HCR_EL2);
+ WRITE_SYSREG(ICH_VMCR_EOI | ICH_VMCR_VENG1, ICH_VMCR_EL2);
+ WRITE_SYSREG(GICH_HCR_EN, ICH_HCR_EL2);
}
/* Set up the per-CPU parts of the GIC for a secondary CPU */
@@ -917,11 +918,11 @@ out:
static void gicv3_hyp_disable(void)
{
- uint32_t hcr;
+ register_t hcr;
- hcr = READ_SYSREG32(ICH_HCR_EL2);
+ hcr = READ_SYSREG(ICH_HCR_EL2);
hcr &= ~GICH_HCR_EN;
- WRITE_SYSREG32(hcr, ICH_HCR_EL2);
+ WRITE_SYSREG(hcr, ICH_HCR_EL2);
isb();
}
@@ -1140,39 +1141,42 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
static void gicv3_hcr_status(uint32_t flag, bool status)
{
- uint32_t hcr;
+ register_t hcr;
- hcr = READ_SYSREG32(ICH_HCR_EL2);
+ hcr = READ_SYSREG(ICH_HCR_EL2);
if ( status )
- WRITE_SYSREG32(hcr | flag, ICH_HCR_EL2);
+ WRITE_SYSREG(hcr | flag, ICH_HCR_EL2);
else
- WRITE_SYSREG32(hcr & (~flag), ICH_HCR_EL2);
+ WRITE_SYSREG(hcr & (~flag), ICH_HCR_EL2);
isb();
}
static unsigned int gicv3_read_vmcr_priority(void)
{
- return ((READ_SYSREG32(ICH_VMCR_EL2) >> ICH_VMCR_PRIORITY_SHIFT) &
+ return ((READ_SYSREG(ICH_VMCR_EL2) >> ICH_VMCR_PRIORITY_SHIFT) &
ICH_VMCR_PRIORITY_MASK);
}
/* Only support reading GRP1 APRn registers */
static unsigned int gicv3_read_apr(int apr_reg)
{
+ register_t apr;
switch ( apr_reg )
{
case 0:
ASSERT(gicv3.nr_priorities > 4 && gicv3.nr_priorities < 8);
- return READ_SYSREG32(ICH_AP1R0_EL2);
+ apr = READ_SYSREG(ICH_AP1R0_EL2);
case 1:
ASSERT(gicv3.nr_priorities > 5 && gicv3.nr_priorities < 8);
- return READ_SYSREG32(ICH_AP1R1_EL2);
+ apr = READ_SYSREG(ICH_AP1R1_EL2);
case 2:
ASSERT(gicv3.nr_priorities > 6 && gicv3.nr_priorities < 8);
- return READ_SYSREG32(ICH_AP1R2_EL2);
+ apr = READ_SYSREG(ICH_AP1R2_EL2);
default:
BUG();
}
+ /* Number of priority levels do not exceed 32bit */
+ return (unsigned int)apr;
}
static bool gicv3_read_pending_state(struct irq_desc *irqd)
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index ad0f7452d0..d750d070b4 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -171,9 +171,9 @@
* GICv3 registers that needs to be saved/restored
*/
struct gic_v3 {
- uint32_t hcr, vmcr, sre_el1;
- uint32_t apr0[4];
- uint32_t apr1[4];
+ register_t hcr, vmcr, sre_el1;
+ register_t apr0[4];
+ register_t apr1[4];
uint64_t lr[16];
};
#endif
--
2.29.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 3/9] arm/gic: Get rid of READ/WRITE_SYSREG32
2021-04-20 7:08 ` [PATCH 3/9] arm/gic: " Michal Orzel
@ 2021-04-20 13:28 ` Julien Grall
2021-04-21 7:48 ` Michal Orzel
0 siblings, 1 reply; 27+ messages in thread
From: Julien Grall @ 2021-04-20 13:28 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis
Hi Michal,
On 20/04/2021 08:08, Michal Orzel wrote:
> AArch64 system registers are 64bit whereas AArch32 ones
> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
> we should get rid of helpers READ/WRITE_SYSREG32
> in favour of using READ/WRITE_SYSREG.
> We should also use register_t type when reading sysregs
> which can correspond to uint64_t or uint32_t.
> Even though many AArch64 sysregs have upper 32bit reserved
> it does not mean that they can't be widen in the future.
>
> Modify types of following members of struct gic_v3 to register_t:
> -hcr(not used at all in Xen)
It looks like we never used it (even in the patch introducing it). So I
would suggest to remove it in a patch before this one.
> -vmcr
> -sre_el1
> -apr0
> -apr1
>
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> ---
> xen/arch/arm/gic-v3-lpi.c | 2 +-
> xen/arch/arm/gic-v3.c | 96 ++++++++++++++++++++-------------------
> xen/include/asm-arm/gic.h | 6 +--
> 3 files changed, 54 insertions(+), 50 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
> index 869bc97fa1..e1594dd20e 100644
> --- a/xen/arch/arm/gic-v3-lpi.c
> +++ b/xen/arch/arm/gic-v3-lpi.c
> @@ -178,7 +178,7 @@ void gicv3_do_LPI(unsigned int lpi)
> irq_enter();
>
> /* EOI the LPI already. */
> - WRITE_SYSREG32(lpi, ICC_EOIR1_EL1);
> + WRITE_SYSREG(lpi, ICC_EOIR1_EL1);
>
> /* Find out if a guest mapped something to this physical LPI. */
> hlpip = gic_get_host_lpi(lpi);
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index ac28013c19..0634013a67 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -246,12 +246,12 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
> */
> static void gicv3_enable_sre(void)
> {
> - uint32_t val;
> + register_t val;
>
> - val = READ_SYSREG32(ICC_SRE_EL2);
> + val = READ_SYSREG(ICC_SRE_EL2);
> val |= GICC_SRE_EL2_SRE;
>
> - WRITE_SYSREG32(val, ICC_SRE_EL2);
> + WRITE_SYSREG(val, ICC_SRE_EL2);
> isb();
> }
>
> @@ -315,16 +315,16 @@ static void restore_aprn_regs(const union gic_state_data *d)
> switch ( gicv3.nr_priorities )
> {
> case 7:
> - WRITE_SYSREG32(d->v3.apr0[2], ICH_AP0R2_EL2);
> - WRITE_SYSREG32(d->v3.apr1[2], ICH_AP1R2_EL2);
> + WRITE_SYSREG(d->v3.apr0[2], ICH_AP0R2_EL2);
> + WRITE_SYSREG(d->v3.apr1[2], ICH_AP1R2_EL2);
> /* Fall through */
> case 6:
> - WRITE_SYSREG32(d->v3.apr0[1], ICH_AP0R1_EL2);
> - WRITE_SYSREG32(d->v3.apr1[1], ICH_AP1R1_EL2);
> + WRITE_SYSREG(d->v3.apr0[1], ICH_AP0R1_EL2);
> + WRITE_SYSREG(d->v3.apr1[1], ICH_AP1R1_EL2);
> /* Fall through */
> case 5:
> - WRITE_SYSREG32(d->v3.apr0[0], ICH_AP0R0_EL2);
> - WRITE_SYSREG32(d->v3.apr1[0], ICH_AP1R0_EL2);
> + WRITE_SYSREG(d->v3.apr0[0], ICH_AP0R0_EL2);
> + WRITE_SYSREG(d->v3.apr1[0], ICH_AP1R0_EL2);
> break;
> default:
> BUG();
> @@ -338,16 +338,16 @@ static void save_aprn_regs(union gic_state_data *d)
> switch ( gicv3.nr_priorities )
> {
> case 7:
> - d->v3.apr0[2] = READ_SYSREG32(ICH_AP0R2_EL2);
> - d->v3.apr1[2] = READ_SYSREG32(ICH_AP1R2_EL2);
> + d->v3.apr0[2] = READ_SYSREG(ICH_AP0R2_EL2);
> + d->v3.apr1[2] = READ_SYSREG(ICH_AP1R2_EL2);
> /* Fall through */
> case 6:
> - d->v3.apr0[1] = READ_SYSREG32(ICH_AP0R1_EL2);
> - d->v3.apr1[1] = READ_SYSREG32(ICH_AP1R1_EL2);
> + d->v3.apr0[1] = READ_SYSREG(ICH_AP0R1_EL2);
> + d->v3.apr1[1] = READ_SYSREG(ICH_AP1R1_EL2);
> /* Fall through */
> case 5:
> - d->v3.apr0[0] = READ_SYSREG32(ICH_AP0R0_EL2);
> - d->v3.apr1[0] = READ_SYSREG32(ICH_AP1R0_EL2);
> + d->v3.apr0[0] = READ_SYSREG(ICH_AP0R0_EL2);
> + d->v3.apr1[0] = READ_SYSREG(ICH_AP1R0_EL2);
> break;
> default:
> BUG();
> @@ -371,15 +371,15 @@ static void gicv3_save_state(struct vcpu *v)
> dsb(sy);
> gicv3_save_lrs(v);
> save_aprn_regs(&v->arch.gic);
> - v->arch.gic.v3.vmcr = READ_SYSREG32(ICH_VMCR_EL2);
> - v->arch.gic.v3.sre_el1 = READ_SYSREG32(ICC_SRE_EL1);
> + v->arch.gic.v3.vmcr = READ_SYSREG(ICH_VMCR_EL2);
> + v->arch.gic.v3.sre_el1 = READ_SYSREG(ICC_SRE_EL1);
> }
>
> static void gicv3_restore_state(const struct vcpu *v)
> {
> - uint32_t val;
> + register_t val;
>
> - val = READ_SYSREG32(ICC_SRE_EL2);
> + val = READ_SYSREG(ICC_SRE_EL2);
> /*
> * Don't give access to system registers when the guest is using
> * GICv2
> @@ -388,7 +388,7 @@ static void gicv3_restore_state(const struct vcpu *v)
> val &= ~GICC_SRE_EL2_ENEL1;
> else
> val |= GICC_SRE_EL2_ENEL1;
> - WRITE_SYSREG32(val, ICC_SRE_EL2);
> + WRITE_SYSREG(val, ICC_SRE_EL2);
>
> /*
> * VFIQEn is RES1 if ICC_SRE_EL1.SRE is 1. This causes a Group0
> @@ -398,9 +398,9 @@ static void gicv3_restore_state(const struct vcpu *v)
> * want before starting to mess with the rest of the GIC, and
> * VMCR_EL1 in particular.
> */
> - WRITE_SYSREG32(v->arch.gic.v3.sre_el1, ICC_SRE_EL1);
> + WRITE_SYSREG(v->arch.gic.v3.sre_el1, ICC_SRE_EL1);
> isb();
> - WRITE_SYSREG32(v->arch.gic.v3.vmcr, ICH_VMCR_EL2);
> + WRITE_SYSREG(v->arch.gic.v3.vmcr, ICH_VMCR_EL2);
> restore_aprn_regs(&v->arch.gic);
> gicv3_restore_lrs(v);
>
> @@ -468,24 +468,25 @@ static void gicv3_mask_irq(struct irq_desc *irqd)
> static void gicv3_eoi_irq(struct irq_desc *irqd)
> {
> /* Lower the priority */
> - WRITE_SYSREG32(irqd->irq, ICC_EOIR1_EL1);
> + WRITE_SYSREG(irqd->irq, ICC_EOIR1_EL1);
> isb();
> }
>
> static void gicv3_dir_irq(struct irq_desc *irqd)
> {
> /* Deactivate */
> - WRITE_SYSREG32(irqd->irq, ICC_DIR_EL1);
> + WRITE_SYSREG(irqd->irq, ICC_DIR_EL1);
> isb();
> }
>
> static unsigned int gicv3_read_irq(void)
> {
> - unsigned int irq = READ_SYSREG32(ICC_IAR1_EL1);
> + register_t irq = READ_SYSREG(ICC_IAR1_EL1);
>
> dsb(sy);
>
> - return irq;
> + /* Number of IRQs do not exceed 32bit. */
If we want to be pedantic, the IRQs are encoded using 23-bit. So maybe
we want to mask them below.
> + return (unsigned int)irq;
NIT: We tend to avoid explicit cast unless they are strictly necessary
because they can be more harmful than implicit cast (the compiler may
not cast every conversion). So I would drop it and just keep the comment.
> }
>
> /*
> @@ -857,16 +858,16 @@ static int gicv3_cpu_init(void)
> gicv3_enable_sre();
>
> /* No priority grouping */
> - WRITE_SYSREG32(0, ICC_BPR1_EL1);
> + WRITE_SYSREG(0, ICC_BPR1_EL1);
>
> /* Set priority mask register */
> - WRITE_SYSREG32(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
> + WRITE_SYSREG(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
>
> /* EOI drops priority, DIR deactivates the interrupt (mode 1) */
> - WRITE_SYSREG32(GICC_CTLR_EL1_EOImode_drop, ICC_CTLR_EL1);
> + WRITE_SYSREG(GICC_CTLR_EL1_EOImode_drop, ICC_CTLR_EL1);
>
> /* Enable Group1 interrupts */
> - WRITE_SYSREG32(1, ICC_IGRPEN1_EL1);
> + WRITE_SYSREG(1, ICC_IGRPEN1_EL1);
>
> /* Sync at once at the end of cpu interface configuration */
> isb();
> @@ -876,15 +877,15 @@ static int gicv3_cpu_init(void)
>
> static void gicv3_cpu_disable(void)
> {
> - WRITE_SYSREG32(0, ICC_CTLR_EL1);
> + WRITE_SYSREG(0, ICC_CTLR_EL1);
> isb();
> }
>
> static void gicv3_hyp_init(void)
> {
> - uint32_t vtr;
> + register_t vtr;
>
> - vtr = READ_SYSREG32(ICH_VTR_EL2);
> + vtr = READ_SYSREG(ICH_VTR_EL2);
> gicv3_info.nr_lrs = (vtr & ICH_VTR_NRLRGS) + 1;
> gicv3.nr_priorities = ((vtr >> ICH_VTR_PRIBITS_SHIFT) &
> ICH_VTR_PRIBITS_MASK) + 1;
> @@ -892,8 +893,8 @@ static void gicv3_hyp_init(void)
> if ( !((gicv3.nr_priorities > 4) && (gicv3.nr_priorities < 8)) )
> panic("GICv3: Invalid number of priority bits\n");
>
> - WRITE_SYSREG32(ICH_VMCR_EOI | ICH_VMCR_VENG1, ICH_VMCR_EL2);
> - WRITE_SYSREG32(GICH_HCR_EN, ICH_HCR_EL2);
> + WRITE_SYSREG(ICH_VMCR_EOI | ICH_VMCR_VENG1, ICH_VMCR_EL2);
> + WRITE_SYSREG(GICH_HCR_EN, ICH_HCR_EL2);
> }
>
> /* Set up the per-CPU parts of the GIC for a secondary CPU */
> @@ -917,11 +918,11 @@ out:
>
> static void gicv3_hyp_disable(void)
> {
> - uint32_t hcr;
> + register_t hcr;
>
> - hcr = READ_SYSREG32(ICH_HCR_EL2);
> + hcr = READ_SYSREG(ICH_HCR_EL2);
> hcr &= ~GICH_HCR_EN;
> - WRITE_SYSREG32(hcr, ICH_HCR_EL2);
> + WRITE_SYSREG(hcr, ICH_HCR_EL2);
> isb();
> }
>
> @@ -1140,39 +1141,42 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>
> static void gicv3_hcr_status(uint32_t flag, bool status)
> {
> - uint32_t hcr;
> + register_t hcr;
>
> - hcr = READ_SYSREG32(ICH_HCR_EL2);
> + hcr = READ_SYSREG(ICH_HCR_EL2);
> if ( status )
> - WRITE_SYSREG32(hcr | flag, ICH_HCR_EL2);
> + WRITE_SYSREG(hcr | flag, ICH_HCR_EL2);
> else
> - WRITE_SYSREG32(hcr & (~flag), ICH_HCR_EL2);
> + WRITE_SYSREG(hcr & (~flag), ICH_HCR_EL2);
> isb();
> }
>
> static unsigned int gicv3_read_vmcr_priority(void)
> {
> - return ((READ_SYSREG32(ICH_VMCR_EL2) >> ICH_VMCR_PRIORITY_SHIFT) &
> + return ((READ_SYSREG(ICH_VMCR_EL2) >> ICH_VMCR_PRIORITY_SHIFT) &
> ICH_VMCR_PRIORITY_MASK);
> }
>
> /* Only support reading GRP1 APRn registers */
> static unsigned int gicv3_read_apr(int apr_reg)
> {
> + register_t apr;
NIT: Please add a newline after the declaration. This will be easier to
read.
> switch ( apr_reg )
> {
> case 0:
> ASSERT(gicv3.nr_priorities > 4 && gicv3.nr_priorities < 8);
> - return READ_SYSREG32(ICH_AP1R0_EL2);
> + apr = READ_SYSREG(ICH_AP1R0_EL2);
> case 1:
> ASSERT(gicv3.nr_priorities > 5 && gicv3.nr_priorities < 8);
> - return READ_SYSREG32(ICH_AP1R1_EL2);
> + apr = READ_SYSREG(ICH_AP1R1_EL2);
> case 2:
> ASSERT(gicv3.nr_priorities > 6 && gicv3.nr_priorities < 8);
> - return READ_SYSREG32(ICH_AP1R2_EL2);
> + apr = READ_SYSREG(ICH_AP1R2_EL2);
> default:
> BUG();
> }
NIT: Please add a newline here. This will be easier to read.
> + /* Number of priority levels do not exceed 32bit */
> + return (unsigned int)apr;
NIT: Same remark as before for the cast.
> }
>
> static bool gicv3_read_pending_state(struct irq_desc *irqd)
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index ad0f7452d0..d750d070b4 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -171,9 +171,9 @@
> * GICv3 registers that needs to be saved/restored
> */
> struct gic_v3 {
> - uint32_t hcr, vmcr, sre_el1;
> - uint32_t apr0[4];
> - uint32_t apr1[4];
> + register_t hcr, vmcr, sre_el1;
> + register_t apr0[4];
> + register_t apr1[4];
> uint64_t lr[16];
> };
> #endif
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 3/9] arm/gic: Get rid of READ/WRITE_SYSREG32
2021-04-20 13:28 ` Julien Grall
@ 2021-04-21 7:48 ` Michal Orzel
2021-04-21 10:21 ` Julien Grall
0 siblings, 1 reply; 27+ messages in thread
From: Michal Orzel @ 2021-04-21 7:48 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis
Hi Julien,
On 20.04.2021 15:28, Julien Grall wrote:
> Hi Michal,
>
> On 20/04/2021 08:08, Michal Orzel wrote:
>> AArch64 system registers are 64bit whereas AArch32 ones
>> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
>> we should get rid of helpers READ/WRITE_SYSREG32
>> in favour of using READ/WRITE_SYSREG.
>> We should also use register_t type when reading sysregs
>> which can correspond to uint64_t or uint32_t.
>> Even though many AArch64 sysregs have upper 32bit reserved
>> it does not mean that they can't be widen in the future.
>>
>> Modify types of following members of struct gic_v3 to register_t:
>> -hcr(not used at all in Xen)
>
> It looks like we never used it (even in the patch introducing it). So I would suggest to remove it in a patch before this one.
>
Ok. In v2 I will add a patch before this one removing hcr from gic_v3.
>> -vmcr
>> -sre_el1
>> -apr0
>> -apr1
>>
>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>> ---
>> xen/arch/arm/gic-v3-lpi.c | 2 +-
>> xen/arch/arm/gic-v3.c | 96 ++++++++++++++++++++-------------------
>> xen/include/asm-arm/gic.h | 6 +--
>> 3 files changed, 54 insertions(+), 50 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
>> index 869bc97fa1..e1594dd20e 100644
>> --- a/xen/arch/arm/gic-v3-lpi.c
>> +++ b/xen/arch/arm/gic-v3-lpi.c
>> @@ -178,7 +178,7 @@ void gicv3_do_LPI(unsigned int lpi)
>> irq_enter();
>> /* EOI the LPI already. */
>> - WRITE_SYSREG32(lpi, ICC_EOIR1_EL1);
>> + WRITE_SYSREG(lpi, ICC_EOIR1_EL1);
>> /* Find out if a guest mapped something to this physical LPI. */
>> hlpip = gic_get_host_lpi(lpi);
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index ac28013c19..0634013a67 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -246,12 +246,12 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
>> */
>> static void gicv3_enable_sre(void)
>> {
>> - uint32_t val;
>> + register_t val;
>> - val = READ_SYSREG32(ICC_SRE_EL2);
>> + val = READ_SYSREG(ICC_SRE_EL2);
>> val |= GICC_SRE_EL2_SRE;
>> - WRITE_SYSREG32(val, ICC_SRE_EL2);
>> + WRITE_SYSREG(val, ICC_SRE_EL2);
>> isb();
>> }
>> @@ -315,16 +315,16 @@ static void restore_aprn_regs(const union gic_state_data *d)
>> switch ( gicv3.nr_priorities )
>> {
>> case 7:
>> - WRITE_SYSREG32(d->v3.apr0[2], ICH_AP0R2_EL2);
>> - WRITE_SYSREG32(d->v3.apr1[2], ICH_AP1R2_EL2);
>> + WRITE_SYSREG(d->v3.apr0[2], ICH_AP0R2_EL2);
>> + WRITE_SYSREG(d->v3.apr1[2], ICH_AP1R2_EL2);
>> /* Fall through */
>> case 6:
>> - WRITE_SYSREG32(d->v3.apr0[1], ICH_AP0R1_EL2);
>> - WRITE_SYSREG32(d->v3.apr1[1], ICH_AP1R1_EL2);
>> + WRITE_SYSREG(d->v3.apr0[1], ICH_AP0R1_EL2);
>> + WRITE_SYSREG(d->v3.apr1[1], ICH_AP1R1_EL2);
>> /* Fall through */
>> case 5:
>> - WRITE_SYSREG32(d->v3.apr0[0], ICH_AP0R0_EL2);
>> - WRITE_SYSREG32(d->v3.apr1[0], ICH_AP1R0_EL2);
>> + WRITE_SYSREG(d->v3.apr0[0], ICH_AP0R0_EL2);
>> + WRITE_SYSREG(d->v3.apr1[0], ICH_AP1R0_EL2);
>> break;
>> default:
>> BUG();
>> @@ -338,16 +338,16 @@ static void save_aprn_regs(union gic_state_data *d)
>> switch ( gicv3.nr_priorities )
>> {
>> case 7:
>> - d->v3.apr0[2] = READ_SYSREG32(ICH_AP0R2_EL2);
>> - d->v3.apr1[2] = READ_SYSREG32(ICH_AP1R2_EL2);
>> + d->v3.apr0[2] = READ_SYSREG(ICH_AP0R2_EL2);
>> + d->v3.apr1[2] = READ_SYSREG(ICH_AP1R2_EL2);
>> /* Fall through */
>> case 6:
>> - d->v3.apr0[1] = READ_SYSREG32(ICH_AP0R1_EL2);
>> - d->v3.apr1[1] = READ_SYSREG32(ICH_AP1R1_EL2);
>> + d->v3.apr0[1] = READ_SYSREG(ICH_AP0R1_EL2);
>> + d->v3.apr1[1] = READ_SYSREG(ICH_AP1R1_EL2);
>> /* Fall through */
>> case 5:
>> - d->v3.apr0[0] = READ_SYSREG32(ICH_AP0R0_EL2);
>> - d->v3.apr1[0] = READ_SYSREG32(ICH_AP1R0_EL2);
>> + d->v3.apr0[0] = READ_SYSREG(ICH_AP0R0_EL2);
>> + d->v3.apr1[0] = READ_SYSREG(ICH_AP1R0_EL2);
>> break;
>> default:
>> BUG();
>> @@ -371,15 +371,15 @@ static void gicv3_save_state(struct vcpu *v)
>> dsb(sy);
>> gicv3_save_lrs(v);
>> save_aprn_regs(&v->arch.gic);
>> - v->arch.gic.v3.vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>> - v->arch.gic.v3.sre_el1 = READ_SYSREG32(ICC_SRE_EL1);
>> + v->arch.gic.v3.vmcr = READ_SYSREG(ICH_VMCR_EL2);
>> + v->arch.gic.v3.sre_el1 = READ_SYSREG(ICC_SRE_EL1);
>> }
>> static void gicv3_restore_state(const struct vcpu *v)
>> {
>> - uint32_t val;
>> + register_t val;
>> - val = READ_SYSREG32(ICC_SRE_EL2);
>> + val = READ_SYSREG(ICC_SRE_EL2);
>> /*
>> * Don't give access to system registers when the guest is using
>> * GICv2
>> @@ -388,7 +388,7 @@ static void gicv3_restore_state(const struct vcpu *v)
>> val &= ~GICC_SRE_EL2_ENEL1;
>> else
>> val |= GICC_SRE_EL2_ENEL1;
>> - WRITE_SYSREG32(val, ICC_SRE_EL2);
>> + WRITE_SYSREG(val, ICC_SRE_EL2);
>> /*
>> * VFIQEn is RES1 if ICC_SRE_EL1.SRE is 1. This causes a Group0
>> @@ -398,9 +398,9 @@ static void gicv3_restore_state(const struct vcpu *v)
>> * want before starting to mess with the rest of the GIC, and
>> * VMCR_EL1 in particular.
>> */
>> - WRITE_SYSREG32(v->arch.gic.v3.sre_el1, ICC_SRE_EL1);
>> + WRITE_SYSREG(v->arch.gic.v3.sre_el1, ICC_SRE_EL1);
>> isb();
>> - WRITE_SYSREG32(v->arch.gic.v3.vmcr, ICH_VMCR_EL2);
>> + WRITE_SYSREG(v->arch.gic.v3.vmcr, ICH_VMCR_EL2);
>> restore_aprn_regs(&v->arch.gic);
>> gicv3_restore_lrs(v);
>> @@ -468,24 +468,25 @@ static void gicv3_mask_irq(struct irq_desc *irqd)
>> static void gicv3_eoi_irq(struct irq_desc *irqd)
>> {
>> /* Lower the priority */
>> - WRITE_SYSREG32(irqd->irq, ICC_EOIR1_EL1);
>> + WRITE_SYSREG(irqd->irq, ICC_EOIR1_EL1);
>> isb();
>> }
>> static void gicv3_dir_irq(struct irq_desc *irqd)
>> {
>> /* Deactivate */
>> - WRITE_SYSREG32(irqd->irq, ICC_DIR_EL1);
>> + WRITE_SYSREG(irqd->irq, ICC_DIR_EL1);
>> isb();
>> }
>> static unsigned int gicv3_read_irq(void)
>> {
>> - unsigned int irq = READ_SYSREG32(ICC_IAR1_EL1);
>> + register_t irq = READ_SYSREG(ICC_IAR1_EL1);
>> dsb(sy);
>> - return irq;
>> + /* Number of IRQs do not exceed 32bit. */
>
> If we want to be pedantic, the IRQs are encoded using 23-bit. So maybe we want to mask them below.
>
>> + return (unsigned int)irq;
>
> NIT: We tend to avoid explicit cast unless they are strictly necessary because they can be more harmful than implicit cast (the compiler may not cast every conversion). So I would drop it and just keep the comment.
>
Ok. I will do:
return (irq & 0xffffff);
>> }
>> /*
>> @@ -857,16 +858,16 @@ static int gicv3_cpu_init(void)
>> gicv3_enable_sre();
>> /* No priority grouping */
>> - WRITE_SYSREG32(0, ICC_BPR1_EL1);
>> + WRITE_SYSREG(0, ICC_BPR1_EL1);
>> /* Set priority mask register */
>> - WRITE_SYSREG32(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
>> + WRITE_SYSREG(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
>> /* EOI drops priority, DIR deactivates the interrupt (mode 1) */
>> - WRITE_SYSREG32(GICC_CTLR_EL1_EOImode_drop, ICC_CTLR_EL1);
>> + WRITE_SYSREG(GICC_CTLR_EL1_EOImode_drop, ICC_CTLR_EL1);
>> /* Enable Group1 interrupts */
>> - WRITE_SYSREG32(1, ICC_IGRPEN1_EL1);
>> + WRITE_SYSREG(1, ICC_IGRPEN1_EL1);
>> /* Sync at once at the end of cpu interface configuration */
>> isb();
>> @@ -876,15 +877,15 @@ static int gicv3_cpu_init(void)
>> static void gicv3_cpu_disable(void)
>> {
>> - WRITE_SYSREG32(0, ICC_CTLR_EL1);
>> + WRITE_SYSREG(0, ICC_CTLR_EL1);
>> isb();
>> }
>> static void gicv3_hyp_init(void)
>> {
>> - uint32_t vtr;
>> + register_t vtr;
>> - vtr = READ_SYSREG32(ICH_VTR_EL2);
>> + vtr = READ_SYSREG(ICH_VTR_EL2);
>> gicv3_info.nr_lrs = (vtr & ICH_VTR_NRLRGS) + 1;
>> gicv3.nr_priorities = ((vtr >> ICH_VTR_PRIBITS_SHIFT) &
>> ICH_VTR_PRIBITS_MASK) + 1;
>> @@ -892,8 +893,8 @@ static void gicv3_hyp_init(void)
>> if ( !((gicv3.nr_priorities > 4) && (gicv3.nr_priorities < 8)) )
>> panic("GICv3: Invalid number of priority bits\n");
>> - WRITE_SYSREG32(ICH_VMCR_EOI | ICH_VMCR_VENG1, ICH_VMCR_EL2);
>> - WRITE_SYSREG32(GICH_HCR_EN, ICH_HCR_EL2);
>> + WRITE_SYSREG(ICH_VMCR_EOI | ICH_VMCR_VENG1, ICH_VMCR_EL2);
>> + WRITE_SYSREG(GICH_HCR_EN, ICH_HCR_EL2);
>> }
>> /* Set up the per-CPU parts of the GIC for a secondary CPU */
>> @@ -917,11 +918,11 @@ out:
>> static void gicv3_hyp_disable(void)
>> {
>> - uint32_t hcr;
>> + register_t hcr;
>> - hcr = READ_SYSREG32(ICH_HCR_EL2);
>> + hcr = READ_SYSREG(ICH_HCR_EL2);
>> hcr &= ~GICH_HCR_EN;
>> - WRITE_SYSREG32(hcr, ICH_HCR_EL2);
>> + WRITE_SYSREG(hcr, ICH_HCR_EL2);
>> isb();
>> }
>> @@ -1140,39 +1141,42 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>> static void gicv3_hcr_status(uint32_t flag, bool status)
>> {
>> - uint32_t hcr;
>> + register_t hcr;
>> - hcr = READ_SYSREG32(ICH_HCR_EL2);
>> + hcr = READ_SYSREG(ICH_HCR_EL2);
>> if ( status )
>> - WRITE_SYSREG32(hcr | flag, ICH_HCR_EL2);
>> + WRITE_SYSREG(hcr | flag, ICH_HCR_EL2);
>> else
>> - WRITE_SYSREG32(hcr & (~flag), ICH_HCR_EL2);
>> + WRITE_SYSREG(hcr & (~flag), ICH_HCR_EL2);
>> isb();
>> }
>> static unsigned int gicv3_read_vmcr_priority(void)
>> {
>> - return ((READ_SYSREG32(ICH_VMCR_EL2) >> ICH_VMCR_PRIORITY_SHIFT) &
>> + return ((READ_SYSREG(ICH_VMCR_EL2) >> ICH_VMCR_PRIORITY_SHIFT) &
>> ICH_VMCR_PRIORITY_MASK);
>> }
>> /* Only support reading GRP1 APRn registers */
>> static unsigned int gicv3_read_apr(int apr_reg)
>> {
>> + register_t apr;
>
> NIT: Please add a newline after the declaration. This will be easier to read.
>
Ok.
>> switch ( apr_reg )
>> {
>> case 0:
>> ASSERT(gicv3.nr_priorities > 4 && gicv3.nr_priorities < 8);
>> - return READ_SYSREG32(ICH_AP1R0_EL2);
>> + apr = READ_SYSREG(ICH_AP1R0_EL2);
>> case 1:
>> ASSERT(gicv3.nr_priorities > 5 && gicv3.nr_priorities < 8);
>> - return READ_SYSREG32(ICH_AP1R1_EL2);
>> + apr = READ_SYSREG(ICH_AP1R1_EL2);
>> case 2:
>> ASSERT(gicv3.nr_priorities > 6 && gicv3.nr_priorities < 8);
>> - return READ_SYSREG32(ICH_AP1R2_EL2);
>> + apr = READ_SYSREG(ICH_AP1R2_EL2);
>> default:
>> BUG();
>> }
>
> NIT: Please add a newline here. This will be easier to read.
>
Ok.
>> + /* Number of priority levels do not exceed 32bit */
>> + return (unsigned int)apr;
>
> NIT: Same remark as before for the cast.
>
Ok. I will do just:
return apr;
>> }
>> static bool gicv3_read_pending_state(struct irq_desc *irqd)
>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>> index ad0f7452d0..d750d070b4 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -171,9 +171,9 @@
>> * GICv3 registers that needs to be saved/restored
>> */
>> struct gic_v3 {
>> - uint32_t hcr, vmcr, sre_el1;
>> - uint32_t apr0[4];
>> - uint32_t apr1[4];
>> + register_t hcr, vmcr, sre_el1;
>> + register_t apr0[4];
>> + register_t apr1[4];
>> uint64_t lr[16];
>> };
>> #endif
>>
>
> Cheers,
>
Cheers
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 3/9] arm/gic: Get rid of READ/WRITE_SYSREG32
2021-04-21 7:48 ` Michal Orzel
@ 2021-04-21 10:21 ` Julien Grall
0 siblings, 0 replies; 27+ messages in thread
From: Julien Grall @ 2021-04-21 10:21 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis
Hi Michal,
On 21/04/2021 08:48, Michal Orzel wrote:
> On 20.04.2021 15:28, Julien Grall wrote:
>> On 20/04/2021 08:08, Michal Orzel wrote:
>>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>>> static unsigned int gicv3_read_irq(void)
>>> {
>>> - unsigned int irq = READ_SYSREG32(ICC_IAR1_EL1);
>>> + register_t irq = READ_SYSREG(ICC_IAR1_EL1);
>>> dsb(sy);
>>> - return irq;
>>> + /* Number of IRQs do not exceed 32bit. */
>>
>> If we want to be pedantic, the IRQs are encoded using 23-bit. So maybe we want to mask them below.
>>
>>> + return (unsigned int)irq;
>>
>> NIT: We tend to avoid explicit cast unless they are strictly necessary because they can be more harmful than implicit cast (the compiler may not cast every conversion). So I would drop it and just keep the comment.
>>
> Ok. I will do:
> return (irq & 0xffffff);
Sounds good, so long we the value is not hardoced but provided through a
define :).
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 4/9] arm/p2m: Get rid of READ/WRITE_SYSREG32
2021-04-20 7:08 [PATCH 0/9] xen/arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
` (2 preceding siblings ...)
2021-04-20 7:08 ` [PATCH 3/9] arm/gic: " Michal Orzel
@ 2021-04-20 7:08 ` Michal Orzel
2021-04-20 13:31 ` Julien Grall
2021-04-20 7:08 ` [PATCH 5/9] arm/mm: " Michal Orzel
` (4 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Michal Orzel @ 2021-04-20 7:08 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk, bertrand.marquis
AArch64 system registers are 64bit whereas AArch32 ones
are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
we should get rid of helpers READ/WRITE_SYSREG32
in favour of using READ/WRITE_SYSREG.
We should also use register_t type when reading sysregs
which can correspond to uint64_t or uint32_t.
Even though many AArch64 sysregs have upper 32bit reserved
it does not mean that they can't be widen in the future.
Modify type of vtcr to register_t.
Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
xen/arch/arm/p2m.c | 8 ++++----
xen/arch/arm/traps.c | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index ac50312620..d414c4feb9 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1973,11 +1973,11 @@ void __init p2m_restrict_ipa_bits(unsigned int ipa_bits)
}
/* VTCR value to be configured by all CPUs. Set only once by the boot CPU */
-static uint32_t __read_mostly vtcr;
+static register_t __read_mostly vtcr;
static void setup_virt_paging_one(void *data)
{
- WRITE_SYSREG32(vtcr, VTCR_EL2);
+ WRITE_SYSREG(vtcr, VTCR_EL2);
/*
* ARM64_WORKAROUND_AT_SPECULATE: We want to keep the TLBs free from
@@ -2000,7 +2000,7 @@ static void setup_virt_paging_one(void *data)
void __init setup_virt_paging(void)
{
/* Setup Stage 2 address translation */
- unsigned long val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
+ register_t val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
#ifdef CONFIG_ARM_32
if ( p2m_ipa_bits < 40 )
@@ -2089,7 +2089,7 @@ void __init setup_virt_paging(void)
pa_range_info[pa_range].pabits,
( MAX_VMID == MAX_VMID_16_BIT ) ? 16 : 8);
#endif
- printk("P2M: %d levels with order-%d root, VTCR 0x%lx\n",
+ printk("P2M: %d levels with order-%d root, VTCR 0x%"PRIregister"\n",
4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, val);
p2m_vmid_allocator_init();
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ccc0827107..c7acdb2087 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -911,7 +911,7 @@ static void _show_registers(const struct cpu_user_regs *regs,
show_registers_32(regs, ctxt, guest_mode, v);
#endif
}
- printk(" VTCR_EL2: %08"PRIx32"\n", READ_SYSREG32(VTCR_EL2));
+ printk(" VTCR_EL2: %"PRIregister"\n", READ_SYSREG(VTCR_EL2));
printk(" VTTBR_EL2: %016"PRIx64"\n", ctxt->vttbr_el2);
printk("\n");
--
2.29.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 4/9] arm/p2m: Get rid of READ/WRITE_SYSREG32
2021-04-20 7:08 ` [PATCH 4/9] arm/p2m: " Michal Orzel
@ 2021-04-20 13:31 ` Julien Grall
0 siblings, 0 replies; 27+ messages in thread
From: Julien Grall @ 2021-04-20 13:31 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis
Hi Michal,
On 20/04/2021 08:08, Michal Orzel wrote:
> AArch64 system registers are 64bit whereas AArch32 ones
> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
> we should get rid of helpers READ/WRITE_SYSREG32
> in favour of using READ/WRITE_SYSREG.
> We should also use register_t type when reading sysregs
> which can correspond to uint64_t or uint32_t.
> Even though many AArch64 sysregs have upper 32bit reserved
> it does not mean that they can't be widen in the future.
>
> Modify type of vtcr to register_t.
>
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 5/9] arm/mm: Get rid of READ/WRITE_SYSREG32
2021-04-20 7:08 [PATCH 0/9] xen/arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
` (3 preceding siblings ...)
2021-04-20 7:08 ` [PATCH 4/9] arm/p2m: " Michal Orzel
@ 2021-04-20 7:08 ` Michal Orzel
2021-04-20 13:37 ` Julien Grall
2021-04-20 7:08 ` [PATCH 6/9] arm/page: " Michal Orzel
` (3 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Michal Orzel @ 2021-04-20 7:08 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk, bertrand.marquis
AArch64 system registers are 64bit whereas AArch32 ones
are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
we should get rid of helpers READ/WRITE_SYSREG32
in favour of using READ/WRITE_SYSREG.
We should also use register_t type when reading sysregs
which can correspond to uint64_t or uint32_t.
Even though many AArch64 sysregs have upper 32bit reserved
it does not mean that they can't be widen in the future.
Modify SCTLR_EL2 accesses to use READ/WRITE_SYSREG.
Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
xen/arch/arm/mm.c | 2 +-
xen/arch/arm/traps.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 59f8a3f15f..0e07335291 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -613,7 +613,7 @@ void __init remove_early_mappings(void)
*/
static void xen_pt_enforce_wnx(void)
{
- WRITE_SYSREG32(READ_SYSREG32(SCTLR_EL2) | SCTLR_Axx_ELx_WXN, SCTLR_EL2);
+ WRITE_SYSREG(READ_SYSREG(SCTLR_EL2) | SCTLR_Axx_ELx_WXN, SCTLR_EL2);
/*
* The TLBs may cache SCTLR_EL2.WXN. So ensure it is synchronized
* before flushing the TLBs.
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index c7acdb2087..e7384381cc 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -915,7 +915,7 @@ static void _show_registers(const struct cpu_user_regs *regs,
printk(" VTTBR_EL2: %016"PRIx64"\n", ctxt->vttbr_el2);
printk("\n");
- printk(" SCTLR_EL2: %08"PRIx32"\n", READ_SYSREG32(SCTLR_EL2));
+ printk(" SCTLR_EL2: %"PRIregister"\n", READ_SYSREG(SCTLR_EL2));
printk(" HCR_EL2: %"PRIregister"\n", READ_SYSREG(HCR_EL2));
printk(" TTBR0_EL2: %016"PRIx64"\n", READ_SYSREG64(TTBR0_EL2));
printk("\n");
--
2.29.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 5/9] arm/mm: Get rid of READ/WRITE_SYSREG32
2021-04-20 7:08 ` [PATCH 5/9] arm/mm: " Michal Orzel
@ 2021-04-20 13:37 ` Julien Grall
2021-04-22 11:47 ` Michal Orzel
0 siblings, 1 reply; 27+ messages in thread
From: Julien Grall @ 2021-04-20 13:37 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis
Hi Michal,
On 20/04/2021 08:08, Michal Orzel wrote:
> AArch64 system registers are 64bit whereas AArch32 ones
> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
> we should get rid of helpers READ/WRITE_SYSREG32
> in favour of using READ/WRITE_SYSREG.
> We should also use register_t type when reading sysregs
> which can correspond to uint64_t or uint32_t.
> Even though many AArch64 sysregs have upper 32bit reserved
> it does not mean that they can't be widen in the future.
>
> Modify SCTLR_EL2 accesses to use READ/WRITE_SYSREG.
SCTLR_EL2 already has bits defined in the range [32:63]. So this change
is going to have a side effect as AFAICT head.S will not touch those
bits. So they are now going to be preserved.
The Arm Arm defines them as unknown if implemented. Therefore shouldn't
we zero them somewhere else?
In any case, I think the commit message ought to contain an analysis for
system registers that happened to have bits defined in the range [32:63].
>
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> ---
> xen/arch/arm/mm.c | 2 +-
> xen/arch/arm/traps.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 59f8a3f15f..0e07335291 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -613,7 +613,7 @@ void __init remove_early_mappings(void)
> */
> static void xen_pt_enforce_wnx(void)
> {
> - WRITE_SYSREG32(READ_SYSREG32(SCTLR_EL2) | SCTLR_Axx_ELx_WXN, SCTLR_EL2);
> + WRITE_SYSREG(READ_SYSREG(SCTLR_EL2) | SCTLR_Axx_ELx_WXN, SCTLR_EL2);
> /*
> * The TLBs may cache SCTLR_EL2.WXN. So ensure it is synchronized
> * before flushing the TLBs.
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index c7acdb2087..e7384381cc 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -915,7 +915,7 @@ static void _show_registers(const struct cpu_user_regs *regs,
> printk(" VTTBR_EL2: %016"PRIx64"\n", ctxt->vttbr_el2);
> printk("\n");
>
> - printk(" SCTLR_EL2: %08"PRIx32"\n", READ_SYSREG32(SCTLR_EL2));
> + printk(" SCTLR_EL2: %"PRIregister"\n", READ_SYSREG(SCTLR_EL2));
> printk(" HCR_EL2: %"PRIregister"\n", READ_SYSREG(HCR_EL2));
> printk(" TTBR0_EL2: %016"PRIx64"\n", READ_SYSREG64(TTBR0_EL2));
> printk("\n");
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 5/9] arm/mm: Get rid of READ/WRITE_SYSREG32
2021-04-20 13:37 ` Julien Grall
@ 2021-04-22 11:47 ` Michal Orzel
2021-04-22 13:40 ` Julien Grall
0 siblings, 1 reply; 27+ messages in thread
From: Michal Orzel @ 2021-04-22 11:47 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis
Hi Julien,
On 20.04.2021 15:37, Julien Grall wrote:
> Hi Michal,
>
> On 20/04/2021 08:08, Michal Orzel wrote:
>> AArch64 system registers are 64bit whereas AArch32 ones
>> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
>> we should get rid of helpers READ/WRITE_SYSREG32
>> in favour of using READ/WRITE_SYSREG.
>> We should also use register_t type when reading sysregs
>> which can correspond to uint64_t or uint32_t.
>> Even though many AArch64 sysregs have upper 32bit reserved
>> it does not mean that they can't be widen in the future.
>>
>> Modify SCTLR_EL2 accesses to use READ/WRITE_SYSREG.
>
> SCTLR_EL2 already has bits defined in the range [32:63]. So this change is going to have a side effect as AFAICT head.S will not touch those bits. So they are now going to be preserved.
>
> The Arm Arm defines them as unknown if implemented. Therefore shouldn't we zero them somewhere else?
>
SCTLR_EL2 is set in head.S using SCTLR_EL2_SET which means that we are zeroing the upper 32bit half.
> In any case, I think the commit message ought to contain an analysis for system registers that happened to have bits defined in the range [32:63].
>
>>
>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>> ---
>> xen/arch/arm/mm.c | 2 +-
>> xen/arch/arm/traps.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 59f8a3f15f..0e07335291 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -613,7 +613,7 @@ void __init remove_early_mappings(void)
>> */
>> static void xen_pt_enforce_wnx(void)
>> {
>> - WRITE_SYSREG32(READ_SYSREG32(SCTLR_EL2) | SCTLR_Axx_ELx_WXN, SCTLR_EL2);
>> + WRITE_SYSREG(READ_SYSREG(SCTLR_EL2) | SCTLR_Axx_ELx_WXN, SCTLR_EL2);
>> /*
>> * The TLBs may cache SCTLR_EL2.WXN. So ensure it is synchronized
>> * before flushing the TLBs.
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index c7acdb2087..e7384381cc 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -915,7 +915,7 @@ static void _show_registers(const struct cpu_user_regs *regs,
>> printk(" VTTBR_EL2: %016"PRIx64"\n", ctxt->vttbr_el2);
>> printk("\n");
>> - printk(" SCTLR_EL2: %08"PRIx32"\n", READ_SYSREG32(SCTLR_EL2));
>> + printk(" SCTLR_EL2: %"PRIregister"\n", READ_SYSREG(SCTLR_EL2));
>> printk(" HCR_EL2: %"PRIregister"\n", READ_SYSREG(HCR_EL2));
>> printk(" TTBR0_EL2: %016"PRIx64"\n", READ_SYSREG64(TTBR0_EL2));
>> printk("\n");
>>
>
> Cheers,
>
Cheers,
Michal
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 5/9] arm/mm: Get rid of READ/WRITE_SYSREG32
2021-04-22 11:47 ` Michal Orzel
@ 2021-04-22 13:40 ` Julien Grall
0 siblings, 0 replies; 27+ messages in thread
From: Julien Grall @ 2021-04-22 13:40 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis
On 22/04/2021 12:47, Michal Orzel wrote:
> Hi Julien,
Hi Michal,
> On 20.04.2021 15:37, Julien Grall wrote:
>> Hi Michal,
>>
>> On 20/04/2021 08:08, Michal Orzel wrote:
>>> AArch64 system registers are 64bit whereas AArch32 ones
>>> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
>>> we should get rid of helpers READ/WRITE_SYSREG32
>>> in favour of using READ/WRITE_SYSREG.
>>> We should also use register_t type when reading sysregs
>>> which can correspond to uint64_t or uint32_t.
>>> Even though many AArch64 sysregs have upper 32bit reserved
>>> it does not mean that they can't be widen in the future.
>>>
>>> Modify SCTLR_EL2 accesses to use READ/WRITE_SYSREG.
>>
>> SCTLR_EL2 already has bits defined in the range [32:63]. So this change is going to have a side effect as AFAICT head.S will not touch those bits. So they are now going to be preserved.
>>
>> The Arm Arm defines them as unknown if implemented. Therefore shouldn't we zero them somewhere else?
>>
> SCTLR_EL2 is set in head.S using SCTLR_EL2_SET which means that we are zeroing the upper 32bit half.
Ah! I couldn't find the place doing it. Thanks!
I think it is important to point in the commit message this is just a
latent bug because the top 32-bit was not used by Xen. Can you update
the commit message?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/9] arm/page: Get rid of READ/WRITE_SYSREG32
2021-04-20 7:08 [PATCH 0/9] xen/arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
` (4 preceding siblings ...)
2021-04-20 7:08 ` [PATCH 5/9] arm/mm: " Michal Orzel
@ 2021-04-20 7:08 ` Michal Orzel
2021-04-21 15:11 ` Julien Grall
2021-04-20 7:08 ` [PATCH 7/9] arm/time,vtimer: " Michal Orzel
` (2 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Michal Orzel @ 2021-04-20 7:08 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk, bertrand.marquis
AArch64 system registers are 64bit whereas AArch32 ones
are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
we should get rid of helpers READ/WRITE_SYSREG32
in favour of using READ/WRITE_SYSREG.
We should also use register_t type when reading sysregs
which can correspond to uint64_t or uint32_t.
Even though many AArch64 sysregs have upper 32bit reserved
it does not mean that they can't be widen in the future.
Modify accesses to CTR_EL0 to use READ/WRITE_SYSREG.
Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
xen/include/asm-arm/page.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 131507a517..c6f9fb0d4e 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -137,10 +137,10 @@ extern size_t dcache_line_bytes;
static inline size_t read_dcache_line_bytes(void)
{
- uint32_t ctr;
+ register_t ctr;
/* Read CTR */
- ctr = READ_SYSREG32(CTR_EL0);
+ ctr = READ_SYSREG(CTR_EL0);
/* Bits 16-19 are the log2 number of words in the cacheline. */
return (size_t) (4 << ((ctr >> 16) & 0xf));
--
2.29.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 6/9] arm/page: Get rid of READ/WRITE_SYSREG32
2021-04-20 7:08 ` [PATCH 6/9] arm/page: " Michal Orzel
@ 2021-04-21 15:11 ` Julien Grall
0 siblings, 0 replies; 27+ messages in thread
From: Julien Grall @ 2021-04-21 15:11 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis
Hi Michal,
On 20/04/2021 08:08, Michal Orzel wrote:
> AArch64 system registers are 64bit whereas AArch32 ones
> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
> we should get rid of helpers READ/WRITE_SYSREG32
> in favour of using READ/WRITE_SYSREG.
> We should also use register_t type when reading sysregs
> which can correspond to uint64_t or uint32_t.
> Even though many AArch64 sysregs have upper 32bit reserved
> it does not mean that they can't be widen in the future.
>
> Modify accesses to CTR_EL0 to use READ/WRITE_SYSREG.
>
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 7/9] arm/time,vtimer: Get rid of READ/WRITE_SYSREG32
2021-04-20 7:08 [PATCH 0/9] xen/arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
` (5 preceding siblings ...)
2021-04-20 7:08 ` [PATCH 6/9] arm/page: " Michal Orzel
@ 2021-04-20 7:08 ` Michal Orzel
2021-04-21 16:01 ` Julien Grall
2021-04-20 7:08 ` [PATCH 8/9] arm: Change type of hsr to register_t Michal Orzel
2021-04-20 7:08 ` [PATCH 9/9] xen/arm64: Remove READ/WRITE_SYSREG32 helper macros Michal Orzel
8 siblings, 1 reply; 27+ messages in thread
From: Michal Orzel @ 2021-04-20 7:08 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk, bertrand.marquis
AArch64 system registers are 64bit whereas AArch32 ones
are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
we should get rid of helpers READ/WRITE_SYSREG32
in favour of using READ/WRITE_SYSREG.
We should also use register_t type when reading sysregs
which can correspond to uint64_t or uint32_t.
Even though many AArch64 sysregs have upper 32bit reserved
it does not mean that they can't be widen in the future.
Modify type of vtimer structure's member: ctl to register_t.
Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
xen/arch/arm/time.c | 28 ++++++++++++++--------------
xen/arch/arm/vtimer.c | 10 +++++-----
xen/include/asm-arm/domain.h | 2 +-
3 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index b0021c2c69..e6c96eb90c 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -145,7 +145,7 @@ void __init preinit_xen_time(void)
preinit_acpi_xen_time();
if ( !cpu_khz )
- cpu_khz = READ_SYSREG32(CNTFRQ_EL0) / 1000;
+ cpu_khz = (unsigned long)(READ_SYSREG(CNTFRQ_EL0) / 1000);
res = platform_init_time();
if ( res )
@@ -205,13 +205,13 @@ int reprogram_timer(s_time_t timeout)
if ( timeout == 0 )
{
- WRITE_SYSREG32(0, CNTHP_CTL_EL2);
+ WRITE_SYSREG(0, CNTHP_CTL_EL2);
return 1;
}
deadline = ns_to_ticks(timeout) + boot_count;
WRITE_SYSREG64(deadline, CNTHP_CVAL_EL2);
- WRITE_SYSREG32(CNTx_CTL_ENABLE, CNTHP_CTL_EL2);
+ WRITE_SYSREG(CNTx_CTL_ENABLE, CNTHP_CTL_EL2);
isb();
/* No need to check for timers in the past; the Generic Timer fires
@@ -223,23 +223,23 @@ int reprogram_timer(s_time_t timeout)
static void timer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
{
if ( irq == (timer_irq[TIMER_HYP_PPI]) &&
- READ_SYSREG32(CNTHP_CTL_EL2) & CNTx_CTL_PENDING )
+ READ_SYSREG(CNTHP_CTL_EL2) & CNTx_CTL_PENDING )
{
perfc_incr(hyp_timer_irqs);
/* Signal the generic timer code to do its work */
raise_softirq(TIMER_SOFTIRQ);
/* Disable the timer to avoid more interrupts */
- WRITE_SYSREG32(0, CNTHP_CTL_EL2);
+ WRITE_SYSREG(0, CNTHP_CTL_EL2);
}
if ( irq == (timer_irq[TIMER_PHYS_NONSECURE_PPI]) &&
- READ_SYSREG32(CNTP_CTL_EL0) & CNTx_CTL_PENDING )
+ READ_SYSREG(CNTP_CTL_EL0) & CNTx_CTL_PENDING )
{
perfc_incr(phys_timer_irqs);
/* Signal the generic timer code to do its work */
raise_softirq(TIMER_SOFTIRQ);
/* Disable the timer to avoid more interrupts */
- WRITE_SYSREG32(0, CNTP_CTL_EL0);
+ WRITE_SYSREG(0, CNTP_CTL_EL0);
}
}
@@ -260,8 +260,8 @@ static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
perfc_incr(virt_timer_irqs);
- current->arch.virt_timer.ctl = READ_SYSREG32(CNTV_CTL_EL0);
- WRITE_SYSREG32(current->arch.virt_timer.ctl | CNTx_CTL_MASK, CNTV_CTL_EL0);
+ current->arch.virt_timer.ctl = READ_SYSREG(CNTV_CTL_EL0);
+ WRITE_SYSREG(current->arch.virt_timer.ctl | CNTx_CTL_MASK, CNTV_CTL_EL0);
vgic_inject_irq(current->domain, current, current->arch.virt_timer.irq, true);
}
@@ -297,9 +297,9 @@ void init_timer_interrupt(void)
/* Sensible defaults */
WRITE_SYSREG64(0, CNTVOFF_EL2); /* No VM-specific offset */
/* Do not let the VMs program the physical timer, only read the physical counter */
- WRITE_SYSREG32(CNTHCTL_EL2_EL1PCTEN, CNTHCTL_EL2);
- WRITE_SYSREG32(0, CNTP_CTL_EL0); /* Physical timer disabled */
- WRITE_SYSREG32(0, CNTHP_CTL_EL2); /* Hypervisor's timer disabled */
+ WRITE_SYSREG(CNTHCTL_EL2_EL1PCTEN, CNTHCTL_EL2);
+ WRITE_SYSREG(0, CNTP_CTL_EL0); /* Physical timer disabled */
+ WRITE_SYSREG(0, CNTHP_CTL_EL2); /* Hypervisor's timer disabled */
isb();
request_irq(timer_irq[TIMER_HYP_PPI], 0, timer_interrupt,
@@ -320,8 +320,8 @@ void init_timer_interrupt(void)
*/
static void deinit_timer_interrupt(void)
{
- WRITE_SYSREG32(0, CNTP_CTL_EL0); /* Disable physical timer */
- WRITE_SYSREG32(0, CNTHP_CTL_EL2); /* Disable hypervisor's timer */
+ WRITE_SYSREG(0, CNTP_CTL_EL0); /* Disable physical timer */
+ WRITE_SYSREG(0, CNTHP_CTL_EL2); /* Disable hypervisor's timer */
isb();
release_irq(timer_irq[TIMER_HYP_PPI], NULL);
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index c2b27915c6..167fc6127a 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -138,8 +138,8 @@ void virt_timer_save(struct vcpu *v)
{
ASSERT(!is_idle_vcpu(v));
- v->arch.virt_timer.ctl = READ_SYSREG32(CNTV_CTL_EL0);
- WRITE_SYSREG32(v->arch.virt_timer.ctl & ~CNTx_CTL_ENABLE, CNTV_CTL_EL0);
+ v->arch.virt_timer.ctl = READ_SYSREG(CNTV_CTL_EL0);
+ WRITE_SYSREG(v->arch.virt_timer.ctl & ~CNTx_CTL_ENABLE, CNTV_CTL_EL0);
v->arch.virt_timer.cval = READ_SYSREG64(CNTV_CVAL_EL0);
if ( (v->arch.virt_timer.ctl & CNTx_CTL_ENABLE) &&
!(v->arch.virt_timer.ctl & CNTx_CTL_MASK))
@@ -159,7 +159,7 @@ void virt_timer_restore(struct vcpu *v)
WRITE_SYSREG64(v->domain->arch.virt_timer_base.offset, CNTVOFF_EL2);
WRITE_SYSREG64(v->arch.virt_timer.cval, CNTV_CVAL_EL0);
- WRITE_SYSREG32(v->arch.virt_timer.ctl, CNTV_CTL_EL0);
+ 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)
@@ -347,7 +347,7 @@ bool vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr)
}
static void vtimer_update_irq(struct vcpu *v, struct vtimer *vtimer,
- uint32_t vtimer_ctl)
+ register_t vtimer_ctl)
{
bool level;
@@ -389,7 +389,7 @@ void vtimer_update_irqs(struct vcpu *v)
* but this requires reworking the arch timer to implement this.
*/
vtimer_update_irq(v, &v->arch.virt_timer,
- READ_SYSREG32(CNTV_CTL_EL0) & ~CNTx_CTL_MASK);
+ READ_SYSREG(CNTV_CTL_EL0) & ~CNTx_CTL_MASK);
/* For the physical timer we rely on our emulated state. */
vtimer_update_irq(v, &v->arch.phys_timer, v->arch.phys_timer.ctl);
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 2d4f38c669..c9277b5c6d 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -36,7 +36,7 @@ struct vtimer {
struct vcpu *v;
int irq;
struct timer timer;
- uint32_t ctl;
+ register_t ctl;
uint64_t cval;
};
--
2.29.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 7/9] arm/time,vtimer: Get rid of READ/WRITE_SYSREG32
2021-04-20 7:08 ` [PATCH 7/9] arm/time,vtimer: " Michal Orzel
@ 2021-04-21 16:01 ` Julien Grall
0 siblings, 0 replies; 27+ messages in thread
From: Julien Grall @ 2021-04-21 16:01 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis
Hi Michal,
On 20/04/2021 08:08, Michal Orzel wrote:
> AArch64 system registers are 64bit whereas AArch32 ones
> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
> we should get rid of helpers READ/WRITE_SYSREG32
> in favour of using READ/WRITE_SYSREG.
> We should also use register_t type when reading sysregs
> which can correspond to uint64_t or uint32_t.
> Even though many AArch64 sysregs have upper 32bit reserved
> it does not mean that they can't be widen in the future.
>
> Modify type of vtimer structure's member: ctl to register_t.
>
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> ---
> xen/arch/arm/time.c | 28 ++++++++++++++--------------
> xen/arch/arm/vtimer.c | 10 +++++-----
> xen/include/asm-arm/domain.h | 2 +-
> 3 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index b0021c2c69..e6c96eb90c 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -145,7 +145,7 @@ void __init preinit_xen_time(void)
> preinit_acpi_xen_time();
>
> if ( !cpu_khz )
> - cpu_khz = READ_SYSREG32(CNTFRQ_EL0) / 1000;
> + cpu_khz = (unsigned long)(READ_SYSREG(CNTFRQ_EL0) / 1000);
From the Arm Arm (DDI 0486 F.c), only the lower 32-bit represents the
clock frequency. The rest are RES0 (IOW can have a different meaning in
the future). So I think you want to mask it.
>
> static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t *r, bool read)
> @@ -347,7 +347,7 @@ bool vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr)
> }
>
> static void vtimer_update_irq(struct vcpu *v, struct vtimer *vtimer,
> - uint32_t vtimer_ctl)
> + register_t vtimer_ctl)
> {
> bool level;
>
> @@ -389,7 +389,7 @@ void vtimer_update_irqs(struct vcpu *v)
> * but this requires reworking the arch timer to implement this.
> */
> vtimer_update_irq(v, &v->arch.virt_timer,
> - READ_SYSREG32(CNTV_CTL_EL0) & ~CNTx_CTL_MASK);
> + READ_SYSREG(CNTV_CTL_EL0) & ~CNTx_CTL_MASK);
AFAICT, CNTx_CTL_MASK is an unsigned int. This will not only mask IMASK
but also the bits 32-63 for Armv8.
So I think you want to switch CTNx_CTL_MASK to return an unsigned long
and explain it in the commit message.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 8/9] arm: Change type of hsr to register_t
2021-04-20 7:08 [PATCH 0/9] xen/arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
` (6 preceding siblings ...)
2021-04-20 7:08 ` [PATCH 7/9] arm/time,vtimer: " Michal Orzel
@ 2021-04-20 7:08 ` Michal Orzel
2021-04-21 19:02 ` Julien Grall
2021-04-20 7:08 ` [PATCH 9/9] xen/arm64: Remove READ/WRITE_SYSREG32 helper macros Michal Orzel
8 siblings, 1 reply; 27+ messages in thread
From: Michal Orzel @ 2021-04-20 7:08 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk, bertrand.marquis
AArch64 system registers are 64bit whereas AArch32 ones
are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
we should get rid of helpers READ/WRITE_SYSREG32
in favour of using READ/WRITE_SYSREG.
We should also use register_t type when reading sysregs
which can correspond to uint64_t or uint32_t.
Even though many AArch64 sysregs have upper 32bit reserved
it does not mean that they can't be widen in the future.
Modify type of hsr to register_t.
When on AArch64 add 32bit RES0 members to structures inside hsr union.
Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
xen/arch/arm/arm64/entry.S | 2 +-
xen/arch/arm/arm64/traps.c | 2 +-
xen/arch/arm/arm64/vsysreg.c | 3 ++-
xen/arch/arm/traps.c | 20 +++++++++-------
xen/arch/arm/vcpreg.c | 13 +++++-----
xen/include/asm-arm/arm32/processor.h | 2 +-
xen/include/asm-arm/arm64/processor.h | 5 ++--
xen/include/asm-arm/hsr.h | 34 ++++++++++++++++++++++++++-
8 files changed, 59 insertions(+), 22 deletions(-)
diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index ab9a65fc14..218b063c97 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -155,7 +155,7 @@
add x21, sp, #UREGS_CPSR
mrs x22, spsr_el2
mrs x23, esr_el2
- stp w22, w23, [x21]
+ stp x22, x23, [x21]
.endm
diff --git a/xen/arch/arm/arm64/traps.c b/xen/arch/arm/arm64/traps.c
index babfc1d884..9113a15c7a 100644
--- a/xen/arch/arm/arm64/traps.c
+++ b/xen/arch/arm/arm64/traps.c
@@ -36,7 +36,7 @@ void do_bad_mode(struct cpu_user_regs *regs, int reason)
union hsr hsr = { .bits = regs->hsr };
printk("Bad mode in %s handler detected\n", handler[reason]);
- printk("ESR=0x%08"PRIx32": EC=%"PRIx32", IL=%"PRIx32", ISS=%"PRIx32"\n",
+ printk("ESR=%#"PRIregister": EC=%"PRIx32", IL=%"PRIx32", ISS=%"PRIx32"\n",
hsr.bits, hsr.ec, hsr.len, hsr.iss);
local_irq_disable();
diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index 41f18612c6..3c10d464e7 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -368,7 +368,8 @@ void do_sysreg(struct cpu_user_regs *regs,
sysreg.op2,
sysreg.read ? "=>" : "<=",
sysreg.reg, regs->pc);
- gdprintk(XENLOG_ERR, "unhandled 64-bit sysreg access %#x\n",
+ gdprintk(XENLOG_ERR, "unhandled 64-bit sysreg access"
+ " %#"PRIregister"\n",
hsr.bits & HSR_SYSREG_REGS_MASK);
inject_undef_exception(regs, hsr);
return;
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index e7384381cc..db15a2c647 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -546,7 +546,7 @@ void inject_undef64_exception(struct cpu_user_regs *regs, int instr_len)
PSR_IRQ_MASK | PSR_DBG_MASK;
regs->pc = handler;
- WRITE_SYSREG32(esr.bits, ESR_EL1);
+ WRITE_SYSREG(esr.bits, ESR_EL1);
}
/* Inject an abort exception into a 64 bit guest */
@@ -580,7 +580,7 @@ static void inject_abt64_exception(struct cpu_user_regs *regs,
regs->pc = handler;
WRITE_SYSREG(addr, FAR_EL1);
- WRITE_SYSREG32(esr.bits, ESR_EL1);
+ WRITE_SYSREG(esr.bits, ESR_EL1);
}
static void inject_dabt64_exception(struct cpu_user_regs *regs,
@@ -919,7 +919,7 @@ static void _show_registers(const struct cpu_user_regs *regs,
printk(" HCR_EL2: %"PRIregister"\n", READ_SYSREG(HCR_EL2));
printk(" TTBR0_EL2: %016"PRIx64"\n", READ_SYSREG64(TTBR0_EL2));
printk("\n");
- printk(" ESR_EL2: %08"PRIx32"\n", regs->hsr);
+ printk(" ESR_EL2: %"PRIregister"\n", regs->hsr);
printk(" HPFAR_EL2: %"PRIregister"\n", READ_SYSREG(HPFAR_EL2));
#ifdef CONFIG_ARM_32
@@ -2004,13 +2004,15 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
break;
default:
- gprintk(XENLOG_WARNING, "Unsupported FSC: HSR=%#x DFSC=%#x\n",
+ gprintk(XENLOG_WARNING, "Unsupported FSC:"
+ " HSR=%#"PRIregister" DFSC=%#x\n",
hsr.bits, xabt.fsc);
}
inject_abt:
- gdprintk(XENLOG_DEBUG, "HSR=0x%x pc=%#"PRIregister" gva=%#"PRIvaddr
- " gpa=%#"PRIpaddr"\n", hsr.bits, regs->pc, gva, gpa);
+ gdprintk(XENLOG_DEBUG, "HSR=%#"PRIregister" pc=%#"PRIregister""
+ " gva=%#"PRIvaddr" gpa=%#"PRIpaddr"\n",
+ hsr.bits, regs->pc, gva, gpa);
if ( is_data )
inject_dabt_exception(regs, gva, hsr.len);
else
@@ -2204,7 +2206,8 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
default:
gprintk(XENLOG_WARNING,
- "Unknown Guest Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
+ "Unknown Guest Trap. HSR=%#"PRIregister" EC=0x%x IL=%x"
+ " Syndrome=0x%"PRIx32"\n",
hsr.bits, hsr.ec, hsr.len, hsr.iss);
inject_undef_exception(regs, hsr);
}
@@ -2242,7 +2245,8 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
break;
}
default:
- printk("Hypervisor Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
+ printk("Hypervisor Trap. HSR=%#"PRIregister" EC=0x%x IL=%x"
+ " Syndrome=0x%"PRIx32"\n",
hsr.bits, hsr.ec, hsr.len, hsr.iss);
do_unexpected_trap("Hypervisor", regs);
}
diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
index 55351fc087..c7f516ee0a 100644
--- a/xen/arch/arm/vcpreg.c
+++ b/xen/arch/arm/vcpreg.c
@@ -385,7 +385,7 @@ void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr)
"%s p15, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
cp32.read ? "mrc" : "mcr",
cp32.op1, cp32.reg, cp32.crn, cp32.crm, cp32.op2, regs->pc);
- gdprintk(XENLOG_ERR, "unhandled 32-bit CP15 access %#x\n",
+ gdprintk(XENLOG_ERR, "unhandled 32-bit CP15 access %#"PRIregister"\n",
hsr.bits & HSR_CP32_REGS_MASK);
inject_undef_exception(regs, hsr);
return;
@@ -454,7 +454,8 @@ void do_cp15_64(struct cpu_user_regs *regs, const union hsr hsr)
"%s p15, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
cp64.read ? "mrrc" : "mcrr",
cp64.op1, cp64.reg1, cp64.reg2, cp64.crm, regs->pc);
- gdprintk(XENLOG_ERR, "unhandled 64-bit CP15 access %#x\n",
+ gdprintk(XENLOG_ERR, "unhandled 64-bit CP15 access"
+ " %#"PRIregister"\n",
hsr.bits & HSR_CP64_REGS_MASK);
inject_undef_exception(regs, hsr);
return;
@@ -585,7 +586,7 @@ void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
"%s p14, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
cp32.read ? "mrc" : "mcr",
cp32.op1, cp32.reg, cp32.crn, cp32.crm, cp32.op2, regs->pc);
- gdprintk(XENLOG_ERR, "unhandled 32-bit cp14 access %#x\n",
+ gdprintk(XENLOG_ERR, "unhandled 32-bit cp14 access %#"PRIregister"\n",
hsr.bits & HSR_CP32_REGS_MASK);
inject_undef_exception(regs, hsr);
return;
@@ -627,7 +628,7 @@ void do_cp14_64(struct cpu_user_regs *regs, const union hsr hsr)
"%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
cp64.read ? "mrrc" : "mcrr",
cp64.op1, cp64.reg1, cp64.reg2, cp64.crm, regs->pc);
- gdprintk(XENLOG_ERR, "unhandled 64-bit CP14 access %#x\n",
+ gdprintk(XENLOG_ERR, "unhandled 64-bit CP14 access %#"PRIregister"\n",
hsr.bits & HSR_CP64_REGS_MASK);
inject_undef_exception(regs, hsr);
}
@@ -658,7 +659,7 @@ void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)
"%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
cp64.read ? "mrrc" : "mcrr",
cp64.op1, cp64.reg1, cp64.reg2, cp64.crm, regs->pc);
- gdprintk(XENLOG_ERR, "unhandled 64-bit CP14 DBG access %#x\n",
+ gdprintk(XENLOG_ERR, "unhandled 64-bit CP14 DBG access %#"PRIregister"\n",
hsr.bits & HSR_CP64_REGS_MASK);
inject_undef_exception(regs, hsr);
@@ -692,7 +693,7 @@ void do_cp10(struct cpu_user_regs *regs, const union hsr hsr)
"%s p10, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
cp32.read ? "mrc" : "mcr",
cp32.op1, cp32.reg, cp32.crn, cp32.crm, cp32.op2, regs->pc);
- gdprintk(XENLOG_ERR, "unhandled 32-bit CP10 access %#x\n",
+ gdprintk(XENLOG_ERR, "unhandled 32-bit CP10 access %#"PRIregister"\n",
hsr.bits & HSR_CP32_REGS_MASK);
inject_undef_exception(regs, hsr);
return;
diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h
index 4e679f3273..395ce10692 100644
--- a/xen/include/asm-arm/arm32/processor.h
+++ b/xen/include/asm-arm/arm32/processor.h
@@ -37,7 +37,7 @@ struct cpu_user_regs
uint32_t pc, pc32;
};
uint32_t cpsr; /* Return mode */
- uint32_t hsr; /* Exception Syndrome */
+ register_t hsr; /* Exception Syndrome */
/* Outer guest frame only from here on... */
diff --git a/xen/include/asm-arm/arm64/processor.h b/xen/include/asm-arm/arm64/processor.h
index 81dfc5e615..40f628d216 100644
--- a/xen/include/asm-arm/arm64/processor.h
+++ b/xen/include/asm-arm/arm64/processor.h
@@ -64,8 +64,9 @@ struct cpu_user_regs
/* Return address and mode */
__DECL_REG(pc, pc32); /* ELR_EL2 */
uint32_t cpsr; /* SPSR_EL2 */
- uint32_t hsr; /* ESR_EL2 */
+ register_t hsr; /* ESR_EL2 */
+ register_t pad1; /* Doubleword-align the user half of the frame */
/* Outer guest frame only from here on... */
union {
@@ -73,8 +74,6 @@ struct cpu_user_regs
uint32_t spsr_svc; /* AArch32 */
};
- uint32_t pad1; /* Doubleword-align the user half of the frame */
-
/* AArch32 guests only */
uint32_t spsr_fiq, spsr_irq, spsr_und, spsr_abt;
diff --git a/xen/include/asm-arm/hsr.h b/xen/include/asm-arm/hsr.h
index 29d4531f40..7424402c6e 100644
--- a/xen/include/asm-arm/hsr.h
+++ b/xen/include/asm-arm/hsr.h
@@ -16,11 +16,14 @@ enum dabt_size {
};
union hsr {
- uint32_t bits;
+ register_t bits;
struct {
unsigned long iss:25; /* Instruction Specific Syndrome */
unsigned long len:1; /* Instruction length */
unsigned long ec:6; /* Exception Class */
+#ifdef CONFIG_ARM_64
+ unsigned long _res0:32;
+#endif
};
/* Common to all conditional exception classes (0x0N, except 0x00). */
@@ -30,6 +33,9 @@ union hsr {
unsigned long ccvalid:1;/* CC Valid */
unsigned long len:1; /* Instruction length */
unsigned long ec:6; /* Exception Class */
+#ifdef CONFIG_ARM_64
+ unsigned long _res0:32;
+#endif
} cond;
struct hsr_wfi_wfe {
@@ -39,6 +45,9 @@ union hsr {
unsigned long ccvalid:1;/* CC Valid */
unsigned long len:1; /* Instruction length */
unsigned long ec:6; /* Exception Class */
+#ifdef CONFIG_ARM_64
+ unsigned long _res0:32;
+#endif
} wfi_wfe;
/* reg, reg0, reg1 are 4 bits on AArch32, the fifth bit is sbzp. */
@@ -53,6 +62,9 @@ union hsr {
unsigned long ccvalid:1;/* CC Valid */
unsigned long len:1; /* Instruction length */
unsigned long ec:6; /* Exception Class */
+#ifdef CONFIG_ARM_64
+ unsigned long _res0:32;
+#endif
} cp32; /* HSR_EC_CP15_32, CP14_32, CP10 */
struct hsr_cp64 {
@@ -66,6 +78,9 @@ union hsr {
unsigned long ccvalid:1;/* CC Valid */
unsigned long len:1; /* Instruction length */
unsigned long ec:6; /* Exception Class */
+#ifdef CONFIG_ARM_64
+ unsigned long _res0:32;
+#endif
} cp64; /* HSR_EC_CP15_64, HSR_EC_CP14_64 */
struct hsr_cp {
@@ -77,6 +92,9 @@ union hsr {
unsigned long ccvalid:1;/* CC Valid */
unsigned long len:1; /* Instruction length */
unsigned long ec:6; /* Exception Class */
+#ifdef CONFIG_ARM_64
+ unsigned long _res0:32;
+#endif
} cp; /* HSR_EC_CP */
/*
@@ -94,6 +112,9 @@ union hsr {
unsigned long ccvalid:1;/* CC Valid */
unsigned long len:1; /* Instruction length */
unsigned long ec:6; /* Exception Class */
+#ifdef CONFIG_ARM_64
+ unsigned long _res0:32;
+#endif
} smc32; /* HSR_EC_SMC32 */
#ifdef CONFIG_ARM_64
@@ -108,6 +129,7 @@ union hsr {
unsigned long res0:3;
unsigned long len:1; /* Instruction length */
unsigned long ec:6;
+ unsigned long _res0:32;
} sysreg; /* HSR_EC_SYSREG */
#endif
@@ -121,6 +143,9 @@ union hsr {
unsigned long res2:14;
unsigned long len:1; /* Instruction length */
unsigned long ec:6; /* Exception Class */
+#ifdef CONFIG_ARM_64
+ unsigned long _res0:32;
+#endif
} iabt; /* HSR_EC_INSTR_ABORT_* */
struct hsr_dabt {
@@ -143,6 +168,9 @@ union hsr {
unsigned long valid:1; /* Syndrome Valid */
unsigned long len:1; /* Instruction length */
unsigned long ec:6; /* Exception Class */
+#ifdef CONFIG_ARM_64
+ unsigned long _res0:32;
+#endif
} dabt; /* HSR_EC_DATA_ABORT_* */
/* Contain the common bits between DABT and IABT */
@@ -156,6 +184,9 @@ union hsr {
unsigned long pad3:14; /* Not common */
unsigned long len:1; /* Instruction length */
unsigned long ec:6; /* Exception Class */
+#ifdef CONFIG_ARM_64
+ unsigned long _res0:32;
+#endif
} xabt;
#ifdef CONFIG_ARM_64
@@ -164,6 +195,7 @@ union hsr {
unsigned long res0:9;
unsigned long len:1; /* Instruction length */
unsigned long ec:6; /* Exception Class */
+ unsigned long _res0:32;
} brk;
#endif
};
--
2.29.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 8/9] arm: Change type of hsr to register_t
2021-04-20 7:08 ` [PATCH 8/9] arm: Change type of hsr to register_t Michal Orzel
@ 2021-04-21 19:02 ` Julien Grall
0 siblings, 0 replies; 27+ messages in thread
From: Julien Grall @ 2021-04-21 19:02 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis
Hi Michal,
On 20/04/2021 08:08, Michal Orzel wrote:
> AArch64 system registers are 64bit whereas AArch32 ones
> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
> we should get rid of helpers READ/WRITE_SYSREG32
> in favour of using READ/WRITE_SYSREG.
> We should also use register_t type when reading sysregs
> which can correspond to uint64_t or uint32_t.
> Even though many AArch64 sysregs have upper 32bit reserved
> it does not mean that they can't be widen in the future.
>
> Modify type of hsr to register_t.
> When on AArch64 add 32bit RES0 members to structures inside hsr union.
>
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> ---
> xen/arch/arm/arm64/entry.S | 2 +-
> xen/arch/arm/arm64/traps.c | 2 +-
> xen/arch/arm/arm64/vsysreg.c | 3 ++-
> xen/arch/arm/traps.c | 20 +++++++++-------
> xen/arch/arm/vcpreg.c | 13 +++++-----
> xen/include/asm-arm/arm32/processor.h | 2 +-
> xen/include/asm-arm/arm64/processor.h | 5 ++--
> xen/include/asm-arm/hsr.h | 34 ++++++++++++++++++++++++++-
> 8 files changed, 59 insertions(+), 22 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index ab9a65fc14..218b063c97 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -155,7 +155,7 @@
> add x21, sp, #UREGS_CPSR
> mrs x22, spsr_el2
> mrs x23, esr_el2
> - stp w22, w23, [x21]
> + stp x22, x23, [x21]
>
> .endm
>
> diff --git a/xen/arch/arm/arm64/traps.c b/xen/arch/arm/arm64/traps.c
> index babfc1d884..9113a15c7a 100644
> --- a/xen/arch/arm/arm64/traps.c
> +++ b/xen/arch/arm/arm64/traps.c
> @@ -36,7 +36,7 @@ void do_bad_mode(struct cpu_user_regs *regs, int reason)
> union hsr hsr = { .bits = regs->hsr };
>
> printk("Bad mode in %s handler detected\n", handler[reason]);
> - printk("ESR=0x%08"PRIx32": EC=%"PRIx32", IL=%"PRIx32", ISS=%"PRIx32"\n",
> + printk("ESR=%#"PRIregister": EC=%"PRIx32", IL=%"PRIx32", ISS=%"PRIx32"\n",
> hsr.bits, hsr.ec, hsr.len, hsr.iss);
>
> local_irq_disable();
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index 41f18612c6..3c10d464e7 100644
> --- a/xen/arch/arm/arm64/vsysreg.c
> +++ b/xen/arch/arm/arm64/vsysreg.c
> @@ -368,7 +368,8 @@ void do_sysreg(struct cpu_user_regs *regs,
> sysreg.op2,
> sysreg.read ? "=>" : "<=",
> sysreg.reg, regs->pc);
> - gdprintk(XENLOG_ERR, "unhandled 64-bit sysreg access %#x\n",
> + gdprintk(XENLOG_ERR, "unhandled 64-bit sysreg access"
> + " %#"PRIregister"\n",
> hsr.bits & HSR_SYSREG_REGS_MASK);
> inject_undef_exception(regs, hsr);
> return;
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index e7384381cc..db15a2c647 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -546,7 +546,7 @@ void inject_undef64_exception(struct cpu_user_regs *regs, int instr_len)
> PSR_IRQ_MASK | PSR_DBG_MASK;
> regs->pc = handler;
>
> - WRITE_SYSREG32(esr.bits, ESR_EL1);
> + WRITE_SYSREG(esr.bits, ESR_EL1);
> }
>
> /* Inject an abort exception into a 64 bit guest */
> @@ -580,7 +580,7 @@ static void inject_abt64_exception(struct cpu_user_regs *regs,
> regs->pc = handler;
>
> WRITE_SYSREG(addr, FAR_EL1);
> - WRITE_SYSREG32(esr.bits, ESR_EL1);
> + WRITE_SYSREG(esr.bits, ESR_EL1);
> }
>
> static void inject_dabt64_exception(struct cpu_user_regs *regs,
> @@ -919,7 +919,7 @@ static void _show_registers(const struct cpu_user_regs *regs,
> printk(" HCR_EL2: %"PRIregister"\n", READ_SYSREG(HCR_EL2));
> printk(" TTBR0_EL2: %016"PRIx64"\n", READ_SYSREG64(TTBR0_EL2));
> printk("\n");
> - printk(" ESR_EL2: %08"PRIx32"\n", regs->hsr);
> + printk(" ESR_EL2: %"PRIregister"\n", regs->hsr);
> printk(" HPFAR_EL2: %"PRIregister"\n", READ_SYSREG(HPFAR_EL2));
>
> #ifdef CONFIG_ARM_32
> @@ -2004,13 +2004,15 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
>
> break;
> default:
> - gprintk(XENLOG_WARNING, "Unsupported FSC: HSR=%#x DFSC=%#x\n",
> + gprintk(XENLOG_WARNING, "Unsupported FSC:"
> + " HSR=%#"PRIregister" DFSC=%#x\n",
Please don't split the message in two. This makes more difficult to grep
bits of the message in the log.
Instead the code should be:
gprintk(XENLOG_WARNING,
"mystring",
...);
For this case, we are tolerated to go past the 80 characters mark.
> hsr.bits, xabt.fsc);
> }
>
> inject_abt:
> - gdprintk(XENLOG_DEBUG, "HSR=0x%x pc=%#"PRIregister" gva=%#"PRIvaddr
> - " gpa=%#"PRIpaddr"\n", hsr.bits, regs->pc, gva, gpa);
> + gdprintk(XENLOG_DEBUG, "HSR=%#"PRIregister" pc=%#"PRIregister""
> + " gva=%#"PRIvaddr" gpa=%#"PRIpaddr"\n",
Same here.
> + hsr.bits, regs->pc, gva, gpa);
> if ( is_data )
> inject_dabt_exception(regs, gva, hsr.len);
> else
> @@ -2204,7 +2206,8 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>
> default:
> gprintk(XENLOG_WARNING,
> - "Unknown Guest Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
> + "Unknown Guest Trap. HSR=%#"PRIregister" EC=0x%x IL=%x"
> + " Syndrome=0x%"PRIx32"\n",
Same here.
> hsr.bits, hsr.ec, hsr.len, hsr.iss);
> inject_undef_exception(regs, hsr);
> }
> @@ -2242,7 +2245,8 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
> break;
> }
> default:
> - printk("Hypervisor Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
> + printk("Hypervisor Trap. HSR=%#"PRIregister" EC=0x%x IL=%x"
> + " Syndrome=0x%"PRIx32"\n",
Same here.
> hsr.bits, hsr.ec, hsr.len, hsr.iss);
> do_unexpected_trap("Hypervisor", regs);
> }
> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
> index 55351fc087..c7f516ee0a 100644
> --- a/xen/arch/arm/vcpreg.c
> +++ b/xen/arch/arm/vcpreg.c
> @@ -385,7 +385,7 @@ void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr)
> "%s p15, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
> cp32.read ? "mrc" : "mcr",
> cp32.op1, cp32.reg, cp32.crn, cp32.crm, cp32.op2, regs->pc);
> - gdprintk(XENLOG_ERR, "unhandled 32-bit CP15 access %#x\n",
> + gdprintk(XENLOG_ERR, "unhandled 32-bit CP15 access %#"PRIregister"\n",
> hsr.bits & HSR_CP32_REGS_MASK);
> inject_undef_exception(regs, hsr);
> return;
> @@ -454,7 +454,8 @@ void do_cp15_64(struct cpu_user_regs *regs, const union hsr hsr)
> "%s p15, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
> cp64.read ? "mrrc" : "mcrr",
> cp64.op1, cp64.reg1, cp64.reg2, cp64.crm, regs->pc);
> - gdprintk(XENLOG_ERR, "unhandled 64-bit CP15 access %#x\n",
> + gdprintk(XENLOG_ERR, "unhandled 64-bit CP15 access" > + " %#"PRIregister"\n",
Same here.
> hsr.bits & HSR_CP64_REGS_MASK);
> inject_undef_exception(regs, hsr);
> return;
> @@ -585,7 +586,7 @@ void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
> "%s p14, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
> cp32.read ? "mrc" : "mcr",
> cp32.op1, cp32.reg, cp32.crn, cp32.crm, cp32.op2, regs->pc);
> - gdprintk(XENLOG_ERR, "unhandled 32-bit cp14 access %#x\n",
> + gdprintk(XENLOG_ERR, "unhandled 32-bit cp14 access %#"PRIregister"\n",
> hsr.bits & HSR_CP32_REGS_MASK);
> inject_undef_exception(regs, hsr);
> return;
> @@ -627,7 +628,7 @@ void do_cp14_64(struct cpu_user_regs *regs, const union hsr hsr)
> "%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
> cp64.read ? "mrrc" : "mcrr",
> cp64.op1, cp64.reg1, cp64.reg2, cp64.crm, regs->pc);
> - gdprintk(XENLOG_ERR, "unhandled 64-bit CP14 access %#x\n",
> + gdprintk(XENLOG_ERR, "unhandled 64-bit CP14 access %#"PRIregister"\n",
> hsr.bits & HSR_CP64_REGS_MASK);
> inject_undef_exception(regs, hsr);
> }
> @@ -658,7 +659,7 @@ void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)
> "%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
> cp64.read ? "mrrc" : "mcrr",
> cp64.op1, cp64.reg1, cp64.reg2, cp64.crm, regs->pc);
> - gdprintk(XENLOG_ERR, "unhandled 64-bit CP14 DBG access %#x\n",
> + gdprintk(XENLOG_ERR, "unhandled 64-bit CP14 DBG access %#"PRIregister"\n",
> hsr.bits & HSR_CP64_REGS_MASK);
>
> inject_undef_exception(regs, hsr);
> @@ -692,7 +693,7 @@ void do_cp10(struct cpu_user_regs *regs, const union hsr hsr)
> "%s p10, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
> cp32.read ? "mrc" : "mcr",
> cp32.op1, cp32.reg, cp32.crn, cp32.crm, cp32.op2, regs->pc);
> - gdprintk(XENLOG_ERR, "unhandled 32-bit CP10 access %#x\n",
> + gdprintk(XENLOG_ERR, "unhandled 32-bit CP10 access %#"PRIregister"\n",
> hsr.bits & HSR_CP32_REGS_MASK);
> inject_undef_exception(regs, hsr);
> return;
> diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h
> index 4e679f3273..395ce10692 100644
> --- a/xen/include/asm-arm/arm32/processor.h
> +++ b/xen/include/asm-arm/arm32/processor.h
> @@ -37,7 +37,7 @@ struct cpu_user_regs
> uint32_t pc, pc32;
> };
> uint32_t cpsr; /* Return mode */
> - uint32_t hsr; /* Exception Syndrome */
> + register_t hsr; /* Exception Syndrome */
I would rather keep this one as uint32_t to stay consistent with the
rest of the structure. If you still want to switch it, then please
switch everything in cpu_user_regs.
>
> /* Outer guest frame only from here on... */
>
> diff --git a/xen/include/asm-arm/arm64/processor.h b/xen/include/asm-arm/arm64/processor.h
> index 81dfc5e615..40f628d216 100644
> --- a/xen/include/asm-arm/arm64/processor.h
> +++ b/xen/include/asm-arm/arm64/processor.h
> @@ -64,8 +64,9 @@ struct cpu_user_regs
> /* Return address and mode */
> __DECL_REG(pc, pc32); /* ELR_EL2 */
> uint32_t cpsr; /* SPSR_EL2 */
Above you use an STP instructions that will write 2 64-bit values: one
in CPSR, the other in HSR.
So this wants to be switched to uint64_t. I guess you didn't notice any
issue because there will thankfully be an implicit padding.
> - uint32_t hsr; /* ESR_EL2 */
> + register_t hsr; /* ESR_EL2 */
All the structure is using uint64_t rather than register_t. Could we do
the same here?
> > + register_t pad1; /* Doubleword-align the user half of the frame */
May I asked, why the padding is moved here rather than kept below?
> /* Outer guest frame only from here on... */
>
> union {
> @@ -73,8 +74,6 @@ struct cpu_user_regs
> uint32_t spsr_svc; /* AArch32 */
> };
>
> - uint32_t pad1; /* Doubleword-align the user half of the frame */
> -
I think this deserves an explanation in the commit message. However, I
believe this is going to corrupt spsr_fiq because we are writing a
64-bit value to spsr_el1 (see the macro entry_guest in
arch/arm/arm64/entry.S).
So the union likely want to be 64-bit.
> /* AArch32 guests only */
> uint32_t spsr_fiq, spsr_irq, spsr_und, spsr_abt;
>
> diff --git a/xen/include/asm-arm/hsr.h b/xen/include/asm-arm/hsr.h
> index 29d4531f40..7424402c6e 100644
> --- a/xen/include/asm-arm/hsr.h
> +++ b/xen/include/asm-arm/hsr.h
> @@ -16,11 +16,14 @@ enum dabt_size {
> };
>
> union hsr {
> - uint32_t bits;
> + register_t bits;
> struct {
> unsigned long iss:25; /* Instruction Specific Syndrome */
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
Given that we don't define any useful, could we avoid the #ifdefery?
> };
>
> /* Common to all conditional exception classes (0x0N, except 0x00). */
> @@ -30,6 +33,9 @@ union hsr {
> unsigned long ccvalid:1;/* CC Valid */
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
> } cond;
>
> struct hsr_wfi_wfe {
> @@ -39,6 +45,9 @@ union hsr {
> unsigned long ccvalid:1;/* CC Valid */
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
> } wfi_wfe;
>
> /* reg, reg0, reg1 are 4 bits on AArch32, the fifth bit is sbzp. */
> @@ -53,6 +62,9 @@ union hsr {
> unsigned long ccvalid:1;/* CC Valid */
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
> } cp32; /* HSR_EC_CP15_32, CP14_32, CP10 */
>
> struct hsr_cp64 {
> @@ -66,6 +78,9 @@ union hsr {
> unsigned long ccvalid:1;/* CC Valid */
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
> } cp64; /* HSR_EC_CP15_64, HSR_EC_CP14_64 */
>
> struct hsr_cp {
> @@ -77,6 +92,9 @@ union hsr {
> unsigned long ccvalid:1;/* CC Valid */
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
> } cp; /* HSR_EC_CP */
>
> /*
> @@ -94,6 +112,9 @@ union hsr {
> unsigned long ccvalid:1;/* CC Valid */
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
> } smc32; /* HSR_EC_SMC32 */
>
> #ifdef CONFIG_ARM_64
> @@ -108,6 +129,7 @@ union hsr {
> unsigned long res0:3;
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6;
> + unsigned long _res0:32;
> } sysreg; /* HSR_EC_SYSREG */
> #endif
>
> @@ -121,6 +143,9 @@ union hsr {
> unsigned long res2:14;
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
> } iabt; /* HSR_EC_INSTR_ABORT_* */
>
> struct hsr_dabt {
> @@ -143,6 +168,9 @@ union hsr {
> unsigned long valid:1; /* Syndrome Valid */
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
> } dabt; /* HSR_EC_DATA_ABORT_* */
>
> /* Contain the common bits between DABT and IABT */
> @@ -156,6 +184,9 @@ union hsr {
> unsigned long pad3:14; /* Not common */
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
> } xabt;
>
> #ifdef CONFIG_ARM_64
> @@ -164,6 +195,7 @@ union hsr {
> unsigned long res0:9;
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> + unsigned long _res0:32;
> } brk;
> #endif
> };
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 9/9] xen/arm64: Remove READ/WRITE_SYSREG32 helper macros
2021-04-20 7:08 [PATCH 0/9] xen/arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
` (7 preceding siblings ...)
2021-04-20 7:08 ` [PATCH 8/9] arm: Change type of hsr to register_t Michal Orzel
@ 2021-04-20 7:08 ` Michal Orzel
2021-04-21 19:16 ` Julien Grall
8 siblings, 1 reply; 27+ messages in thread
From: Michal Orzel @ 2021-04-20 7:08 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk, bertrand.marquis
AArch64 system registers are 64bit whereas AArch32 ones
are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
we should get rid of helpers READ/WRITE_SYSREG32
in favour of using READ/WRITE_SYSREG.
We should also use register_t type when reading sysregs
which can correspond to uint64_t or uint32_t.
Even though many AArch64 sysregs have upper 32bit reserved
it does not mean that they can't be widen in the future.
As there are no other places in the code using READ/WRITE_SYSREG32,
remove the helper macros.
Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
xen/arch/arm/vcpreg.c | 16 ++++++++++++++++
xen/include/asm-arm/arm64/sysregs.h | 5 -----
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
index c7f516ee0a..6cb7f3108c 100644
--- a/xen/arch/arm/vcpreg.c
+++ b/xen/arch/arm/vcpreg.c
@@ -48,6 +48,7 @@
*/
/* The name is passed from the upper macro to workaround macro expansion. */
+#ifdef CONFIG_ARM_32
#define TVM_REG(sz, func, reg...) \
static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
{ \
@@ -61,6 +62,21 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
\
return true; \
}
+#else /* CONFIG_ARM_64 */
+#define TVM_REG(sz, func, reg...) \
+static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
+{ \
+ struct vcpu *v = current; \
+ bool cache_enabled = vcpu_has_cache_enabled(v); \
+ \
+ GUEST_BUG_ON(read); \
+ WRITE_SYSREG(*r, reg); \
+ \
+ p2m_toggle_cache(v, cache_enabled); \
+ \
+ return true; \
+}
+#endif
#define TVM_REG32(regname, xreg) TVM_REG(32, vreg_emulate_##regname, xreg)
#define TVM_REG64(regname, xreg) TVM_REG(64, vreg_emulate_##regname, xreg)
diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h
index 077fd95fb7..83a1157ac4 100644
--- a/xen/include/asm-arm/arm64/sysregs.h
+++ b/xen/include/asm-arm/arm64/sysregs.h
@@ -86,11 +86,6 @@
#endif
/* Access to system registers */
-
-#define READ_SYSREG32(name) ((uint32_t)READ_SYSREG64(name))
-
-#define WRITE_SYSREG32(v, name) WRITE_SYSREG64((uint64_t)v, name)
-
#define WRITE_SYSREG64(v, name) do { \
uint64_t _r = v; \
asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \
--
2.29.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 9/9] xen/arm64: Remove READ/WRITE_SYSREG32 helper macros
2021-04-20 7:08 ` [PATCH 9/9] xen/arm64: Remove READ/WRITE_SYSREG32 helper macros Michal Orzel
@ 2021-04-21 19:16 ` Julien Grall
2021-04-27 7:16 ` Michal Orzel
0 siblings, 1 reply; 27+ messages in thread
From: Julien Grall @ 2021-04-21 19:16 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis
Hi Michal,
On 20/04/2021 08:08, Michal Orzel wrote:
> AArch64 system registers are 64bit whereas AArch32 ones
> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
> we should get rid of helpers READ/WRITE_SYSREG32
> in favour of using READ/WRITE_SYSREG.
> We should also use register_t type when reading sysregs
> which can correspond to uint64_t or uint32_t.
> Even though many AArch64 sysregs have upper 32bit reserved
> it does not mean that they can't be widen in the future.
As a general comment, all your commit message contains information about
the goal (which is great). But they don't go much in details about the
actual changes. I have tried to point out where I think those details
would be helpful.
>
> As there are no other places in the code using READ/WRITE_SYSREG32,
> remove the helper macros.
>
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> ---
> xen/arch/arm/vcpreg.c | 16 ++++++++++++++++
> xen/include/asm-arm/arm64/sysregs.h | 5 -----
> 2 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
> index c7f516ee0a..6cb7f3108c 100644
> --- a/xen/arch/arm/vcpreg.c
> +++ b/xen/arch/arm/vcpreg.c
> @@ -48,6 +48,7 @@
> */
>
> /* The name is passed from the upper macro to workaround macro expansion. */
> +#ifdef CONFIG_ARM_32
> #define TVM_REG(sz, func, reg...) \
> static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
> { \
> @@ -61,6 +62,21 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
> \
> return true; \
> }
> +#else /* CONFIG_ARM_64 */
> +#define TVM_REG(sz, func, reg...) \
> +static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
> +{ \
> + struct vcpu *v = current; \
> + bool cache_enabled = vcpu_has_cache_enabled(v); \
> + \
> + GUEST_BUG_ON(read); \
> + WRITE_SYSREG(*r, reg); \
> + \
> + p2m_toggle_cache(v, cache_enabled); \
> + \
> + return true; \
> +}
> +#endif
It would be nice if this change can be explained in the commit message.
However, I think we can avoid the duplication by aliasing TVM_REG32() to
TVM_REG64() on Arm64.
>
> #define TVM_REG32(regname, xreg) TVM_REG(32, vreg_emulate_##regname, xreg)
> #define TVM_REG64(regname, xreg) TVM_REG(64, vreg_emulate_##regname, xreg)
> diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h
> index 077fd95fb7..83a1157ac4 100644
> --- a/xen/include/asm-arm/arm64/sysregs.h
> +++ b/xen/include/asm-arm/arm64/sysregs.h
> @@ -86,11 +86,6 @@
> #endif
>
> /* Access to system registers */
> -
> -#define READ_SYSREG32(name) ((uint32_t)READ_SYSREG64(name))
> -
> -#define WRITE_SYSREG32(v, name) WRITE_SYSREG64((uint64_t)v, name)
> -
> #define WRITE_SYSREG64(v, name) do { \
> uint64_t _r = v; \
> asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 9/9] xen/arm64: Remove READ/WRITE_SYSREG32 helper macros
2021-04-21 19:16 ` Julien Grall
@ 2021-04-27 7:16 ` Michal Orzel
2021-04-27 8:30 ` Julien Grall
0 siblings, 1 reply; 27+ messages in thread
From: Michal Orzel @ 2021-04-27 7:16 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis
Hi Jilien,
On 21.04.2021 21:16, Julien Grall wrote:
> Hi Michal,
>
> On 20/04/2021 08:08, Michal Orzel wrote:
>> AArch64 system registers are 64bit whereas AArch32 ones
>> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
>> we should get rid of helpers READ/WRITE_SYSREG32
>> in favour of using READ/WRITE_SYSREG.
>> We should also use register_t type when reading sysregs
>> which can correspond to uint64_t or uint32_t.
>> Even though many AArch64 sysregs have upper 32bit reserved
>> it does not mean that they can't be widen in the future.
>
> As a general comment, all your commit message contains information about the goal (which is great). But they don't go much in details about the actual changes. I have tried to point out where I think those details would be helpful.
>
>>
>> As there are no other places in the code using READ/WRITE_SYSREG32,
>> remove the helper macros.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>> ---
>> xen/arch/arm/vcpreg.c | 16 ++++++++++++++++
>> xen/include/asm-arm/arm64/sysregs.h | 5 -----
>> 2 files changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
>> index c7f516ee0a..6cb7f3108c 100644
>> --- a/xen/arch/arm/vcpreg.c
>> +++ b/xen/arch/arm/vcpreg.c
>> @@ -48,6 +48,7 @@
>> */
>> /* The name is passed from the upper macro to workaround macro expansion. */
>> +#ifdef CONFIG_ARM_32
>> #define TVM_REG(sz, func, reg...) \
>> static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
>> { \
>> @@ -61,6 +62,21 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
>> \
>> return true; \
>> }
>> +#else /* CONFIG_ARM_64 */
>> +#define TVM_REG(sz, func, reg...) \
>> +static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
>> +{ \
>> + struct vcpu *v = current; \
>> + bool cache_enabled = vcpu_has_cache_enabled(v); \
>> + \
>> + GUEST_BUG_ON(read); \
>> + WRITE_SYSREG(*r, reg); \
>> + \
>> + p2m_toggle_cache(v, cache_enabled); \
>> + \
>> + return true; \
>> +}
>> +#endif
>
> It would be nice if this change can be explained in the commit message. However, I think we can avoid the duplication by aliasing TVM_REG32() to TVM_REG64() on Arm64.
>
Unfortunately we cannot. This is the only working solution for now.
If we do the alias we will get many errors due to incompatbile pointer type in vreg_emulate_*.
Without a proper change in this functions we can't do that.
I will modify it once I start working on vreg_emulate topic but it requires more investigation.
For now I'd suggest to keep it as it is + explain the change in the commit.
I will push v2 soon.
>> #define TVM_REG32(regname, xreg) TVM_REG(32, vreg_emulate_##regname, xreg)
>> #define TVM_REG64(regname, xreg) TVM_REG(64, vreg_emulate_##regname, xreg)
>> diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h
>> index 077fd95fb7..83a1157ac4 100644
>> --- a/xen/include/asm-arm/arm64/sysregs.h
>> +++ b/xen/include/asm-arm/arm64/sysregs.h
>> @@ -86,11 +86,6 @@
>> #endif
>> /* Access to system registers */
>> -
>> -#define READ_SYSREG32(name) ((uint32_t)READ_SYSREG64(name))
>> -
>> -#define WRITE_SYSREG32(v, name) WRITE_SYSREG64((uint64_t)v, name)
>> -
>> #define WRITE_SYSREG64(v, name) do { \
>> uint64_t _r = v; \
>> asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \
>>
>
> Cheers,
>
Cheers,
Michal
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 9/9] xen/arm64: Remove READ/WRITE_SYSREG32 helper macros
2021-04-27 7:16 ` Michal Orzel
@ 2021-04-27 8:30 ` Julien Grall
0 siblings, 0 replies; 27+ messages in thread
From: Julien Grall @ 2021-04-27 8:30 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis
On 27/04/2021 08:16, Michal Orzel wrote:
> Hi Jilien,
Hi,
> On 21.04.2021 21:16, Julien Grall wrote:
>> Hi Michal,
>>
>> On 20/04/2021 08:08, Michal Orzel wrote:
>>> AArch64 system registers are 64bit whereas AArch32 ones
>>> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
>>> we should get rid of helpers READ/WRITE_SYSREG32
>>> in favour of using READ/WRITE_SYSREG.
>>> We should also use register_t type when reading sysregs
>>> which can correspond to uint64_t or uint32_t.
>>> Even though many AArch64 sysregs have upper 32bit reserved
>>> it does not mean that they can't be widen in the future.
>>
>> As a general comment, all your commit message contains information about the goal (which is great). But they don't go much in details about the actual changes. I have tried to point out where I think those details would be helpful.
>>
>>>
>>> As there are no other places in the code using READ/WRITE_SYSREG32,
>>> remove the helper macros.
>>>
>>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>>> ---
>>> xen/arch/arm/vcpreg.c | 16 ++++++++++++++++
>>> xen/include/asm-arm/arm64/sysregs.h | 5 -----
>>> 2 files changed, 16 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
>>> index c7f516ee0a..6cb7f3108c 100644
>>> --- a/xen/arch/arm/vcpreg.c
>>> +++ b/xen/arch/arm/vcpreg.c
>>> @@ -48,6 +48,7 @@
>>> */
>>> /* The name is passed from the upper macro to workaround macro expansion. */
>>> +#ifdef CONFIG_ARM_32
>>> #define TVM_REG(sz, func, reg...) \
>>> static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
>>> { \
>>> @@ -61,6 +62,21 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
>>> \
>>> return true; \
>>> }
>>> +#else /* CONFIG_ARM_64 */
>>> +#define TVM_REG(sz, func, reg...) \
>>> +static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
>>> +{ \
>>> + struct vcpu *v = current; \
>>> + bool cache_enabled = vcpu_has_cache_enabled(v); \
>>> + \
>>> + GUEST_BUG_ON(read); \
>>> + WRITE_SYSREG(*r, reg); \
>>> + \
>>> + p2m_toggle_cache(v, cache_enabled); \
>>> + \
>>> + return true; \
>>> +}
>>> +#endif
>>
>> It would be nice if this change can be explained in the commit message. However, I think we can avoid the duplication by aliasing TVM_REG32() to TVM_REG64() on Arm64.
>>
> Unfortunately we cannot. This is the only working solution for now.
> If we do the alias we will get many errors due to incompatbile pointer type in vreg_emulate_*.
> Without a proper change in this functions we can't do that.
Right, so the prototype needs to stay the same. How about provide a
wrapper WRITE_SYSREG_SZ(sz, val, sysreg) internal to vcpreg.c?
On 32-bit it would expand to WRITE_SYSREG##sz(val, sysreg) and on
64-bit, it would expand to WRITE_SYSREG().
> I will modify it once I start working on vreg_emulate topic but it requires more investigation.
While it would be great to get rid of {READ, WRITE}_SYSREG32, I don't
want to duplicate the code (even temporarily) just for the sake for
removing the helpers.
If the new proposal I suggested above doesn't work, then I would
strongly prefer to keep the existing code until vreg_emulate_* is reworked.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread