Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
From: Arnd Bergmann <arnd@arndb.de>
To: Michael Kelley <mikelley@microsoft.com>
Cc: Will Deacon <will@kernel.org>, Ard Biesheuvel <ardb@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Marc Zyngier <maz@kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	gregkh <gregkh@linuxfoundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	linux-efi <linux-efi@vger.kernel.org>,
	linux-arch <linux-arch@vger.kernel.org>,
	"olaf@aepfle.de" <olaf@aepfle.de>,
	Andy Whitcroft <apw@canonical.com>,
	vkuznets <vkuznets@redhat.com>, Jason Wang <jasowang@redhat.com>,
	"marcelo.cerri@canonical.com" <marcelo.cerri@canonical.com>,
	KY Srinivasan <kys@microsoft.com>,
	Sunil Muthuswamy <sunilmut@microsoft.com>,
	Boqun Feng <boqun.feng@gmail.com>
Subject: Re: [PATCH v6 01/10] arm64: hyperv: Add core Hyper-V include files
Date: Wed, 18 Mar 2020 11:10:06 +0100
Message-ID: <CAK8P3a02EULGxyuKFq8YnbG8BQ_m-RKciaNEc9ZbdP2yz9dt+Q@mail.gmail.com> (raw)
In-Reply-To: <MW2PR2101MB10524879CD685710A51AB740D7F70@MW2PR2101MB1052.namprd21.prod.outlook.com>

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

  reply index

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-14 15:35 [PATCH v6 00/10] Subject: Enable Linux guests on Hyper-V on ARM64 Michael Kelley
2020-03-14 15:35 ` [PATCH v6 01/10] arm64: hyperv: Add core Hyper-V include files Michael Kelley
2020-03-15 17:31   ` Marc Zyngier
2020-03-18  0:10     ` Michael Kelley
2020-03-16  8:47   ` Arnd Bergmann
2020-03-18  0:12     ` Michael Kelley
2020-03-18 10:10       ` Arnd Bergmann [this message]
2020-03-19 21:31         ` Michael Kelley
2020-03-20 16:38           ` Arnd Bergmann
2020-03-14 15:35 ` [PATCH v6 02/10] arm/arm64: smccc-1.1: Add vendor specific owner definition Michael Kelley
2020-03-14 15:35 ` [PATCH v6 03/10] arm64: hyperv: Add hypercall and register access functions Michael Kelley
2020-03-14 15:35 ` [PATCH v6 04/10] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages Michael Kelley
2020-03-16  8:22   ` Arnd Bergmann
2020-03-16  8:30     ` gregkh
2020-03-16  8:30     ` Marc Zyngier
2020-03-16  8:32       ` Arnd Bergmann
2020-03-18  0:15         ` Michael Kelley
2020-03-18  9:57           ` Arnd Bergmann
2020-03-19 21:43             ` Michael Kelley
2020-03-20 16:28               ` Arnd Bergmann
2020-03-20 17:22                 ` Michael Kelley
2020-03-14 15:35 ` [PATCH v6 05/10] arm64: hyperv: Add interrupt handlers for VMbus and stimer Michael Kelley
2020-03-16  8:25   ` Arnd Bergmann
2020-03-18  0:16     ` Michael Kelley
2020-03-18  9:52       ` Arnd Bergmann
2020-03-14 15:35 ` [PATCH v6 06/10] arm64: hyperv: Add kexec and panic handlers Michael Kelley
2020-03-15 18:15   ` Marc Zyngier
2020-03-18  0:17     ` Michael Kelley
2020-03-14 15:35 ` [PATCH v6 07/10] arm64: hyperv: Initialize hypervisor on boot Michael Kelley
2020-03-16  8:29   ` Arnd Bergmann
2020-03-18  0:17     ` Michael Kelley
2020-03-18  9:44       ` Arnd Bergmann
2020-03-14 15:35 ` [PATCH v6 08/10] Drivers: hv: vmbus: Add hooks for per-CPU IRQ Michael Kelley
2020-03-14 15:35 ` [PATCH v6 09/10] arm64: efi: Export screen_info Michael Kelley
2020-03-16  8:20   ` Arnd Bergmann
2020-03-18  0:18     ` Michael Kelley
2020-03-18  9:26       ` Arnd Bergmann
2020-03-19 21:46         ` Michael Kelley
2020-05-13 14:26           ` Nikhil Mahale
2020-05-18  4:25             ` Nikhil Mahale
2020-05-18 12:51               ` Ard Biesheuvel
2020-05-22 11:14                 ` Nikhil Mahale
2020-05-22 11:32                   ` Ard Biesheuvel
2020-03-14 15:35 ` [PATCH v6 10/10] Drivers: hv: Enable Hyper-V code to be built on ARM64 Michael Kelley
     [not found] ` <20200318031130.5476-1-hdanton@sina.com>
2020-03-19 21:04   ` [PATCH v6 03/10] arm64: hyperv: Add hypercall and register access functions Michael Kelley

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAK8P3a02EULGxyuKFq8YnbG8BQ_m-RKciaNEc9ZbdP2yz9dt+Q@mail.gmail.com \
    --to=arnd@arndb.de \
    --cc=apw@canonical.com \
    --cc=ardb@kernel.org \
    --cc=boqun.feng@gmail.com \
    --cc=catalin.marinas@arm.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jasowang@redhat.com \
    --cc=kys@microsoft.com \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marcelo.cerri@canonical.com \
    --cc=mark.rutland@arm.com \
    --cc=maz@kernel.org \
    --cc=mikelley@microsoft.com \
    --cc=olaf@aepfle.de \
    --cc=sunilmut@microsoft.com \
    --cc=vkuznets@redhat.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-HyperV Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hyperv/0 linux-hyperv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hyperv linux-hyperv/ https://lore.kernel.org/linux-hyperv \
		linux-hyperv@vger.kernel.org
	public-inbox-index linux-hyperv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hyperv


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git