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 <ehabk...@redhat.com> 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 <ehabk...@redhat.com> 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 <da...@gibson.dropbear.id.au> 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 <imamm...@redhat.com>
> >>>>>>>
> >>>>>>> Reviewed-by: David Gibson <da...@gibson.dropbear.id.au>
> >>>>>>>
> >>>>>>>> ---
> >>>>>>>> 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;
?
> 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 agree. This might require duplicating cpu_by_arch_id() and
> >> cpu_exists() into machine-specific code, but this doesn't sound
> >> too bad: there's only one user of cpu_by_arch_id() (that's
> >> x86-specific code living inside monitor.c), and one user of
> >> cpu_exists() (that's s390-specific code).>>
> >> (Maybe those users could be rewritten to use
> >> MachineState::possible_cpus, like pc_find_cpu_slot()).
>
>
> --
>
> Thanks,
>
> David / dhildenb
--
Eduardo
