Re: [PATCH 2/9] arm/domain: Get rid of READ/WRITE_SYSREG32
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
---
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
Re: [PATCH 2/9] arm/domain: Get rid of READ/WRITE_SYSREG32
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
>> ---
>> 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
Re: [PATCH 2/9] arm/domain: Get rid of READ/WRITE_SYSREG32
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
---
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
[PATCH 2/9] arm/domain: Get rid of READ/WRITE_SYSREG32
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
---
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
