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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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

[...]

+/* 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.




+
+    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@xxxxxxxxxxxxx>
+ *       Tony Krowiak <akrowia@xxxxxxxxxxxxx>
+ *       Martin Schwidefsky <schwidefsky@xxxxxxxxxx>
+ *       Harald Freudenberger <freude@xxxxxxxxxx>
+ */
+
+#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





[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux