Re: [PATCH v6 01/10] arm64: hyperv: Add core Hyper-V include files

[Arnd Bergmann <arnd@arndb.de>]

On Wed, Mar 18, 2020 at 1:12 AM Michael Kelley <mikelley@microsoft.com> wrote:
> 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 <mikelley@microsoft.com> wrote:
> >
> > > +
> > > +/* 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.

Just drop the __packed annotations then, they just confuse the compiler
in this case. In particular, when the compiler thinks that a structure is
misaligned, it tries to avoid using load/store instructions on it that are
inefficient or trap with misaligned code, so having default alignment
produces better object code.

> > 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?

Yes, device drivers should generally define data structures using
the __le32, __le64 etc types, and use the conversion functions
to access them. Building with 'make C=1' usually tells you when
you have mismatched annotations.

> 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.

In general, the use of big-endian software on Linux is declining, however

- arm64 as an architecture is meant to support both endian types,
  and we still try to ensure it works either way as long as there are
  users that depend on it.

- The remaining users of big-endian software are probably
  more likely to run on virtual machines than on real hardware

- Any device driver should generally be written against portable
  interfaces, even if you think you know how it will be used. As
  driver writers tend to look at existing code for new drivers, it's
  better to have them all be portable. (This is a similar argument
  to the irqchip interface).

Even if you don't convert any of the existing architecture independent
code to run both ways, I see no reason to not do it for new drivers.

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

How so? If this file is arm64 specific, there should be no need to make
x86 do the same change.

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

Ok.

        Arnd