On Thu, 24 Mar 2022 08:39:29 +0100
Nico Boehr <nrb@xxxxxxxxxxxxx> wrote:
> On Wed, 2022-03-23 at 18:45 +0100, Claudio Imbrenda wrote:
> > On Wed, 23 Mar 2022 18:03:20 +0100
> > Nico Boehr <nrb@xxxxxxxxxxxxx> 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"
hmm, it seems like that without guarded storage LC is ignored, and the
size is hardcoded to 1024.
this is getting a little out of hand now
I think you should make this into a separate test
>
> 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.
it seems indeed so. maybe add a comment to explain
>
