[Qemu-devel] [PATCH] ARM: ACPI: Fix MPIDR value in ACPI table

[Qemu-devel] [PATCH] ARM: ACPI: Fix MPIDR value in ACPI table

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Use mp_affinity of ARMCPU as the CPU MPIDR instead of the CPU index.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
This patch is based on below patch.
http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg06919.html

 hw/arm/virt-acpi-build.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 29a1980..1c621cb 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -451,13 +451,15 @@ build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
     for (i = 0; i < guest_info->smp_cpus; i++) {
         AcpiMadtGenericInterrupt *gicc = acpi_data_push(table_data,
                                                      sizeof *gicc);
+        ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
+
         gicc->type = ACPI_APIC_GENERIC_INTERRUPT;
         gicc->length = sizeof(*gicc);
         if (guest_info->gic_version == 2) {
             gicc->base_address = memmap[VIRT_GIC_CPU].base;
         }
         gicc->cpu_interface_number = i;
-        gicc->arm_mpidr = i;
+        gicc->arm_mpidr = armcpu->mp_affinity;
         gicc->uid = i;
         if (test_bit(i, cpuinfo->found_cpus)) {
             gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
-- 
2.0.4

Re: [Qemu-devel] [Qemu-arm] [PATCH] ARM: ACPI: Fix MPIDR value in ACPI table

[Peter Crosthwaite]

On Sat, Oct 31, 2015 at 2:50 AM, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> Use mp_affinity of ARMCPU as the CPU MPIDR instead of the CPU index.
>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> This patch is based on below patch.
> http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg06919.html
>
>  hw/arm/virt-acpi-build.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 29a1980..1c621cb 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -451,13 +451,15 @@ build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
>      for (i = 0; i < guest_info->smp_cpus; i++) {
>          AcpiMadtGenericInterrupt *gicc = acpi_data_push(table_data,
>                                                       sizeof *gicc);
> +        ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
> +
>          gicc->type = ACPI_APIC_GENERIC_INTERRUPT;
>          gicc->length = sizeof(*gicc);
>          if (guest_info->gic_version == 2) {
>              gicc->base_address = memmap[VIRT_GIC_CPU].base;
>          }
>          gicc->cpu_interface_number = i;
> -        gicc->arm_mpidr = i;
> +        gicc->arm_mpidr = armcpu->mp_affinity;

As a general rule, hw/ should not be reaching into the CPU state
struct like this. What is the real HW equivalent of this query?

Regards,
Peter

>          gicc->uid = i;
>          if (test_bit(i, cpuinfo->found_cpus)) {
>              gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
> --
> 2.0.4
>
>
>

Re: [Qemu-devel] [Qemu-arm] [PATCH] ARM: ACPI: Fix MPIDR value in ACPI table

[Peter Maydell]

On 31 October 2015 at 18:53, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> On Sat, Oct 31, 2015 at 2:50 AM, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> Use mp_affinity of ARMCPU as the CPU MPIDR instead of the CPU index.
>>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>> This patch is based on below patch.
>> http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg06919.html
>>
>>  hw/arm/virt-acpi-build.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 29a1980..1c621cb 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -451,13 +451,15 @@ build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
>>      for (i = 0; i < guest_info->smp_cpus; i++) {
>>          AcpiMadtGenericInterrupt *gicc = acpi_data_push(table_data,
>>                                                       sizeof *gicc);
>> +        ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
>> +
>>          gicc->type = ACPI_APIC_GENERIC_INTERRUPT;
>>          gicc->length = sizeof(*gicc);
>>          if (guest_info->gic_version == 2) {
>>              gicc->base_address = memmap[VIRT_GIC_CPU].base;
>>          }
>>          gicc->cpu_interface_number = i;
>> -        gicc->arm_mpidr = i;
>> +        gicc->arm_mpidr = armcpu->mp_affinity;
>
> As a general rule, hw/ should not be reaching into the CPU state
> struct like this. What is the real HW equivalent of this query?

This is just doing the same thing the hw/arm/virt.c code does
to populate the dt... (In real firmware it would presumably
either (a) have a fixed ACPI table or (b) maybe read the MPIDR,
but neither of those really fits here I think.)

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH] ARM: ACPI: Fix MPIDR value in ACPI table

[Peter Crosthwaite]

On Sat, Oct 31, 2015 at 5:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 31 October 2015 at 18:53, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
>> On Sat, Oct 31, 2015 at 2:50 AM, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
>>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>>
>>> Use mp_affinity of ARMCPU as the CPU MPIDR instead of the CPU index.
>>>
>>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>> ---
>>> This patch is based on below patch.
>>> http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg06919.html
>>>
>>>  hw/arm/virt-acpi-build.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index 29a1980..1c621cb 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -451,13 +451,15 @@ build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
>>>      for (i = 0; i < guest_info->smp_cpus; i++) {
>>>          AcpiMadtGenericInterrupt *gicc = acpi_data_push(table_data,
>>>                                                       sizeof *gicc);
>>> +        ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
>>> +
>>>          gicc->type = ACPI_APIC_GENERIC_INTERRUPT;
>>>          gicc->length = sizeof(*gicc);
>>>          if (guest_info->gic_version == 2) {
>>>              gicc->base_address = memmap[VIRT_GIC_CPU].base;
>>>          }
>>>          gicc->cpu_interface_number = i;
>>> -        gicc->arm_mpidr = i;
>>> +        gicc->arm_mpidr = armcpu->mp_affinity;
>>
>> As a general rule, hw/ should not be reaching into the CPU state
>> struct like this. What is the real HW equivalent of this query?
>
> This is just doing the same thing the hw/arm/virt.c code does
> to populate the dt... (In real firmware it would presumably
> either (a) have a fixed ACPI table or (b) maybe read the MPIDR,
> but neither of those really fits here I think.)

So, I think this is just another case of the MPIDR information flow
going the wrong way. It should go from board to all of CPU, DT and now
this. I guess we can just fix this incrementally when we fix the
implicit setting of MPIDR in mach-virt.

Regards,
Peter

>
> thanks
> -- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH] ARM: ACPI: Fix MPIDR value in ACPI table

[Peter Maydell]

On 3 November 2015 at 04:33, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> So, I think this is just another case of the MPIDR information flow
> going the wrong way. It should go from board to all of CPU, DT and now
> this. I guess we can just fix this incrementally when we fix the
> implicit setting of MPIDR in mach-virt.

The difficulty with that is that to support KVM we need
to let KVM (ie the CPU object) override the board's ideas
about mpidr, because the kernel doesn't yet support letting
the board model inform it about what mpidr values to use.
So we probably need to have 'board model sets cpu property,
everything else reads cpu property which might or might not
be what the board hoped for'.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] ARM: ACPI: Fix MPIDR value in ACPI table

[Peter Maydell]

On 31 October 2015 at 09:50, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> Use mp_affinity of ARMCPU as the CPU MPIDR instead of the CPU index.
>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> This patch is based on below patch.
> http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg06919.html
>



Applied to target-arm.next, thanks.

-- PMM