Re: [kvm-unit-tests PATCH v4 1/7] lib: s390x: Add ap library

[Anthony Krowiak <akrowiak@linux.ibm.com>]

On 2/6/24 3:48 AM, Harald Freudenberger wrote:
> On 2024-02-05 19:15, Anthony Krowiak wrote:
>> I made a few comments and suggestions. I am not very well-versed in
>> the inline assembly code, so I'll leave that up to someone who is more
>> knowledgeable. I copied @Harald since I believe it was him who wrote
>> it.
>>
>> On 2/2/24 9:59 AM, Janosch Frank wrote:
>>> Add functions and definitions needed to test the Adjunct
>>> Processor (AP) crypto interface.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>   lib/s390x/ap.c | 97 
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   lib/s390x/ap.h | 88 +++++++++++++++++++++++++++++++++++++++++++++
>>>   s390x/Makefile |  1 +
>>>   3 files changed, 186 insertions(+)
>>>   create mode 100644 lib/s390x/ap.c
>>>   create mode 100644 lib/s390x/ap.h
>>>
>>> diff --git a/lib/s390x/ap.c b/lib/s390x/ap.c
>>> new file mode 100644
>>> index 00000000..9560ff64
>>> --- /dev/null
>>> +++ b/lib/s390x/ap.c
>>> @@ -0,0 +1,97 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * AP crypto functions
>>> + *
>>> + * Some parts taken from the Linux AP driver.
>>> + *
>>> + * Copyright IBM Corp. 2024
>>> + * Author: Janosch Frank <frankja@linux.ibm.com>
>>> + *       Tony Krowiak <akrowia@linux.ibm.com>
>>> + *       Martin Schwidefsky <schwidefsky@de.ibm.com>
>>> + *       Harald Freudenberger <freude@de.ibm.com>
>>> + */
>>> +
>>> +#include <libcflat.h>
>>> +#include <interrupt.h>
>>> +#include <ap.h>
>>> +#include <asm/time.h>
>>> +#include <asm/facility.h>
>>> +
>>> +int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status 
>>> *apqsw,
>>> +         struct pqap_r2 *r2)
>>> +{
>>> +    struct pqap_r0 r0 = {
>>> +        .ap = ap,
>>> +        .qn = qn,
>>> +        .fc = PQAP_TEST_APQ
>>> +    };
>>> +    uint64_t bogus_cc = 2;
>>> +    int cc;
>>> +
>>> +    /*
>>> +     * Test AP Queue
>>> +     *
>>> +     * Writes AP configuration information to the memory pointed
>>> +     * at by GR2.
>>> +     *
>>> +     * Inputs: GR0
>>> +     * Outputs: GR1 (APQSW), GR2 (tapq data)
>>> +     * Synchronous
>>> +     */
>>> +    asm volatile(
>>> +        "       tmll    %[bogus_cc],3\n"
>>> +        "    lgr    0,%[r0]\n"
>>> +        "    .insn    rre,0xb2af0000,0,0\n" /* PQAP */
>>> +        "    stg    1,%[apqsw]\n"
>>> +        "    stg    2,%[r2]\n"
>>> +        "    ipm    %[cc]\n"
>>> +        "    srl    %[cc],28\n"
>>> +        : [apqsw] "=&T" (*apqsw), [r2] "=&T" (*r2), [cc] "=&d" (cc)
>>> +        : [r0] "d" (r0), [bogus_cc] "d" (bogus_cc));
>>> +
>>> +    return cc;
>>> +}
>>> +
>>> +int ap_pqap_qci(struct ap_config_info *info)
>>> +{
>>> +    struct pqap_r0 r0 = { .fc = PQAP_QUERY_AP_CONF_INFO };
>>> +    uint64_t bogus_cc = 2;
>>> +    int cc;
>>> +
>>> +    /*
>>> +     * Query AP Configuration Information
>>> +     *
>>> +     * Writes AP configuration information to the memory pointed
>>> +     * at by GR2.
>>> +     *
>>> +     * Inputs: GR0, GR2 (QCI block address)
>>> +     * Outputs: memory at GR2 address
>>> +     * Synchronous
>>> +     */
>>> +    asm volatile(
>>> +        "       tmll    %[bogus_cc],3\n"
>>> +        "    lgr    0,%[r0]\n"
>>> +        "    lgr    2,%[info]\n"
>>> +        "    .insn    rre,0xb2af0000,0,0\n" /* PQAP */
>>> +        "    ipm    %[cc]\n"
>>> +        "    srl    %[cc],28\n"
>>> +        : [cc] "=&d" (cc)
>>> +        : [r0] "d" (r0), [info] "d" (info), [bogus_cc] "d" (bogus_cc)
>>> +        : "cc", "memory", "0", "2");
>>> +
>>> +    return cc;
>>> +}
>>> +
>>> +/* Will later be extended to a proper setup function */
>>> +bool ap_setup(void)
>>> +{
>>> +    /*
>>> +     * Base AP support has no STFLE or SCLP feature bit but the
>>> +     * PQAP QCI support is indicated via stfle bit 12. As this
>>> +     * library relies on QCI we bail out if it's not available.
>>> +     */
>>> +    if (!test_facility(12))
>>> +        return false;
>>
>>
>> The STFLE.12 can be turned off when starting the guest, so this may
>> not be a valid test.
>>
>> We use the ap_instructions_available function (in ap.h) which executes
>> the TAPQ command to verify whether the AP instructions are installed
>> or not. Maybe you can do something similar here:
>>
>>
>> static inline bool ap_instructions_available(void)
>>
>> {
>>     unsigned long reg0 = 0x0000;
>>     unsigned long reg1 = 0;
>>
>>     asm volatile(
>>         "    lgr    0,%[reg0]\n"        /* qid into gr0 */
>>         "    lghi    1,0\n"            /* 0 into gr1 */
>>         "    lghi    2,0\n"            /* 0 into gr2 */
>>         "    .insn    rre,0xb2af0000,0,0\n"    /* PQAP(TAPQ) */
>>         "0:    la    %[reg1],1\n"        /* 1 into reg1 */
>>         "1:\n"
>>         EX_TABLE(0b, 1b)
>>         : [reg1] "+&d" (reg1)
>>         : [reg0] "d" (reg0)
>>         : "cc", "0", "1", "2");
>>     return reg1 != 0;
>> }
>
> Well, as Janos wrote - this lib functions rely on having QCI available.
> However, be carefully using the above function as it is setting up
> an exception table entry which is to catch the illegal instruction
> which will arise when no AP bus support is available.
>
>>> +
>>> +    return true;
>>> +}
>>> diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
>>> new file mode 100644
>>> index 00000000..b806513f
>>> --- /dev/null
>>> +++ b/lib/s390x/ap.h
>>> @@ -0,0 +1,88 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * AP definitions
>>> + *
>>> + * Some parts taken from the Linux AP driver.
>>> + *
>>> + * Copyright IBM Corp. 2024
>>> + * Author: Janosch Frank <frankja@linux.ibm.com>
>>> + *       Tony Krowiak <akrowia@linux.ibm.com>
>>> + *       Martin Schwidefsky <schwidefsky@de.ibm.com>
>>> + *       Harald Freudenberger <freude@de.ibm.com>
>>> + */
>>> +
>>> +#ifndef _S390X_AP_H_
>>> +#define _S390X_AP_H_
>>> +
>>> +enum PQAP_FC {
>>> +    PQAP_TEST_APQ,
>>> +    PQAP_RESET_APQ,
>>> +    PQAP_ZEROIZE_APQ,
>>> +    PQAP_QUEUE_INT_CONTRL,
>>> +    PQAP_QUERY_AP_CONF_INFO,
>>> +    PQAP_QUERY_AP_COMP_TYPE,
>
> What is that       ^^^^^^^^^^^^^ ?
> The QACT ? But that's only one PQAP subfuntion.


PQAP(QBAP)?


>
>>> +    PQAP_BEST_AP,
>
> And this ^^^^^^^^^^^^ ? Never heard about.
>
>>
>>
>> Maybe use abbreviations like your function names above?
>>
>>     PQAP_TAPQ,
>>     PQAP_RAPQ,
>>     PQAP_ZAPQ,
>>     PQAP_AQIC,
>>     PQAP_QCI,
>>     PQAP_QACT,
>>     PQAP_QBAP
>>
>>
>>> +};
>>> +
>>> +struct ap_queue_status {
>>> +    uint32_t pad0;            /* Ignored padding for zArch */
>>
>>
>> Bit 0 is the E (queue empty) bit.
>>
>>
>>> +    uint32_t empty        : 1;
>>> +    uint32_t replies_waiting: 1;
>>> +    uint32_t full        : 1;
>>> +    uint32_t pad1        : 4;
>>> +    uint32_t irq_enabled    : 1;
>>> +    uint32_t rc        : 8;
>>> +    uint32_t pad2        : 16;
>>> +} __attribute__((packed))  __attribute__((aligned(8)));
>>> +_Static_assert(sizeof(struct ap_queue_status) == sizeof(uint64_t), 
>>> "APQSW size");
>>> +
>>> +struct ap_config_info {
>>> +    uint8_t apsc     : 1;    /* S bit */
>>> +    uint8_t apxa     : 1;    /* N bit */
>>> +    uint8_t qact     : 1;    /* C bit */
>>> +    uint8_t rc8a     : 1;    /* R bit */
>>> +    uint8_t l     : 1;    /* L bit */
>>> +    uint8_t lext     : 3;    /* Lext bits */
>>> +    uint8_t reserved2[3];
>>> +    uint8_t Na;        /* max # of APs - 1 */
>>> +    uint8_t Nd;        /* max # of Domains - 1 */
>>> +    uint8_t reserved6[10];
>>> +    uint32_t apm[8];    /* AP ID mask */
>>> +    uint32_t aqm[8];    /* AP (usage) queue mask */
>>> +    uint32_t adm[8];    /* AP (control) domain mask */
>>> +    uint8_t _reserved4[16];
>>> +} __attribute__((aligned(8))) __attribute__ ((__packed__));
>>> +_Static_assert(sizeof(struct ap_config_info) == 128, "PQAP QCI size");
>>> +
>>> +struct pqap_r0 {
>>> +    uint32_t pad0;
>>> +    uint8_t fc;
>>> +    uint8_t t : 1;        /* Test facilities (TAPQ)*/
>>> +    uint8_t pad1 : 7;
>>> +    uint8_t ap;
>>
>>
>> This is the APID part of an APQN, so how about renaming to 'apid'
>>
>>
>>> +    uint8_t qn;
>>
>>
>> This is the APQI  part of an APQN, so how about renaming to 'apqi'
>>
>>
>>> +} __attribute__((packed)) __attribute__((aligned(8)));
>>> +
>>> +struct pqap_r2 {
>>> +    uint8_t s : 1;        /* Special Command facility */
>>> +    uint8_t m : 1;        /* AP4KM */
>>> +    uint8_t c : 1;        /* AP4KC */
>>> +    uint8_t cop : 1;    /* AP is in coprocessor mode */
>>> +    uint8_t acc : 1;    /* AP is in accelerator mode */
>>> +    uint8_t xcp : 1;    /* AP is in XCP-mode */
>>> +    uint8_t n : 1;        /* AP extended addressing facility */
>>> +    uint8_t pad_0 : 1;
>>> +    uint8_t pad_1[3];
>>
>>
>> Is there a reason why the 'Classification'  field is left out?
>>
>>
>>> +    uint8_t at;
>>> +    uint8_t nd;
>>> +    uint8_t pad_6;
>>> +    uint8_t pad_7 : 4;
>>> +    uint8_t qd : 4;
>>> +} __attribute__((packed))  __attribute__((aligned(8)));
>>> +_Static_assert(sizeof(struct pqap_r2) == sizeof(uint64_t), "pqap_r2 
>>> size");
>>> +
>
> Isn't this all going into the kernel? So consider using the
> arch/s390/include/asm/ap.h header file instead of re-defining the 
> structs.
> Feel free to improve this file with reworked structs and field names if
> that's required.
>
>>> +bool ap_setup(void);
>>> +int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status 
>>> *apqsw,
>>> +         struct pqap_r2 *r2);
>>> +int ap_pqap_qci(struct ap_config_info *info);
>>> +#endif
>>> diff --git a/s390x/Makefile b/s390x/Makefile
>>> index 7fce9f9d..4f6c627d 100644
>>> --- a/s390x/Makefile
>>> +++ b/s390x/Makefile
>>> @@ -110,6 +110,7 @@ cflatobjs += lib/s390x/malloc_io.o
>>>   cflatobjs += lib/s390x/uv.o
>>>   cflatobjs += lib/s390x/sie.o
>>>   cflatobjs += lib/s390x/fault.o
>>> +cflatobjs += lib/s390x/ap.o
>>>     OBJDIRS += lib/s390x
>>>
>