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

[Janosch Frank <frankja@linux.ibm.com>]

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@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;

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.
>