| Message ID | 20221021153128.44226-5-ayankuma@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Arm: Enable GICv3 for AArch32 | expand |
On 10/21/22 18:31, Ayan Kumar Halder wrote: Hi Ayan > Refer Arm IHI 0069H ID020922, > The upper 32 bits of GICR_TYPER represent the affinity > whereas the lower 32 bits represent the other bits (eg processor > number, etc). > MPIDR_AFFINITY_LEVEL() returns a 32 bit number on aarch32. Thus, this > is appended to return GICR_TYPER register. > > Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> > --- > xen/arch/arm/vgic-v3.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index c31140eb20..d86b41a39f 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -190,14 +190,18 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, > > case VREG64(GICR_TYPER): > { > - uint64_t typer, aff; > + uint64_t typer; > + uint32_t aff; > > if ( !vgic_reg64_check_access(dabt) ) goto bad_width; > - aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 56 | > - MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 | > - MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 | > - MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32); > + aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 24 | > + MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 16 | > + MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 8 | > + MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0)); > typer = aff; > + > + typer = typer << 32; > + > /* We use the VCPU ID as the redistributor ID in bits[23:8] */ > typer |= v->vcpu_id << GICR_TYPER_PROC_NUM_SHIFT; > I don't see an issue I just want to propose alternatives that I think would reduce the changes, hopefully without breaking it. So, other ways would be either to assign v->arch.vmpidr to a new variable uint64_t vmpidr and operate on this (without changing the shifts), or to leave the type of aff uint64_t, adjust the shifts and do typer = aff << 32.
Hi, On 21/10/2022 23:07, Xenia Ragiadakou wrote: > On 10/21/22 18:31, Ayan Kumar Halder wrote: > Hi Ayan > >> Refer Arm IHI 0069H ID020922, >> The upper 32 bits of GICR_TYPER represent the affinity >> whereas the lower 32 bits represent the other bits (eg processor >> number, etc). >> MPIDR_AFFINITY_LEVEL() returns a 32 bit number on aarch32. Thus, this >> is appended to return GICR_TYPER register. The last sentence doesn't seem to match your modification below. >> >> Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> >> --- >> xen/arch/arm/vgic-v3.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c >> index c31140eb20..d86b41a39f 100644 >> --- a/xen/arch/arm/vgic-v3.c >> +++ b/xen/arch/arm/vgic-v3.c >> @@ -190,14 +190,18 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct >> vcpu *v, mmio_info_t *info, >> case VREG64(GICR_TYPER): >> { >> - uint64_t typer, aff; >> + uint64_t typer; >> + uint32_t aff; >> if ( !vgic_reg64_check_access(dabt) ) goto bad_width; >> - aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 56 | >> - MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 | >> - MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 | >> - MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32); >> + aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 24 | >> + MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 16 | >> + MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 8 | >> + MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0)); >> typer = aff; >> + >> + typer = typer << 32; I find the "typer = aff; typer = typer << 32" quite confusing to read. In fact, my first instinct would be to combine the two but this would do the wrong thing. So I would prefer if we use a different approach (see below). >> + >> /* We use the VCPU ID as the redistributor ID in bits[23:8] */ >> typer |= v->vcpu_id << GICR_TYPER_PROC_NUM_SHIFT; > > I don't see an issue I just want to propose alternatives that I think > would reduce the changes, hopefully without breaking it. > So, other ways would be either to assign v->arch.vmpidr to a new > variable uint64_t vmpidr and operate on this (without changing the > shifts), or to leave the type of aff uint64_t, adjust the shifts and do > typer = aff << 32. How about making MPIDR_AFFINITY_LEVEL returning a 64-bit value? The other option would be to use what Xenia's last option. I.e: " to level the type of aff uint64_t, adjust the shifts and do typer = aff << 32". This would need a suitable comment though explain why the shift can't be fold. Cheers,
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index c31140eb20..d86b41a39f 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -190,14 +190,18 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, case VREG64(GICR_TYPER): { - uint64_t typer, aff; + uint64_t typer; + uint32_t aff; if ( !vgic_reg64_check_access(dabt) ) goto bad_width; - aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 56 | - MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 | - MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 | - MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32); + aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 24 | + MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 16 | + MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 8 | + MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0)); typer = aff; + + typer = typer << 32; + /* We use the VCPU ID as the redistributor ID in bits[23:8] */ typer |= v->vcpu_id << GICR_TYPER_PROC_NUM_SHIFT;
Refer Arm IHI 0069H ID020922, The upper 32 bits of GICR_TYPER represent the affinity whereas the lower 32 bits represent the other bits (eg processor number, etc). MPIDR_AFFINITY_LEVEL() returns a 32 bit number on aarch32. Thus, this is appended to return GICR_TYPER register. Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> --- xen/arch/arm/vgic-v3.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)