Re: [PATCH v8 02/12] s390x/cpu_topology: CPU topology objects and structures

[Pierre Morel]

On 7/14/22 12:38, Janis Schoetterl-Glausch wrote:
> On 7/13/22 16:59, Pierre Morel wrote:
> > On 7/12/22 17:40, Janis Schoetterl-Glausch wrote:
> > > On 6/20/22 16:03, Pierre Morel wrote:

> [...]
>
> > > > +}
> > > > +
> > > > +/*
> > > > + * s390_topology_new_cpu:
> > > > + * @core_id: the core ID is machine wide
> > > > + *
> > > > + * We have a single book returned by s390_get_topology(),
> > > > + * then we build the hierarchy on demand.
> > > > + * Note that we do not destroy the hierarchy on error creating
> > > > + * an entry in the topology, we just keep it empty.
> > > > + * We do not need to worry about not finding a topology level
> > > > + * entry this would have been caught during smp parsing.
> > > > + */
> > > > +bool s390_topology_new_cpu(MachineState *ms, int core_id, Error **errp)
> > > > +{
> > > > +    S390TopologyBook *book;
> > > > +    S390TopologySocket *socket;
> > > > +    S390TopologyCores *cores;
> > > > +    int nb_cores_per_socket;
> > >
> > > num_cores_per_socket instead?
> > >
> > > > +    int origin, bit;
> > > > +
> > > > +    book = s390_get_topology();
> > > > +
> > > > +    nb_cores_per_socket = ms->smp.cores * ms->smp.threads;
> > >
> > > We don't support the multithreading facility, do we?
> > > So, I think we should assert smp.threads == 1 somewhere.
> > > In any case I think the correct expression would round the threads up to the 
> > > next power of 2,
> > > because the core_id has the thread id in the lower bits, but threads per core 
> > > doesn't need to be
> > > a power of 2 according to the architecture.
> >
> > That is right.
> > I will add that.
>
> Add the assert?
> It should probably be somewhere else.

That is sure.
I thought about put a fatal error report during the initialization in 
the s390_topology_setup()

> And you can set thread > 1 today, so we'd need to handle that. (increase the 
> number of cpus instead and print a warning?)
>
> [...]

this would introduce arch dependencies in the hw/core/
I think that the error report for Z is enough.

So once we support Multithreading in the guest we can adjust it easier 
without involving the common code.

Or we can introduce a thread_supported in SMPCompatProps, which would be 
good.
I would prefer to propose this outside of the series and suppress the 
fatal error once it is adopted.

> > > > +
> > > > +/*
> > > > + * Setting the first topology: 1 book, 1 socket
> > > > + * This is enough for 64 cores if the topology is flat (single socket)
> > > > + */
> > > > +void s390_topology_setup(MachineState *ms)
> > > > +{
> > > > +    DeviceState *dev;
> > > > +
> > > > +    /* Create BOOK bridge device */
> > > > +    dev = qdev_new(TYPE_S390_TOPOLOGY_BOOK);
> > > > +    object_property_add_child(qdev_get_machine(),
> > > > +                              TYPE_S390_TOPOLOGY_BOOK, OBJECT(dev));
> > >
> > > Why add it to the machine instead of directly using a static?
> >
> > For my opinion it is a characteristic of the machine.
> >
> > > So it's visible to the user via info qtree or something?
> >
> > It is already visible to the user on info qtree.
> >
> > > Would that even be the appropriate location to show that?
> >
> > That is a very good question and I really appreciate if we discuss on the 
> > design before diving into details.
> >
> > The idea is to have the architecture details being on qtree as object so we can 
> > plug new drawers/books/socket/cores and in the future when the infrastructure 
> > allows it unplug them.
>
> Would it not be more accurate to say that we plug in new cpus only?
> Since you need to specify the topology up front with -smp and it cannot change 
> after.

smp specify the maximum we can have.
I thought we can add dynamically elements inside this maximum set.

> So that all is static, books/sockets might be completely unpopulated, but they 
> still exist in a way.
> As far as I understand, STSI only allows for cpus to change, nothing above it.

I thought we want to plug new books or drawers but I may be wrong.

> > There is a info numa (info cpus does not give a lot info) to give information 
> > on nodes but AFAIU, a node is more a theoritical that can be used above the 
> > virtual architecture, sockets/cores, to specify characteristics like distance 
> > and associated memory.
>
> https://qemu.readthedocs.io/en/latest/interop/qemu-qmp-ref.html#qapidoc-2391
> shows that the relevant information can be queried via qmp.
> When I tried it on s390x it only showed the core_id, but we should be able to 
> add the rest.

yes, sure.

> Am I correct in my understanding, that there are two reasons to have the 
> hierarchy objects:
> 1. Caching the topology instead of computing it when STSI is called
> 2. So they show up in info qtree
>
> ?

and have the possibility to add the objects dynamically. yes


> [...]
>
> > > > +    /*
> > > > +     * Each CPU inside a socket will be represented by a bit in a 64bit
> > > > +     * unsigned long. Set on plug and clear on unplug of a CPU.
> > > > +     * All CPU inside a mask share the same dedicated, polarity and
> > > > +     * cputype values.
> > > > +     * The origin is the offset of the first CPU in a mask.
> > > > +     */
> > > > +struct S390TopologyCores {
> > > > +    DeviceState parent_obj;
> > > > +    int id;
> > > > +    bool dedicated;
> > > > +    uint8_t polarity;
> > > > +    uint8_t cputype;
> > >
> > > Why not snake_case for cpu type?
> >
> > I do not understand what you mean.
>
> I'm suggesting s/cputype/cpu_type/

ok


Thanks,

regards,
Pierre

--
Pierre Morel
IBM Lab Boeblingen