Re: [RFC PATCH v1 04/12] Arm: GICv3: Emulate GICR_TYPER on AArch32

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxx>
> > ---
> >   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,

--
Julien Grall