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

Re: [Xen-devel] [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

On 09.08.17 14:58, Jan Beulich wrote:
On 09.08.17 at 12:10, <julien.grall@xxxxxxx> wrote:
CC "THE REST" maintainers to get an opinion on the public headers.

Please be more specific as to what you expect - the only public
header affected here is ARM-specific.

On 08/08/17 21:08, Volodymyr Babchuk wrote:
+typedef struct {
+    uint32_t a[4];
+} xen_arm_smccc_uid;

This is not the normal way of encoding a UID type.
Just to be clear: you are proposing to store UID in such struct
struct uuid_t {
    unsigned32  time_low;
    unsigned16  time_mid;
    unsigned16  time_hi_and_version;
    unsigned8   clock_seq_hi_and_reserved;
    unsigned8   clock_seq_low;
    byte        node[6];

+#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)      \
+    ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ),                     \

This is not C89 compatible.
Oops, sorry. Didn't knew that XEN should be C89 compatible.
Is there any guide for novices? I didn't found anything useful in docs/ (not even coding style document). On wiki I have found only "Submitting_Xen_Project_Patches" page, which is very helpful, but it does not cover topics like which C standard to use.

+        ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0),              \
+        ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
+ * Hypervisor Service version.
+ *
+ * We can't use XEN version here, because of SMCCC requirements:
+ * Major revision should change every time SMC/HVC function is removed.
+ * Minor revision should change every time SMC/HVC function is added.
+ * So, it is SMCCC protocol revision code, not XEN version.

I don't understand this explanation - how is the situation here
different from some arbitrary, non-toolstack-only hypercall?
Because this is generic interface that should be supported by all hypervisors, including XEN. Think about this as a way for a guest to determine under which hypervisor it operates, and which functions it provides.

+ * Those values are subjected to change, when interface will be extended.
+ * They should not be stored in public/asm-arm/smc.h because they should
+ * be queried by guest using SMC/HVC interface.
+ */

The comment says the values shouldn't be put here, but then
they're still being put here?
Yeah, sorry. Missed this.

+/* Hypervisor Service UID. Randomly generated with 3rd party tool.  */
+#define XEN_SMCCC_UID XEN_ARM_SMCCC_UID(0xa71812dc, 0xc698, 0x4369, \
+                                        0x9a, 0xcf, 0x79, 0xd1, \
+                                        0x8d, 0xde, 0xe6, 0x67)

Why is the right side using _ARM_ in its name, but the macro being
defined isn't?


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.