[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/8] s390x/css: IO instr handler ending contr
From: |
Thomas Huth |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/8] s390x/css: IO instr handler ending control |
Date: |
Mon, 9 Oct 2017 13:07:27 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
On 09.10.2017 12:54, Halil Pasic wrote:
>
>
> On 10/09/2017 10:20 AM, Thomas Huth wrote:
>> On 04.10.2017 17:41, Halil Pasic wrote:
>>> CSS code needs to tell the IO instruction handlers located in how should
>>
>> located in how?
>>
>
> First, thanks for your review!
>
> Wanted to say: in target/s390x/ioinst.c just forgot to copy paste.
>
>>> the emulated instruction be ended. Currently this is done by returning
>>> generic (POSIX) error codes, and mapping them to outcomes like condition
>>> codes. This makes bugs easy to create and hard to recognise.
>>>
>>> As a preparation for moving a way form (mis)using generic error codes for
>>> flow control let us introduce a struct which tells the instruction
>>> handler function how to end the instruction, in a more straight-forward
>>> and less ambiguous way.
>>>
>>> Signed-off-by: Halil Pasic <address@hidden>
>>> ---
>>> include/hw/s390x/css.h | 12 ++++++++++++
>>> 1 file changed, 12 insertions(+)
>>>
>>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
>>> index 0653d3c9be..66916b6546 100644
>>> --- a/include/hw/s390x/css.h
>>> +++ b/include/hw/s390x/css.h
>>> @@ -75,6 +75,18 @@ typedef struct CMBE {
>>> uint32_t reserved[7];
>>> } QEMU_PACKED CMBE;
>>>
>>> +/* 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?
First, returning a struct is ugly in most cases, since it might need to
be passed on the stack if it is bigger than 8 bytes. Ok, that's likely
not the case here (if the compiler / ABI is smart enough - I did not
check), but still, if I see something like this, there is an alarm
signal somewhere in my head that starts to ring...
Then, in the follow up patches, you do something like this:
return (IOInstEnding){.cc = 0};
... and that just looks very, very ugly in my eyes. The more I look at
it, the more I think we really want to have a named enum instead. That
will give you some sort of basic type safety and semantics, too, and
we'll also get proper names for those magic values - otherwise I'll
always have to look up what cc = 2 or cc = 3 means... (I always keep
forgetting what each value means...)
Thomas
- Re: [Qemu-devel] [PATCH v2 3/8] s390x: improve error handling for SSCH and RSCH, (continued)
Re: [Qemu-devel] [PATCH v2 3/8] s390x: improve error handling for SSCH and RSCH, Dong Jia Shi, 2017/10/10
[Qemu-devel] [PATCH v2 5/8] s390x: refactor error handling for CSCH handler, Halil Pasic, 2017/10/04
[Qemu-devel] [PATCH v2 2/8] s390x/css: IO instr handler ending control, Halil Pasic, 2017/10/04
- Re: [Qemu-devel] [PATCH v2 2/8] s390x/css: IO instr handler ending control, Thomas Huth, 2017/10/09
- Re: [Qemu-devel] [PATCH v2 2/8] s390x/css: IO instr handler ending control, Halil Pasic, 2017/10/09
- Re: [Qemu-devel] [PATCH v2 2/8] s390x/css: IO instr handler ending control,
Thomas Huth <=
- Re: [Qemu-devel] [PATCH v2 2/8] s390x/css: IO instr handler ending control, Halil Pasic, 2017/10/09
- Re: [Qemu-devel] [PATCH v2 2/8] s390x/css: IO instr handler ending control, Thomas Huth, 2017/10/10
- Re: [Qemu-devel] [PATCH v2 2/8] s390x/css: IO instr handler ending control, Cornelia Huck, 2017/10/10
- Re: [Qemu-devel] [PATCH v2 2/8] s390x/css: IO instr handler ending control, Halil Pasic, 2017/10/10
- Re: [Qemu-devel] [PATCH v2 2/8] s390x/css: IO instr handler ending control, Halil Pasic, 2017/10/10
- Re: [Qemu-devel] [PATCH v2 2/8] s390x/css: IO instr handler ending control, Thomas Huth, 2017/10/12
- Re: [Qemu-devel] [PATCH v2 2/8] s390x/css: IO instr handler ending control, Halil Pasic, 2017/10/12
- Re: [Qemu-devel] [PATCH v2 2/8] s390x/css: IO instr handler ending control, Halil Pasic, 2017/10/17
- Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/8] s390x/css: IO instr handler ending control, Thomas Huth, 2017/10/17
- Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/8] s390x/css: IO instr handler ending control, Cornelia Huck, 2017/10/17