Re: [PATCH v7 04/13] s390x: topology: implementating Store Topology System Information

[Pierre Morel]

On 5/24/22 13:08, Thomas Huth wrote:
> On 20/04/2022 13.57, Pierre Morel wrote:
> > The handling of STSI is enhanced with the interception of the
> > function code 15 for storing CPU topology.
> >
> > Using the objects built during the pluging of CPU, we build the
>
> s/pluging/plugging/

Yes

> > SYSIB 15_1_x structures.
> >
> > With this patch the maximum MNEST level is 2, this is also
> > the only level allowed and only SYSIB 15_1_2 will be built.
> >
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> ...
> > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> > index f6969b76c5..a617c943ff 100644
> > --- a/target/s390x/cpu.h
> > +++ b/target/s390x/cpu.h
> > @@ -889,4 +889,5 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
> >   #include "exec/cpu-all.h"
> > +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar);
> >   #endif
>
> Please keep an empty line before the #endif

OK

> > diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
> > new file mode 100644
> > index 0000000000..7f6db18829
> > --- /dev/null
> > +++ b/target/s390x/cpu_topology.c
> > @@ -0,0 +1,112 @@
> > +/*
> > + * QEMU S390x CPU Topology
> > + *
> > + * Copyright IBM Corp. 2021
>
> 2022 ?

Yes too

> > + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or 
> > (at
> > + * your option) any later version. See the COPYING file in the top-level
> > + * directory.
> > + */
> ...
> > +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
> > +{
> > +    const MachineState *machine = MACHINE(qdev_get_machine());
> > +    void *p;
> > +    int ret, cc;
> > +
> > +    /*
> > +     * Until the SCLP STSI Facility reporting the MNEST value is used,
> > +     * a sel2 value of 2 is the only value allowed in STSI 15.1.x.
> > +     */
> > +    if (sel2 != 2) {
> > +        setcc(cpu, 3);
> > +        return;
> > +    }
> > +
> > +    p = g_malloc0(TARGET_PAGE_SIZE);
> > +
> > +    setup_stsi(machine, p, 2);
> > +
> > +    if (s390_is_pv()) {
> > +        ret = s390_cpu_pv_mem_write(cpu, 0, p, TARGET_PAGE_SIZE);
> > +    } else {
> > +        ret = s390_cpu_virt_mem_write(cpu, addr, ar, p, 
> > TARGET_PAGE_SIZE);
> > +    }
> > +    cc = ret ? 3 : 0;
> > +    setcc(cpu, cc);
>
> Just a matter of taste (i.e. keep it if you like) - but you could 
> scratch the cc variable in this function by just doing:
>
>      setcc(cpu, ret ? 3 : 0);

OK, I will changes all occurences

Thanks,
Pierre

> > +    g_free(p);
> > +}
> > +
>
>   Thomas

--
Pierre Morel
IBM Lab Boeblingen