Re: [PATCH v15 03/11] target/s390x/cpu topology: handle STSI(15) and build the SYSIB

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

On Mon, 2023-02-06 at 11:06 +0100, Pierre Morel wrote:
> 
> On 2/3/23 18:36, Nina Schoetterl-Glausch wrote:
> > On Wed, 2023-02-01 at 14:20 +0100, Pierre Morel wrote:
> > > > On interception of STSI(15.1.x) the System Information Block
> > > > (SYSIB) is built from the list of pre-ordered topology entries.
> > > > 
> > > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > > > ---
> > > >   include/hw/s390x/cpu-topology.h |  22 +++
> > > >   include/hw/s390x/sclp.h         |   1 +
> > > >   target/s390x/cpu.h              |  72 +++++++
> > > >   hw/s390x/cpu-topology.c         |  10 +
> > > >   target/s390x/kvm/cpu_topology.c | 335 ++++++++++++++++++++++++++++++++
> > > >   target/s390x/kvm/kvm.c          |   5 +-
> > > >   target/s390x/kvm/meson.build    |   3 +-
> > > >   7 files changed, 446 insertions(+), 2 deletions(-)
> > > >   create mode 100644 target/s390x/kvm/cpu_topology.c
> > > > 
[...]
> > 
> > > > + */
> > > > +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
> > > > +{
> > > > +    SysIB sysib = {0};
> > > > +    int len;
> > > > +
> > > > +    if (!s390_has_topology() || sel2 < 2 || sel2 > SCLP_READ_SCP_INFO_MNEST) {
> > > > +        setcc(cpu, 3);
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    s390_order_tle();
> > > > +
> > > > +    len = setup_stsi(&sysib.sysib_151x, sel2);
> > > > +
> > > > +    if (len < 0) {
> > 
> > I stumbled a bit over this, maybe rename len to r.
> 
> Why ? it is the length used to fill the length field of the SYSIB.

Well a negative length doesn't make sense, that's what confused me for a bit.
It's the error value of course, I proposed renaming it to the more generic r return value,
to signify that the return value isn't exactly the length.

You're solution of using 0 is fine with me, too.
> 
> May be it would be clearer if we give back the length to write and 0 on 
> error then we have here:
> 
> 	if (!len) {
> 		setcc(cpu, 3);
> 		return;
> 	}
> 
> > 
> > > > +        setcc(cpu, 3);
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    sysib.sysib_151x.length = cpu_to_be16(len);
> > > > +    s390_cpu_virt_mem_write(cpu, addr, ar, &sysib, len);
> > > > +    setcc(cpu, 0);
> > > > +
> > > > +    s390_free_tle();
> > > > +}
> 
> Thanks for the comments.
> If there are only comments and cosmetic changes will I get your RB if I 
> change them according to your wishes?

Usually I review the whole series and then comment, this time sent feedback early,
so the review isn't as deep. Probably easiest to give you the R-b for v16.
My impression is that the series is close to final.
> 
> Regards,
> Pierre
>