Re: [PATCH 3/9] arm/gic: Get rid of READ/WRITE_SYSREG32
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 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 & 0xff); Sounds good, so long we the value is not hardoced but provided through a define :). Cheers, -- Julien Grall
Re: [PATCH 3/9] arm/gic: Get rid of READ/WRITE_SYSREG32
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 >> --- >> 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(>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) >>
Re: [PATCH 3/9] arm/gic: 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 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 --- 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(>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,
[PATCH 3/9] arm/gic: 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 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 --- 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(>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(>arch.gic); gicv3_restore_lrs(v);