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
 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 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.
 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.

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

Hi,

See below.

> -----Original Message-----
> From: Pavel Fedin [mailto:p.fedin@samsung.com]
> Sent: Friday, 29 May, 2015 9:45 AM
> To: Shlomo Pongratz; qemu-devel@nongnu.org
> Cc: peter.maydell@linaro.org; '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
>

Hi!

> 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.

 Yes, of course. Just replace a #define with something like:

 int cpus_per_cluster = 8;

 and you're done with this. This provides good backwards compatibility as well as
possibility to change this on a per-machine basis.

> 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?

 Just make it a part of your V3. I'm perfectly OK with that. Looks like maintainers don't
want to integrate it alone.

> I intend to release the V3 version during this week.

 Good.

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

Hi again!

> I intend to release the V3 version during this week.

 One more note. I remember that i suggested to merge ITS_CONTROL and ITS_TRANSLATION into
one region in virt memory map. I was wrong, don't do this and leave them as they are.
There is in-kernel ITS implementation suggested on ARM-Linux mailing list, and qemu
support will rely on this (it will have to pass CONTROL region to KVM and intercept
TRANSLATION region).

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

Hi

> -----Original Message-----
> From: Pavel Fedin [mailto:p.fedin@samsung.com]
> Sent: Monday, 01 June, 2015 12:04 PM
> To: Shlomo Pongratz; qemu-devel@nongnu.org
> Cc: peter.maydell@linaro.org; 'Ashok Kumar'
> Subject: RE: [Qemu-devel] [PATCH] Use Aff1 with mpidr
> 
>  Hi again!
> 
> > I intend to release the V3 version during this week.
> 
>  One more note. I remember that i suggested to merge ITS_CONTROL and
> ITS_TRANSLATION into one region in virt memory map. I was wrong, don't do
> this and leave them as they are.
> There is in-kernel ITS implementation suggested on ARM-Linux mailing list,
> and qemu support will rely on this (it will have to pass CONTROL region to
> KVM and intercept TRANSLATION region).
> 

Hi Pavel,

You mean that I put your patch https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg04495.html
The first patch of my V3 patch series, with the sole exception of fixing the ITS control & ITS translation.

Best regards,

S.P.


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

Hi!

> You mean that I put your patch
https://lists.gnu.org/archive/html/qemu-devel/201505/msg04495.html

 Yes. You can of course further modify it, like changing #define to a variable which can
be set from outside.

> The first patch of my V3 patch series, with the sole exception of fixing the ITS control
& ITS
> translation.

 ITS memory regions is another thing, it is in virt machine definition.

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