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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

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 ?

--- 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;
+
         /* 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 :-

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,