From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:48 AM

On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley [off-list ref] wrote:

quoted

+
+/* Define input and output layout for Get VP Register hypercall */
+struct hv_get_vp_register_input {
+       u64 partitionid;
+       u32 vpindex;
+       u8  inputvtl;
+       u8  padding[3];
+       u32 name0;
+       u32 name1;
+} __packed;

Are you sure these need to be made byte-aligned according to the
specification? If the structure itself is aligned to 64 bit, better mark only
the individual fields that are misaligned as __packed.

If the structure is aligned to only 32-bit addresses instead of
64-bit, mark it as "__packed __aligned(4)" to let the compiler
generate better code for accessing it.

None of the fields are misaligned and it will always be aligned to 64-bit
addresses, so there should be no padding needed or added.  There was
a discussion of __packed and the Hyper-V data structures in general on
LKML here:  https://lkml.org/lkml/2018/11/30/848.  Adding __packed was
done as a preventative measure, not because anything was actually
broken.  Marking as __aligned(8) here would indicate the correct intent,
though the use of the structure always ensures 64-bit alignment.

Also, in order to write portable code, it would be helpful to mark
all the fields as explicitly little-endian, and use __le32_to_cpu()
etc for accessing them.

There's an opening comment in this file stating that all data
structures shared between Hyper-V and a guest VM are little
endian.  Is there some other marking to consider using?

We have definitely not allowed for the case of Hyper-V running on
a big endian architecture.  There are a *lot* of messages and data
structures passed between the guest and Hyper-V, and coding
to handle either endianness is a big project.  I'm doubtful
of the value until and unless we actually have a need for it.

quoted

+/* Define synthetic interrupt controller message flags. */
+union hv_message_flags {
+       __u8 asu8;
+       struct {
+               __u8 msg_pending:1;
+               __u8 reserved:7;
+       } __packed;
+};

For similar reasons, please avoid bit fields and just use a
bit mask on the first member of the union.

Unfortunately, changing to a bit mask ripples into
architecture independent code and into the x86
implementation.  I'd prefer not to drag that complexity
into this patch set.

The __packed annotation here is not meaningful,
as the total size is already only a single byte.

Agreed.

quoted

+/* Define port identifier type. */
+union hv_port_id {
+       __u32 asu32;
+       struct {
+               __u32 id:24;
+               __u32 reserved:8;
+       }  __packed u;
+};

Here, the __packed annotation is inconsistent with the use
in the rest of the file: marking only one member of the union
as __packed means that the union itself is still expected to
be 4 byte aligned. I would expect that either all of these
structures have a sensible alignment, or they are all
completely unaligned.

Agreed.  Looks like it is wrong on the x86 side too.  

quoted

+ * Use the Hyper-V provided stimer0 as the timer that is made
+ * available to the architecture independent Hyper-V drivers.
+ */
+#define hv_init_timer(timer, tick) \
+               hv_set_vpreg(HV_REGISTER_STIMER0_COUNT + (2*timer), tick)
+#define hv_init_timer_config(timer, val) \
+               hv_set_vpreg(HV_REGISTER_STIMER0_CONFIG + (2*timer), val)
+#define hv_get_current_tick(tick) \
+               (tick = hv_get_vpreg(HV_REGISTER_TIME_REFCOUNT))

In general, we prefer inline functions over macros in header files.

I can change the "set" calls to inline functions.  As you can see, the "get"
functions are coded and used in architecture independent code and on
the x86 side in a way that won't convert to inline functions.

quoted

+#if IS_ENABLED(CONFIG_HYPERV)
+#define hv_enable_stimer0_percpu_irq(irq)      enable_percpu_irq(irq, 0)
+#define hv_disable_stimer0_percpu_irq(irq)     disable_percpu_irq(irq)
+#endif

Should there be an #else definition here? It helps readability
to have the two versions (with and without hyperv support) close
together rather than in different files. If there is no other
definition, just drop the #if.

Agreed.  I'll figure out a way to handle this better.

     Arnd