Re: [kvm-unit-tests PATCH v2 4/9] s390x: smp: add test for SIGP_STORE_ADTL_STATUS order

Nico Boehr
2022-03-24T07:39:39Z

On Wed, 2022-03-23 at 18:45 +0100, Claudio Imbrenda wrote:
> On Wed, 23 Mar 2022 18:03:20 +0100
> Nico Boehr <nrb@linux.ibm.com> wrote:
>
[...]
> > +
> > +static int memisset(void *s, int c, size_t n)
>
> function should return bool..

Sure, changed.

[...]
> > +static void test_store_adtl_status(void)
> > +{
> >
[...]
> > +
> > +       report_prefix_push("unaligned");
> > +       smp_cpu_stop(1);
> > +
> > +       cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
> > +                 (unsigned long)&adtl_status + 256, &status);
> > +       report(cc == 1, "CC = 1");
> > +       report(status == SIGP_STATUS_INVALID_PARAMETER, "status =
> > INVALID_PARAMETER");
>
> and check again that nothing has been written to

Oh, thanks. Fixed.

[...]
> > +static void test_store_adtl_status_unavail(void)
> > +{
> > +       uint32_t status = 0;
> > +       int cc;
> > +
> > +       report_prefix_push("store additional status unvailable");
> > +
> > +       if (have_adtl_status()) {
> > +               report_skip("guarded-storage or vector facility
> > installed");
> > +               goto out;
> > +       }
> > +
> > +       report_prefix_push("not accepted");
> > +       smp_cpu_stop(1);
> > +
> > +       cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
> > +                 (unsigned long)&adtl_status, &status);
> > +
> > +       report(cc == 1, "CC = 1");
> > +       report(status == SIGP_STATUS_INVALID_ORDER,
> > +              "status = INVALID_ORDER");
> > +
>
> I would still check that nothing is written even when the order is
> rejected

Won't hurt, added.

[...]
> > +static void restart_write_vector(void)
> > +{
> > +       uint8_t *vec_reg;
> > +       /*
> > +        * vlm handles at most 16 registers at a time
> > +        */
>
> this comment can /* go on a single line */

OK

[...]
> > +               /*
> > +                * i+1 to avoid zero content
> > +                */
>
> same /* here */

OK, changed.

[...]
> > +static void __store_adtl_status_vector_lc(unsigned long lc)
> > +{
> > +       uint32_t status = -1;
> > +       struct psw psw;
> > +       int cc;
> > +
> > +       report_prefix_pushf("LC %lu", lc);
> > +
> > +       if (!test_facility(133) && lc) {
> > +               report_skip("not supported, no guarded-storage
> > facility");
> > +               goto out;
> > +       }
>
> I think this ^ should not be there at all

It must be. If we don't have guarded-storage only LC 0 is allowed:

"When the guarded-storage facility is not installed, the
length and alignment of the MCESA is 1024 bytes.
When the guarded-storage facility is installed, the
length characteristic (LC) in bits 60-63 of the
MCESAD specifies the length and alignment of the
MCESA as a power of two"

See below for the reason why we don't have gs here.

[...]
> > diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> > index 1600e714c8b9..843fd323bce9 100644
> > --- a/s390x/unittests.cfg
> > +++ b/s390x/unittests.cfg
> > @@ -74,9 +74,29 @@ extra_params=-device diag288,id=watchdog0 --
> > watchdog-action inject-nmi
> >  file = stsi.elf
> >  extra_params=-name kvm-unit-test --uuid 0fb84a86-727c-11ea-bc55-
> > 0242ac130003 -smp 1,maxcpus=8
> >
> > -[smp]
> > +[smp-kvm]
> >  file = smp.elf
> >  smp = 2
> > +accel = kvm
> > +extra_params = -cpu host,gs=on,vx=on
> > +
> > +[smp-no-vec-no-gs-kvm]
> > +file = smp.elf
> > +smp = 2
> > +accel = kvm
> > +extra_params = -cpu host,gs=off,vx=off
> > +
> > +[smp-tcg]
> > +file = smp.elf
> > +smp = 2
> > +accel = tcg
> > +extra_params = -cpu qemu,vx=on
>
> why not gs=on as well?

I am not an expert in QEMU CPU model, but it seems to me TCG doesn't
support it.