On 6/10/22 12:37, Janis Schoetterl-Glausch wrote:
On 6/10/22 11:31, Janosch Frank wrote:
On 6/8/22 15:33, Janis Schoetterl-Glausch wrote:
The translation-exception identification (TEID) contains information to
identify the cause of certain program exceptions, including translation
exceptions occurring during dynamic address translation, as well as
protection exceptions.
The meaning of fields in the TEID is complex, depending on the exception
occurring and various potentially installed facilities.
Rework the type describing the TEID, in order to ease decoding.
Change the existing code interpreting the TEID and extend it to take the
installed suppression-on-protection facility into account.
Signed-off-by: Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx>
---
lib/s390x/asm/interrupt.h | 61 +++++++++++++++++++++++++++---------
lib/s390x/fault.h | 30 +++++-------------
lib/s390x/fault.c | 65 ++++++++++++++++++++++++++-------------
lib/s390x/interrupt.c | 2 +-
s390x/edat.c | 26 ++++++++++------
5 files changed, 115 insertions(+), 69 deletions(-)
diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
index d9ab0bd7..3ca6bf76 100644
--- a/lib/s390x/asm/interrupt.h
+++ b/lib/s390x/asm/interrupt.h
@@ -20,23 +20,56 @@
union teid {
unsigned long val;
- struct {
- unsigned long addr:52;
- unsigned long fetch:1;
- unsigned long store:1;
- unsigned long reserved:6;
- unsigned long acc_list_prot:1;
- /*
- * depending on the exception and the installed facilities,
- * the m field can indicate several different things,
- * including whether the exception was triggered by a MVPG
- * instruction, or whether the addr field is meaningful
- */
- unsigned long m:1;
- unsigned long asce_id:2;
+ union {
+ /* common fields DAT exc & protection exc */
+ struct {
+ uint64_t addr : 52 - 0;
+ uint64_t acc_exc_f_s : 54 - 52;
I'd either name it acc_exc_fs or spell it out properly.
+ uint64_t side_effect_acc : 55 - 54;
+ uint64_t /* reserved */ : 62 - 55;
+ uint64_t asce_id : 64 - 62;
+ };
+ /* DAT exc */
+ struct {
+ uint64_t /* pad */ : 61 - 0;
+ uint64_t dat_move_page : 62 - 61;
+ };
+ /* suppression on protection */
+ struct {
+ uint64_t /* pad */ : 60 - 0;
+ uint64_t sop_acc_list : 61 - 60;
+ uint64_t sop_teid_predictable : 62 - 61;
+ };
+ /* enhanced suppression on protection 2 */
+ struct {
+ uint64_t /* pad */ : 56 - 0;
+ uint64_t esop2_prot_code_0 : 57 - 56;
+ uint64_t /* pad */ : 60 - 57;
+ uint64_t esop2_prot_code_1 : 61 - 60;
+ uint64_t esop2_prot_code_2 : 62 - 61;
+ };
Quite messy, would it be more readable to unionize the fields that overlap?
Not sure, I prefer this because it reflects the structure of the PoP,
where there is a section for DAT exceptions, SOP, ESOP1, ESOP2.
It's not exactly like this in the code because I factored out common fields,
and I removed the struct for ESOP1 because it was mostly redundant with SOP.
Well, the rest of the code is readable and I can compare the struct to
the POP so I'm okish with this.
};
};
+enum prot_code {
+ PROT_KEY_LAP,
That's key OR LAP, right?
Yes, do you want me to make that explicit?
Yes
+ PROT_DAT,
+ PROT_KEY,
+ PROT_ACC_LIST,
+ PROT_LAP,
+ PROT_IEP,
+};
+
Yes, I like that more than my quick fixes :-)
+static void print_decode_pgm_prot(union teid teid, bool dat)
+{
+ switch (get_supp_on_prot_facility()) {
+ case SOP_NONE:
+ printf("Type: ?\n");
+ break;
+ case SOP_BASIC:
+ if (teid.sop_teid_predictable && dat && teid.sop_acc_list)
+ printf("Type: ACC\n");
+ else
+ printf("Type: ?\n");
+ break;
I'm wondering if we should cut off the two possibilities above to make it a bit more sane. The SOP facility is about my age now and ESOP1 has been introduced with z10 if I'm not mistaken so it's not young either.
So
case SOP_NONE:
case SOP_BASIC:
assert(false);
?
I'd check (e)sop on initialization and abort early so we never need to
worry about it in other files.
Do we have tests that require SOP/no-SOP?
No, just going for correctness.
