Linux-EFI 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: Fri, 20 Mar 2020 17:38:09 +0100
Message-ID: <CAK8P3a0y==1RsoMOnME9bgP5V_mts4rbaUW08Tt7mS9csXBqDw@mail.gmail.com> (raw)
In-Reply-To: <MW2PR2101MB1052686237C57955148F173ED7F40@MW2PR2101MB1052.namprd21.prod.outlook.com>

On Thu, Mar 19, 2020 at 10:31 PM Michael Kelley <mikelley@microsoft.com> wrote:
> From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 3:10 AM
> > 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.
>
> So I'm confused a bit.  Were the original concerns in the above LKML
> discussion bogus?  Is it legal for the compiler to reorder fields or add
> padding, even if the layout of fields in the structure doesn't require it?
> If the compiler *could* do such, then it seems like keeping the __packed
> would be appropriate per the LKML discussion.

The padding is defined in the ELF psABI document for a particular
architecture. In theory an architecture might require padding around
smaller members, but they generally don't when you look at the ones
that Linux runs on. The few odd ones are those that require less
padding, only aligning members to 16 or 32 bit rather than natural
alignment, or padding the size of the structure to 32 bit even if it
only contains 8-bit or 16-bit members. When you have structures
in which every member is naturally aligned and the size it a multiple
of 32 bit, better leave out the __packed.

Aside from generating sub-optimal code, the __packed annotation
can also lead to undefined behavior, if you pass a pointer to
an unaligned member into a function call that takes an aligned
pointer. Newer compilers warn about this.

> > > 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.
>
> This file, hyperv-tlfs.h, is duplicating some definitions on the x86 and
> ARM64 sides that are used by arch independent code, and this is one
> of those definitions.  I had held off on breaking the file into arch
> independent and arch specific portions because the Hyper-V team has
> left some gray areas for functionality that isn't yet used on the ARM64
> side.  So in some cases, it's hard to know what functionality to put
> into the arch independent portion.
>
> But I think I'll go ahead and make the separation with reasonably good
> accuracy, and update the x86 side accordingly.  That will reduce the size
> of this patch set to contain only the things that we know are ARM64
> specific and which are actually used by the ARM64 code.  Things like the
> hv_message_flags will go into the arch independent portion so that
> they can be used by the arch independent code without cluttering up
> the arch specific code.  Making the change will help reduce any
> confusion about what is ARM64-specific. The other core #include file,
> mshyperv.h, has already been done this way.

Ok, sounds good.

     Arnd

  reply index

Thread overview: 40+ 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
2020-03-19 21:31         ` Michael Kelley
2020-03-20 16:38           ` Arnd Bergmann [this message]
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-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='CAK8P3a0y==1RsoMOnME9bgP5V_mts4rbaUW08Tt7mS9csXBqDw@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-EFI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-efi/0 linux-efi/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-efi linux-efi/ https://lore.kernel.org/linux-efi \
		linux-efi@vger.kernel.org
	public-inbox-index linux-efi

Example config snippet for mirrors

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


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