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

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Thu, 24 Mar 2022 08:39:29 +0100
Nico Boehr <nrb@linux.ibm.com> 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@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"

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

>