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;
>> + 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.
>
>> };
>> };
>> +enum prot_code {
>> + PROT_KEY_LAP,
>
> That's key OR LAP, right?
Yes, do you want me to make that explicit?
>
>> + 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);
?
>
> Do we have tests that require SOP/no-SOP?
No, just going for correctness.
