Re: [Qemu-devel] [PULL 00/22] QOM CPUState patch queue 2015-07-06

[Peter Crosthwaite]

On Mon, Jul 6, 2015 at 7:25 PM, Peter Crosthwaite
<address@hidden> wrote:
> On Mon, Jul 6, 2015 at 5:24 PM, Andreas Färber <address@hidden> wrote:
>> Am 07.07.2015 um 01:13 schrieb Andreas Färber:
>>> Hello Peter,
>>>
>>> This is my QOM CPU patch queue. Please pull.
>>>
>>> Note: For time reasons I did not give this queue as much testing as usual,
>>> in particular BSD and non-x86 KVM hosts were not covered.
>>>
>>> Regards,
>>> Andreas
>>>
>>> Cc: Peter Maydell <address@hidden>
>>>
>>> Cc: Eduardo Habkost <address@hidden>
>>> Cc: Peter Crosthwaite <address@hidden>
>>>
>>> The following changes since commit 7edd8e4660beb301d527257f8e04ebec0f841cb0:
>>>
>>>   Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into 
>>> staging (2015-07-06 14:03:44 +0100)
>>>
>>> are available in the git repository at:
>>>
>>>   git://github.com/afaerber/qemu-cpu.git tags/qom-cpu-for-peter
>>>
>>> for you to fetch changes up to 116382f1504d655a1afdf3eac18d276a200428b7:
>>>
>>>   disas: cris: QOMify target specific disas setup (2015-07-06 22:36:17 
>>> +0200)
>>>
>>> ----------------------------------------------------------------
>>> QOM CPUState and X86CPU
>>>
>>> * Further QOM'ification of CPU initialization
>>> * Propagation of CPUState arguments and elimination of ENV_GET_CPU() usage
>>> * cpu_set_pc() abstraction
>>> * CPUClass::disas_set_info() hook
>>>
>>> ----------------------------------------------------------------
>> [...]
>>
>> Self-nack, hurry is never good:
>>
>>   /aarch64/qom/xlnx-ep108:
>> qemu-system-aarch64: Trying to use more CPUs than allowed max of 1
>> Broken pipe
>> FAIL
>>
>> Peter C., any ideas why this is regressing?
>>
>
> This:
>
> +    if (cpu >= max_cpus) {
> +        error_setg(errp, "Trying to use more CPUs than allowed max of %d\n",
> +                    max_cpus);
> +        return -1;
>
> xlnx-ep108 doesn't care about the -smp argument, it creates all 6 CPUs
> regardless of -smp. This is because the number of CPUs is not flexible
> in reality. It is also a heterogeneous arch (with R5s abd A53s) so
> trying to limit the grand total of CPUs is ambiguous (do you remove
> a53s or r5s for -smp < 6?).
>
> Can this check be dropped or is this a bug in xlnx where we should
> overcome by just forcing smp_cpus = 6 at machine level?
>
> Regards,
> Peter
>

This is the fix (patch 5):

@@ -531,11 +531,11 @@ static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);

 static int cpu_get_free_index(Error **errp)
 {
-    int cpu = find_first_zero_bit(cpu_index_map, max_cpus);
+    int cpu = find_first_zero_bit(cpu_index_map, MAX_CPUMASK_BITS);

-    if (cpu >= max_cpus) {
-        error_setg(errp, "Trying to use more CPUs than allowed max of %d",
-                   max_cpus);
+    if (cpu >= MAX_CPUMASK_BITS) {
+        error_setg(errp, "Trying to use more CPUs than max of %d",
+                   MAX_CPUMASK_BITS);
         return -1;
     }

My thinking is, that the existing linear allocator for cpu-indicies
does not have max_cpus based maximum index enforcement and adding it
is not necessarily in the scope of Bharata's patch series. Given than
this extra feature regresses without being critical to PPC CPU hotplug
removal, we can just remove it from the patch and instead only enforce
that the indicies are within the hard limits of the bitmap itself.

We can follow-up with a more correct max check once we have the needed
fix to either ep108 or core code, but this edit is hopefully enough
for this PULL?

An amended version of the PULL with the fix squashed in is available at:

https://github.com/pcrost/qemu.git qom-cpu-for-peter

Regards,
Peter

>
>> Sorry,
>> Andreas
>>
>> --
>> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>> GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
>> 21284 (AG Nürnberg)
>>