| Message ID | 20230105145313.168489-5-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: CPU Topology | expand |
On 05/01/2023 15.53, Pierre Morel wrote: > The maximum nested topology entries is used by the guest to know > how many nested topology are available on the machine. > > Currently, SCLP READ SCP INFO reports MNEST = 0, which is the > equivalent of reporting the default value of 2. > Let's use the default SCLP value of 2 and increase this value in the > future patches implementing higher levels. I'm confused ... so does a SCLP value of 2 mean a MNEST level of 4 ? > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > include/hw/s390x/sclp.h | 5 +++-- > hw/s390x/sclp.c | 4 ++++ > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h > index 712fd68123..4ce852473c 100644 > --- a/include/hw/s390x/sclp.h > +++ b/include/hw/s390x/sclp.h > @@ -112,12 +112,13 @@ typedef struct CPUEntry { > } QEMU_PACKED CPUEntry; > > #define SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET 128 > -#define SCLP_READ_SCP_INFO_MNEST 2 > +#define SCLP_READ_SCP_INFO_MNEST 4 ... since you update it to 4 here. > typedef struct ReadInfo { > SCCBHeader h; > uint16_t rnmax; > uint8_t rnsize; > - uint8_t _reserved1[16 - 11]; /* 11-15 */ > + uint8_t _reserved1[15 - 11]; /* 11-14 */ > + uint8_t stsi_parm; /* 15-16 */ > uint16_t entries_cpu; /* 16-17 */ > uint16_t offset_cpu; /* 18-19 */ > uint8_t _reserved2[24 - 20]; /* 20-23 */ > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c > index eff74479f4..07e3cb4cac 100644 > --- a/hw/s390x/sclp.c > +++ b/hw/s390x/sclp.c > @@ -20,6 +20,7 @@ > #include "hw/s390x/event-facility.h" > #include "hw/s390x/s390-pci-bus.h" > #include "hw/s390x/ipl.h" > +#include "hw/s390x/cpu-topology.h" > > static inline SCLPDevice *get_sclp_device(void) > { > @@ -125,6 +126,9 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) > > /* CPU information */ > prepare_cpu_entries(machine, entries_start, &cpu_count); > + if (s390_has_topology()) { > + read_info->stsi_parm = SCLP_READ_SCP_INFO_MNEST; This seems to be in contradiction to what you've said in the commit description - you set it to 4 and not to 2. Thomas > + } > read_info->entries_cpu = cpu_to_be16(cpu_count); > read_info->offset_cpu = cpu_to_be16(offset_cpu); > read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
On Thu, 2023-01-05 at 15:53 +0100, Pierre Morel wrote: > The maximum nested topology entries is used by the guest to know > how many nested topology are available on the machine. > > Currently, SCLP READ SCP INFO reports MNEST = 0, which is the > equivalent of reporting the default value of 2. > Let's use the default SCLP value of 2 and increase this value in the > future patches implementing higher levels. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> if you address the issues Thomas found with the commit description and the nits below. > --- > include/hw/s390x/sclp.h | 5 +++-- > hw/s390x/sclp.c | 4 ++++ > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h > index 712fd68123..4ce852473c 100644 > --- a/include/hw/s390x/sclp.h > +++ b/include/hw/s390x/sclp.h > @@ -112,12 +112,13 @@ typedef struct CPUEntry { > } QEMU_PACKED CPUEntry; > > #define SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET 128 > -#define SCLP_READ_SCP_INFO_MNEST 2 > +#define SCLP_READ_SCP_INFO_MNEST 4 > typedef struct ReadInfo { > SCCBHeader h; > uint16_t rnmax; > uint8_t rnsize; > - uint8_t _reserved1[16 - 11]; /* 11-15 */ > + uint8_t _reserved1[15 - 11]; /* 11-14 */ > + uint8_t stsi_parm; /* 15-16 */ The numbering here is the same as the one in the arch doc, instead of the maybe more usual one where the right number is exclusive. So 15-16 looks like a two byte field, so just do 15 or just drop it. > uint16_t entries_cpu; /* 16-17 */ > uint16_t offset_cpu; /* 18-19 */ > uint8_t _reserved2[24 - 20]; /* 20-23 */ > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c > index eff74479f4..07e3cb4cac 100644 > --- a/hw/s390x/sclp.c > +++ b/hw/s390x/sclp.c > @@ -20,6 +20,7 @@ > #include "hw/s390x/event-facility.h" > #include "hw/s390x/s390-pci-bus.h" > #include "hw/s390x/ipl.h" > +#include "hw/s390x/cpu-topology.h" > > static inline SCLPDevice *get_sclp_device(void) > { > @@ -125,6 +126,9 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) > > /* CPU information */ > prepare_cpu_entries(machine, entries_start, &cpu_count); > + if (s390_has_topology()) { > + read_info->stsi_parm = SCLP_READ_SCP_INFO_MNEST; > + } Maybe move that up a bit, not sure if it belongs under the CPU information section. I'd leave prepare_cpu_entries and read_info->entries_cpu = adjacent in any case. > read_info->entries_cpu = cpu_to_be16(cpu_count); > read_info->offset_cpu = cpu_to_be16(offset_cpu); > read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
On 1/11/23 09:57, Thomas Huth wrote: > On 05/01/2023 15.53, Pierre Morel wrote: >> The maximum nested topology entries is used by the guest to know >> how many nested topology are available on the machine. >> >> Currently, SCLP READ SCP INFO reports MNEST = 0, which is the >> equivalent of reporting the default value of 2. >> Let's use the default SCLP value of 2 and increase this value in the >> future patches implementing higher levels. > > I'm confused ... so does a SCLP value of 2 mean a MNEST level of 4 ? Sorry, I forgot to change this. MNEST = 0 means no MNEST support and only socket is supported so it is like MNEST = 2. MNEST != 0 set the maximum nested level and correct values may be 2,3 or 4. But this setting to 4 should already have been done in previous patch where we introduced the books and drawers. I change the commit message with: --- s390x/sclp: reporting the maximum nested topology entries The maximum nested topology entries is used by the guest to know how many nested topology are available on the machine. Let's return this information to the guest. --- > >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> include/hw/s390x/sclp.h | 5 +++-- >> hw/s390x/sclp.c | 4 ++++ >> 2 files changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h >> index 712fd68123..4ce852473c 100644 >> --- a/include/hw/s390x/sclp.h >> +++ b/include/hw/s390x/sclp.h >> @@ -112,12 +112,13 @@ typedef struct CPUEntry { >> } QEMU_PACKED CPUEntry; >> #define SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET 128 >> -#define SCLP_READ_SCP_INFO_MNEST 2 >> +#define SCLP_READ_SCP_INFO_MNEST 4 > > ... since you update it to 4 here. Yes, in fact this should be set in the previous patch already to 4. So I will do that. > >> typedef struct ReadInfo { >> SCCBHeader h; >> uint16_t rnmax; >> uint8_t rnsize; >> - uint8_t _reserved1[16 - 11]; /* 11-15 */ >> + uint8_t _reserved1[15 - 11]; /* 11-14 */ >> + uint8_t stsi_parm; /* 15-16 */ >> uint16_t entries_cpu; /* 16-17 */ >> uint16_t offset_cpu; /* 18-19 */ >> uint8_t _reserved2[24 - 20]; /* 20-23 */ >> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >> index eff74479f4..07e3cb4cac 100644 >> --- a/hw/s390x/sclp.c >> +++ b/hw/s390x/sclp.c >> @@ -20,6 +20,7 @@ >> #include "hw/s390x/event-facility.h" >> #include "hw/s390x/s390-pci-bus.h" >> #include "hw/s390x/ipl.h" >> +#include "hw/s390x/cpu-topology.h" >> static inline SCLPDevice *get_sclp_device(void) >> { >> @@ -125,6 +126,9 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB >> *sccb) >> /* CPU information */ >> prepare_cpu_entries(machine, entries_start, &cpu_count); >> + if (s390_has_topology()) { >> + read_info->stsi_parm = SCLP_READ_SCP_INFO_MNEST; > > This seems to be in contradiction to what you've said in the commit > description - you set it to 4 and not to 2. Yes, I change the commit message. Thanks. Regards, Pierre
On 1/11/23 18:52, Nina Schoetterl-Glausch wrote: > On Thu, 2023-01-05 at 15:53 +0100, Pierre Morel wrote: >> The maximum nested topology entries is used by the guest to know >> how many nested topology are available on the machine. >> >> Currently, SCLP READ SCP INFO reports MNEST = 0, which is the >> equivalent of reporting the default value of 2. >> Let's use the default SCLP value of 2 and increase this value in the >> future patches implementing higher levels. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > > Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > if you address the issues Thomas found with the commit description > and the nits below. Thanks. > >> --- >> include/hw/s390x/sclp.h | 5 +++-- >> hw/s390x/sclp.c | 4 ++++ >> 2 files changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h >> index 712fd68123..4ce852473c 100644 >> --- a/include/hw/s390x/sclp.h >> +++ b/include/hw/s390x/sclp.h >> @@ -112,12 +112,13 @@ typedef struct CPUEntry { >> } QEMU_PACKED CPUEntry; >> >> #define SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET 128 >> -#define SCLP_READ_SCP_INFO_MNEST 2 >> +#define SCLP_READ_SCP_INFO_MNEST 4 >> typedef struct ReadInfo { >> SCCBHeader h; >> uint16_t rnmax; >> uint8_t rnsize; >> - uint8_t _reserved1[16 - 11]; /* 11-15 */ >> + uint8_t _reserved1[15 - 11]; /* 11-14 */ >> + uint8_t stsi_parm; /* 15-16 */ > > The numbering here is the same as the one in the arch doc, instead > of the maybe more usual one where the right number is exclusive. > So 15-16 looks like a two byte field, so just do 15 or just drop it. Yes right, thanks. > >> uint16_t entries_cpu; /* 16-17 */ >> uint16_t offset_cpu; /* 18-19 */ >> uint8_t _reserved2[24 - 20]; /* 20-23 */ >> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >> index eff74479f4..07e3cb4cac 100644 >> --- a/hw/s390x/sclp.c >> +++ b/hw/s390x/sclp.c >> @@ -20,6 +20,7 @@ >> #include "hw/s390x/event-facility.h" >> #include "hw/s390x/s390-pci-bus.h" >> #include "hw/s390x/ipl.h" >> +#include "hw/s390x/cpu-topology.h" >> >> static inline SCLPDevice *get_sclp_device(void) >> { >> @@ -125,6 +126,9 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) >> >> /* CPU information */ >> prepare_cpu_entries(machine, entries_start, &cpu_count); >> + if (s390_has_topology()) { >> + read_info->stsi_parm = SCLP_READ_SCP_INFO_MNEST; >> + } > > Maybe move that up a bit, not sure if it belongs under the CPU information section. > I'd leave prepare_cpu_entries and read_info->entries_cpu = adjacent in any case. > Yes, I do that. Thanks, Regards, Pierre
On Tue, 2023-01-17 at 18:36 +0100, Pierre Morel wrote: > > On 1/11/23 09:57, Thomas Huth wrote: > > On 05/01/2023 15.53, Pierre Morel wrote: > > > The maximum nested topology entries is used by the guest to know > > > how many nested topology are available on the machine. > > > > > > Currently, SCLP READ SCP INFO reports MNEST = 0, which is the > > > equivalent of reporting the default value of 2. > > > Let's use the default SCLP value of 2 and increase this value in the > > > future patches implementing higher levels. > > > > I'm confused ... so does a SCLP value of 2 mean a MNEST level of 4 ? > > Sorry, I forgot to change this. > MNEST = 0 means no MNEST support and only socket is supported so it is > like MNEST = 2. > MNEST != 0 set the maximum nested level and correct values may be 2,3 or 4. > But this setting to 4 should already have been done in previous patch > where we introduced the books and drawers. I think setting it to 4 here is fine/preferable, since 2 is the default unless you tell the guest that more are available, which you do in this patch. It's only the commit description that is confusing. > > I change the commit message with: > --- > s390x/sclp: reporting the maximum nested topology entries > > The maximum nested topology entries is used by the guest to know > how many nested topology are available on the machine. > > Let's return this information to the guest. > --- > > > > > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > > > --- > > > include/hw/s390x/sclp.h | 5 +++-- > > > hw/s390x/sclp.c | 4 ++++ > > > 2 files changed, 7 insertions(+), 2 deletions(-) > > > > > > diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h > > > index 712fd68123..4ce852473c 100644 > > > --- a/include/hw/s390x/sclp.h > > > +++ b/include/hw/s390x/sclp.h > > > @@ -112,12 +112,13 @@ typedef struct CPUEntry { > > > } QEMU_PACKED CPUEntry; > > > #define SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET 128 > > > -#define SCLP_READ_SCP_INFO_MNEST 2 > > > +#define SCLP_READ_SCP_INFO_MNEST 4 > > > > ... since you update it to 4 here. > > Yes, in fact this should be set in the previous patch already to 4. > So I will do that. > > > > > > typedef struct ReadInfo { > > > SCCBHeader h; > > > uint16_t rnmax; > > > uint8_t rnsize; > > > - uint8_t _reserved1[16 - 11]; /* 11-15 */ > > > + uint8_t _reserved1[15 - 11]; /* 11-14 */ > > > + uint8_t stsi_parm; /* 15-16 */ > > > uint16_t entries_cpu; /* 16-17 */ > > > uint16_t offset_cpu; /* 18-19 */ > > > uint8_t _reserved2[24 - 20]; /* 20-23 */ > > > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c > > > index eff74479f4..07e3cb4cac 100644 > > > --- a/hw/s390x/sclp.c > > > +++ b/hw/s390x/sclp.c > > > @@ -20,6 +20,7 @@ > > > #include "hw/s390x/event-facility.h" > > > #include "hw/s390x/s390-pci-bus.h" > > > #include "hw/s390x/ipl.h" > > > +#include "hw/s390x/cpu-topology.h" > > > static inline SCLPDevice *get_sclp_device(void) > > > { > > > @@ -125,6 +126,9 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB > > > *sccb) > > > /* CPU information */ > > > prepare_cpu_entries(machine, entries_start, &cpu_count); > > > + if (s390_has_topology()) { > > > + read_info->stsi_parm = SCLP_READ_SCP_INFO_MNEST; > > > > This seems to be in contradiction to what you've said in the commit > > description - you set it to 4 and not to 2. > > Yes, I change the commit message. > > Thanks. > > Regards, > Pierre >
On 1/17/23 20:58, Nina Schoetterl-Glausch wrote: > On Tue, 2023-01-17 at 18:36 +0100, Pierre Morel wrote: >> >> On 1/11/23 09:57, Thomas Huth wrote: >>> On 05/01/2023 15.53, Pierre Morel wrote: >>>> The maximum nested topology entries is used by the guest to know >>>> how many nested topology are available on the machine. >>>> >>>> Currently, SCLP READ SCP INFO reports MNEST = 0, which is the >>>> equivalent of reporting the default value of 2. >>>> Let's use the default SCLP value of 2 and increase this value in the >>>> future patches implementing higher levels. >>> >>> I'm confused ... so does a SCLP value of 2 mean a MNEST level of 4 ? >> >> Sorry, I forgot to change this. >> MNEST = 0 means no MNEST support and only socket is supported so it is >> like MNEST = 2. >> MNEST != 0 set the maximum nested level and correct values may be 2,3 or 4. >> But this setting to 4 should already have been done in previous patch >> where we introduced the books and drawers. > > I think setting it to 4 here is fine/preferable, since 2 is the default unless > you tell the guest that more are available, which you do in this patch. > It's only the commit description that is confusing. Yes. Only the commit message is confusing, it is to be set on 4 in this patch. I change the commit message to: --- s390x/sclp: reporting the maximum nested topology entries The maximum nested topology entries is used by the guest to know how many nested topology are available on the machine. Let change the MNEST value from 2 to 4 in the SCLP READ INFO structure now that we support books and drawers. --- is it OK ? Regards, Pierre > >> >> I change the commit message with: >> --- >> s390x/sclp: reporting the maximum nested topology entries >> >> The maximum nested topology entries is used by the guest to know >> how many nested topology are available on the machine. >> >> Let's return this information to the guest. >> --- >> >>> >>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>> --- >>>> include/hw/s390x/sclp.h | 5 +++-- >>>> hw/s390x/sclp.c | 4 ++++ >>>> 2 files changed, 7 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h >>>> index 712fd68123..4ce852473c 100644 >>>> --- a/include/hw/s390x/sclp.h >>>> +++ b/include/hw/s390x/sclp.h >>>> @@ -112,12 +112,13 @@ typedef struct CPUEntry { >>>> } QEMU_PACKED CPUEntry; >>>> #define SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET 128 >>>> -#define SCLP_READ_SCP_INFO_MNEST 2 >>>> +#define SCLP_READ_SCP_INFO_MNEST 4 >>> >>> ... since you update it to 4 here. >> >> Yes, in fact this should be set in the previous patch already to 4. >> So I will do that. >> >>> >>>> typedef struct ReadInfo { >>>> SCCBHeader h; >>>> uint16_t rnmax; >>>> uint8_t rnsize; >>>> - uint8_t _reserved1[16 - 11]; /* 11-15 */ >>>> + uint8_t _reserved1[15 - 11]; /* 11-14 */ >>>> + uint8_t stsi_parm; /* 15-16 */ >>>> uint16_t entries_cpu; /* 16-17 */ >>>> uint16_t offset_cpu; /* 18-19 */ >>>> uint8_t _reserved2[24 - 20]; /* 20-23 */ >>>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >>>> index eff74479f4..07e3cb4cac 100644 >>>> --- a/hw/s390x/sclp.c >>>> +++ b/hw/s390x/sclp.c >>>> @@ -20,6 +20,7 @@ >>>> #include "hw/s390x/event-facility.h" >>>> #include "hw/s390x/s390-pci-bus.h" >>>> #include "hw/s390x/ipl.h" >>>> +#include "hw/s390x/cpu-topology.h" >>>> static inline SCLPDevice *get_sclp_device(void) >>>> { >>>> @@ -125,6 +126,9 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB >>>> *sccb) >>>> /* CPU information */ >>>> prepare_cpu_entries(machine, entries_start, &cpu_count); >>>> + if (s390_has_topology()) { >>>> + read_info->stsi_parm = SCLP_READ_SCP_INFO_MNEST; >>> >>> This seems to be in contradiction to what you've said in the commit >>> description - you set it to 4 and not to 2. >> >> Yes, I change the commit message. >> >> Thanks. >> >> Regards, >> Pierre >> >
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h index 712fd68123..4ce852473c 100644 --- a/include/hw/s390x/sclp.h +++ b/include/hw/s390x/sclp.h @@ -112,12 +112,13 @@ typedef struct CPUEntry { } QEMU_PACKED CPUEntry; #define SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET 128 -#define SCLP_READ_SCP_INFO_MNEST 2 +#define SCLP_READ_SCP_INFO_MNEST 4 typedef struct ReadInfo { SCCBHeader h; uint16_t rnmax; uint8_t rnsize; - uint8_t _reserved1[16 - 11]; /* 11-15 */ + uint8_t _reserved1[15 - 11]; /* 11-14 */ + uint8_t stsi_parm; /* 15-16 */ uint16_t entries_cpu; /* 16-17 */ uint16_t offset_cpu; /* 18-19 */ uint8_t _reserved2[24 - 20]; /* 20-23 */ diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c index eff74479f4..07e3cb4cac 100644 --- a/hw/s390x/sclp.c +++ b/hw/s390x/sclp.c @@ -20,6 +20,7 @@ #include "hw/s390x/event-facility.h" #include "hw/s390x/s390-pci-bus.h" #include "hw/s390x/ipl.h" +#include "hw/s390x/cpu-topology.h" static inline SCLPDevice *get_sclp_device(void) { @@ -125,6 +126,9 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) /* CPU information */ prepare_cpu_entries(machine, entries_start, &cpu_count); + if (s390_has_topology()) { + read_info->stsi_parm = SCLP_READ_SCP_INFO_MNEST; + } read_info->entries_cpu = cpu_to_be16(cpu_count); read_info->offset_cpu = cpu_to_be16(offset_cpu); read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
The maximum nested topology entries is used by the guest to know how many nested topology are available on the machine. Currently, SCLP READ SCP INFO reports MNEST = 0, which is the equivalent of reporting the default value of 2. Let's use the default SCLP value of 2 and increase this value in the future patches implementing higher levels. Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> --- include/hw/s390x/sclp.h | 5 +++-- hw/s390x/sclp.c | 4 ++++ 2 files changed, 7 insertions(+), 2 deletions(-)