Re: [kvm-unit-tests PATCH v2 1/6] s390x: add function to set DAT mode for all interrupts

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

On Wed, 17 May 2023 14:25:01 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> Quoting Claudio Imbrenda (2023-05-16 19:17:24)
> [...]
> > > diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> > > index 3f993a363ae2..1180ec44d72f 100644
> > > --- a/lib/s390x/interrupt.c
> > > +++ b/lib/s390x/interrupt.c  
> [...]
> > > +void irq_set_dat_mode(bool dat, uint64_t as)
> > > +{  
> [...]
> > > +     for (struct psw *irq_psw = irq_psws[0]; irq_psw != NULL; irq_psw++) {  
> > 
> > just call it psw, or cur_psw, it's a little confusing otherwise  
> 
> will do. 
> 
> [...]
> > alternatively, you can redefine psw with a bitfield (as you mentioned
> > offline):
> > 
> > cur_psw->mask.dat = dat;
> > if (dat)
> >         cur_psw->mask.as = as;  
> 
> Yep, I'll go with that.
> 
> >   
> > > +             else
> > > +                     irq_psw->mask |= PSW_MASK_DAT | as << (63 - 16);  
> > 
> > otherwise here you're ORing stuff to other stuff, if you had 3 and you
> > OR 0 you get 3, but you actually want 0  
> 
> And that's the advantage of the bitfield. :)
> 
> >   
> > > +     }
> > > +
> > > +     mb();  
> > 
> > what's the purpose of this?  
> 
> Make sure that the lowcore really has been written, but I think it's quite
> useless, since a function is a sequence point, right?

yes