Add OPENSSL_s390xcap environment variable #6813

p-steuer · 2018-07-30T15:45:29Z

This is the next patch in my s390 series.

I reworked the environment variable's semantics based on a discussion with @dot-asm :
#4542 (comment)

documentation is added or updated

dot-asm · 2018-07-31T10:49:31Z

crypto/s390xcap.c

@@ -7,6 +7,7 @@
 * https://www.openssl.org/source/license.html
 */

+#include <ctype.h>


Consider intenal/ctype.h that is guaranteed to be locale-neutral.

dot-asm · 2018-07-31T10:52:45Z

crypto/s390xcap.c

+    char *tok_begin, *tok_end, *buff, tok[3][LEN + 1];
+    int rc, off, i;
+
+    buff = OPENSSL_malloc(strlen(env) + 1);


No, no OPENSSL_malloc. This is used as "constructor" and calling OPENSSL_malloc will bind it to malloc/free, while we leave it, binding OPENSSL_malloc to specific memory management, to application. Just use malloc (or stdrdup), and naturally free.

dot-asm · 2018-07-31T11:16:37Z

crypto/s390xcap.c

@@ -64,4 +113,475 @@ void OPENSSL_cpuid_setup(void)
    sigaction(SIGFPE, &oact, NULL);
    sigaction(SIGILL, &oact, NULL);
    sigprocmask(SIG_SETMASK, &oset, NULL);
+
+    OPENSSL_s390x_functions();


Any specific reason for splitting OPENSSL_s390x_facilities? Since facilities zero whole thing, one can say that it's only appropriate that it initializes whole thing...

I did the facility detection / function detection split so that the environment variable's stfle mask will affect function detection e.g., disabling an msa level.

Ah! Right :-)

It probably makes sense to move the zeroing of the query function's part over to OPENSSL_s390x_functions though.

p-steuer · 2018-07-31T18:42:06Z

Integrated your feedback. I used malloc instead of strdup because of the latter's feature test macro requirements.

dot-asm · 2018-07-31T20:19:52Z

crypto/s390xcap.c

+    (sscanf(tok_begin,							\
+            " " STR(NAME) " : %" STR(LEN) "[^:] : "			\
+            "%" STR(LEN) "s %" STR(LEN) "s ",				\
+            tok[0], tok[1], tok[2]) == 2) {				\


What's with three % and check for 2?

idea was to enforce correct input format i.e., fail if third % catches garbage .

It's not like it gives you any advantage. We don't interact with user telling that it if format was wrong, so it doesn't matter what kind of garbage we reject. I mean %128s followed by scanf("%llx") is sufficient. But all right...

[...] so it doesn't matter what kind of garbage we reject

I agree but i dont see how trailing garbage would be rejected if i didnt do the third % and the == 2 check.

Basically question is what is it one tries to achieve here, to ignore "maliciously" malformed input or "honest-mistake" malformed input? If former, then user is interested to fulfill the criteria, and won't ever pass input that triggers the check. If latter, then right thing to do would be to tell user that something is not right. But we don't want to do that. And in such case scanf("%llx") should be sufficient. But once again, it [third %] is all right. It might be viewed as a bit counter-intuitive, but all right...

dot-asm · 2018-07-31T20:27:19Z

doc/man3/OPENSSL_s390xcap.pod

+
+Disables the vector facility.
+
+=item OPENSSL_s390xcap="km:~0x2800:~0;kimd:~0xc000000:~0"


I wonder if following would be usable/appreciated. ~0x100 alone or ~0x100: would mean that second word is zeroed. While :~0x100 would mean first work is not modified.

[... ] ~0x100: would mean that second word is zeroed. While :~0x100 would mean first work is not modified.

Did you mean an omitted mask value is interpreted as ~0 ("dont modify") ? .. the quote suggests otherwise: omitting second word means 0 ("zeroize"), omitting first word means ~0 ("dont modify"), so i dont get the logic here.

As it is now, you have to specify both words i.e., keep the ~0 if you dont want to modify detected capabilities.

First of all I want to emphasize that remark started with "I wonder". In other words it's rather a question than actual suggestion.

Did you mean an omitted mask value is interpreted as ~0 ("dont modify") ?

For first word, yes, but opposite for second. So that :~0x100 would be equivalent to ~0:~0x100, but ~0x100 would be equivalent to ~0x100:0. I recognize that it might appear "quirky". It might be more appropriate to arrange that omitted value is always equivalent to ~0...

It appears "quirky" because i dont get the rationale behind it. I mean, why would one want to make disabling all higher function codes (second word) the default if one decides to disable a lower (first word) function code ? E.g. if one disables sha3 fcs for kimd, why should disabling ghash fc be the default?

As for "omitted value means ~0": this makes sense to me. Its a trade of between not requiring the user to specify ~0 and adding complexity to parsing code.

E.g. if one disables sha3 fcs for kimd, why should disabling ghash fc be the default?

Just in case, rationale was based on assumption that new stuff is always denoted by more significant bits. So that if user wanted to suppress something new, chances are that [s]he wants to suppress even everything newer. In this case assuming 0 for second word would be kind of intuitive. This assumption turns to be wrong in this case. It's all right :-)

dot-asm · 2018-07-31T20:33:42Z

doc/man3/OPENSSL_s390xcap.pod

+
+=head1 DESCRIPTION
+
+libcrypto supports z architecture instruction set extensions. These


I always thought of it as "z/Architecture"...

yes that's the official spelling. will correct it.

Maybe even "z processor" in NAME section? "zSeries processor" if one wants to minimize occurrences of "z/Architecture"?

i try to distinguish between the ISA name (z/Architecture) and the name of the processor implementing the ISA. The latter has been changed several times in the past. I think "IBM z" (short for "IBM zEnterprise System") is the official wording at this time.

dot-asm · 2018-07-31T21:02:24Z

crypto/s390xcap.c

@@ -13,15 +13,51 @@
 #include <setjmp.h>
 #include <signal.h>
 #include "internal/cryptlib.h"
+#include "internal/ctype.h"


Initially remark about ctype.h was kind of generic, i.e. without considering whole picture. But it's just one call and sscanf is surely "hungry" for locale. But what's done is done :-)

p-steuer · 2018-08-01T15:34:20Z

Fixed spelling in docs. Reworked the stfle parsing to better reflect the situation that the number of words returned by stfle varies over machine generations. I played a bit with the idea of omitted mask values being interpreted as ~0 but since there is no suitable sscanf format string and strtok ignores multiple delimiters, i ended up with more complex and less robust "char-by-char" parsing code.

dot-asm · 2018-08-01T18:49:26Z

doc/man3/OPENSSL_s390xcap.pod

+ #50 message-security assist extension 4
+ #51 message-security assist extension 3
+ :
+ #45 message-security assist extension 8


This is confusing in sense that there is no mark when you switch to next word. For example 46 and 50 are from different words. In ia32cap.pod second word is denoted as 64+n. Maybe it would be appropriate even here? The remark naturally applies even to function capability vectors...

When listing the significant bits i use the format described above: that is, the 64-bit words/masks are separated by the colons.
Im not sure if adding 64+n notation will make it less or more confusing.. ?

Problem of course is that we both know what's going on and only imagine how readable would this be to an unprepared reader. At the same time reader won't be totally unprepared, as [s]he has to make connections between this text and z/Architecture Principles of Operations. And that's where it gets extra tricky. In the manual bits from multiple words are numbered monotonically, but they are numbered in reverse order. For example vector facility is bit 129. While here it's denoted as 62 (btw, colon is missing, right?). Sigh... Maybe it would be appropriate to make the table three-column, something like

#129 #::62 vector facility

First column would be the number from manual and second would be right shift that would help to calculate actual values. Values from first word could be denoted with n::, from second as :n:

:n:

Or maybe even :1<<n:

(btw, colon is missing, right?)

No, it's not, sorry about distraction.

a need to reference POP bit counting

I'd say it's inevitable. Inquisitive reader would surely like to know what does e.g. "message-security assist" entail, and would have to turn to the manual. And [as] it is now would just get disoriented...

One can also look at it as following. Things should be verifiable. And easier it is to verify, better. It's only appropriate to present the POP numbering and bit numbering in word next to each other, so that one can double-check that everything [is] in shape when something doesn't appear right. I mean imagine I assign a ~~variable~~ value and get unexpected result. I might need to double-check whole chain from manual to actual assignment...

Okay, in this case i ll also add a"see also" section with reference to latest POP.

Id still prefer a list (which reflects the env var format) as described above instead of a table and repeat e.g. "::" in front of all third word bit. what do you think ?

mask bits could still be denoted by 1<<n to make it even more clear, what is meant by the non-POP number.

Problem I have with : is that they are experienced as vertical ellipses, as if there is [generic] gap between significant bits, rather than word delimiters. But all right, the fact that there will "64*n-ish" gap in first column it might be ok. E.g.

#17 #1<<46 message-security assist : #76 #1<<51 message-security assist 3 : #129 #1<<62 vector facility

Might work...

dot-asm · 2018-08-02T11:20:43Z

crypto/s390x_arch.h

@@ -45,6 +45,9 @@ struct OPENSSL_s390xcap_st {

 extern struct OPENSSL_s390xcap_st OPENSSL_s390xcap_P;

+/* Max number of 64-bit words returned by STFLE */
+#  define S390X_STFLE_MAX	3


BTW, this reminds me. We did reserve one word for future extensions, didn't we? While you are at it, could you add "currently" to commentary?

p-steuer · 2018-08-02T16:02:42Z

Added POP ref to docs and explaination how the list of significant bits is to be understood. Also removed the PRNOs trng bit as significant for it is not at this time. Vector and sha3/shake bits are also not significant yet but i didnt remove them for the patches are already in my queue.

The OPENSSL_s390xcap environment variable is used to set bits in the s390x capability vector to zero. This simplifies testing of different code paths. Signed-off-by: Patrick Steuer <patrick.steuer@de.ibm.com>

Signed-off-by: Patrick Steuer <patrick.steuer@de.ibm.com>

p-steuer · 2018-08-06T12:12:02Z

Squashed everything and rebased (due to sha3 merge). It needs re-approval, no ?

p-steuer · 2018-09-12T09:49:21Z

Ready label removed? Does it need re-approval ?

p-steuer · 2018-10-12T18:59:16Z

Ping. Now after 1.1.1 release this feature would be ready for master.

The OPENSSL_s390xcap environment variable is used to set bits in the s390x capability vector to zero. This simplifies testing of different code paths. Signed-off-by: Patrick Steuer <patrick.steuer@de.ibm.com> Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #6813)

Signed-off-by: Patrick Steuer <patrick.steuer@de.ibm.com> Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #6813)

dot-asm · 2018-10-17T12:05:42Z

Merged. Thanks!

The OPENSSL_s390xcap environment variable is used to set bits in the s390x capability vector to zero. This simplifies testing of different code paths. Signed-off-by: Patrick Steuer <patrick.steuer@de.ibm.com> Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#6813) (cherry picked from commit f39ad8d)

Signed-off-by: Patrick Steuer <patrick.steuer@de.ibm.com> Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#6813) (cherry picked from commit d68af00)

The OPENSSL_s390xcap environment variable is used to set bits in the s390x capability vector to zero. This simplifies testing of different code paths. Signed-off-by: Patrick Steuer <patrick.steuer@de.ibm.com> Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#6813) (cherry picked from commit f39ad8d) Fixup header include Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com>

Signed-off-by: Patrick Steuer <patrick.steuer@de.ibm.com> Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#6813) (cherry picked from commit d68af00)

The OPENSSL_s390xcap environment variable is used to set bits in the s390x capability vector to zero. This simplifies testing of different code paths. Signed-off-by: Patrick Steuer <patrick.steuer@de.ibm.com> Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#6813) (cherry picked from commit f39ad8d) Fixup header include Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com>

Signed-off-by: Patrick Steuer <patrick.steuer@de.ibm.com> Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#6813) (cherry picked from commit d68af00)

p-steuer force-pushed the s390x-cap branch from 7e7ce1b to 2425227 Compare July 30, 2018 20:12

dot-asm reviewed Jul 31, 2018

View reviewed changes

dot-asm reviewed Aug 1, 2018

View reviewed changes

mattcaswell added this to the Post 1.1.1 milestone Aug 2, 2018

dot-asm reviewed Aug 2, 2018

View reviewed changes

dot-asm approved these changes Aug 3, 2018

View reviewed changes

dot-asm added the approval: review pending This pull request needs review by a committer label Aug 3, 2018

p-steuer added 2 commits August 6, 2018 14:00

s390x assembly pack: add OPENSSL_s390xcap environment variable.

5b66ac7

The OPENSSL_s390xcap environment variable is used to set bits in the s390x capability vector to zero. This simplifies testing of different code paths. Signed-off-by: Patrick Steuer <patrick.steuer@de.ibm.com>

s390x assembly pack: add OPENSSL_s390xcap man page.

fe5437e

Signed-off-by: Patrick Steuer <patrick.steuer@de.ibm.com>

p-steuer force-pushed the s390x-cap branch from 8603f14 to fe5437e Compare August 6, 2018 12:10

dot-asm approved these changes Aug 6, 2018

View reviewed changes

richsalz approved these changes Aug 6, 2018

View reviewed changes

richsalz added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Aug 6, 2018

mattcaswell removed the approval: done This pull request has the required number of approvals label Sep 11, 2018

levitte approved these changes Oct 12, 2018

View reviewed changes

levitte added the approval: done This pull request has the required number of approvals label Oct 12, 2018

dot-asm closed this Oct 17, 2018

p-steuer deleted the s390x-cap branch February 7, 2019 16:21


		Disables the vector facility.

		=item OPENSSL_s390xcap="km:~0x2800:~0;kimd:~0xc000000:~0"


		=head1 DESCRIPTION

		libcrypto supports z architecture instruction set extensions. These

Add OPENSSL_s390xcap environment variable #6813

Add OPENSSL_s390xcap environment variable #6813

Conversation

p-steuer commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-steuer commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dot-asm Aug 1, 2018 • edited

Choose a reason for hiding this comment

p-steuer Aug 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-steuer Aug 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-steuer commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dot-asm Aug 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dot-asm Aug 2, 2018 • edited

Choose a reason for hiding this comment

dot-asm Aug 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dot-asm Aug 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-steuer commented Aug 2, 2018

p-steuer commented Aug 6, 2018

p-steuer commented Sep 12, 2018

p-steuer commented Oct 12, 2018

dot-asm commented Oct 17, 2018

dot-asm Aug 1, 2018 •

edited

p-steuer Aug 1, 2018 •

edited

p-steuer Aug 1, 2018 •

edited

dot-asm Aug 2, 2018 •

edited

dot-asm Aug 2, 2018 •

edited

dot-asm Aug 2, 2018 •

edited

dot-asm Aug 2, 2018 •

edited