Re: [Qemu-devel] [PATCH v2 2/8] s390x/css: IO instr handler ending control

[Cornelia Huck]

On Mon, 9 Oct 2017 12:54:03 +0200
Halil Pasic <address@hidden> wrote:

> On 10/09/2017 10:20 AM, Thomas Huth wrote:
> > On 04.10.2017 17:41, Halil Pasic wrote:  

> >> +/* IO instructions conclude according this */
> >> +typedef struct IOInstEnding {
> >> +        /*
> >> +         * General semantic of cc codes of IO instructions is (brief):
> >> +         * 0 -- produced expected result
> >> +         * 1 --  status conditions were present or produced alternate 
> >> result
> >> +         * 2 -- ineffective, because busy with previously initiated 
> >> function
> >> +         * 3 -- ineffective, not operational
> >> +         */
> >> +        int cc;
> >> +} IOInstEnding;  
> > 
> > Why do you need a struct for this? Do you plan to extend it later? If
> > so, I think you should mention that in the patch description. If not,
> > please use a named enum or a "typedef unsigned int IOInstEnding" instead.
> > 
> >  Thomas  
> 
> We may, we may not. In the previous version we also had to support
> do end a certain instruction with an addressing exception, but this
> is going away in patch #3. Honestly I don't expect this being extended.
> 
> I have other reasons for the struct. Type safety and clear semantics,
> and frankly at least for s390 and linux I don't see any downsides given
> what is written in the "zSeries ELF Application Binary Interface Supplement".
> Can you please explain to me what is the problem with using this struct, and
> what is the benefit switching to a unsigned int?

Honestly, I fail to see the benefit of using a struct here... it's just
a condition code, and while adding a comment what the various codes
mean for I/O instructions is a good idea, I think having to use a
IOInstEnding struct just renders the code less readable.

[I haven't had time to look at the rest of the patches yet.]