Re: [XEN v3 04/12] xen/Arm: vGICv3: Adapt emulation of GICR_TYPER for AArch32

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 22/11/2022 20:37, Julien Grall wrote:
> Hi Ayan,
Hi Julien,

I need a clarification.

> On 11/11/2022 14:17, Ayan Kumar Halder wrote:
> > Refer ARM DDI 0487I.a ID081822, G8-9650, G8.2.113
> > Aff3 does not exist on AArch32.
> > Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106
> > Aff3 does not exist on Armv7 (ie arm32).
> >
> > Thus, access to aff3 have been contained within "#ifdef CONFIG_ARM_64".
> > Also, v->arch.vmpidr is a 32 bit register on AArch32. So, we have 
> > copied it to
> > 'uint64_t vmpidr' to perform the shifts.
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > ---
> >
> > Changes from :-
> > v1 - Assigned v->arch.vmpidr to "uint64_t vmpdir". Then, we can use
> > MPIDR_AFFINITY_LEVEL macros to extract the affinity value.
> >
> > v2 - 1. "MPIDR_AFFINITY_LEVEL(vmpidr, 3)" is contained within
> > "#ifdef CONFIG_ARM_64".
> > 2. Updated commit message.
> >
> >   xen/arch/arm/vgic-v3.c | 12 ++++++++----
> >   1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> > index 3f4509dcd3..a7a935ff57 100644
> > --- a/xen/arch/arm/vgic-v3.c
> > +++ b/xen/arch/arm/vgic-v3.c
> > @@ -191,12 +191,16 @@ 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 vmpidr = v->arch.vmpidr;
> The type-widening here deserve an in-code comment. Otherwise, it would 
> be easier for someone to decide to open-code v->arch.vmpidr again.
Does this comment look fine ?

        /*
         * This is to enable shifts greater than 32 bits which would have
         * otherwise caused overflow (as v->arch.vmpidr is 32 bit on 
AArch32).
         */
        uint64_t vmpidr = v->arch.vmpidr;

- Ayan

> >             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 = (
> > +#ifdef CONFIG_ARM_64
> > +               MPIDR_AFFINITY_LEVEL(vmpidr, 3) << 56 |
> > +#endif
> > +               MPIDR_AFFINITY_LEVEL(vmpidr, 2) << 48 |
> > +               MPIDR_AFFINITY_LEVEL(vmpidr, 1) << 40 |
> > +               MPIDR_AFFINITY_LEVEL(vmpidr, 0) << 32);
> >           typer = aff;
> >           /* We use the VCPU ID as the redistributor ID in bits[23:8] */
> >           typer |= v->vcpu_id << GICR_TYPER_PROC_NUM_SHIFT;
> Cheers,