Re: [kvm-unit-tests PATCH v5 1/2] s390x: Add specification exception test

[Janosch Frank]

On 8/26/22 13:23, Janis Schoetterl-Glausch wrote:
> On Wed, 2022-08-24 at 11:35 +0200, Janosch Frank wrote:
> > On 7/20/22 16:25, Janis Schoetterl-Glausch wrote:
> > > Generate specification exceptions and check that they occur.
> > >
> > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> > > ---
> > >    s390x/Makefile           |   1 +
> > >    lib/s390x/asm/arch_def.h |   5 ++
> > >    s390x/spec_ex.c          | 180 +++++++++++++++++++++++++++++++++++++++
> > >    s390x/unittests.cfg      |   3 +
> > >    4 files changed, 189 insertions(+)
> > >    create mode 100644 s390x/spec_ex.c
> > >
> > >
> > > +
> > > +/*
> > > + * Load possibly invalid psw, but setup fixup_psw before,
> > > + * so that fixup_invalid_psw() can bring us back onto the right track.

Not sure if the second line is needed as fixup_psw is a descriptive name 
already.

> > > + * Also acts as compiler barrier, -> none required in expect/check_invalid_psw
> > > + */
> > > +static void load_psw(struct psw psw)
> > > +{
> > > +       uint64_t scratch;
> > > +
>
> [...]
>
> > /*
> > Store a valid mask and the address of the nop into the fixup PSW.
> > Then load the possibly invalid PSW.
> > */
>
> This seems a bit redundant given the function comment, but I can
> drop a comment in here describing how the fixup psw is computed.

Well, I skipped the function comment, got confused by the addr asm 
variable and then decided to propose the comment.

It's a bit confusing since you have the invalid PSW and the global fixup 
PSW in one function.

Maybe something like:
/* From here to the lpswe we're computing and setting the fixup PSW */

> > > +       fixup_psw.mask = extract_psw_mask();
> > > +       asm volatile ( "larl       %[scratch],0f\n"
> > > +               "  stg     %[scratch],%[addr]\n"
> > > +               "  lpswe   %[psw]\n"
> > > +               "0:        nop\n"
> > > +               : [scratch] "=&d"(scratch),
> > > +                 [addr] "=&T"(fixup_psw.addr)
> >
> > s/addr/psw_addr/ ?
> >
> > > +               : [psw] "Q"(psw)
> > > +               : "cc", "memory"
> > > +       );
> > > +}
> > > +
> > > +static void load_short_psw(struct short_psw psw)
> > > +{
> > > +       uint64_t scratch;
> > > +
> > > +       fixup_psw.mask = extract_psw_mask();
> > > +       asm volatile ( "larl       %[scratch],0f\n"
> > > +               "  stg     %[scratch],%[addr]\n"
> > > +               "  lpsw    %[psw]\n"
> > > +               "0:        nop\n"
> > > +               : [scratch] "=&d"(scratch),
> > > +                 [addr] "=&T"(fixup_psw.addr)
> > > +               : [psw] "Q"(psw)
> > > +               : "cc", "memory"
> > > +       );
> >
> > Same story.
>
> Do you want me to repeat the comments here or just rename addr?

Just rename addr

> [...]
>
> > > +static int not_even(void)
> > > +{
> > > +       uint64_t quad[2] __attribute__((aligned(16))) = {0};
> > > +
> > > +       asm volatile (".insn       rxy,0xe3000000008f,%%r7,%[quad]" /* lpq 
> > > %%r7,%[quad] */
> > > +                     : : [quad] "T"(quad)
> >
> > Is there a reason you never put a space after the constraint?
>
> TBH I never noticed I'm unusual in that regard. I guess I tend to think
> of the operand and constraint as one entity.
> I'll add the spaces.
>
> > > +                     : "%r7", "%r8"
> > > +       );
> > > +       return 0;
> > > +}
> > > +
>
> [...]