Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Shlomo Pongratz]

Hi,

See below.

> -----Original Message-----
> From: Pavel Fedin [mailto:address@hidden
> Sent: Friday, 29 May, 2015 9:45 AM
> To: Shlomo Pongratz; address@hidden
> Cc: address@hidden; 'Ashok Kumar'
> Subject: RE: [PATCH] Use Aff1 with mpidr
> 
>  Hi!
> 
> > I see that you added mpidr to ARMCpu and initialized it in virt.c then
> > you use it in
> mpidr_read.
> > Thus you must fix all other virtual machines in hw/arm not just virt.c
> > as it is not
> initialized for
> > them.
> 
>  The only change in virt.c is:
> --- cut ---
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c index a7f9a10..a1186c5 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -317,7 +317,11 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo
> *vbi)
>                                          "enable-method", "psci");
>          }
> 
> -        qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg", cpu);
> +        /*
> +         * If cpus node's #address-cells property is set to 1
> +         * The reg cell bits [23:0] must be set to bits [23:0] of MPIDR_EL1.
> +         */
> +        qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg",
> + armcpu->mpidr);
>          g_free(nodename);
>      }
>  }
> --- cut ---
>  So that it takes MPIDR instead of just CPU index. Theoretically - yes, may be
> other machines should also be changed, but:
> 1. They are 32-bit, so MPIDR == index for them, because there are no more
> than 8 CPUs.
> 2. Those machines AFAIK do not compose device tree by themselves. They
> use pre-made ones instead, coming for example with kernel.
>  Only virt currently can be a 64-bit machine and cares about more than 8
> CPUs.
>  As to MPIDR initialization, it is done in arm_cpu_initfn(), therefore all ARM
> CPUs get this automatically. There's no need to modify code for every
> machine.

I think we should take Igor's comment into account. The CPUS_PER_CLUSTER should 
not be a constant, and maybe should be initialized in arm_cpu_initfn and 
aarch64_{a53|a57}_initfn, as psci need to know it.

>  I would kindly ask you to use my patch in your next series, or base
> something on it, if you dislike the implementation, but it's crucial for KVM
> that MPIDR values can be obtained from the host. Your original
> implementation cannot do this by design, this is why i made my own solution.
> See kvm_arch_init_vcpu() in my patch - without this CPUs beyond #7 will not
> power up in KVM.

I understand that you suggest that I'll not use my 1 & 4 patches. I have no 
problem with that, but how do I commit? Should I write that the patch series is 
pending until your patch series is integrated?

>  And when are you planning to post v3? I am waiting for it to be integrated, 
> in
> order to add KVM vGICv3. Otherwise i'm afraid there are too many collisions.

I intend to release the V3 version during this week.
I have some issues with the new groups code that was introduce to GICv2 but 
they are minor.

> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
>