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

[Pierre Morel]

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

--
Pierre Morel
IBM Lab Boeblingen