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@xxxxxxxxxxxxx>
> > > > ---
> > > > 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
>
