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

[Halil Pasic]

On 10/09/2017 01:09 PM, Cornelia Huck wrote:
> 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


Type safety. For instance had I forgotten let's say a return -ENODEV
somewhere, my version would not compile. On the contrary with an enum (like
Thomas has proposed) it compiles just fine with my setup -- I did not try
what would happen if we call setcc(cpu, -ENODEV)

> a condition code, and while adding a comment what the various codes

Right, but it's a different _type_ of condition code (than the POSIX errno
codes for instance) so that's why it's modeled as a different type.

> mean for I/O instructions is a good idea, I think having to use a
> IOInstEnding struct just renders the code less readable.

It's probably a matter of taste. I find, for example
return (IOInstEnding){.cc = 1}
in this case more readable than
return -ENOSYS
because from the first one I know we have detected a condition
which we want to handle by setting cc 1 for the instruction. The
later requires me figuring out in the context of which instruction
handler am I called, go trough the whole call chain looking for
possible re-mappings (like for -EACCES) and finally examining the
switch statement of the instruction handler carefully.

Could you please tell me what exactly is difficult to read for
you (form what I've proposed).

Regards,
Halil


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