Incorrect behavior for LPSW in z/Arch mode when LPSW field contains invalid ESA PSW with E bit (bit 12) set to 0

ivan-w commented 7 years ago

This is an issue uncovered by a previous issue (#180).

When in z/Arch mode, a LPSW addressing an ESA mode PSW is issued and the ESA mode PSW has the E bit (bit 12) set to 0, the results is an early specification exception because of an invalid resulting PSW.

However, SA22-7832-10 Page 10-53 in the LPSW chapter specify on multiple occasions that bit 12 is checked BEFORE the new PSW is loaded. Therefore the Specification Exception should be generated BEFORE the PSW is loaded (before invoking s390_load_psw in control.c/load_program_status_word).

However, the z/Arch principle of operations specify it is model dependent whether an actual PIC 6 is generated - so it should either be a PIC 6 on the LPSW - or Bit 12 of the ESA PSW should be ignored (and forced to 0 in the z/Arch PSW). But if it generates a PIC 6, it should NEVER generate an early specification exception. This is an instruction suppression condition, the PSW should remain unchanged, and the ILC should reflect the length for the LPSW instruction.

Note this is NOT a pressing issue... Notably, the z/Arch POP contradicts itself in the Programming note for LPSW, and there are very little chance it will have any direct effect on any program.

jphartmann commented 7 years ago

OK, so Hyperion is a model that does not do the early program check; program check it will sooner or later. Let's let sleeping dogs lie.

Suggest we close this one.

ivan-w commented 7 years ago

John...

The "Early Specification Exception" term is a bit misleading ! It actually means the specification check occurs AFTER the LPSW is done (it's "early" because it occurs before the next instruction is fetched/executed and the PSW validity is being checked in between instruction execution). The LPSW should either fail at LPSW time (Specification Exception - LPSW Instruction suppressed), or it shouldn't fail at all.

So the instruction either fails in the right way, or nothing fails... But it shouldn't fail in the wrong way.

ivan-w commented 7 years ago

Actually : I modify my statement : If an LPSW is directed at an ESA PSW image with BIT 12 set to 0 :
Either

LPSW should end in PIC 6 - LPSW Instruction suppressed (PSW NOT changed).
LPSW completes and then subsequently, an early exception occurs, but BIT 12 in the newly loaded PSW should be set to 1 (not 0 as it is the case right now).

Currently, LPSW loads the new z/Arch PSW, set bit 12 to 0 THEN checks if bit 12 is 1 in the loaded ESA PSW, and if it isn't, fires an early specification exception.

It's a simple fix !

ivan-w commented 7 years ago

The issue is that the PIC 6 is fired - and the old PGM PSW is a perfectly valid z/Arch PSW... and the ILC is 0 and it doesn't point to the LPSW instruction

ivan-w commented 7 years ago

Either LPSW should check that bit 12 is not 0 (and otherwise PIC 6 with the current PSW with IA + 6) or it should have bit 12 inverted in the PSW (set to 1) and do the early specification exception because the z/arch PSW is invalid (bit 12 MUST be 0)

jphartmann commented 7 years ago

I think we are in agreement apart from a number 6.

I misused the word early. Let's call LPSW checking bit 12 for 1 the synchronous check. Either, as you say, it should take the synchronous check, nullify and set the old PSW address to LPSW + 4 (+4 as it is not LPSWE, which would be +6); or it should store the entire expanded PSW with bit 12 inverted as the new PSW. That is an "early program check", because the instruction has not been fetched and as a result opcode has not been decoded.

The PSW stored: PSW=0082000000000000 0000000000000000 is indeed incorrect; the second byte should be 8a.

I can see why the MVS developers insisted on the synchronous behaviour; the old PSW is not much to go by. But that would have been a ViCom issue; curious that it made it into the architecture.

ivan-w commented 7 years ago

Yes.. IA+4 !

--Ivan

On 1/8/2017 11:35 AM, John P. Hartmann wrote:

I think we are in agreement apart from a number 6.

I misused the word early. Let's call LPSW checking bit 12 for 1 the synchronous check. Either, as you say, it should take the synchronous check, nullify and set the old PSW address to LPSW + 4 (+4 as it is not LPSWE, which would be +6); or it should store the entire expanded PSW with bit 12 inverted as the new PSW. That is an "early program check", because the instruction has not been fetched and as a result opcode has not been decoded.

The PSW stored: PSW=0082000000000000 0000000000000000 is indeed incorrect; the second byte should be 8a.

I can see why the MVS developers insisted on the synchronous behaviour; the old PSW is not much to go by. But that would have been a ViCom issue; curious that it made it into the architecture.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/hercules-390/hyperion/issues/181#issuecomment-271142832, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjMW1fLMmfIblvdlprtaWOkORz7z6aGks5rQLv2gaJpZM4Lc_AW.

ivan-w commented 7 years ago

And.. not nullified, but suppressed

--Ivan

On 1/8/2017 11:35 AM, John P. Hartmann wrote:

I think we are in agreement apart from a number 6.

I misused the word early. Let's call LPSW checking bit 12 for 1 the synchronous check. Either, as you say, it should take the synchronous check, nullify and set the old PSW address to LPSW + 4 (+4 as it is not LPSWE, which would be +6); or it should store the entire expanded PSW with bit 12 inverted as the new PSW. That is an "early program check", because the instruction has not been fetched and as a result opcode has not been decoded.

The PSW stored: PSW=0082000000000000 0000000000000000 is indeed incorrect; the second byte should be 8a.

I can see why the MVS developers insisted on the synchronous behaviour; the old PSW is not much to go by. But that would have been a ViCom issue; curious that it made it into the architecture.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/hercules-390/hyperion/issues/181#issuecomment-271142832, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjMW1fLMmfIblvdlprtaWOkORz7z6aGks5rQLv2gaJpZM4Lc_AW.

jphartmann commented 7 years ago

Ah, yes. Nullified; definitely. We don't want anyone looping here.

ivan-w commented 7 years ago

In z/Arch, if we go the instruction suppression route, the check should be done early in order to preserve the BAR.

ivan-w commented 7 years ago

No no.. "Suppressed" ! NOT nullified !

ivan-w commented 7 years ago

John, There are 4 ways an instruction can complete :

Complete : The instruction performed its task, and the CPU should execute the next instruction
Suppressed : the instruction did NOT perform its task and the PSW IA points to the next instruction
Nullified : The instruction did not complete, but a transient (such as a translation exception) prevented the instruction to complete. The PSW IA still points to the same instruction
Terminated : The instruction did not complete, but the state of the system is undefined (this is usually due to a machine check)

jphartmann commented 7 years ago

Amen. Though I'd add for terminated that the PSW address &c are valid except on an exigent machine check, so that the control program may handle the programming error.

ivan-w commented 7 years ago

So what's you'r opinion : Check the PSW early and PIC 6 and suppress, or load the PSW and early PIC 6 because the resulting PSW is invalid ? (both are valid)

jphartmann commented 7 years ago

Ivan, I have no preference, but the synchronous check gives more information, so perhaps that one. Whatever is the simplest to implement. I doubt anyone else has enough thumbs to get into this hole.

ivan-w commented 7 years ago

All right with me... I'll do that - I'll git the thingy, but you'll all have to test it - I haven't even tried the new build procedure yet

jphartmann commented 7 years ago

Fine.

However, as far as Hyperion is concerned, the old build process it still there, though you do need SoftFloat-3a to configure it and link it. 1Stop is just a shortcut (apart from the update of floating point support).

ivan-w commented 7 years ago

Proposed patch : $ git diff diff --git a/control.c b/control.c index 242f0b5..d10325c 100644 --- a/control.c +++ b/control.c @@ -1981,6 +1981,15 @@ int amode64; / Fetch new PSW from operand address / STORE_DW ( dword, ARCH_DEP(vfetch8) ( effective_addr2, b2, regs ) );

+#if defined(FEATURE_ESAME)

/ Check bit 12 - PIC 6 if bit 12 of ESA PSW is 0 on a LPSW in z/Arch mode /
/ Do this before BEAR is set /
if((dword[1] & BIT(PSW_EC_BIT))==0)
{
ARCH_DEP(program_interrupt) (regs, PGM_SPECIFICATION_EXCEPTION);
} +#endif
/ Set the breaking event address register / SET_BEAR_REG(regs, regs->ip - 4);

Does it look right ?

ivan-w commented 7 years ago

The fact that softfloat-3a is required is fine... but WHY does it need to reside in the build directory ?

ivan-w commented 7 years ago

And if it is a requirement - and softfloat-3a only has a purpose of existence because of hercules - it SHOULD be part of hercules.

jphartmann commented 7 years ago

SoftFloat-3a does not have to be where it is put by default. That is being sorted out "as we speak".

The hard requirement is that Hyperion configure can generate -I and -L to point to it. Of course, if you want it in a non-standard place, you put it there and you build it there. You can also, on UNIX, have SoftFloat-3a somewhere else and still enjoy the benefit of 1Stop by judicious use of symbolic link(s).

There are several reasons why a subcomponent might be a separate github project (and for that matter not even owned by hercules-390):

The author is committed to maintaining his (m/f) work and thus does not wish everyone to walk all over it.
To reduce the amount of recompile. It takes more than half an hour to build Hyperion on an atom processor.
And the clincher: Licence. Only way to combine non Q with Q is to have it happen by the user. No idea what people who build packages for the various distributions do. Ignore licensing issues, I expect.

So even if SoftFloat-3a is useful with Hyperion only, it has to be separate from it.

Anything being installed globally, such as zlib, is a different story, of course.

srorso commented 7 years ago

A couple of notes….

The author is committed to maintaining his (m/f) work and thus does not wish everyone to walk all over it.

John’s comment is appropriate for the general case. In the specific case of SoftFloat-3a For Hercules, while I am thrilled with what I learned In developing the modifications and would really like the opportunity to make any further changes that might be required, I have little fear that someone will walk all over it. Floating point seems to be a part of the developer map that would have, in Oldden Tymes, been labeled, “Thar be Dragons an’ Monsters,” and developers would sooner embrace the third rail of an underground light rail transit system than mess with floating point. John, I am missing the “m/f” reference. Can you elaborate (of-list if needed)?

…Ignore licensing issues, I expect

In some cases, I am sure that’s true, but in others, not so much. According to the Wikipedia article (here: https://en.wikipedia.org/wiki/Q_Public_License), the originators of the Q license, Trolltech eventually dual-licensed the product that used it (Qt) because the Q license limited use of the free version of Qt. And that means at least some people were respecting the license by not using the product under unacceptable license terms.

Anything being installed globally, such as zlib, is a different story, of course.

In the Windows world, where there are lots more places on the map with dragons an’ monsters, zlib is not installed globally, or even commonly, except, except perhaps as something linked to an installed binary. It does not commonly exist as a development library. So Windows builders must specifically install it and its companions bzip2 and pcre, and we are left with the task of packaging up something that a) works and b) is reasonably compliant with license requirements. Which often start with reproduction of the copyright and license documents for the separately licensed product....something that seems to be missing from any Windows binary for Hercules I have found. Sadly, that includes the binaries I built, but I am smarter now and will fix that.

(Forgive me in the above paragraph for somewhat conflating the license requirements of a binary executable distribution and a packaged distribution of the components and packages needed to build Hercules in Windows. Notwithstanding conflation, the issue remains.)

And this is my real concern about the Q license—not that there is anything that can be done about it. It was written in the last century as a license for a library (and not a good one at that) and it has not been touched since. GPL at least has evolved through three versions and a “Lesser” variant that simplifies the task of building a library for inclusion in programs licensed under different terms. GPL 3, for example, would probably allow packaging of things like zlib with a GPL-licensed package under the “system libraries” exception, which first appeared in GPL 3.

But this is the hand we are dealt….

Back to decimal floating point validation (one pervasive issue found so far: no scaled results on trappable underflow/overflow.)

Best Regards,

Steve Orso

jphartmann commented 7 years ago

Returning to the original issue: z/CMS runs in z Architecture mode, but CMS itself is not 64-bit capable. Thus on an interrupt the ESA old PSW is constructed from the first and last four bytes of the full 16-byte old PSW, but bit 12 is left as zero to indicate that this is not the real interrupt. Bit 12 is not turned on until the PSW is used to redispatch. Anything in CMS that is not aware of this will cause the program check, so it may be a good idea to take the error as part of the LPSW instruction and nullify.

ivan-w commented 7 years ago

On 3/7/2017 2:26 PM, John P. Hartmann wrote:

Returning to the original issue: z/CMS runs in z Architecture mode, but CMS itself is not 64-bit capable. Thus on an interrupt the ESA old PSW is constructed from the first and last four bytes of the full 16-byte old PSW, but bit 12 is left as zero to indicate that this is not the real interrupt. Bit 12 is not turned on until the PSW is used to redispatch. Anything in CMS that is not aware of this will cause the program check, so it may be a good idea to take the error as part of the LPSW instruction and nullify.

John,

Make it an ARCHLVL thing maybe if you wish... I understand the LPSW instruction in z/Arch mode can possibly throw a specification exception if bit E is 0 (this is defined as model dependent) - but then the instruction is not nullified, it is terminated. Nullification on a specification exception only occurs on an early specification exception (PSW didn't pass validation after it was loaded).

On nullification, the IA remains the same. On termination, the IA points to the next instruction.

--Ivan

ivan-w commented 7 years ago

Issue stale. Closing

hercules-390 / hyperion

Incorrect behavior for LPSW in z/Arch mode when LPSW field contains invalid ESA PSW with E bit (bit 12) set to 0 #181