From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::343; helo=mail-wm1-x343.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm1-x343.google.com (mail-wm1-x343.google.com [IPv6:2a00:1450:4864:20::343]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B08C0211982F7 for ; Tue, 4 Dec 2018 01:33:28 -0800 (PST) Received: by mail-wm1-x343.google.com with SMTP id r11-v6so8747232wmb.2 for ; Tue, 04 Dec 2018 01:33:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=lvDlOtG6jNA5BbOE7fm4GlWZFUJZMCDt1eAkEynNhy4=; b=He6+advdE3abqLswqZQ3B73zkL6kzZBUU35ShPKL7b2HffNiWYv+z2J2BkHKVkyQm4 A0K9sOcAHu/BELxjJ4uUMAL0/YBeUBcUJBZKWW0wRQ/Y3KdvXcD3hPsgwsmyvoonQScu S+pL0vKO+mzxYW5NDKmowF07lZibthbNQSkQg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=lvDlOtG6jNA5BbOE7fm4GlWZFUJZMCDt1eAkEynNhy4=; b=J0IOkzJK+0f/IaYIJSQx00ptClu/r/H2xJcqED62EnLjbcphSwOZ+SK3IGP1jF2fJ1 N5LcykKfGvBcOMrEe7fschhGDeRovqgSgpPXdzsxLWuee1J2IUwdkVezVAr2cBASlpAJ FLZ3TU1K6vKcZgc6NYSb9HseoMiFEL5uCjCYg6BPuIAfXKEB6nv5H+3CZWqjL6+hueNu T6d0sDqFk5MsfOSttAON1xK4OVPS1VM0xjUrK0hLGJlIJiKTx/exebbR/pZ6DLZJaLEC 3HDfyBTaNpGJQDsCXjKFcHMatc9jFQheV8E8d/92e1Zmf7vc/eHQ+kLtHuMIYkBz4++Q TDCQ== X-Gm-Message-State: AA+aEWZUK8R2iHkbgldO+241yOld87abLN9GWytDuvaLqVTcaaU0Jh2A ekmdD7FQdCPtxdNpK+tdvcD4sA== X-Google-Smtp-Source: AFSGD/XpPLIUxuF7FeW/zyU4T3gT8DHNs8+6XayKHdNqtoj3fbsxN6PwAX525JpXwGTpxtvZ8vChPA== X-Received: by 2002:a1c:7619:: with SMTP id r25mr12411141wmc.7.1543916006731; Tue, 04 Dec 2018 01:33:26 -0800 (PST) Received: from [100.66.247.109] (183.82.136.77.rev.sfr.net. [77.136.82.183]) by smtp.gmail.com with ESMTPSA id l37sm22522776wre.69.2018.12.04.01.33.25 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 04 Dec 2018 01:33:25 -0800 (PST) Mime-Version: 1.0 (1.0) From: Ard Biesheuvel X-Mailer: iPhone Mail (16A366) In-Reply-To: Date: Tue, 4 Dec 2018 10:33:23 +0100 Cc: Leif Lindholm , "edk2-devel@lists.01.org" , Michael D Kinney Message-Id: <0C3C717E-BA00-46A9-844D-DE013F89E944@linaro.org> References: <20180921082542.35768-1-christopher.co@microsoft.com> <20180921082542.35768-10-christopher.co@microsoft.com> <20181101182020.w5qvmjbi3ukhxf2t@bivouac.eciton.net> <20181203094241.n6nbk4zvs73xf4k3@bivouac.eciton.net> To: Chris Co Subject: Re: [PATCH edk2-platforms 09/27] Silicon/NXP: Add headers for SoC-specific i.MX packages to use X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 Dec 2018 09:33:29 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable > On 4 Dec 2018, at 02:44, Chris Co wrote: >=20 >=20 >=20 >> -----Original Message----- >> From: Leif Lindholm >> Sent: Monday, December 3, 2018 1:43 AM >> To: Chris Co >> Cc: edk2-devel@lists.01.org; Ard Biesheuvel ; >> Michael D Kinney >> Subject: Re: [PATCH edk2-platforms 09/27] Silicon/NXP: Add headers for So= C- >> specific i.MX packages to use >>=20 >> On Sat, Dec 01, 2018 at 12:22:17AM +0000, Chris Co wrote: >>>> If using EFI_ACPI prefix, these #defines really should be in edk2 >>>> MdePkg. And CSRT itself is, so that might not be a bad idea. >>>>=20 >>>>> + >>>>> +#pragma pack(push, 1) >>>>=20 >>>> I don't see this #pragma making any difference to the structs below, >>>> can it be dropped? >>>=20 >>> The pragma pack is defensive. Without it, we rely on the compiler >>> packing structures by default and this may not happen on 64 bit >>> compiles. >>=20 >> I understand that is what the pragma does. My comment was because all of t= he >> variables in the below structs are naturally aligned. >> The reason I dislike its use when effectively a no-op, is that it makes i= t look like it >> it isn't a no-op. >>=20 >> If it covers a larger set of structs, some of which require the packed at= tribute I'm >> OK with that. But I'm not a fan of adding it "just in case" without conte= mplating >> the statement's (lack of) effect. >>=20 >> Regards, >>=20 >> Leif >>=20 >=20 > Makes sense. I am checking to make sure this pragma wasn't included due to= some observed compiler behavior on our end, since this header is also used o= n our ARM64 work. > Will remove it once confirmed it is safe. I=E2=80=99d prefer to keep the pragma. It doesn=E2=80=99t only remove paddin= g, it also informs the compiler that the whole struct may appear at any unal= igned offset. On 32 bit ARM, this means the compiler will refrain from using= load/store double or load/store multiple to access the contents when derefe= rencing a pointer to such a struct, which would result in a crash otherwise.= >=20 >>> I have addressed the remaining feedback and will resubmit with v2. >>>=20 >>> Thanks, >>> Chris >>>=20 >>>>> +//--------------------------------------------------------------- >>>>> +---- >>>>> +----- // CSRT Resource Group header 24 bytes long >>>>> +//--------------------------------------------------------------- >>>>> +---- >>>>> +----- >>>>> +typedef struct { >>>>> + UINT32 Length; >>>>> + UINT32 VendorID; >>>>> + UINT32 SubVendorId; >>>>> + UINT16 DeviceId; >>>>> + UINT16 SubdeviceId; >>>>> + UINT16 Revision; >>>>> + UINT16 Reserved; >>>>> + UINT32 SharedInfoLength; >>>>> +} EFI_ACPI_5_0_CSRT_RESOURCE_GROUP_HEADER; >>>>> + >>>>> +//--------------------------------------------------------------- >>>>> +---- >>>>> +----- // CSRT Resource Descriptor 12 bytes total >>>>> +//--------------------------------------------------------------- >>>>> +---- >>>>> +----- >>>>> +typedef struct { >>>>> + UINT32 Length; >>>>> + UINT16 ResourceType; >>>>> + UINT16 ResourceSubType; >>>>> + UINT32 UID; >>>>> +} EFI_ACPI_5_0_CSRT_RESOURCE_DESCRIPTOR_HEADER; >>>>> +#pragma pack (pop)