From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 277C6211C6063 for ; Fri, 1 Feb 2019 04:14:17 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 731B78E678; Fri, 1 Feb 2019 12:14:16 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-49.rdu2.redhat.com [10.10.120.49]) by smtp.corp.redhat.com (Postfix) with ESMTP id 565A6600CC; Fri, 1 Feb 2019 12:14:15 +0000 (UTC) To: Nikita Leshenko , edk2-devel@lists.01.org Cc: liran.alon@oracle.com References: <20190131100724.20907-1-nikita.leshchenko@oracle.com> <20190131100724.20907-12-nikita.leshchenko@oracle.com> From: Laszlo Ersek Message-ID: <3a80c695-9802-1f27-54f5-c55435e518a6@redhat.com> Date: Fri, 1 Feb 2019 13:14:14 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190131100724.20907-12-nikita.leshchenko@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Fri, 01 Feb 2019 12:14:16 +0000 (UTC) Subject: Re: [PATCH 11/14] OvmfPkg/MptScsiDxe: Initialize hardware 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: Fri, 01 Feb 2019 12:14:17 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Nikita, I've jumped ahead a little bit to point out other style issues, so you can address them at once in v2. These are again general comments, likely applying to several patches in the series. On 01/31/19 11:07, Nikita Leshenko wrote: > +typedef struct { > + UINT8 WhoInit; > + UINT8 Reserved1; > + UINT8 ChainOffset; > + UINT8 Function; > + UINT8 Flags; > + UINT8 MaxDevices; > + UINT8 MaxBuses; > + UINT8 MessageFlags; > + UINT32 MessageContext; > + UINT16 ReplyFrameSize; > + UINT16 Reserved2; > + UINT32 HostMfaHighAddr; > + UINT32 SenseBufferHighAddr; > +} __attribute__((aligned(8), packed)) MPT_IO_CONTROLLER_INIT_REQUEST; // Align required by HW (1) For packing, you need to say: #pragma pack (1) ... #pragma pack () (2) For enforcing an alignment of 8 bytes, I think you'll have to create an outer union first, and place a UINT64 member into it, as one member. The other member can be the structure you actually need. And, all further variable definitions (with static or auto storage duration) will have to occur through the union type. For dynamically allocated objects, the maximum alignment is ensured anyway. Perhaps other edk2 maintainers can give you a better tip for enforcing a minimum alignment for local & global variables; I'm not aware of any other portable method than the union. > +STATIC > +EFI_STATUS > +MptDoorbell ( > + IN MPT_SCSI_DEV *Dev, > + IN UINT8 DoorbellFunc, > + IN UINT8 DoorbellArg > + ) > +{ > + return Out32 (Dev, MPT_REG_DOORBELL, (DoorbellFunc << 24) | (DoorbellArg << 16)); > +} (3) The DoorbellFunc variable is of type UINT8, which (in practice) is "unsigned char". As the operand of the << operator, it is promoted to "int" (because "int" can represent all values of "unsigned char", on all the platforms that we care about). In practice, our "int" representation is "31 value bits, 1 sign bit, no padding bits, and two's complement representation". When you left shift a UINT8 value by 24 bits, it's possible that you sign a nonzero bit into the sign bit -- more precisely, the mathematical result of the bitwise left shift cannot be represented in the signed int result. According to the specification of the << operator, this is undefined behavior. In other words, please cast DoorbellFunc to UINT32 before applying the shift. > + MPT_IO_CONTROLLER_INIT_REQUEST Req = { > + .WhoInit = MPT_IOC_WHOINIT_ROM_BIOS, > + .Function = MPT_MESSAGE_HDR_FUNCTION_IOC_INIT, > + .MaxDevices = 1, > + .MaxBuses = 1, > + .ReplyFrameSize = sizeof (MPT_SCSI_IO_ERROR_REPLY), > + }; (4) Okay, so this uses two language features simultaneously that are not permitted in edk2 code: - declaration that is not at the top of a block, - designated initializer. You'll have to set these fields one by one (with assignments), or else introduce a static constant object as a "template", and then set the local variable with CopyMem(). (In edk2 you can't use structure assignment either, you can only assign scalars. This limitation is not from the C language of course; it is because otherwise some compilers cannot be prevented from generating calls to intrinsics, which are not available in the firmware execution environment.) I guess I'll also mention that compound literals are forbidden too. Basically, stick with C89. Thanks Laszlo