Re: [PATCH v14 04/11] s390x/sclp: reporting the maximum nested topology entries

[Nina Schoetterl-Glausch]

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
>