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

[Harald Freudenberger <freude@linux.ibm.com>]

On 2024-02-06 16:55, Anthony Krowiak wrote:
> On 2/6/24 8:42 AM, Janosch Frank wrote:
>> On 2/5/24 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>
>> 
>> [...]
>> 
>>>> +/* 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:
>> 
>> This library relies on QCI, hence we only check for stfle.
>> I see no sense in manually probing the whole APQN space.
> 
> 
> Makes sense. I was thrown off by the PQAP_FC enumeration which
> includes all of the AP function codes.
> 
> 
>> 
>> 
>> If stfle 12 is indicated I'd expect AP instructions to not generate 
>> exceptions or do they in a sane CPU model?
> 
> 
> No, I would not expect PQAP(QCI) to generate an exception if STFLE 12
> is indicated.
> 

Hm, I am not sure if you can rely just on checking stfle bit 12 and if 
that's available assume
you have AP instructions. I never tried this. But as far as I know the 
KVM guys there is a chance
that you see a stfle bit 12 but get an illegal instruction exception the 
moment you call
an AP instruction... Maybe check this before relying on such a thing.

>> 
>> 
>>>> +
>>>> +    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,
>>>> +    PQAP_BEST_AP,
>>> 
>>> 
>>> Maybe use abbreviations like your function names above?
>>> 
>>>     PQAP_TAPQ,
>>>     PQAP_RAPQ,
>>>     PQAP_ZAPQ,
>>>     PQAP_AQIC,
>>>     PQAP_QCI,
>>>     PQAP_QACT,
>>>     PQAP_QBAP
>>> 
>> 
>> Hmmmmmmm(TM)
>> My guess is that I tried making these constants readable without 
>> consulting architecture documents. But another option is using the 
>> constants that you suggested and adding comments with a long version.
> 
> 
> I think that works out better; you won't have to abbreviate the longer
> version which will make it easier to understand.
> 
> 
>> 
>> Will do
>> 
>> [...]
>> 
>>>> +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'
>> 
>> Hmm Linux uses qid
>> I'll change it to the Linux naming convention, might take me a while 
>> though
> 
> 
> Well, the AP bus uses qid, but the vfio_ap module and the architecture
> doc uses APQN. In any case, it's a nit and I'm not terribly concerned
> about it.
> 
> 
>> 
>>> 
>>> 
>>>> +} __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?
>>> 
>> 
>> It's not used in this library and therefore I chose to not name it to 
>> make structs a bit more readable.
> 
> 
> Okay, not a problem.
> 
> 
>> 
>>> 
>>>> +    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");
>>>> +
>>>> +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
>>