All of lore.kernel.org
 help / color / mirror / Atom feed
From: Michal Orzel <michal.orzel@arm.com>
To: Julien Grall <julien@xen.org>, xen-devel@lists.xenproject.org
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	bertrand.marquis@arm.com
Subject: Re: [PATCH 3/9] arm/gic: Get rid of READ/WRITE_SYSREG32
Date: Wed, 21 Apr 2021 09:48:53 +0200	[thread overview]
Message-ID: <fd5924c5-685d-a9fc-1116-c1b8909c9aea@arm.com> (raw)
In-Reply-To: <1a087bed-94e5-bada-76c4-92e0c429cce6@xen.org>

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


  reply	other threads:[~2021-04-21  7:49 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 13:04   ` Julien Grall
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
2021-04-21 10:20       ` Julien Grall
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 [this message]
2021-04-21 10:21       ` Julien Grall
2021-04-20  7:08 ` [PATCH 4/9] arm/p2m: " Michal Orzel
2021-04-20 13:31   ` Julien Grall
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
2021-04-22 13:40       ` Julien Grall
2021-04-20  7:08 ` [PATCH 6/9] arm/page: " Michal Orzel
2021-04-21 15:11   ` Julien Grall
2021-04-20  7:08 ` [PATCH 7/9] arm/time,vtimer: " 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-21 19:02   ` Julien Grall
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
2021-04-27  8:30       ` Julien Grall

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fd5924c5-685d-a9fc-1116-c1b8909c9aea@arm.com \
    --to=michal.orzel@arm.com \
    --cc=Volodymyr_Babchuk@epam.com \
    --cc=bertrand.marquis@arm.com \
    --cc=julien@xen.org \
    --cc=sstabellini@kernel.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.