add walking region/segment/page table to fix #185

[mfsysprog]

this fix is to correctly load a config file fixing #185. I've tried a couple of files (of different length) and they all seem to be properly loaded. I have not been able to test the ESA-390 part of the code, but I've tried to make sure looking at the old code that it still will work. I also did not test the scsiboot, I don't know how that works, but it should still work.

[mfsysprog]

I see in the travis build that some builds fail on initializing the loop counter variable i inside the for statement.

scescsi.c:343:4: error: 'for' loop initial declarations are only allowed in C99 mode

    for(unsigned int i = 0 ; i < entries  ; i++)

If I fix this by declaring the unsigned int i outside of the for loop and commit / push that change, will this retrigger the travis build and will this pull request automatically take that fix? I don't have that much experience with pull request.

[mfsysprog]

There is also an incompatible assignment in the ESA-390 mode that I will fix.

scescsi.c:255:11: warning: assignment from incompatible pointer type [enabled by default]

       ste = (DBLWRD*)(sysblk.mainstor + sto);

[mfsysprog]

If I fix this by declaring the unsigned int i outside of the for loop and commit / push that change, will this retrigger the travis build and will this pull request automatically take that fix? I don't have that much experience with pull request.

I already figured out that I could just add another commit and push. Currently running travis builds, fingers crossed :-)

[Fish-Git]

Seems reasonable to me!

[Fish-Git]

I take that back. This pull does not look okay. It doesn't even compile! I can see what you're trying to do however. Let me see if I can fix your code to the way I believe it should be, and then I will submit my version of your code to you for review/approval.

p.s. How do I test it?

[mfsysprog]

Well, I'm certainly open to suggestions. But I had no problem compiling and also the travis runs compiled ok, so can you explain what part does not compile for you? I tested with a ubuntu 18.04 installation as described by the op of the original ticket.

Perhaps easiest would be if I send you the dasd, config and some instructions?

[Fish-Git]

Well, I'm certainly open to suggestions

Here's my (untested!) version of scescsi.c:

• scescsi.c.txt

(Note: you'll obviously need to rename it from "scescsi.c.txt" to just "scescsi.c". GitHub doesn't allow attaching .c files for some strange reason!)

But I had no problem compiling and also the travis runs compiled ok

Well I don't know what the heck you or travis compiled, but it obviously wasn't your code!

I cloned your repository (https://github.com/mfsysprog/hyperion-1) and the scescsi.c file in your repository (which is ultimately what would be pulled into SDL Hyperion's repository) I'm seeing obvious errors that no C compiler on earth would accept:

static void ARCH_DEP(base_segment_walk)(CREG sto, int fd, U32 size)
{
   /* we want to find valid page table origins */
   CREG pto;
   /* segment tables have 2048 entries max */
   /* tablelength 0 means 512 entries, 1 means 1024, etc. */
   unsigned int entries = ((sto & REGTAB_TL) + 1) * 512;
   /* remove bit definitions to form table origin */


#if defined(FEATURE_001_ZARCH_INSTALLED_FACILITY)
   sto &= REGTAB_TO;
#else /*!defined(FEATURE_001_ZARCH_INSTALLED_FACILITY)*/
   sto &= STD_STO;
#endif /*!defined(FEATURE_001_ZARCH_INSTALLED_FACILITY)*/


#if defined(FEATURE_001_ZARCH_INSTALLED_FACILITY)
    DBLWRD *ste;
#else /*!defined(FEATURE_001_ZARCH_INSTALLED_FACILITY)*/
    FWORD *ste;
#endif /*!defined(FEATURE_001_ZARCH_INSTALLED_FACILITY)*/


   /* walk trough all table entries to find valid items */
   unsigned int i;
   for(i = 0 ; i < entries  ; i++)

https://github.com/mfsysprog/hyperion-1/blob/ca65411d2f54ce01fb0f14802d5a63bb26a532bf/scescsi.c#L231-L254

In C, all variable declarations need to come before any code, and we clearly see your DBLWRD *ste; and FWORD *ste; statements -- as well as your unsigned int i; statement -- all coming after your sto &= REGTAB_TO; and sto &= STD_STO; statements.

Like I said, there is NO COMPILER ON EARTH that would accept such a clear and obvious error.

I don't know what you and travis compiled, but it wasn't your code! It was something else. Maybe you just compiled 4.3? (i.e. without your changes?)

But like I said, as it stands, I cannot accept this pull request. I'm sorry.

If my replacement code is acceptable to you (if you like it), I can go ahead and commit it under your name (i.e. I can commit my suggested code giving you credit as being the author instead of me).

But I don't know what else to do! Your changes are bad!

Sorry!   :(

[mfsysprog]

I'm more that happy to accept your version of the code, it is the result that matters. I will test it today.

I understand your point now! Never realised that (ansi) c has the requirement to declare variables before any code. It seems my code will compile since gcc (and most other modern compilers) allow late declaration of variables, but technically it is not valid ansi c code. Point taken!
See:
https://stackoverflow.com/questions/288441/variable-declaration-placement-in-c/4105334

[mfsysprog]

Can you give me the definitions for the error messages that you added to msgenu.h for your version of the code?
It won't compile without them ;-)

[Fish-Git]

Can you give me the definitions for the error messages that you added to msgenu.h for your version of the code?
It won't compile without them ;-)

(Oops!) Sorry. Forgot about that. Here you go:

• msgenu.h.txt

#define HHC00658 "I/O error on read(): rc=%d: \"%s\""
#define HHC00659 "%s is outside of main storage"

[mfsysprog]

I succesfully tested your code, no problems!
Shall we close this pull request, so you can send in your own fix?

[Fish-Git]

succesfully tested your code, no problems!

Fantastic!

Shall we close this pull request, so you can send in your own fix?

Sure. But first, who should get the credit? Should I commit my code under your name? (So you get credit for it?) Or should I commit it under my own name? I would prefer that you get credit for the actual fix (since it was you that diagnosed the problem and figured out how to fix it), but I don't want you to be unfairly blamed for any problems there might be with my code either. So I will only commit it under your name with your express permission.

So... yes or no? Do you want me to commit my algorithm under your name as the author? Or would you rather me not do that?

Let me know. It's your choice!

[mfsysprog]

I don't really care about if I get the code credit or not. I'm just happy I managed to figure out what was happening. So feel free to commit under my name, or under your own. Both options are fine with me.

[wrljet]

https://github.com/mfsysprog/hyperion-1/blob/ca65411d2f54ce01fb0f14802d5a63bb26a532bf/scescsi.c#L231-L254

In C, all variable declarations need to come before any code, and we clearly see your DBLWRD *ste; and FWORD *ste; statements -- as well as your unsigned int i; statement -- all coming after your sto &= REGTAB_TO; and sto &= STD_STO; statements.

Like I said, there is NO COMPILER ON EARTH that would accept such a clear and obvious error.

Fish,

That's not quite true. Since the C99 standard, mixing declarations with code is allowed.

Microsoft has supported C99 style variable declarations since MSVC++ version 12 (Visual Studio 2013). gcc supported that language feature far earlier.

I'm not suggesting any coding style change for Hercules here. Just pointing out that is a feature of C that has been around for a long time.

Bill

[Fish-Git]

That's not quite true. Since the C99 standard, mixing declarations with code is allowed.

(Doh!) You are of course absolutely right. Thank you, Bill.

Microsoft has supported C99 style variable declarations since MSVC++ version 12 (Visual Studio 2013). gcc supported that language feature far earlier.

Right again.

I can see I'm going to have to make a greater effort to try and upgrade my 12+ year old Visual Studio 2008 to a more modern version. I know I'm going to hate doing so (later versions of VStudio do not impress me), but I can see my continued delay in doing so will only serve to cause more problems down the road, not less.   :(

(sigh!) If only there was a way to just replace the compiler part of my VS2008 install! Doing that is absolutely something I wouldn't hesitate doing. I just don't want to lose my comfortable VS2008 user interface sweater, you know?   :)

I'll have to do some research in that area....

[Fish-Git]

I don't really care about if I get the code credit or not. I'm just happy I managed to figure out what was happening.

Me too!   :)

So feel free to commit under my name, or under your own. Both options are fine with me.

10-4. I'll make the commit right away and close this pull request once I do.

THANK YOU for the time and effort (and patience with me!) you've spent working on this issue, Erik. Mucho appreciated!

[Fish-Git]

The fix (commit b4c7fee) has now been committed, so I am closing this pull request.

Thank you for your effort working on this issue, Erik! Much appreciated!

[Peter-J-Jansen]

The walking region/segment/page table fix had still a few errors around variables "pages" and "tables", which I've now corrected and tested, but hopefully without introducing other errors for Erik or Fish or others. Please let me know if that were to be the case.

Thanks!

Peter

[Fish-Git]

Erik (@mfsysprog),

Can you do us a favor and do a git pull to pick up Peter's commit and then do your test again please? Peter has made an update to the hwl loader logic and we want to be sure he didn't accidentally break anything for you.

Thanks!

[mfsysprog]

Tested, it still works fine.

[mfsysprog]

One small observation, the HHC00661I Hardware loader SCCB = message will appear twice, since the first time the guest will do a SCCB_HWL_TYPE_INFO request to determine the size (in pages) of the file to be loaded into memory and the second time it will do a SCCB_HWL_TYPE_LOAD to request the actual load, but the message doesn't distinguish between the two types of requests.

So testing with a config statement HWLDR CONFIG /usr/local/share/hercules/zzsacard.bin will show up as:

HHC00661I Hardware loader SCCB = 0x7FE4A000                                                                                                                                                                        
HHC00661I Hardware loader SCCB = 0x7FE4A000                                                                                                                                                                        
HHC00651I Loading /usr/local/share/hercules/zzsacard.bin   

[Fish-Git]

Tested, it still works fine.

Thanks!

One small observation, the HHC00661I Hardware loader SCCB = message will appear twice...

What do you suggest? Don't issue the message on an INFO request, and instead only issue it for a LOAD request? Or perhaps just a separate message number or message text? (e.g. identify what type of request it was, or use a different message number for INFO and a different one for LOAD?)

Either way is fine by me and would be a simple change to make. Let me know which you prefer. Thanks!

[mfsysprog]

I didn't add the messages, so I'm fine with whatever solution.