From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 5AB078038D for ; Fri, 10 Mar 2017 11:36:40 -0800 (PST) Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D7774C057FA4; Fri, 10 Mar 2017 19:36:40 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-26.phx2.redhat.com [10.3.117.26]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v2AJacrD019625; Fri, 10 Mar 2017 14:36:39 -0500 To: Jordan Justen References: <20170223014814.10937-1-lersek@redhat.com> <20170223014814.10937-12-lersek@redhat.com> <148914088572.26490.12215430240628550434@jljusten-skl> From: Laszlo Ersek Cc: edk2-devel-01 Message-ID: <55e1eade-1d06-4b04-3e07-5ceef61a5602@redhat.com> Date: Fri, 10 Mar 2017 20:36:36 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <148914088572.26490.12215430240628550434@jljusten-skl> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Fri, 10 Mar 2017 19:36:40 +0000 (UTC) Subject: Re: [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 Mar 2017 19:36:40 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/10/17 11:14, Jordan Justen wrote: > On 2017-02-22 17:48:13, Laszlo Ersek wrote: >> + // >> + ScratchBuffer->Features = mSmiFeatures; >> + Status = QemuFwCfgS3WriteBytes (mRequestedFeaturesItem, > > I noticed a fair amount of function calls where the first args are > appearing on the first line, and then the rest appear on subsequent > lines indented two spaces. Yes, that's intentional. The coding style nominally requires each argument on a separate line (if they don't all fit on a single line), but that wastes a huge amount of vertical space (which is bad because less code fits in a screenful). So I frequently follow a semi-compressed style where I place the arguments continuously, but wherever I have to break the line (because of the 80 char limit) I stick with the indentation required by the coding style. Also, when I apply this style, I don't break the final closing paren off to a separate line (because this style is primarily contiguous). I mostly employ this style when the arguments are otherwise simple (i.e., when the "one arg per line" style does not improve clarity, just wastes a lot of space). This is not new, I've been coding like this in edk2 virtually forever. > >> + sizeof ScratchBuffer->Features); >> + if (RETURN_ERROR (Status)) { >> + goto FatalError; >> + } >> + > > I can't say that the coding style prohibits this, but it does seem odd > looking. I would expect to see: > > [1] > > Status = QemuFwCfgS3WriteBytes ( > mRequestedFeaturesItem, > sizeof ScratchBuffer->Features > ); > > The coding style examples look like this: > > Status = QemuFwCfgS3WriteBytes ( > mRequestedFeaturesItem, > sizeof ScratchBuffer->Features > ); > > But this looks less like what I'm used to actually seeing in EDK II. > (Look at DXE Core, for example. It looks like [1].) > > I personally think this is okay to save a line, but it doesn't seem to > follow the coding style doc: > > Status = QemuFwCfgS3WriteBytes ( > mRequestedFeaturesItem, > sizeof ScratchBuffer->Features); > > Now, we all know EDK II has a style of it's own, but outide EDK II, I > might expect something like: > > Status = QemuFwCfgS3WriteBytes(mRequestedFeaturesItem, > sizeof ScratchBuffer->Features); > > Which once again shows arguements on subsequent lines are lined up. > Based on that, I think [1] is the best style for EDK II code. Yes, [1] is the official style, and I stick with that when the arguments are complex or long, or require separate comments. However, as I said above, when the arguments are simple, it makes sense to place them contiguously (together with the final paren), while preserving the edk2 indentation. Again, I've been doing this for years, consistently, and you've never seemed to take issue with it. For example, some snippets from the virtio block driver: fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 290) // fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 291) // virtio-blk header in first desc fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 292) // 7fcacd6c92616 (jljusten 2012-10-12 18:54:17 +0000 293) VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request, e371e7e545b26 (jljusten 2012-10-12 18:54:35 +0000 294) VRING_DESC_F_NEXT, &Indices); fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 310) // fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 311) // VRING_DESC_F_WRITE is interpreted from the host's point of view. fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 312) // 7fcacd6c92616 (jljusten 2012-10-12 18:54:17 +0000 313) VirtioAppendDesc (&Dev->Ring, (UINTN) Buffer, (UINT32) BufferSize, fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 314) VRING_DESC_F_NEXT | (RequestIsWrite ? 0 : VRING_DESC_F_WRITE), e371e7e545b26 (jljusten 2012-10-12 18:54:35 +0000 315) &Indices); fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 316) } fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 318) // fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 319) // host status in last (second or third) desc fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 320) // 7fcacd6c92616 (jljusten 2012-10-12 18:54:17 +0000 321) VirtioAppendDesc (&Dev->Ring, (UINTN) &HostStatus, sizeof HostStatus, e371e7e545b26 (jljusten 2012-10-12 18:54:35 +0000 322) VRING_DESC_F_WRITE, &Indices); 6476804e3cd2e (Laszlo Ersek 2013-12-18 19:57:46 +0000 674) Status = VIRTIO_CFG_READ (Dev, Topology.PhysicalBlockExp, 6476804e3cd2e (Laszlo Ersek 2013-12-18 19:57:46 +0000 675) &PhysicalBlockExp); More recent code from e.g. "SmmAccessPei.c": 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 186) return SmramAccessGetCapabilities (This->LockState, This->OpenState, 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 187) SmramMapSize, SmramMap); 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 267) DEBUG ((EFI_D_ERROR, "%a: no SMRAM with host bridge DID=0x%04x; only " 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 268) "DID=0x%04x (Q35) is supported\n", __FUNCTION__, HostBridgeDevId, 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 269) INTEL_Q35_MCH_DEVICE_ID)); 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 309) // 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 310) // Given the zero graphics memory sizes configured above, set the 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 311) // graphics-related stolen memory bases to the same as TOLUD. 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 312) // 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 313) PciWrite32 (DRAMC_REGISTER_Q35 (MCH_GBSM), 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 314) TopOfLowRamMb << MCH_GBSM_MB_SHIFT); 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 315) PciWrite32 (DRAMC_REGISTER_Q35 (MCH_BGSM), 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 316) TopOfLowRamMb << MCH_BGSM_MB_SHIFT); 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 348) Status = SmramAccessGetCapabilities (mAccess.LockState, mAccess.OpenState, 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 349) &SmramMapSize, SmramMap); 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 376) CopyMem (GuidHob, &SmramMap[DescIdxSmmS3ResumeState], 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 377) sizeof SmramMap[DescIdxSmmS3ResumeState]); Brand new code from "OvmfPkg/AcpiPlatformDxe/BootScript.c": df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 107) Context->WritePointers = AllocatePool (WritePointerCount * df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 108) sizeof *Context->WritePointers); df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 191) DEBUG ((DEBUG_VERBOSE, "%a: 0x%04x/[0x%08x+%d] := 0x%Lx (%Lu)\n", df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 192) __FUNCTION__, PointerItem, PointerOffset, PointerSize, PointerValue, df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 193) (UINT64)S3Context->Used)); df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 247) Status = gBS->LocateProtocol (&gEfiS3SaveStateProtocolGuid, df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 248) NULL /* Registration */, (VOID **)&S3SaveState); I've been entirely consistent about this. Thanks Laszlo