[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 05/10] arm/gic: Get rid of READ/WRITE_SYSREG32
Hi Julien, On 27.04.2021 12:02, Julien Grall wrote: > > > On 27/04/2021 10:35, Michal Orzel wrote: >> AArch64 registers are 64bit whereas AArch32 registers >> 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 registers 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: >> -vmcr >> -sre_el1 >> -apr0 >> -apr1 >> >> Add new macro GICC_IAR_INTID_MASK containing the mask >> for INTID field of ICC_IAR0/1_EL1 register. >> >> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx> >> --- >> Changes since v1: >> -Remove hcr member of gic_v3 in a seperate patch >> -Add macro GICC_IAR_INTID_MASK > > This change needs to be explained in the commit message. Saying something > like: > > "Only the first 23-bits of IAR contains the interrupt number. The rest are > RES0. Therefore, take the opportunity to mask the bits [23:31] as they should > be used for an IRQ number (we don't know how the top bits will be used). > " > Ok, will change in v3. >> -Remove explicit cast in favor of implicit cast >> --- >> xen/arch/arm/gic-v3-lpi.c | 2 +- >> xen/arch/arm/gic-v3.c | 98 ++++++++++++++++--------------- >> xen/include/asm-arm/gic.h | 6 +- >> xen/include/asm-arm/gic_v3_defs.h | 2 + >> 4 files changed, 58 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..b86f040589 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; >> + /* IRQs are encoded using 23bit. */ >> + return (irq & GICC_IAR_INTID_MASK); >> } >> /* >> @@ -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,44 @@ 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 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 5069ab4aac..c7f0c343d1 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 vmcr, sre_el1; >> - uint32_t apr0[4]; >> - uint32_t apr1[4]; >> + register_t vmcr, sre_el1; >> + register_t apr0[4]; >> + register_t apr1[4]; >> uint64_t lr[16]; >> }; >> #endif >> diff --git a/xen/include/asm-arm/gic_v3_defs.h >> b/xen/include/asm-arm/gic_v3_defs.h >> index 5a578e7c11..34ed5f857d 100644 >> --- a/xen/include/asm-arm/gic_v3_defs.h >> +++ b/xen/include/asm-arm/gic_v3_defs.h >> @@ -45,6 +45,8 @@ >> #define GICC_SRE_EL2_DIB (1UL << 2) >> #define GICC_SRE_EL2_ENEL1 (1UL << 3) >> +#define GICC_IAR_INTID_MASK (0xFFFFFF) >> + >> /* Additional bits in GICD_TYPER defined by GICv3 */ >> #define GICD_TYPE_ID_BITS_SHIFT 19 >> #define GICD_TYPE_ID_BITS(r) ((((r) >> GICD_TYPE_ID_BITS_SHIFT) & 0x1f) >> + 1) >> >
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |