Re: [Qemu-devel] [RFC v2 3/6] possible_cpus: add CPUArchId::type field

[David Hildenbrand]

On 10.11.2017 13:58, Eduardo Habkost wrote:
> On Fri, Nov 10, 2017 at 01:34:42PM +0100, David Hildenbrand wrote:
>> On 10.11.2017 11:14, Cornelia Huck wrote:
>>> On Thu, 9 Nov 2017 18:02:35 -0200
>>> Eduardo Habkost <address@hidden> wrote:
>>>
>>>> On Thu, Nov 09, 2017 at 05:58:03PM +1100, David Gibson wrote:
>>>>> On Tue, Nov 07, 2017 at 04:04:04PM +0100, Cornelia Huck wrote:  
>>>>>> On Mon, 6 Nov 2017 16:02:16 -0200
>>>>>> Eduardo Habkost <address@hidden> wrote:
>>>>>>   
>>>>>>> On Tue, Oct 31, 2017 at 03:01:14PM +0100, Igor Mammedov wrote:  
>>>>>>>> On Thu, 19 Oct 2017 17:31:51 +1100
>>>>>>>> David Gibson <address@hidden> wrote:
>>>>>>>>     
>>>>>>>>> On Wed, Oct 18, 2017 at 01:12:12PM +0200, Igor Mammedov wrote:    
>>>>>>>>>> For enabling early cpu to numa node configuration at runtime
>>>>>>>>>> qmp_query_hotpluggable_cpus() should provide a list of available
>>>>>>>>>> cpu slots at early stage, before machine_init() is called and
>>>>>>>>>> the 1st cpu is created, so that mgmt might be able to call it
>>>>>>>>>> and use output to set numa mapping.
>>>>>>>>>> Use MachineClass::possible_cpu_arch_ids() callback to set
>>>>>>>>>> cpu type info, along with the rest of possible cpu properties,
>>>>>>>>>> to let machine define which cpu type* will be used.
>>>>>>>>>>
>>>>>>>>>> * for SPAPR it will be a spapr core type and for ARM/s390x/x86
>>>>>>>>>>   a respective descendant of CPUClass.
>>>>>>>>>>
>>>>>>>>>> Move parse_numa_opts() in vl.c after cpu_model is parsed into
>>>>>>>>>> cpu_type so that possible_cpu_arch_ids() would know which
>>>>>>>>>> cpu_type to use during layout initialization.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Igor Mammedov <address@hidden>      
>>>>>>>>>
>>>>>>>>> Reviewed-by: David Gibson <address@hidden>
>>>>>>>>>     
>>>>>>>>>> ---
>>>>>>>>>>   v2:
>>>>>>>>>>      - fix NULL dereference caused by not initialized
>>>>>>>>>>        MachineState::cpu_type at the time parse_numa_opts()
>>>>>>>>>>        were called
>>>>>>>>>> ---
>>>>>>>>>>  include/hw/boards.h        |  2 ++
>>>>>>>>>>  hw/arm/virt.c              |  3 ++-
>>>>>>>>>>  hw/core/machine.c          | 12 ++++++------
>>>>>>>>>>  hw/i386/pc.c               |  4 +++-
>>>>>>>>>>  hw/ppc/spapr.c             | 13 ++++++++-----
>>>>>>>>>>  hw/s390x/s390-virtio-ccw.c |  1 +
>>>>>>>>>>  vl.c                       |  3 +--
>>>>>>>>>>  7 files changed, 23 insertions(+), 15 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>>>>>>>>>> index 191a5b3..fa21758 100644
>>>>>>>>>> --- a/include/hw/boards.h
>>>>>>>>>> +++ b/include/hw/boards.h
>>>>>>>>>> @@ -80,6 +80,7 @@ void machine_set_cpu_numa_node(MachineState 
>>>>>>>>>> *machine,
>>>>>>>>>>   * CPUArchId:
>>>>>>>>>>   * @arch_id - architecture-dependent CPU ID of present or possible 
>>>>>>>>>> CPU      
>>>>>>>>>
>>>>>>>>> I know this isn't really in scope for this patch, but is @arch_id here
>>>>>>>>> supposed to have meaning defined by the target, or by the machine?
>>>>>>>>>
>>>>>>>>> If it's the machime, it could do with a rename - "arch" means target
>>>>>>>>> to most people (thanks to Linux).
>>>>>>>>>
>>>>>>>>> If it's the target, it's kind of bogus, because it doesn't necessarily
>>>>>>>>> have a clear meaning per target - get_arch_id in CPUClass has the same
>>>>>>>>> problem, which is probably one reason it's basically only used by the
>>>>>>>>> x86 code at present.
>>>>>>>>>
>>>>>>>>> e.g. for target/ppc, what do we use?  There's the PIR, which is in the
>>>>>>>>> CPU.. but only on some cpu models, not all.  There will generally be
>>>>>>>>> some kind of master PIC id, but there are different PIC models on
>>>>>>>>> different boards.  What goes in the devicetree?  Well only some
>>>>>>>>> machines use devicetree, and they might define the cpu reg 
>>>>>>>>> differently.
>>>>>>>>>
>>>>>>>>> Board designs will generally try to make some if not all of those
>>>>>>>>> possible values equal for simplicity, but there's still no real way of
>>>>>>>>> defining a sensible arch_id independent of machine / board.    
>>>>>>>> I'd say arch_id is machine specific so far, it was introduced when we
>>>>>>>> didn't have CpuInstanceProperties and at that time we considered only
>>>>>>>> vcpus (threads) and doesn't really apply to spapr cores.
>>>>>>>>
>>>>>>>> In general we could do away with arch_id and use CpuInstanceProperties
>>>>>>>> instead, but arch_id also serves aux purpose, it allows machine to
>>>>>>>> pre-calculate(cache) apic-id/mpidr values in one place and then they
>>>>>>>> are/(could be) used by arch in-depended code to build acpi tables.
>>>>>>>> So if we drop arch_id we would need to introduce a machine hook,
>>>>>>>> which would translate CpuInstanceProperties into current arch_id.    
>>>>>>>
>>>>>>> I think we need to do a better to job documenting where exactly
>>>>>>> we expect arch_id to be used and how, so people know what it's
>>>>>>> supposed to return.
>>>>>>>
>>>>>>> If the only place where it's useful now is ACPI code (is it?),
>>>>>>> should we rename it to something like get_acpi_id()?  
>>>>>>
>>>>>> It is also used in hw/s390x/sclp.c to fill out a control block, so acpi
>>>>>> isn't the only user.  
>>>>>
>>>>> Yeah.. this is kind of bogus.  The s390 use is in machine specific
>>>>> code, so it's basically just re-using the field for an unrelated usage
>>>>> to the x86/arm one (ACPI).
>>
>> as index == arch_id on s390x, that code could easily be changed to
>> something like:
>>
>> @@ -45,7 +45,7 @@ static void prepare_cpu_entries(SCLPDevice *sclp,
>> CPUEntry *entry, int *count)
>>          if (!ms->possible_cpus->cpus[i].cpu) {
>>              continue;
>>          }
>> -        entry[*count].address = ms->possible_cpus->cpus[i].arch_id;
>> +        entry[*count].address = i;
> 
> What about decoupling it from the array index, by using:
>     entry[*count].address = ms->possible_cpus->cpus[i].props.core_id;
> or:
>     entry[*count].address = S390_CPU(ms->possible_cpus->cpus[i].cpu)->core_id;
> ?

Yes, we could do that, but doesn't really matter for now. I would ACK
either :)

> 
> 
>>          entry[*count].type = 0;
>>          memcpy(entry[*count].features, features, sizeof(features));
>>          (*count)++;
>>
>> arch_id just looked like the right thing to use (documentation issue
>> mentioned above)
>>
>>
>>>>>
>>>>> If we can't assign a universal meaning to the field (even if the
>>>>> actual values are per-machine) - and I don't think we can - then I
>>>>> really don't think it belongs in CPUState.  A machine hook which
>>>>> translates an ArchId to an acpi_id is the correct solution I believe.
>>>>> Or even an ACPIMachine interface (to be implemented by machines which
>>>>> do ACPI) which has a method to do this.
>>>>>
>>>>> Since both the assignment and use are in machine type specific code
>>>>> for s390, it can have its own field in the s390 specific cpu subclass.
>>
>> s390x doesn't need arch_id at all.
>>
>> cs->cpu_index can be used.
> 
> What about the cpu_exists() check in s390_cpu_realizefn()?  It
> could be moved to a new s390_machine_device_pre_plug() method
> that just checks ms->possible_cpus->cpus[cpu->env.core_id].cpu.
> 

I always hated that part (cpu_exists()). We can completely drop
cpu_exists() on s390x and simply add that check for pre plug as you
said, fine with me!
-- 

Thanks,

David / dhildenb