| Message ID | 1626281596-31061-3-git-send-email-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: CPU Topology | expand |
Pierre Morel <pmorel@linux.ibm.com> writes: > Drawers and Books are levels 4 and 3 of the S390 CPU > topology. > We allow the user to define these levels and we will > store the values inside the S390CcwMachineState. Double-checking: are these members specific to S390? > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- [...] > diff --git a/qapi/machine.json b/qapi/machine.json > index c3210ee1fb..98aff804c6 100644 > --- a/qapi/machine.json > +++ b/qapi/machine.json > @@ -883,6 +883,8 @@ ## # @CpuInstanceProperties: # # List of properties to be used for hotplugging a CPU instance, # it should be passed by management with device_add command when # a CPU is being hotplugged. # # @node-id: NUMA node ID the CPU belongs to # @socket-id: socket number within node/board the CPU belongs to Missing: documentation for your new members. # @die-id: die number within node/board the CPU belongs to (Since 4.1) # @core-id: core number within die the CPU belongs to # @thread-id: thread number within core the CPU belongs to # # Note: currently there are 5 properties that could be present # but management should be prepared to pass through other # properties with device_add command to allow for future # interface extension. This also requires the filed names to be kept in # sync with the properties passed to -device/device_add. # # Since: 2.7 ## { 'struct': 'CpuInstanceProperties', > 'data': { '*node-id': 'int', > '*socket-id': 'int', > '*die-id': 'int', > + '*drawer-id': 'int', > + '*book-id': 'int', > '*core-id': 'int', > '*thread-id': 'int' > } [...]
On 7/15/21 8:16 AM, Markus Armbruster wrote: > Pierre Morel <pmorel@linux.ibm.com> writes: > >> Drawers and Books are levels 4 and 3 of the S390 CPU >> topology. >> We allow the user to define these levels and we will >> store the values inside the S390CcwMachineState. > > Double-checking: are these members specific to S390? Yes AFAIK > >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> --- > > [...] > >> diff --git a/qapi/machine.json b/qapi/machine.json >> index c3210ee1fb..98aff804c6 100644 >> --- a/qapi/machine.json >> +++ b/qapi/machine.json >> @@ -883,6 +883,8 @@ > ## > # @CpuInstanceProperties: > # > # List of properties to be used for hotplugging a CPU instance, > # it should be passed by management with device_add command when > # a CPU is being hotplugged. > # > # @node-id: NUMA node ID the CPU belongs to > # @socket-id: socket number within node/board the CPU belongs to > > Missing: documentation for your new members. Oh yes, right forgot these, thanks. > > # @die-id: die number within node/board the CPU belongs to (Since 4.1) > # @core-id: core number within die the CPU belongs to > # @thread-id: thread number within core the CPU belongs to > # > # Note: currently there are 5 properties that could be present > # but management should be prepared to pass through other > # properties with device_add command to allow for future > # interface extension. This also requires the filed names to be kept in > # sync with the properties passed to -device/device_add. > # > # Since: 2.7 > ## > { 'struct': 'CpuInstanceProperties', >> 'data': { '*node-id': 'int', >> '*socket-id': 'int', >> '*die-id': 'int', >> + '*drawer-id': 'int', >> + '*book-id': 'int', >> '*core-id': 'int', >> '*thread-id': 'int' >> } > > [...] > >
Pierre Morel <pmorel@linux.ibm.com> writes: > On 7/15/21 8:16 AM, Markus Armbruster wrote: >> Pierre Morel <pmorel@linux.ibm.com> writes: >> >>> Drawers and Books are levels 4 and 3 of the S390 CPU >>> topology. >>> We allow the user to define these levels and we will >>> store the values inside the S390CcwMachineState. >> >> Double-checking: are these members specific to S390? > > Yes AFAIK Makes me wonder whether they should be conditional on TARGET_S390X. What happens when you specify them for another target? Silently ignored, or error?
On Thu, Jul 15 2021, Markus Armbruster <armbru@redhat.com> wrote: > Pierre Morel <pmorel@linux.ibm.com> writes: > >> On 7/15/21 8:16 AM, Markus Armbruster wrote: >>> Pierre Morel <pmorel@linux.ibm.com> writes: >>> >>>> Drawers and Books are levels 4 and 3 of the S390 CPU >>>> topology. >>>> We allow the user to define these levels and we will >>>> store the values inside the S390CcwMachineState. >>> >>> Double-checking: are these members specific to S390? >> >> Yes AFAIK > > Makes me wonder whether they should be conditional on TARGET_S390X. > > What happens when you specify them for another target? Silently > ignored, or error? I'm wondering whether we should include them in the base machine state and treat them as we treat 'dies' (i.e. the standard parser errors out if they are set, and only the s390x parser supports them.) [I remember that we had a discussion ages ago about which parameters should be included in the cpu topology, but I cannot recall the details :/]
On Fri, Jul 16, 2021 at 11:10:04AM +0200, Cornelia Huck wrote: > On Thu, Jul 15 2021, Markus Armbruster <armbru@redhat.com> wrote: > > > Pierre Morel <pmorel@linux.ibm.com> writes: > > > >> On 7/15/21 8:16 AM, Markus Armbruster wrote: > >>> Pierre Morel <pmorel@linux.ibm.com> writes: > >>> > >>>> Drawers and Books are levels 4 and 3 of the S390 CPU > >>>> topology. > >>>> We allow the user to define these levels and we will > >>>> store the values inside the S390CcwMachineState. > >>> > >>> Double-checking: are these members specific to S390? > >> > >> Yes AFAIK > > > > Makes me wonder whether they should be conditional on TARGET_S390X. > > > > What happens when you specify them for another target? Silently > > ignored, or error? > > I'm wondering whether we should include them in the base machine state > and treat them as we treat 'dies' (i.e. the standard parser errors out > if they are set, and only the s390x parser supports them.) To repeat what i just wrote in my reply to patch 1, I think we ought to think about a different approach to handling the usage constraints, which doesn't require full re-implementation of the smp_parse method each time. There should be a way for each target to report topology constraints, such the the single smp_parse method can do the right thing, especially wrt error reporting for unsupported values. Regards, Daniel
On Thu, Jul 15, 2021 at 08:16:32AM +0200, Markus Armbruster wrote: > Pierre Morel <pmorel@linux.ibm.com> writes: > > > Drawers and Books are levels 4 and 3 of the S390 CPU > > topology. > > We allow the user to define these levels and we will > > store the values inside the S390CcwMachineState. > > Double-checking: are these members specific to S390? > > > > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > > --- > > [...] > > > diff --git a/qapi/machine.json b/qapi/machine.json > > index c3210ee1fb..98aff804c6 100644 > > --- a/qapi/machine.json > > +++ b/qapi/machine.json > > @@ -883,6 +883,8 @@ > ## > # @CpuInstanceProperties: > # > # List of properties to be used for hotplugging a CPU instance, > # it should be passed by management with device_add command when > # a CPU is being hotplugged. > # > # @node-id: NUMA node ID the CPU belongs to > # @socket-id: socket number within node/board the CPU belongs to > > Missing: documentation for your new members. It is also missing in qemu-options.hx which covers -smp To quote the lscpu manpage, it seems drawer/book fit inbetween NUMA node and socket level: CPU The logical CPU number of a CPU as used by the Linux kernel. CORE The logical core number. A core can contain several CPUs. SOCKET The logical socket number. A socket can contain several cores. BOOK The logical book number. A book can contain several sockets. DRAWER The logical drawer number. A drawer can contain several books. NODE The logical NUMA node number. A node can contain several drawers. Regards, Daniel
On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote: > On Fri, Jul 16, 2021 at 11:10:04AM +0200, Cornelia Huck wrote: >> On Thu, Jul 15 2021, Markus Armbruster <armbru@redhat.com> wrote: >> >> > Pierre Morel <pmorel@linux.ibm.com> writes: >> > >> >> On 7/15/21 8:16 AM, Markus Armbruster wrote: >> >>> Pierre Morel <pmorel@linux.ibm.com> writes: >> >>> >> >>>> Drawers and Books are levels 4 and 3 of the S390 CPU >> >>>> topology. >> >>>> We allow the user to define these levels and we will >> >>>> store the values inside the S390CcwMachineState. >> >>> >> >>> Double-checking: are these members specific to S390? >> >> >> >> Yes AFAIK >> > >> > Makes me wonder whether they should be conditional on TARGET_S390X. >> > >> > What happens when you specify them for another target? Silently >> > ignored, or error? >> >> I'm wondering whether we should include them in the base machine state >> and treat them as we treat 'dies' (i.e. the standard parser errors out >> if they are set, and only the s390x parser supports them.) > > To repeat what i just wrote in my reply to patch 1, I think we ought to > think about a different approach to handling the usage constraints, > which doesn't require full re-implementation of the smp_parse method > each time. There should be a way for each target to report topology > constraints, such the the single smp_parse method can do the right > thing, especially wrt error reporting for unsupported values. That would mean that all possible fields would need to go into common code, right? I'm wondering whether there are more architecture/cpu specific values lurking in the corner, it would get unwieldy if we need to go beyond the existing fields and drawers/books.
On Fri, Jul 16, 2021 at 12:44:49PM +0200, Cornelia Huck wrote: > On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote: > > > On Fri, Jul 16, 2021 at 11:10:04AM +0200, Cornelia Huck wrote: > >> On Thu, Jul 15 2021, Markus Armbruster <armbru@redhat.com> wrote: > >> > >> > Pierre Morel <pmorel@linux.ibm.com> writes: > >> > > >> >> On 7/15/21 8:16 AM, Markus Armbruster wrote: > >> >>> Pierre Morel <pmorel@linux.ibm.com> writes: > >> >>> > >> >>>> Drawers and Books are levels 4 and 3 of the S390 CPU > >> >>>> topology. > >> >>>> We allow the user to define these levels and we will > >> >>>> store the values inside the S390CcwMachineState. > >> >>> > >> >>> Double-checking: are these members specific to S390? > >> >> > >> >> Yes AFAIK > >> > > >> > Makes me wonder whether they should be conditional on TARGET_S390X. > >> > > >> > What happens when you specify them for another target? Silently > >> > ignored, or error? > >> > >> I'm wondering whether we should include them in the base machine state > >> and treat them as we treat 'dies' (i.e. the standard parser errors out > >> if they are set, and only the s390x parser supports them.) > > > > To repeat what i just wrote in my reply to patch 1, I think we ought to > > think about a different approach to handling the usage constraints, > > which doesn't require full re-implementation of the smp_parse method > > each time. There should be a way for each target to report topology > > constraints, such the the single smp_parse method can do the right > > thing, especially wrt error reporting for unsupported values. > > That would mean that all possible fields would need to go into common > code, right? Yes, that is an implication of what i'm suggesting. > I'm wondering whether there are more architecture/cpu specific values > lurking in the corner, it would get unwieldy if we need to go beyond the > existing fields and drawers/books. Is the book/drawer thing architecture specific, or is it machine type / CPU specific. ie do /all/ the s390x machine types / CPUS QEMU support the book/drawer concept, or only a subset. If only a subset, then restricting it per target on QAPI doesn't fully solve the root problem, and we instead are better focusing on accurate runtime error reporting. Regards, Daniel
On 7/16/21 11:23 AM, Daniel P. Berrangé wrote: > On Thu, Jul 15, 2021 at 08:16:32AM +0200, Markus Armbruster wrote: >> Pierre Morel <pmorel@linux.ibm.com> writes: >> >>> Drawers and Books are levels 4 and 3 of the S390 CPU >>> topology. >>> We allow the user to define these levels and we will >>> store the values inside the S390CcwMachineState. >> >> Double-checking: are these members specific to S390? >> >>> >>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>> --- >> >> [...] >> >>> diff --git a/qapi/machine.json b/qapi/machine.json >>> index c3210ee1fb..98aff804c6 100644 >>> --- a/qapi/machine.json >>> +++ b/qapi/machine.json >>> @@ -883,6 +883,8 @@ >> ## >> # @CpuInstanceProperties: >> # >> # List of properties to be used for hotplugging a CPU instance, >> # it should be passed by management with device_add command when >> # a CPU is being hotplugged. >> # >> # @node-id: NUMA node ID the CPU belongs to >> # @socket-id: socket number within node/board the CPU belongs to >> >> Missing: documentation for your new members. > > It is also missing in qemu-options.hx which covers -smp > > To quote the lscpu manpage, it seems drawer/book fit inbetween > NUMA node and socket level: > > CPU The logical CPU number of a CPU as used by the Linux kernel. > > CORE The logical core number. A core can contain several CPUs. > > SOCKET The logical socket number. A socket can contain several cores. > > BOOK The logical book number. A book can contain several sockets. > > DRAWER The logical drawer number. A drawer can contain several books. > > NODE The logical NUMA node number. A node can contain several drawers. > > Regards, > Daniel > Yes, seems I missed a fundamental change. Will adapt this too.
On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote: > On Fri, Jul 16, 2021 at 12:44:49PM +0200, Cornelia Huck wrote: >> On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote: >> >> > On Fri, Jul 16, 2021 at 11:10:04AM +0200, Cornelia Huck wrote: >> >> On Thu, Jul 15 2021, Markus Armbruster <armbru@redhat.com> wrote: >> >> >> >> > Pierre Morel <pmorel@linux.ibm.com> writes: >> >> > >> >> >> On 7/15/21 8:16 AM, Markus Armbruster wrote: >> >> >>> Pierre Morel <pmorel@linux.ibm.com> writes: >> >> >>> >> >> >>>> Drawers and Books are levels 4 and 3 of the S390 CPU >> >> >>>> topology. >> >> >>>> We allow the user to define these levels and we will >> >> >>>> store the values inside the S390CcwMachineState. >> >> >>> >> >> >>> Double-checking: are these members specific to S390? >> >> >> >> >> >> Yes AFAIK >> >> > >> >> > Makes me wonder whether they should be conditional on TARGET_S390X. >> >> > >> >> > What happens when you specify them for another target? Silently >> >> > ignored, or error? >> >> >> >> I'm wondering whether we should include them in the base machine state >> >> and treat them as we treat 'dies' (i.e. the standard parser errors out >> >> if they are set, and only the s390x parser supports them.) >> > >> > To repeat what i just wrote in my reply to patch 1, I think we ought to >> > think about a different approach to handling the usage constraints, >> > which doesn't require full re-implementation of the smp_parse method >> > each time. There should be a way for each target to report topology >> > constraints, such the the single smp_parse method can do the right >> > thing, especially wrt error reporting for unsupported values. >> >> That would mean that all possible fields would need to go into common >> code, right? > > Yes, that is an implication of what i'm suggesting. > >> I'm wondering whether there are more architecture/cpu specific values >> lurking in the corner, it would get unwieldy if we need to go beyond the >> existing fields and drawers/books. > > Is the book/drawer thing architecture specific, or is it machine > type / CPU specific. ie do /all/ the s390x machine types / CPUS > QEMU support the book/drawer concept, or only a subset. Should not be by machine type, but might be by cpu model (e.g. older hardware lacking the needed support for exposing this to the guest.) IBM folks, please correct me if I'm wrong. > If only a subset, then restricting it per target on QAPI doesn't > fully solve the root problem, and we instead are better focusing > on accurate runtime error reporting. Nod. Runtime error reporting should also be more flexible.
On 7/19/21 5:50 PM, Cornelia Huck wrote: > On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote: > >> On Fri, Jul 16, 2021 at 12:44:49PM +0200, Cornelia Huck wrote: >>> On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote: >>> >>>> On Fri, Jul 16, 2021 at 11:10:04AM +0200, Cornelia Huck wrote: >>>>> On Thu, Jul 15 2021, Markus Armbruster <armbru@redhat.com> wrote: >>>>> >>>>>> Pierre Morel <pmorel@linux.ibm.com> writes: >>>>>> >>>>>>> On 7/15/21 8:16 AM, Markus Armbruster wrote: >>>>>>>> Pierre Morel <pmorel@linux.ibm.com> writes: >>>>>>>> >>>>>>>>> Drawers and Books are levels 4 and 3 of the S390 CPU >>>>>>>>> topology. >>>>>>>>> We allow the user to define these levels and we will >>>>>>>>> store the values inside the S390CcwMachineState. >>>>>>>> >>>>>>>> Double-checking: are these members specific to S390? >>>>>>> >>>>>>> Yes AFAIK >>>>>> >>>>>> Makes me wonder whether they should be conditional on TARGET_S390X. >>>>>> >>>>>> What happens when you specify them for another target? Silently >>>>>> ignored, or error? >>>>> >>>>> I'm wondering whether we should include them in the base machine state >>>>> and treat them as we treat 'dies' (i.e. the standard parser errors out >>>>> if they are set, and only the s390x parser supports them.) >>>> >>>> To repeat what i just wrote in my reply to patch 1, I think we ought to >>>> think about a different approach to handling the usage constraints, >>>> which doesn't require full re-implementation of the smp_parse method >>>> each time. There should be a way for each target to report topology >>>> constraints, such the the single smp_parse method can do the right >>>> thing, especially wrt error reporting for unsupported values. >>> >>> That would mean that all possible fields would need to go into common >>> code, right? >> >> Yes, that is an implication of what i'm suggesting. >> >>> I'm wondering whether there are more architecture/cpu specific values >>> lurking in the corner, it would get unwieldy if we need to go beyond the >>> existing fields and drawers/books. >> >> Is the book/drawer thing architecture specific, or is it machine >> type / CPU specific. ie do /all/ the s390x machine types / CPUS >> QEMU support the book/drawer concept, or only a subset. > > Should not be by machine type, but might be by cpu model (e.g. older > hardware lacking the needed support for exposing this to the guest.) IBM > folks, please correct me if I'm wrong. Looks correct to me this is an information indicated by a facility introduced with Z10 if I do not make an error. Regards, Pierre > >> If only a subset, then restricting it per target on QAPI doesn't >> fully solve the root problem, and we instead are better focusing >> on accurate runtime error reporting. > > Nod. Runtime error reporting should also be more flexible. >
On Tue, Jul 20 2021, Pierre Morel <pmorel@linux.ibm.com> wrote: > On 7/19/21 5:50 PM, Cornelia Huck wrote: >> On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote: >>> Is the book/drawer thing architecture specific, or is it machine >>> type / CPU specific. ie do /all/ the s390x machine types / CPUS >>> QEMU support the book/drawer concept, or only a subset. >> >> Should not be by machine type, but might be by cpu model (e.g. older >> hardware lacking the needed support for exposing this to the guest.) IBM >> folks, please correct me if I'm wrong. > > > Looks correct to me this is an information indicated by a facility > introduced with Z10 if I do not make an error. Hm. Would that become a problem if we made availability of parameters dependent upon a value in the machine (see the other thread I cc:ed you on?)
On 7/20/21 10:20 AM, Cornelia Huck wrote: > On Tue, Jul 20 2021, Pierre Morel <pmorel@linux.ibm.com> wrote: > >> On 7/19/21 5:50 PM, Cornelia Huck wrote: >>> On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote: >>>> Is the book/drawer thing architecture specific, or is it machine >>>> type / CPU specific. ie do /all/ the s390x machine types / CPUS >>>> QEMU support the book/drawer concept, or only a subset. >>> >>> Should not be by machine type, but might be by cpu model (e.g. older >>> hardware lacking the needed support for exposing this to the guest.) IBM >>> folks, please correct me if I'm wrong. >> >> >> Looks correct to me this is an information indicated by a facility >> introduced with Z10 if I do not make an error. > > Hm. Would that become a problem if we made availability of parameters > dependent upon a value in the machine (see the other thread I cc:ed you > on?) > Why? The parameter can always be there, it is just that with older cpu model we will not report the topology information to the guest. The discussion on dies and on smp_dies_supported in this thread will be interesting to follow because in my opinion dies for X or books/drawers for Z are to be treated equally. Regards, Pierre
On Tue, Jul 20 2021, Pierre Morel <pmorel@linux.ibm.com> wrote: > On 7/20/21 10:20 AM, Cornelia Huck wrote: >> On Tue, Jul 20 2021, Pierre Morel <pmorel@linux.ibm.com> wrote: >> >>> On 7/19/21 5:50 PM, Cornelia Huck wrote: >>>> On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote: >>>>> Is the book/drawer thing architecture specific, or is it machine >>>>> type / CPU specific. ie do /all/ the s390x machine types / CPUS >>>>> QEMU support the book/drawer concept, or only a subset. >>>> >>>> Should not be by machine type, but might be by cpu model (e.g. older >>>> hardware lacking the needed support for exposing this to the guest.) IBM >>>> folks, please correct me if I'm wrong. >>> >>> >>> Looks correct to me this is an information indicated by a facility >>> introduced with Z10 if I do not make an error. >> >> Hm. Would that become a problem if we made availability of parameters >> dependent upon a value in the machine (see the other thread I cc:ed you >> on?) >> > > Why? > The parameter can always be there, it is just that with older cpu model > we will not report the topology information to the guest. If we are fine with a configuration like that, sure. > > The discussion on dies and on smp_dies_supported in this thread will be > interesting to follow because in my opinion dies for X or books/drawers > for Z are to be treated equally. Agreed. There was also talk of "clusters" in that thread, which I assume are yet anothor machine specific parameter.
On Tue, Jul 20, 2021 at 10:46:31AM +0200, Pierre Morel wrote: > > > On 7/20/21 10:20 AM, Cornelia Huck wrote: > > On Tue, Jul 20 2021, Pierre Morel <pmorel@linux.ibm.com> wrote: > > > > > On 7/19/21 5:50 PM, Cornelia Huck wrote: > > > > On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > > Is the book/drawer thing architecture specific, or is it machine > > > > > type / CPU specific. ie do /all/ the s390x machine types / CPUS > > > > > QEMU support the book/drawer concept, or only a subset. > > > > > > > > Should not be by machine type, but might be by cpu model (e.g. older > > > > hardware lacking the needed support for exposing this to the guest.) IBM > > > > folks, please correct me if I'm wrong. > > > > > > > > > Looks correct to me this is an information indicated by a facility > > > introduced with Z10 if I do not make an error. > > > > Hm. Would that become a problem if we made availability of parameters > > dependent upon a value in the machine (see the other thread I cc:ed you > > on?) > > > > Why? > The parameter can always be there, it is just that with older cpu model we > will not report the topology information to the guest. I mostly see this as an error reporting problem, perhaps with an optional capabilitty reporting facility. eg if someone requests 'z9' or older together with books > 1 or drawers > 1, it would be ideal if QEMU reports an error message, rather than silently ignoring it and not reporting this info to the guest. Taking it further, it some app queries CPU models via QMP, it could be desirable if the CPU model data returned indicates whether drawers/books > 1 are conceptually available. Likewise for dies on x86, since I cna't imagine guest OS doing sensible things with an i486 CPU and dies=2 Regards, Daniel
On 7/20/21 11:19 AM, Daniel P. Berrangé wrote: > On Tue, Jul 20, 2021 at 10:46:31AM +0200, Pierre Morel wrote: >> >> >> On 7/20/21 10:20 AM, Cornelia Huck wrote: >>> On Tue, Jul 20 2021, Pierre Morel <pmorel@linux.ibm.com> wrote: >>> >>>> On 7/19/21 5:50 PM, Cornelia Huck wrote: >>>>> On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote: >>>>>> Is the book/drawer thing architecture specific, or is it machine >>>>>> type / CPU specific. ie do /all/ the s390x machine types / CPUS >>>>>> QEMU support the book/drawer concept, or only a subset. >>>>> >>>>> Should not be by machine type, but might be by cpu model (e.g. older >>>>> hardware lacking the needed support for exposing this to the guest.) IBM >>>>> folks, please correct me if I'm wrong. >>>> >>>> >>>> Looks correct to me this is an information indicated by a facility >>>> introduced with Z10 if I do not make an error. >>> >>> Hm. Would that become a problem if we made availability of parameters >>> dependent upon a value in the machine (see the other thread I cc:ed you >>> on?) >>> >> >> Why? >> The parameter can always be there, it is just that with older cpu model we >> will not report the topology information to the guest. > > I mostly see this as an error reporting problem, perhaps with an optional > capabilitty reporting facility. > > eg if someone requests 'z9' or older together with books > 1 or > drawers > 1, it would be ideal if QEMU reports an error message, > rather than silently ignoring it and not reporting this info to > the guest. > > Taking it further, it some app queries CPU models via QMP, it > could be desirable if the CPU model data returned indicates whether > drawers/books > 1 are conceptually available. Likewise for dies > on x86, since I cna't imagine guest OS doing sensible things > with an i486 CPU and dies=2 > > Regards, > Daniel > Thanks, I will rework this based on what is done on x86 and dies. regards, Pierre
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 899d3a4137..42d9be7333 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -591,12 +591,17 @@ static void s390_smp_parse(MachineState *ms, QemuOpts *opts) unsigned cpus = qemu_opt_get_number(opts, "cpus", 1); unsigned sockets = qemu_opt_get_number(opts, "sockets", 1); unsigned cores = qemu_opt_get_number(opts, "cores", 1); + unsigned drawers = qemu_opt_get_number(opts, "drawers", 1); + unsigned books = qemu_opt_get_number(opts, "books", 1); + S390CcwMachineState *s390ms = S390_CCW_MACHINE(ms); if (opts) { - if (cpus == 0 || sockets == 0 || cores == 0) { + if (cpus == 0 || drawers == 0 || books == 0 || sockets == 0 || + cores == 0) { error_report("cpu topology: " - "sockets (%u), cores (%u) or number of CPU(%u) " - "can not be zero", sockets, cores, cpus); + "drawers (%u) books (%u) sockets (%u), cores (%u) " + "or number of CPU(%u) can not be zero", drawers, + books, sockets, cores, cpus); exit(1); } } @@ -608,12 +613,13 @@ static void s390_smp_parse(MachineState *ms, QemuOpts *opts) } if (!qemu_opt_find(opts, "cores")) { - cores = ms->smp.max_cpus / sockets; + cores = ms->smp.max_cpus / (drawers * books * sockets); } - if (sockets * cores != ms->smp.max_cpus) { - error_report("Invalid CPU topology: sockets (%u) * cores (%u) " - "!= maxcpus (%u)", sockets, cores, ms->smp.max_cpus); + if (drawers * books * sockets * cores != ms->smp.max_cpus) { + error_report("Invalid CPU topology: drawers (%u) books (%u) " + "sockets (%u) * cores (%u) != maxcpus (%u)", + drawers, books, sockets, cores, ms->smp.max_cpus); exit(1); } @@ -621,6 +627,8 @@ static void s390_smp_parse(MachineState *ms, QemuOpts *opts) ms->smp.cpus = cpus; ms->smp.cores = cores; ms->smp.sockets = sockets; + s390ms->drawers = drawers; + s390ms->books = books; } static void ccw_machine_class_init(ObjectClass *oc, void *data) diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h index 3331990e02..fb3c3a50ce 100644 --- a/include/hw/s390x/s390-virtio-ccw.h +++ b/include/hw/s390x/s390-virtio-ccw.h @@ -28,6 +28,8 @@ struct S390CcwMachineState { bool dea_key_wrap; bool pv; uint8_t loadparm[8]; + int drawers; + int books; }; struct S390CcwMachineClass { diff --git a/qapi/machine.json b/qapi/machine.json index c3210ee1fb..98aff804c6 100644 --- a/qapi/machine.json +++ b/qapi/machine.json @@ -883,6 +883,8 @@ 'data': { '*node-id': 'int', '*socket-id': 'int', '*die-id': 'int', + '*drawer-id': 'int', + '*book-id': 'int', '*core-id': 'int', '*thread-id': 'int' } diff --git a/softmmu/vl.c b/softmmu/vl.c index 4df1496101..fc73107b91 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -699,6 +699,12 @@ static QemuOptsList qemu_smp_opts = { }, { .name = "dies", .type = QEMU_OPT_NUMBER, + }, { + .name = "books", + .type = QEMU_OPT_NUMBER, + }, { + .name = "drawers", + .type = QEMU_OPT_NUMBER, }, { .name = "cores", .type = QEMU_OPT_NUMBER,
Drawers and Books are levels 4 and 3 of the S390 CPU topology. We allow the user to define these levels and we will store the values inside the S390CcwMachineState. Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> --- hw/s390x/s390-virtio-ccw.c | 22 +++++++++++++++------- include/hw/s390x/s390-virtio-ccw.h | 2 ++ qapi/machine.json | 2 ++ softmmu/vl.c | 6 ++++++ 4 files changed, 25 insertions(+), 7 deletions(-)