Re: [kvm-unit-tests PATCH v1 2/2] s390x: add cmm migration test

Nico Boehr
2022-05-10T13:35:40Z

On Mon, 2022-05-09 at 15:58 +0200, Claudio Imbrenda wrote:
> > +       for (i = 0; i < NUM_PAGES; i++) {
> > +               switch(i % 4) {
> > +                       case 0:
> > +                               essa(ESSA_SET_STABLE, (unsigned
> > long)pagebuf[i]);
> > +                       break;
> > +                       case 1:
> > +                               essa(ESSA_SET_UNUSED, (unsigned
> > long)pagebuf[i]);
> > +                       break;
> > +                       case 2:
> > +                               essa(ESSA_SET_VOLATILE, (unsigned
> > long)pagebuf[i]);
> > +                       break;
> > +                       case 3:
> > +                               essa(ESSA_SET_POT_VOLATILE,
> > (unsigned long)pagebuf[i]);
> > +                       break;
>
> const int essa_commands[4] = {ESSA_SET_STABLE, ESSA_SET_UNUSED, ...
>
> for (i = 0; i < NUM_PAGES; i++)
>         essa(essa_commands[i % 4], ...
>
> I think it would look more compact and more readable

I like your idea a lot, but the compiler doesn't :-(:

/home/nrb/kvm-unit-tests/lib/asm/cmm.h: In function ‘main’:
/home/nrb/kvm-unit-tests/lib/asm/cmm.h:32:9: error: ‘asm’ operand 2
probably does not match constraints [-Werror]
32 | asm volatile(".insn
rrf,0xb9ab0000,%[extr_state],%[addr],%[new_state],0" \
| ^~~
/home/nrb/kvm-unit-tests/lib/asm/cmm.h:32:9: error: impossible
constraint in ‘asm’

To satify the "i" constraint, new_state needs to be a compile time
constant, which it won't be any more with your suggestion
unfortunately.

We can do crazy stuff like this in cmm.h:

#define __essa(state) \
asm volatile(".insn
rrf,0xb9ab0000,%[extr_state],%[addr],%[new_state],0" \
: [extr_state] "=d" (extr_state) \
: [addr] "a" (paddr), [new_state] "i"
(state));
static unsigned long essa(uint8_t state, unsigned long paddr)
{
uint64_t extr_state = 0;

switch(state) {
case ESSA_SET_STABLE:
__essa(ESSA_SET_STABLE);
break;
case ESSA_SET_UNUSED:
__essa(ESSA_SET_UNUSED);
break;
case ESSA_SET_VOLATILE:
__essa(ESSA_SET_VOLATILE);
break;
case ESSA_SET_POT_VOLATILE:
__essa(ESSA_SET_POT_VOLATILE);
break;
}

return (unsigned long)extr_state;
}

But that essentially just shifts the readability problem to a different
file. What do you think?

Or we make essa a marco, which doesn't sound like a particularily
attractive alternative either.