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

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

On Tue, 10 May 2022 15:25:09 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> 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 :-(:

ouch, I had completely forgotten about that.

> 
> /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.

ok, next less ugly thing: unroll the loop

for (i = 0; i < NUM_PAGES; i += 4) {
	essa(ESSA_SET_STABLE, (unsigned long)pagebuf[i]);
	essa(ESSA_SET_UNUSED, (unsigned long)pagebuf[i + 1]);
	... etc
}

maybe assert(NUM_PAGES % 4 == 0) before that, just for good measure