Re: [kvm-unit-tests PATCH v2 3/3] s390x: Rework TEID decoding and usage

[Janis Schoetterl-Glausch <scgl@linux.ibm.com>]

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@linux.ibm.com>
>> ---
>>   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.