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

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

On 07/11/2022 14:00, Ayan Kumar Halder wrote:
> On 07/11/2022 11:54, Julien Grall wrote:
> > Hi Ayan,
> Hi Julien,
>
> I need one clarification.
>
> > On 07/11/2022 11:33, Ayan Kumar Halder wrote:
> > > On 06/11/2022 18:04, Julien Grall wrote:
> > > > Hi Ayan,
> > > Hi Julien,
> > >
> > > I need a clarification.
> > >
> > > > In the title you are using AArch32 but below you are using...
> > > >
> > > > On 31/10/2022 15:13, Ayan Kumar Halder wrote:
> > > > > v->arch.vmpidr is assigned to uint64_t variable. This is to enable 
> > > > > left shifts
> > > > > for Aarch32 so that one can extract affinity bits.
> > > > ... Aarch32. The naming also seem to be inconsistent across your 
> > > > series. AFAIU, it should be AArch32. So please look at all your 
> > > > commits and make sure you use the same everywhere.
> > > Ack
> > > > > This is then assigned to 'typer' so that the affinity bits form the 
> > > > > upper 32 bits.
> > > > > 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).
> > > > >
> > > > > Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxx>
> > > > > ---
> > > > >
> > > > > Changes from :-
> > > > > 1. v1 - Assigned v->arch.vmpidr to "uint64_t vmpdir". Then, we can use
> > > > > MPIDR_AFFINITY_LEVEL macros to extract the affinity value.
> > > > >
> > > > >   xen/arch/arm/vgic-v3.c | 10 ++++++----
> > > > >   1 file changed, 6 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> > > > > index 3f4509dcd3..e5e6f2c573 100644
> > > > > --- a/xen/arch/arm/vgic-v3.c
> > > > > +++ b/xen/arch/arm/vgic-v3.c
> > > > > @@ -191,13 +191,15 @@ 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;
> > > > >             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(vmpidr, 3) << 56 |
> > > > Shouldn't we #ifdef this level for 32-bit? Or maybe check if the 
> > > > domain is 64-bit so we are using consistently regardless of the 
> > > > hypervisor bitness.
> > > We have typecasted "v->arch.vmpidr" (which is 32bit for AArch32 and 
> > > 64bit for AArch64)  to vmpidr (uint64_t).
> > > So, we don't need to have any #ifdef for AArch32 or AArch64.
> > This is not related to the typecast. This is more that fact that 
> > affinity level 3 doesn't exist for 32-bit guest. For instance vpsci.c 
> > will protect level 3 with an #ifdef.
> Just to make sure, I understand you. You are suggesting this ?
Yes with...

> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -191,13 +191,15 @@ 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;
>
>           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;
> +
... this spurious change dropped.

>           /* We use the VCPU ID as the redistributor ID in bits[23:8] */
>           typer |= v->vcpu_id << GICR_TYPER_PROC_NUM_SHIFT;
>
> If so, then we can drop the patch "[XEN v2 02/12] xen/Arm: GICv3: Move 
> the macros to compute the affnity level to arm64/arm32"
> Also, we should do the following change :-
Yes but in a separate patch (we should keep vGIC and GIC changes separate).

> ayankuma@xcbayankuma41x:/scratch/ayankuma/r52_xen/xen-pristine$ git diff 
> xen/arch/arm/gic-v3.c
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index d8ce0f46c6..e7d5338152 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -527,7 +527,10 @@ static void gicv3_set_pending_state(struct irq_desc 
> *irqd, bool pending)
>   static inline uint64_t gicv3_mpidr_to_affinity(int cpu)
>   {
>        uint64_t mpidr = cpu_logical_map(cpu);
> -     return (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 32 |
> +     return (
> +#ifdef CONFIG_ARM_64
> +             MPIDR_AFFINITY_LEVEL(mpidr, 3) << 32 |
> +#endif
>                MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
>                MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8  |
>                MPIDR_AFFINITY_LEVEL(mpidr, 0));
> @@ -720,7 +723,10 @@ static int __init gicv3_populate_rdist(void)
>        * Convert affinity to a 32bit value that can be matched to 
> GICR_TYPER
>        * bits [63:32]
>        */
> -    aff = (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
> +    aff = (
> +#ifdef CONFIG_ARM_64
> +           MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
> +#endif
>              MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
>              MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
>              MPIDR_AFFINITY_LEVEL(mpidr, 0));
> @@ -972,7 +978,10 @@ static void gicv3_send_sgi_list(enum gic_sgi sgi, 
> const cpumask_t *cpumask)
>            * Prepare affinity path of the cluster for which SGI is 
> generated
>            * along with SGI number
>            */
> -        val = (MPIDR_AFFINITY_LEVEL(cluster_id, 3) << 48  |
> +        val = (
> +#ifdef CONFIG_ARM_64
> +               MPIDR_AFFINITY_LEVEL(cluster_id, 3) << 48  |
> +#endif
>                  MPIDR_AFFINITY_LEVEL(cluster_id, 2) << 32  |
>                  sgi << 24                                  |
>                  MPIDR_AFFINITY_LEVEL(cluster_id, 1) << 16  |
>
> - Ayan
>
> > Cheers,
--
Julien Grall