Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Facility Enable command should not check for validity of the bit #353

Closed
arfineman opened this issue Dec 31, 2020 · 25 comments
Closed

Facility Enable command should not check for validity of the bit #353

arfineman opened this issue Dec 31, 2020 · 25 comments
Assignees
Labels
Enhancement This issue does not describe a problem but rather describes a suggested change or improvement.

Comments

@arfineman
Copy link
Contributor

arfineman commented Dec 31, 2020

Facility bits are a moving target and change as new hardware is introduced. The Facility Enable command should not check for validity of the bit and allow for any bit to be turned on, especially those that are listed as reserved. Please note that this request is not about implementing a specific facility, but rather simply allowing the bit to be turned on.

@Fish-Git
Copy link
Member

The Facility Enable command should not check for validity of the bit and allow for any bit to be turned on, ...

I disagree. Certain facility bit combinations are, according to the published architecture (i.e. the Principles of Operation), invalid, and should technically not be allowed. For example:

Bit Meaning When Bit Is One
45 The distinct-operands, fast-BCR-serialization, high-word, and population-count facilities, the interlocked-access facility 1, and the load/store-on-condition facility 1 are installed in the z/Architecture architectural mode.
61 The miscellaneous-instruction-extensions facility 3 is installed in the z/Architecture architectural mode. When bit 61 is one, bit 45 is also one.

The above is because the Misc. Instr. Ext. 3 facility includes an extension to the Population Count instruction, so if facility 61 exists, it follows that facility 34 must as well.

Another example is the Transactional-Execution Facility (bit 73) and the Constrained-Transactional-Execution Facility (bit 50). You cannot enable facility 50 without first enabling bit 73 beforehand (and vice-versa: you can't disable facility 73 without first disabling facility 50).

Hercules is designed to enforce such requirements to prevent invalid facility bit combinations from ever occurring.

... especially those that are listed as reserved.

Well... I'm not sure I necessarily agree with allowing "Reserved for IBM use" facility bits to be set or cleared without restriction, but I guess I could certainly agree to allowing bits defined as "Assigned to IBM internal use" to be set or cleared without restriction. That sounds okay I guess. Dangerous/risky on the part of the user wanting to do so perhaps, but I suppose it's something we could reasonably allow.

Personally, I think doing so is asking for trouble however, but I guess if the user insists on pretending some unknown facility is installed when it really isn't, I guess we should maybe allow that. If the user insists on shooting themselves in the foot, then I guess we should let them do that!   ;-)

How do the other developers feel about this?

@Fish-Git Fish-Git added Enhancement This issue does not describe a problem but rather describes a suggested change or improvement. QUESTION... A question was asked but has not been answered yet, -OR- additional feedback is requested. labels Dec 31, 2020
@wably
Copy link
Member

wably commented Dec 31, 2020

I'm more on the side of Fish's argument on this one. However, I do see the other side of it. There are instances today where Hercules allows stuff that is historically or technically inaccurate as a way to accommodate some needs. A good example is the ability to execute many of the S/390 and z/Architecture instructions while in S/370 mode. This is something that can clearly get a user into trouble if they don't know what they are doing, but if they do know what they are doing it can be very useful. Setting on certain facility bits might offer similar benefits - if the user understands the effects and perhaps is using it to get around a problem.

Perhaps Aaron could tell us about a use case, that is, a real example of why you would want to do this and what problem it would solve?

Regards,
Bob

@arfineman
Copy link
Contributor Author

arfineman commented Dec 31, 2020 via email

@wably
Copy link
Member

wably commented Dec 31, 2020

Aaron,

I was hoping that you had an actual example of where some operating system or other programming might not start-up or operate because one of the facility bits was not enabled. I'm getting the sense now that you might be looking for this selective bit enablement as a 'just in case I need it'.

The main thing for me is that these bits are an indication, in part, of what Hercules actually supports (just like real iron) and so if a bit is off it should be off. Setting it on by force would probably cause some future operating system to fail because Hercules doesn't have the underlying support. I don't see the need to try to avoid a release change just to set a facility bit, because a release would be required anyway to add the underlying support that the bit represents.

Because of the need for the associated underlying support for any bit setting, this isn't quite the same as 'bending the architecture', in the context that you used that phrase.

So, I am not convinced of the need. But, I am not the one you have to convince. I am only weighing in because Fish asked for other opinions from the team. Maybe Peter or Ivan have some thoughts.

Regards,
Bob

@wrljet
Copy link
Member

wrljet commented Dec 31, 2020

Bob, I agree with your sentiment here.

Setting feature bits that don't actually do anything just seems like a mistake. If a target operating system needs a facility to be present, lying about it won't make it work.

Bill

@arfineman
Copy link
Contributor Author

arfineman commented Jan 1, 2021 via email

@Fish-Git
Copy link
Member

Fish-Git commented Jan 1, 2021

Aaron,

Please see the following two GitHub Issue comments:

Thanks!

@Fish-Git
Copy link
Member

Fish-Git commented Jan 1, 2021

We already know the message "HHC00892E Facility( BIT55 ) does not exist for z/Arch" is wrong.

Well, okay, I would agree that the text of the message isn't the greatest and could maybe be tweaked to be a bit clearer. How about "assigned or reserved to IBM"? Would that be better?

But as far as allowing currently unsupported bits to be forcibly enabled, I still think it's a bad idea. I mean, while I think I understand your argument, Aaron, (and am trying to argue in your favor), I personally still feel doing so would be a mistake on the user's part. But as I said in my earlier reply, I'm willing to argue in favor of allowing the user to shoot themselves in the foot if they really insist on doing so. I personally don't like it (and personally feel doing so is a mistake), but if they (you) want to do that, I would be reluctantly willing to allow that.

But then the subject of for which bits do we want to allow this for comes up. All bits? (any bit?) Or just the ones that are defined but currently unsupported by Hercules? (Because we already support that!)

Then too, there is the question of how to deal with bits which are currently unassigned (undefined) as well as those that are "Assigned to IBM internals use" and "Reserved for IBM use". How should we deal with those?

For example, facility bits 29, 56, 79, 83-127, 132, 136, 137, 143, 150, 153, 154, 157-167 and all bits greater than 168 are currently undefined. They're neither "Assigned to IBM internals use" nor "Reserved for IBM use". They're not even defined! Should we allow those bits to be enabled? IMHO we should not.

But those that are defined as "Assigned to IBM internals use" or "Reserved for IBM use"? I feel allowing the user to enable them seems reasonable. Dangerous and foolish on the part of the user IMHO, but I don't see any real need to disallow it. Those bits are "defined" after all. We just don't know what they actually do. They're undocumented. Defined, but undocumented. If the user wants to enable any of those bits, then I think we should allow it.

But the undefined bits? (i.e. 29, 56, ... etc) Nope. I think we should still disallow messing with those bits.

Are there any other developers (or users!) out there wanting to chime in with their own opinion on the matter?

@Fish-Git Fish-Git added the Discussion Developers are invited to discuss a design change or solution to a coding problem. label Jan 1, 2021
@Fish-Git Fish-Git self-assigned this Jan 1, 2021
@Peter-J-Jansen
Copy link
Collaborator

Bob and Bill and Fish,

I agree with the sentiments and arguments you provided, and also think we have other more pressing priorities.

Cheers,

Peter

Merry Christmas and a Happy and Healthy New Year to All !!

@Fish-Git
Copy link
Member

Fish-Git commented Jan 1, 2021

... and also think we have other more pressing priorities.

I presume you mean TXF?   ;-)

(Or is there some other issue we should be prioritizing our efforts on?)

@mcisho
Copy link
Contributor

mcisho commented Jan 1, 2021

Personally I think anyone who wishes to shoot themselves in the foot, or anywhere else, should be allowed to do so, providing they do not expect someone else to bandage them afterwards.

Hercules is open source, if Mr Fineman wants the Facility command to manipulate any and every facility bit, he is free to modify the source to do whatever he wishes.

I agree with Peter.

Happy New Year.

@Peter-J-Jansen
Copy link
Collaborator

Peter-J-Jansen commented Jan 2, 2021

Yep, TXF is my main focus. I need to do something else now and then mostly to keep my motivation going.  :-)

Cheers,

Peter

@Fish-Git
Copy link
Member

Fish-Git commented Jan 2, 2021

FWIW, I already have the changes that Aaron requested coded and tested and they work fine. I could commit the fix for this GitHub Issue right now since it appears everyone agrees his request seems reasonable, but I'm going to hold off for a few days while I take care of some other things first.

Technically, the way it was coded before (i.e. the way it's currently coded in our repository) is actually wrong, as Aaron pointed out. All of the facility bits that are defined but currently "undocumented" (i.e. all of the facility bits the Principles of Operation identifies as either "Assigned to IBM internals use" or "Reserved for IBM use") technically should be in our list of valid facility bits that can be enabled or disabled (set or cleared). Only those bits which the Principles of Operation doesn't define (i.e. those I mentioned earlier: bits 29, 56, 79, 83-127, ... etc) should not be in our list. Those bits cannot be changed. But the ones currently defined as "Assigned to IBM internals use" or "Reserved for IBM use"? Sure! There's no reason why those can't be enabled if the user really wants to.

Anyway, that's the fix I already have coded and tested and will be committing probably within the next few days after I finish up with the other stuff I'm working on.

Thanks for everyone's feedback! Much appreciated, guys!

@mcisho
Copy link
Contributor

mcisho commented Jan 3, 2021

We have three groups of bits that might cause unpredictable results, those bits for features Hercules doesn't currently support, those bits for IBM's use, and those bits that aren't defined. If we allow a user to potentially break things with two of the groups, why not the third? STFLE currently returns 192 bits, who knows how many bits it will return in the future, and what those bits might or might not mean. If this change is going to be made, I vote for all or nothing!

@mcisho
Copy link
Contributor

mcisho commented Jan 3, 2021

I forgot to mention that a z15 returns facility bit 132 turned on, even though it is apparently undefined.

@mcisho
Copy link
Contributor

mcisho commented Jan 3, 2021

Looking at my historical notes I see that a zEC12 and a z13 also returned facility bit 132 turned on. I've also just noticed that the z15 returned facility bits 150 and 153 turned on, even though they are also apparently undefined.

@Fish-Git
Copy link
Member

Fish-Git commented Jan 3, 2021

If we allow a user to potentially break things with two of the groups, why not the third? STFLE currently returns 192 bits, who knows how many bits it will return in the future, and what those bits might or might not mean. If this change is going to be made, I vote for all or nothing!

I forgot to mention that a z15 returns facility bit 132 turned on, even though it is apparently undefined.

Looking at my historical notes I see that a zEC12 and a z13 also returned facility bit 132 turned on. I've also just noticed that the z15 returned facility bits 150 and 153 turned on, even though they are also apparently undefined.

I understand your argument, Ian, and on the face of it (at first glance), it seems to be a reasonable one.

HOWEVER... Doing so would increase the size of the facility query all display by another 88 lines or so, instead of by only 21 lines or so. And then the question of course arises, what about those bits beyond the last currently defined bit? (bit 168?) Those bits are just as undefined as the others! Should we allow the user to modify any of those bits too? (i.e. any bit beyond bit 168? i.e. bits 169-32767?) Doing so would increase the size of the display by thousands of lines! AND it would also prevent Hercules from being able to define its own bits too! (which are currently defined just beyond the last currently defined bit, bit 168, at facility bit positions 192-213.) Where do we draw the line?

I personally draw the line at the undefined bits. I'm willing to allow the user to modify an additional 21 new bits (currently defined as for IBM use), but not another 88 nor especially not another several thousand.

I vote for only adding the "IBM" bits to the set of bits that the user is allowed to modify, but not any of the bits currently in the "undefined" set.

But as always, I will entertain arguments to the contrary.

@mcisho
Copy link
Contributor

mcisho commented Jan 4, 2021

STFLE currently returns 192 bits, and those are the only bits I would suggest allowing the user to modify. After all, real hardware is already returning several of the "undefined" set. As we know, just 'cos it's not in (the publicly available versions of) Principles of Operation doesn't mean it doesn't exist, e.g. the SIGA and CHSC instructions.

Hopefully, STFLE returning 256 (or more) bits will be so far in the future I won't even be able to give a damn!

@Fish-Git
Copy link
Member

Fish-Git commented Jan 4, 2021

STFLE currently returns 192 bits, and those are the only bits I would suggest allowing the user to modify.

I suppose we could do that. There's a part of me that feels doing so is a bit too much (no pun intended), but I can't explain why. It just feels wrong somehow. But I'll admit your argument is sound. It does make sense I guess. How do the other developers feel about it?

@wrljet
Copy link
Member

wrljet commented Jan 4, 2021

Fish wrote, in part:

HOWEVER... Doing so would increase the size of the facility query all display by another 88 lines or so, instead of by only 21 lines or so. And then the question of course arises, what about those bits beyond the last currently defined bit? (bit 168?) Those bits are just as undefined as the others! Should we allow the user to modify any of those bits too? (i.e. any bit beyond bit 168? i.e. bits 169-32767?) Doing so would increase the size of the display by thousands of lines! AND it would also prevent Hercules from being able to define its own bits too! (which are currently defined just beyond the last currently defined bit, bit 168, at facility bit positions 192-213.) Where do we draw the line?

Wild idea...
How about keeping the current display as-is, but adding a slightly new form of the command (perhaps facility query allx) for people who want to see 88 more lines?

@arfineman
Copy link
Contributor Author

arfineman commented Jan 4, 2021 via email

@wrljet
Copy link
Member

wrljet commented Jan 4, 2021

How about a bit map like this and display a '1' for every on bit

Yes, I was thinking about that, too.

But it does leave the hard work to the reader. :-)

@Fish-Git
Copy link
Member

Fish-Git commented Jan 4, 2021

How about a bit map like this and display a '1' for every on bit:

We already have that in the FACILITY QUERY RAW command:

HHC01603I help facility
HHC01603I
HHC01602I Command               Description
HHC01602I ----------------      -------------------------------------------------------
HHC01602I facility             *Enable/Disable/Query z/Arch STFLE Facility bits
HHC01603I
HHC01603I Format: FACILITY  ENABLE | DISABLE    | bit   [ archlvl ]
HHC01603I         FACILITY  QUERY  SHORT | LONG | RAW
HHC01603I         FACILITY  QUERY   | bit | ALL
HHC01603I         FACILITY  QUERY  ENABLED | DISABLED   [ LONG ]
HHC01603I
HHC01603I ALL is a synonym for SHORT. RAW displays the hex string. ENABLED displays
HHC01603I only facilities which are enabled. DISABLED shows only disabled failities.
HHC01603I LONG sorts the display by Long Description. SHORT is the default. 
HHC01603I is the SHORT facility name to be enabled or disabled. bit may be entered
HHC01603I as either a numeric bit number or 'BITnnn'.
HHC01603I
HHC01603I facility query raw
HHC00894I z/Arch facility list: F3F4FFFB FCFDEC24 205C0000 00000000 00000000 00000000 00000000 00000000, HERC: 4F1A0000 00000000

@Fish-Git
Copy link
Member

Fish-Git commented Jan 4, 2021

I have the change ready that defines all of the "Assigned to IBM internal use" bits, all of the "Reserved for IBM use" bits (there's only one: bit 147), as well as all currently undefined bits up to bit number 200 (i.e. far enough beyond the currently last defined bit of 168 that hopefully we shouldn't need to worry about things for a while).

Should I commit it?

@Fish-Git
Copy link
Member

Fish-Git commented Jan 5, 2021

Fixed by commit ac43035.

Closing.

@Fish-Git Fish-Git removed Discussion Developers are invited to discuss a design change or solution to a coding problem. QUESTION... A question was asked but has not been answered yet, -OR- additional feedback is requested. labels Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement This issue does not describe a problem but rather describes a suggested change or improvement.
Projects
None yet
Development

No branches or pull requests

6 participants